Bug 1093573 part 13 - Handle closing legacy generators correctly. r=wingo,shu

--HG--
extra : rebase_source : d561bc9b8893e8cdff0941530a00824e47d097fc

js/src/jit-test/tests/debug/onExceptionUnwind-11.js

@@ -0,0 +1,29 @@
+// Closing legacy generators should not invoke the onExceptionUnwind hook.
+
+var g = newGlobal();
+var dbg = Debugger(g);
+dbg.onExceptionUnwind = function (frame, exc) {
+    log += "ERROR";
+    assertEq(0, 1);
+};
+g.eval(`
+var log = "";
+function f() {
+    function gen() {
+        try {
+            log += "yield";
+            yield 3;
+            yield 4;
+        } catch(e) {
+            log += "catch";
+        } finally {
+            log += "finally";
+        }
+    };
+    var it = gen();
+    assertEq(it.next(), 3);
+    it.close();
+};
+f();
+`);
+assertEq(g.log, "yieldfinally");

js/src/jit-test/tests/generators/closing-osr.js

@@ -0,0 +1,24 @@
+// OSR into a |finally| block while closing a legacy generator should work.
+var log = "";
+function f() {
+    try {
+	try {
+	    log += "a";
+	    yield 2;
+	    log += "b";
+	    yield 3;
+	} finally {
+	    log += "c";
+	    for (var i=0; i<20; i++) {};
+	    log += "d";
+	}
+    } catch(e) {
+	log += "e";
+    }
+    log += "f";
+}
+
+var it = f();
+assertEq(it.next(), 2);
+it.close();
+assertEq(log, "acd");

js/src/jit-test/tests/jaeger/closure-04.js

@@ -1,5 +1,12 @@
+var depth = 0;
 test();
 function test() {
+  // |test()| is called recursively. When the generator runs in the JIT, the
+  // recursion limit is ~20x higher than in the interpreter. Limit the depth
+  // here so that the test doesn't timeout or becomes extremely slow.
+  if (++depth > 400)
+     return;
+
   var catch1, catch2, catch3, finally1, finally2, finally3;
   function gen() {
     yield 1;

js/src/jit/BaselineCompiler.cpp

@@ -3429,8 +3429,8 @@ typedef bool (*InterpretResumeFn)(JSContext *, HandleObject, HandleValue, Handle
                                   MutableHandleValue);
 static const VMFunction InterpretResumeInfo = FunctionInfo<InterpretResumeFn>(jit::InterpretResume);
 
-typedef bool (*GeneratorThrowFn)(JSContext *, HandleObject, HandleValue);
-static const VMFunction GeneratorThrowInfo = FunctionInfo<GeneratorThrowFn>(js::GeneratorThrow);
+typedef bool (*GeneratorThrowFn)(JSContext *, HandleObject, HandleValue, uint32_t);
+static const VMFunction GeneratorThrowInfo = FunctionInfo<GeneratorThrowFn>(js::GeneratorThrowOrClose);
 
 bool
 BaselineCompiler::emit_JSOP_RESUME()
@@ -3447,29 +3447,6 @@ BaselineCompiler::emit_JSOP_RESUME()
     Register genObj = regs.takeAny();
     masm.unboxObject(frame.addressOfStackValue(frame.peek(-2)), genObj);
 
-    if (resumeKind == GeneratorObject::CLOSE) {
-        // Resume the CLOSE operation in the interpreter. This is only used for
-        // legacy generators and requires some complicated exception handling
-        // logic to only execute |finally| blocks and then return to the caller
-        // without throwing. Furthermore, because it only executes |finally|
-        // blocks, it's unlikely to benefit from JIT compilation.
-
-        ValueOperand retVal = regs.takeAnyValue();
-        masm.loadValue(frame.addressOfStackValue(frame.peek(-1)), retVal);
-
-        prepareVMCall();
-        pushArg(ImmGCPtr(cx->names().close));
-        pushArg(retVal);
-        pushArg(genObj);
-
-        if (!callVM(InterpretResumeInfo))
-            return false;
-
-        frame.popn(2);
-        frame.push(R0);
-        return true;
-    }
-
     // Load callee.
     Register callee = regs.takeAny();
     masm.unboxObject(Address(genObj, GeneratorObject::offsetOfCalleeSlot()), callee);
@@ -3594,7 +3571,7 @@ BaselineCompiler::emit_JSOP_RESUME()
                         Address(genObj, GeneratorObject::offsetOfYieldIndexSlot()));
         masm.jump(scratch1);
     } else {
-        MOZ_ASSERT(resumeKind == GeneratorObject::THROW);
+        MOZ_ASSERT(resumeKind == GeneratorObject::THROW || resumeKind == GeneratorObject::CLOSE);
 
         // Update the frame's frameSize field.
         Register scratch3 = regs.takeAny();
@@ -3605,6 +3582,7 @@ BaselineCompiler::emit_JSOP_RESUME()
         masm.store32(scratch2, Address(BaselineFrameReg, BaselineFrame::reverseOffsetOfFrameSize()));
 
         prepareVMCall();
+        pushArg(Imm32(resumeKind));
         pushArg(retVal);
         pushArg(genObj);
 
@@ -3631,9 +3609,11 @@ BaselineCompiler::emit_JSOP_RESUME()
     prepareVMCall();
     if (resumeKind == GeneratorObject::NEXT) {
         pushArg(ImmGCPtr(cx->names().next));
-    } else {
-        MOZ_ASSERT(resumeKind == GeneratorObject::THROW);
+    } else if (resumeKind == GeneratorObject::THROW) {
         pushArg(ImmGCPtr(cx->names().throw_));
+    } else {
+        MOZ_ASSERT(resumeKind == GeneratorObject::CLOSE);
+        pushArg(ImmGCPtr(cx->names().close));
     }
 
     masm.loadValue(frame.addressOfStackValue(frame.peek(-1)), retVal);

js/src/jit/IonFrames.cpp

@@ -508,6 +508,32 @@ ForcedReturn(JSContext *cx, const JitFrameIterator &frame, jsbytecode *pc,
     *calledDebugEpilogue = true;
 }
 
+static void
+HandleClosingGeneratorReturn(JSContext *cx, const JitFrameIterator &frame, jsbytecode *pc,
+                             jsbytecode *unwoundScopeToPc, ResumeFromException *rfe,
+                             bool *calledDebugEpilogue)
+{
+    // If we're closing a legacy generator, we need to return to the caller
+    // after executing the |finally| blocks. This is very similar to a forced
+    // return from the debugger.
+
+    if (!cx->isExceptionPending())
+        return;
+    RootedValue exception(cx);
+    if (!cx->getPendingException(&exception))
+        return;
+    if (!exception.isMagic(JS_GENERATOR_CLOSING))
+        return;
+
+    cx->clearPendingException();
+    frame.baselineFrame()->setReturnValue(UndefinedValue());
+
+    if (cx->compartment()->debugMode() && unwoundScopeToPc)
+        frame.baselineFrame()->setUnwoundScopeOverridePc(unwoundScopeToPc);
+
+    ForcedReturn(cx, frame, pc, rfe, calledDebugEpilogue);
+}
+
 static void
 HandleExceptionBaseline(JSContext *cx, const JitFrameIterator &frame, ResumeFromException *rfe,
                         jsbytecode **unwoundScopeToPc, bool *calledDebugEpilogue)
@@ -527,7 +553,10 @@ HandleExceptionBaseline(JSContext *cx, const JitFrameIterator &frame, ResumeFrom
         return;
     }
 
-    if (cx->isExceptionPending() && cx->compartment()->debugMode()) {
+    RootedValue exception(cx);
+    if (cx->isExceptionPending() && cx->compartment()->debugMode() &&
+        cx->getPendingException(&exception) && !exception.isMagic(JS_GENERATOR_CLOSING))
+    {
         BaselineFrame *baselineFrame = frame.baselineFrame();
         switch (Debugger::onExceptionUnwind(cx, baselineFrame)) {
           case JSTRAP_ERROR:
@@ -549,8 +578,10 @@ HandleExceptionBaseline(JSContext *cx, const JitFrameIterator &frame, ResumeFrom
         }
     }
 
-    if (!script->hasTrynotes())
+    if (!script->hasTrynotes()) {
+        HandleClosingGeneratorReturn(cx, frame, pc, *unwoundScopeToPc, rfe, calledDebugEpilogue);
         return;
+    }
 
     JSTryNote *tn = script->trynotes()->vector;
     JSTryNote *tnEnd = tn + script->trynotes()->length;
@@ -584,6 +615,13 @@ HandleExceptionBaseline(JSContext *cx, const JitFrameIterator &frame, ResumeFrom
         switch (tn->kind) {
           case JSTRY_CATCH:
             if (cx->isExceptionPending()) {
+                // If we're closing a legacy generator, we have to skip catch
+                // blocks.
+                if (!cx->getPendingException(&exception))
+                    continue;
+                if (exception.isMagic(JS_GENERATOR_CLOSING))
+                    continue;
+
                 // Ion can compile try-catch, but bailing out to catch
                 // exceptions is slow. Reset the warm-up counter so that if we
                 // catch many exceptions we won't Ion-compile the script.
@@ -628,6 +666,7 @@ HandleExceptionBaseline(JSContext *cx, const JitFrameIterator &frame, ResumeFrom
         }
     }
 
+    HandleClosingGeneratorReturn(cx, frame, pc, *unwoundScopeToPc, rfe, calledDebugEpilogue);
 }
 
 struct AutoDeleteDebugModeOSRInfo

js/src/vm/GeneratorObject.cpp

@@ -107,14 +107,21 @@ GeneratorObject::finalSuspend(JSContext *cx, HandleObject obj)
 }
 
 bool
-js::GeneratorThrow(JSContext *cx, HandleObject obj, HandleValue arg)
+js::GeneratorThrowOrClose(JSContext *cx, HandleObject obj, HandleValue arg, uint32_t resumeKind)
 {
     GeneratorObject *genObj = &obj->as<GeneratorObject>();
-    cx->setPendingException(arg);
-    if (genObj->isNewborn())
-        genObj->setClosed();
-    else
-        genObj->setRunning();
+    if (resumeKind == GeneratorObject::THROW) {
+        cx->setPendingException(arg);
+        if (genObj->isNewborn())
+            genObj->setClosed();
+        else
+            genObj->setRunning();
+    } else {
+        MOZ_ASSERT(resumeKind == GeneratorObject::CLOSE);
+        MOZ_ASSERT(genObj->is<LegacyGeneratorObject>());
+        cx->setPendingException(MagicValue(JS_GENERATOR_CLOSING));
+        genObj->setClosing();
+    }
     return false;
 }
 
@@ -160,13 +167,8 @@ GeneratorObject::resume(JSContext *cx, InterpreterActivation &activation,
         return true;
 
       case THROW:
-        return GeneratorThrow(cx, genObj, arg);
-
       case CLOSE:
-        MOZ_ASSERT(genObj->is<LegacyGeneratorObject>());
-        cx->setPendingException(MagicValue(JS_GENERATOR_CLOSING));
-        genObj->setClosing();
-        return false;
+        return GeneratorThrowOrClose(cx, genObj, arg, resumeKind);
 
       default:
         MOZ_CRASH("bad resumeKind");

js/src/vm/GeneratorObject.h

@@ -220,7 +220,7 @@ class StarGeneratorObject : public GeneratorObject
     static const Class class_;
 };
 
-bool GeneratorThrow(JSContext *cx, HandleObject obj, HandleValue val);
+bool GeneratorThrowOrClose(JSContext *cx, HandleObject obj, HandleValue val, uint32_t resumeKind);
 
 } // namespace js
 

js/src/vm/Interpreter.cpp

@@ -1028,24 +1028,29 @@ HandleError(JSContext *cx, InterpreterRegs &regs)
   again:
     if (cx->isExceptionPending()) {
         /* Call debugger throw hooks. */
-        JSTrapStatus status = Debugger::onExceptionUnwind(cx, regs.fp());
-        switch (status) {
-          case JSTRAP_ERROR:
+        RootedValue exception(cx);
+        if (!cx->getPendingException(&exception))
             goto again;
 
-          case JSTRAP_CONTINUE:
-          case JSTRAP_THROW:
-            break;
+        if (!exception.isMagic(JS_GENERATOR_CLOSING)) {
+            JSTrapStatus status = Debugger::onExceptionUnwind(cx, regs.fp());
+            switch (status) {
+              case JSTRAP_ERROR:
+                goto again;
 
-          case JSTRAP_RETURN:
-            ForcedReturn(cx, si, regs);
-            return SuccessfulReturnContinuation;
+              case JSTRAP_CONTINUE:
+              case JSTRAP_THROW:
+                break;
 
-          default:
-            MOZ_CRASH("Bad Debugger::onExceptionUnwind status");
+              case JSTRAP_RETURN:
+                ForcedReturn(cx, si, regs);
+                return SuccessfulReturnContinuation;
+
+              default:
+                MOZ_CRASH("Bad Debugger::onExceptionUnwind status");
+            }
         }
 
-        RootedValue exception(cx);
         for (TryNoteIter tni(cx, regs); !tni.done(); ++tni) {
             JSTryNote *tn = *tni;