Bug 1346862 - Fix IteratorClose due to non-local jumps being catchable by try statements inside for-of

Issue #74

js/src/frontend/BytecodeEmitter.cpp

@@ -2065,7 +2065,11 @@ class ForOfLoopControl : public LoopControl
 
     bool emitIteratorClose(BytecodeEmitter* bce,
                            CompletionKind completionKind = CompletionKind::Normal) {
-        return bce->emitIteratorClose(completionKind, allowSelfHosted_);
+        ptrdiff_t start = bce->offset();
+        if (!bce->emitIteratorClose(completionKind, allowSelfHosted_))
+            return false;
+        ptrdiff_t end = bce->offset();
+        return bce->tryNoteList.append(JSTRY_FOR_OF_ITERCLOSE, 0, start, end);
     }
 
     bool emitPrepareForNonLocalJump(BytecodeEmitter* bce, bool isTarget) {
@@ -2614,17 +2618,6 @@ BytecodeEmitter::emitUint32Operand(JSOp op, uint32_t operand)
     return true;
 }
 
-bool
-BytecodeEmitter::flushPops(int* npops)
-{
-    MOZ_ASSERT(*npops != 0);
-    if (!emitUint16Operand(JSOP_POPN, *npops))
-        return false;
-
-    *npops = 0;
-    return true;
-}
-
 namespace {
 
 class NonLocalExitControl
@@ -2718,8 +2711,9 @@ NonLocalExitControl::prepareForNonLocalJump(BytecodeEmitter::NestableControl* ta
     bool emitIteratorCloseAtTarget = emitIteratorClose && kind_ != Continue;
 
     auto flushPops = [&npops](BytecodeEmitter* bce) {
-        if (npops && !bce->flushPops(&npops))
+        if (npops && !bce->emitUint16Operand(JSOP_POPN, npops))
             return false;
+        npops = 0;
         return true;
     };
 

js/src/frontend/BytecodeEmitter.h

@@ -452,8 +452,6 @@ struct MOZ_STACK_CLASS BytecodeEmitter
 
     JSOp strictifySetNameOp(JSOp op);
 
-    MOZ_MUST_USE bool flushPops(int* npops);
-
     MOZ_MUST_USE bool emitCheck(ptrdiff_t delta, ptrdiff_t* offset);
 
     // Emit one bytecode.

js/src/jit/JitFrames.cpp

@@ -436,6 +436,8 @@ HandleExceptionIon(JSContext* cx, const InlineFrameIterator& frame, ResumeFromEx
     if (!script->hasTrynotes())
         return;
 
+    bool inForOfIterClose = false;
+
     for (TryNoteIterIon tni(cx, frame); !tni.done(); ++tni) {
         JSTryNote* tn = *tni;
 
@@ -447,12 +449,23 @@ HandleExceptionIon(JSContext* cx, const InlineFrameIterator& frame, ResumeFromEx
             CloseLiveIteratorIon(cx, frame, tn);
             break;
 
+          case JSTRY_FOR_OF_ITERCLOSE:
+            inForOfIterClose = true;
+            break;
+
           case JSTRY_FOR_OF:
+            inForOfIterClose = false;
+            break;
+
           case JSTRY_LOOP:
             break;
 
           case JSTRY_CATCH:
             if (cx->isExceptionPending()) {
+                // See corresponding comment in ProcessTryNotes.
+                if (inForOfIterClose)
+                    break;
+
                 // 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.
@@ -583,6 +596,7 @@ ProcessTryNotesBaseline(JSContext* cx, const JitFrameIterator& frame, Environmen
                         ResumeFromException* rfe, jsbytecode** pc)
 {
     RootedScript script(cx, frame.baselineFrame()->script());
+    bool inForOfIterClose = false;
 
     for (TryNoteIterBaseline tni(cx, frame.baselineFrame(), *pc); !tni.done(); ++tni) {
         JSTryNote* tn = *tni;
@@ -593,7 +607,11 @@ ProcessTryNotesBaseline(JSContext* cx, const JitFrameIterator& frame, Environmen
             // If we're closing a legacy generator, we have to skip catch
             // blocks.
             if (cx->isClosingGenerator())
-                continue;
+                break;
+
+            // See corresponding comment in ProcessTryNotes.
+            if (inForOfIterClose)
+                break;
 
             SettleOnTryNote(cx, tn, frame, ei, rfe, pc);
 
@@ -609,6 +627,10 @@ ProcessTryNotesBaseline(JSContext* cx, const JitFrameIterator& frame, Environmen
           }
 
           case JSTRY_FINALLY: {
+            // See corresponding comment in ProcessTryNotes.
+            if (inForOfIterClose)
+                break;
+
             SettleOnTryNote(cx, tn, frame, ei, rfe, pc);
             rfe->kind = ResumeFromException::RESUME_FINALLY;
             rfe->target = script->baselineScript()->nativeCodeForPC(script, *pc);
@@ -652,7 +674,14 @@ ProcessTryNotesBaseline(JSContext* cx, const JitFrameIterator& frame, Environmen
             break;
           }
 
+          case JSTRY_FOR_OF_ITERCLOSE:
+            inForOfIterClose = true;
+            break;
+
           case JSTRY_FOR_OF:
+            inForOfIterClose = false;
+            break;
+
           case JSTRY_LOOP:
             break;
 

js/src/jsscript.h

@@ -85,6 +85,7 @@ enum JSTryNoteKind {
     JSTRY_FOR_IN,
     JSTRY_FOR_OF,
     JSTRY_LOOP,
+    JSTRY_FOR_OF_ITERCLOSE,
     JSTRY_DESTRUCTURING_ITERCLOSE
 };
 

js/src/shell/js.cpp

@@ -2599,6 +2599,8 @@ TryNoteName(JSTryNoteKind kind)
         return "for-of";
       case JSTRY_LOOP:
         return "loop";
+      case JSTRY_FOR_OF_ITERCLOSE:
+        return "for-of-iterclose";
       case JSTRY_DESTRUCTURING_ITERCLOSE:
         return "dstr-iterclose";
     }
@@ -2612,14 +2614,14 @@ TryNotes(JSContext* cx, HandleScript script, Sprinter* sp)
     if (!script->hasTrynotes())
         return true;
 
-    if (sp->put("\nException table:\nkind             stack    start      end\n") < 0)
+    if (sp->put("\nException table:\nkind               stack    start      end\n") < 0)
         return false;
 
     JSTryNote* tn = script->trynotes()->vector;
     JSTryNote* tnlimit = tn + script->trynotes()->length;
     do {
         uint32_t startOff = script->pcToOffset(script->main()) + tn->start;
-        if (!sp->jsprintf(" %-14s %6u %8u %8u\n",
+        if (!sp->jsprintf(" %-16s %6u %8u %8u\n",
                           TryNoteName(static_cast<JSTryNoteKind>(tn->kind)),
                           tn->stackDepth, startOff, startOff + tn->length))
         {

js/src/tests/ecma_6/Statements/for-of-iterator-close-throw.js

@@ -0,0 +1,35 @@
+function test() {
+    var returnCalled = 0;
+    var returnCalledExpected = 0;
+    var catchEntered = 0;
+    var finallyEntered = 0;
+    var finallyEnteredExpected = 0;
+    var iterable = {};
+    iterable[Symbol.iterator] = makeIterator({
+        ret: function() {
+            returnCalled++;
+            throw 42;
+        }
+    });
+
+    // inner try cannot catch IteratorClose throwing
+    assertThrowsValue(function() {
+        for (var x of iterable) {
+            try {
+                return;
+            } catch (e) {
+                catchEntered++;
+            } finally {
+                finallyEntered++;
+            }
+        }
+    }, 42);
+    assertEq(returnCalled, ++returnCalledExpected);
+    assertEq(catchEntered, 0);
+    assertEq(finallyEntered, ++finallyEnteredExpected);
+}
+
+test();
+
+if (typeof reportCompare === "function")
+    reportCompare(0, 0);

js/src/vm/Interpreter.cpp

@@ -1156,6 +1156,7 @@ enum HandleErrorContinuation
 static HandleErrorContinuation
 ProcessTryNotes(JSContext* cx, EnvironmentIter& ei, InterpreterRegs& regs)
 {
+    bool inForOfIterClose = false;
     for (TryNoteIterInterpreter tni(cx, regs); !tni.done(); ++tni) {
         JSTryNote* tn = *tni;
 
@@ -1164,10 +1165,38 @@ ProcessTryNotes(JSContext* cx, EnvironmentIter& ei, InterpreterRegs& regs)
             /* Catch cannot intercept the closing of a generator. */
             if (cx->isClosingGenerator())
                 break;
+
+            // If IteratorClose due to abnormal completion threw inside a
+            // for-of loop, it is not catchable by try statements inside of
+            // the for-of loop.
+            //
+            // This is handled by this weirdness in the exception handler
+            // instead of in bytecode because it is hard to do so in bytecode:
+            //
+            //   1. IteratorClose emitted due to abnormal completion (break,
+            //   throw, return) are emitted inline, at the source location of
+            //   the break, throw, or return statement. For example:
+            //
+            //     for (x of iter) {
+            //       try { return; } catch (e) { }
+            //     }
+            //
+            //   From the try-note nesting's perspective, the IteratorClose
+            //   resulting from |return| is covered by the inner try, when it
+            //   should not be.
+            //
+            //   2. Try-catch notes cannot be disjoint. That is, we can't have
+            //   multiple notes with disjoint pc ranges jumping to the same
+            //   catch block.
+            if (inForOfIterClose)
+                break;
             SettleOnTryNote(cx, tn, ei, regs);
             return CatchContinuation;
 
           case JSTRY_FINALLY:
+            // See note above.
+            if (inForOfIterClose)
+                break;
             SettleOnTryNote(cx, tn, ei, regs);
             return FinallyContinuation;
 
@@ -1206,7 +1235,14 @@ ProcessTryNotes(JSContext* cx, EnvironmentIter& ei, InterpreterRegs& regs)
             break;
           }
 
+          case JSTRY_FOR_OF_ITERCLOSE:
+            inForOfIterClose = true;
+            break;
+
           case JSTRY_FOR_OF:
+            inForOfIterClose = false;
+            break;
+
           case JSTRY_LOOP:
             break;