Bug 1093573 part 13 - Handle closing legacy generators correctly. r=wingo,shu
authorJan de Mooij <jdemooij@mozilla.com>
Thu, 13 Nov 2014 17:39:51 +0100
changeset 215529 8792056f152cf79804df03bc7d130ad68ae0ded7
parent 215528 5e645894f6bf0b32c5d17823b44c91863c1b3beb
child 215530 76fdd0f934c191a3ca2d050e81cad97f2b1298a6
push id27818
push userryanvm@gmail.com
push dateThu, 13 Nov 2014 20:19:09 +0000
treeherdermozilla-central@292ed84594c1 [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewerswingo, shu
bugs1093573
milestone36.0a1
first release with
nightly linux32
nightly linux64
nightly mac
nightly win32
nightly win64
last release without
nightly linux32
nightly linux64
nightly mac
nightly win32
nightly win64
Bug 1093573 part 13 - Handle closing legacy generators correctly. r=wingo,shu
js/src/jit-test/tests/debug/onExceptionUnwind-11.js
js/src/jit-test/tests/generators/closing-osr.js
js/src/jit-test/tests/jaeger/closure-04.js
js/src/jit/BaselineCompiler.cpp
js/src/jit/IonFrames.cpp
js/src/vm/GeneratorObject.cpp
js/src/vm/GeneratorObject.h
js/src/vm/Interpreter.cpp
new file mode 100644
--- /dev/null
+++ b/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");
new file mode 100644
--- /dev/null
+++ b/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");
--- a/js/src/jit-test/tests/jaeger/closure-04.js
+++ b/js/src/jit-test/tests/jaeger/closure-04.js
@@ -1,10 +1,17 @@
+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;
     try {
       try {
         try {
           yield 1;
         } finally {
--- a/js/src/jit/BaselineCompiler.cpp
+++ b/js/src/jit/BaselineCompiler.cpp
@@ -3424,57 +3424,34 @@ BaselineCompiler::emit_JSOP_FINALYIELDRV
     masm.loadValue(frame.addressOfReturnValue(), JSReturnOperand);
     return emitReturn();
 }
 
 typedef bool (*InterpretResumeFn)(JSContext *, HandleObject, HandleValue, HandlePropertyName,
                                   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()
 {
     GeneratorObject::ResumeKind resumeKind = GeneratorObject::getResumeKind(pc);
 
     frame.syncStack(0);
     masm.checkStackAlignment();
 
     GeneralRegisterSet regs(GeneralRegisterSet::All());
     regs.take(BaselineFrameReg);
 
     // Load generator object.
     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);
 
     // Load the script. Note that we don't relazify generator scripts, so it's
     // guaranteed to be non-lazy.
     Register scratch1 = regs.takeAny();
     masm.loadPtr(Address(callee, JSFunction::offsetOfNativeOrScript()), scratch1);
@@ -3589,27 +3566,28 @@ BaselineCompiler::emit_JSOP_RESUME()
     masm.pushValue(retVal);
 
     if (resumeKind == GeneratorObject::NEXT) {
         // Mark as running and jump to the generator's JIT code.
         masm.storeValue(Int32Value(GeneratorObject::YIELD_INDEX_RUNNING),
                         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();
         masm.computeEffectiveAddress(Address(BaselineFrameReg, BaselineFrame::FramePointerOffset),
                                      scratch2);
         masm.movePtr(scratch2, scratch3);
         masm.subPtr(BaselineStackReg, scratch2);
         masm.store32(scratch2, Address(BaselineFrameReg, BaselineFrame::reverseOffsetOfFrameSize()));
 
         prepareVMCall();
+        pushArg(Imm32(resumeKind));
         pushArg(retVal);
         pushArg(genObj);
 
         JitCode *code = cx->runtime()->jitRuntime()->getVMWrapper(GeneratorThrowInfo);
         if (!code)
             return false;
 
         // Create the frame descriptor.
@@ -3626,19 +3604,21 @@ BaselineCompiler::emit_JSOP_RESUME()
     }
 
     // If the generator script has no JIT code, call into the VM.
     masm.bind(&interpret);
 
     prepareVMCall();
     if (resumeKind == GeneratorObject::NEXT) {
         pushArg(ImmGCPtr(cx->names().next));
+    } else if (resumeKind == GeneratorObject::THROW) {
+        pushArg(ImmGCPtr(cx->names().throw_));
     } else {
-        MOZ_ASSERT(resumeKind == GeneratorObject::THROW);
-        pushArg(ImmGCPtr(cx->names().throw_));
+        MOZ_ASSERT(resumeKind == GeneratorObject::CLOSE);
+        pushArg(ImmGCPtr(cx->names().close));
     }
 
     masm.loadValue(frame.addressOfStackValue(frame.peek(-1)), retVal);
     pushArg(retVal);
     pushArg(genObj);
 
     if (!callVM(InterpretResumeInfo))
         return false;
--- a/js/src/jit/IonFrames.cpp
+++ b/js/src/jit/IonFrames.cpp
@@ -504,16 +504,42 @@ ForcedReturn(JSContext *cx, const JitFra
         return;
     }
 
     // DebugEpilogue threw an exception. Propagate to the caller frame.
     *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)
 {
     MOZ_ASSERT(frame.isBaselineJS());
     MOZ_ASSERT(!*calledDebugEpilogue);
 
     RootedScript script(cx);
     jsbytecode *pc;
@@ -522,17 +548,20 @@ HandleExceptionBaseline(JSContext *cx, c
     // We may be propagating a forced return from the interrupt
     // callback, which cannot easily force a return.
     if (cx->isPropagatingForcedReturn()) {
         cx->clearPropagatingForcedReturn();
         ForcedReturn(cx, frame, pc, rfe, calledDebugEpilogue);
         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:
             // Uncatchable exception.
             MOZ_ASSERT(!cx->isExceptionPending());
             break;
 
           case JSTRAP_CONTINUE:
@@ -544,18 +573,20 @@ HandleExceptionBaseline(JSContext *cx, c
             ForcedReturn(cx, frame, pc, rfe, calledDebugEpilogue);
             return;
 
           default:
             MOZ_CRASH("Invalid trap status");
         }
     }
 
-    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;
 
     uint32_t pcOffset = uint32_t(pc - script->main());
     ScopeIter si(frame.baselineFrame(), pc, cx);
     for (; tn != tnEnd; ++tn) {
         if (pcOffset < tn->start)
@@ -579,16 +610,23 @@ HandleExceptionBaseline(JSContext *cx, c
         // Compute base pointer and stack pointer.
         rfe->framePointer = frame.fp() - BaselineFrame::FramePointerOffset;
         rfe->stackPointer = rfe->framePointer - BaselineFrame::Size() -
             (script->nfixed() + tn->stackDepth) * sizeof(Value);
 
         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.
                 script->resetWarmUpCounter();
 
                 // Resume at the start of the catch block.
                 rfe->kind = ResumeFromException::RESUME_CATCH;
                 jsbytecode *catchPC = script->main() + tn->start + tn->length;
@@ -623,16 +661,17 @@ HandleExceptionBaseline(JSContext *cx, c
           case JSTRY_LOOP:
             break;
 
           default:
             MOZ_CRASH("Invalid try note");
         }
     }
 
+    HandleClosingGeneratorReturn(cx, frame, pc, *unwoundScopeToPc, rfe, calledDebugEpilogue);
 }
 
 struct AutoDeleteDebugModeOSRInfo
 {
     BaselineFrame *frame;
     explicit AutoDeleteDebugModeOSRInfo(BaselineFrame *frame) : frame(frame) { MOZ_ASSERT(frame); }
     ~AutoDeleteDebugModeOSRInfo() { frame->deleteDebugModeOSRInfo(); }
 };
--- a/js/src/vm/GeneratorObject.cpp
+++ b/js/src/vm/GeneratorObject.cpp
@@ -102,24 +102,31 @@ GeneratorObject::finalSuspend(JSContext 
 
     if (genObj->is<LegacyGeneratorObject>() && !closing)
         return ThrowStopIteration(cx);
 
     return true;
 }
 
 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;
 }
 
 bool
 GeneratorObject::resume(JSContext *cx, InterpreterActivation &activation,
                         HandleObject obj, HandleValue arg, GeneratorObject::ResumeKind resumeKind)
 {
     Rooted<GeneratorObject*> genObj(cx, &obj->as<GeneratorObject>());
@@ -155,23 +162,18 @@ GeneratorObject::resume(JSContext *cx, I
     activation.regs().sp[-1] = arg;
 
     switch (resumeKind) {
       case NEXT:
         genObj->setRunning();
         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");
     }
 }
 
 bool
 LegacyGeneratorObject::close(JSContext *cx, HandleObject obj)
--- a/js/src/vm/GeneratorObject.h
+++ b/js/src/vm/GeneratorObject.h
@@ -215,17 +215,17 @@ class LegacyGeneratorObject : public Gen
 };
 
 class StarGeneratorObject : public GeneratorObject
 {
   public:
     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
 
 template<>
 inline bool
 JSObject::is<js::GeneratorObject>() const
 {
     return is<js::LegacyGeneratorObject>() || is<js::StarGeneratorObject>();
--- a/js/src/vm/Interpreter.cpp
+++ b/js/src/vm/Interpreter.cpp
@@ -1023,34 +1023,39 @@ HandleError(JSContext *cx, InterpreterRe
     MOZ_ASSERT(regs.fp()->script()->containsPC(regs.pc));
 
     ScopeIter si(regs.fp(), regs.pc, cx);
     bool ok = false;
 
   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;
-
-          case JSTRAP_RETURN:
-            ForcedReturn(cx, si, regs);
-            return SuccessfulReturnContinuation;
-
-          default:
-            MOZ_CRASH("Bad Debugger::onExceptionUnwind status");
+        if (!exception.isMagic(JS_GENERATOR_CLOSING)) {
+            JSTrapStatus status = Debugger::onExceptionUnwind(cx, regs.fp());
+            switch (status) {
+              case JSTRAP_ERROR:
+                goto again;
+
+              case JSTRAP_CONTINUE:
+              case JSTRAP_THROW:
+                break;
+
+              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;
 
             // Unwind the scope to the beginning of the JSOP_TRY.
             UnwindScope(cx, si, UnwindScopeToTryPc(regs.fp()->script(), tn));
 
             /*
              * Set pc to the first bytecode after the the try note to point