Bug 1480390: Move ForOfIterClose logic inside TryNoteIter r=tcampbell
authorIain Ireland <iireland@mozilla.com>
Fri, 11 Jan 2019 18:05:36 +0000
changeset 453523 025feea5945b
parent 453522 24b9c0eb34c0
child 453524 b3d4bff86800
push id35360
push usernbeleuzu@mozilla.com
push dateSat, 12 Jan 2019 09:39:47 +0000
treeherdermozilla-central@cb35977ae7a4 [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewerstcampbell
bugs1480390
milestone66.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 1480390: Move ForOfIterClose logic inside TryNoteIter r=tcampbell This patch was intended to be a pure refactoring of existing code with no side-effects, moving the logic for handling for-of/for-of-iterclose trynotes inside TryNoteIter to avoid duplicating logic in all users of TryNoteIter. However, it turns out that there was a subtle preexisting bug in TryNoteIter that is fixed by the refactoring. Specifically, the logic to skip from a for-of-iterclose to its enclosing for-of must run before the logic to skip trynotes based on stack depth. Otherwise, the stack depth code may filter out the enclosing for-of (see the attached test case for an example) and we will skip too many try-notes. Differential Revision: https://phabricator.services.mozilla.com/D14783
js/src/jit-test/tests/for-of/throw-during-break.js
js/src/jit/JitFrames.cpp
js/src/vm/Interpreter.cpp
js/src/vm/Interpreter.h
new file mode 100644
--- /dev/null
+++ b/js/src/jit-test/tests/for-of/throw-during-break.js
@@ -0,0 +1,42 @@
+var whoCaught = "nobody"
+
+function* wrapNoThrow() {
+    let iter = {
+      [Symbol.iterator]() {
+        return this;
+      },
+      next() {
+        return { value: 10, done: false };
+      },
+      return() { throw "nonsense"; }
+    };
+    for (const i of iter)
+      yield i;
+  }
+function foo() {
+    try {
+	l2:
+	for (j of wrapNoThrow()) {
+	    try {
+		for (i of [1,2,3]) {
+		    try {
+			break l2;
+		    } catch(e) {
+			whoCaught = "inner"
+		    }
+		}
+	    } catch (e) {
+		whoCaught = "inner2"
+	    }
+	}
+    } catch (e) {
+	whoCaught = "correct"
+    }
+}
+
+try {
+    foo();
+} catch (e) { whoCaught = "outer" }
+
+
+assertEq(whoCaught, "correct");
--- a/js/src/jit/JitFrames.cpp
+++ b/js/src/jit/JitFrames.cpp
@@ -195,53 +195,29 @@ static void HandleExceptionIon(JSContext
     }
   }
 
   RootedScript script(cx, frame.script());
   if (!script->hasTrynotes()) {
     return;
   }
 
-  bool inForOfIterClose = false;
-
   for (TryNoteIterIon tni(cx, frame); !tni.done(); ++tni) {
     const JSTryNote* tn = *tni;
-
     switch (tn->kind) {
       case JSTRY_FOR_IN:
       case JSTRY_DESTRUCTURING:
-        // See corresponding comment in ProcessTryNotes.
-        if (inForOfIterClose) {
-          break;
-        }
-
         MOZ_ASSERT_IF(tn->kind == JSTRY_FOR_IN,
                       JSOp(*(script->offsetToPC(tn->start + tn->length))) ==
                           JSOP_ENDITER);
         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.
           script->resetWarmUpCounter();
 
           if (*hitBailoutException) {
             break;
           }
@@ -260,16 +236,21 @@ static void HandleExceptionIon(JSContext
             return;
           }
 
           *hitBailoutException = true;
           MOZ_ASSERT(cx->isExceptionPending());
         }
         break;
 
+      case JSTRY_FOR_OF:
+      case JSTRY_LOOP:
+        break;
+
+      // JSTRY_FOR_OF_ITERCLOSE is handled internally by the try note iterator.
       default:
         MOZ_CRASH("Unexpected try note");
     }
   }
 }
 
 static void OnLeaveBaselineFrame(JSContext* cx, const JSJitFrameIter& frame,
                                  jsbytecode* pc, ResumeFromException* rfe,
@@ -344,75 +325,55 @@ class TryNoteIterBaseline : public TryNo
       : TryNoteIter(cx, frame->script(), pc, BaselineFrameStackDepthOp(frame)) {
   }
 };
 
 // Close all live iterators on a BaselineFrame due to exception unwinding. The
 // pc parameter is updated to where the envs have been unwound to.
 static void CloseLiveIteratorsBaselineForUncatchableException(
     JSContext* cx, const JSJitFrameIter& frame, jsbytecode* pc) {
-  bool inForOfIterClose = false;
   for (TryNoteIterBaseline tni(cx, frame.baselineFrame(), pc); !tni.done();
        ++tni) {
     const JSTryNote* tn = *tni;
     switch (tn->kind) {
       case JSTRY_FOR_IN: {
-        // See corresponding comment in ProcessTryNotes.
-        if (inForOfIterClose) {
-          break;
-        }
-
         uint8_t* framePointer;
         uint8_t* stackPointer;
         BaselineFrameAndStackPointersFromTryNote(tn, frame, &framePointer,
                                                  &stackPointer);
         Value iterValue(*(Value*)stackPointer);
         RootedObject iterObject(cx, &iterValue.toObject());
         UnwindIteratorForUncatchableException(iterObject);
         break;
       }
 
-      case JSTRY_FOR_OF_ITERCLOSE:
-        inForOfIterClose = true;
-        break;
-
-      case JSTRY_FOR_OF:
-        inForOfIterClose = false;
-        break;
-
       default:
         break;
     }
   }
 }
 
 static bool ProcessTryNotesBaseline(JSContext* cx, const JSJitFrameIter& frame,
                                     EnvironmentIter& ei,
                                     ResumeFromException* rfe, jsbytecode** pc) {
   RootedScript script(cx, frame.baselineFrame()->script());
-  bool inForOfIterClose = false;
 
   for (TryNoteIterBaseline tni(cx, frame.baselineFrame(), *pc); !tni.done();
        ++tni) {
     const JSTryNote* tn = *tni;
 
     MOZ_ASSERT(cx->isExceptionPending());
     switch (tn->kind) {
       case JSTRY_CATCH: {
         // If we're closing a legacy generator, we have to skip catch
         // blocks.
         if (cx->isClosingGenerator()) {
           break;
         }
 
-        // See corresponding comment in ProcessTryNotes.
-        if (inForOfIterClose) {
-          break;
-        }
-
         SettleOnTryNote(cx, tn, frame, ei, rfe, pc);
 
         // 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.
@@ -420,58 +381,43 @@ static bool ProcessTryNotesBaseline(JSCo
         rfe->kind = ResumeFromException::RESUME_CATCH;
         rfe->target =
             script->baselineScript()->nativeCodeForPC(script, *pc, &slotInfo);
         MOZ_ASSERT(slotInfo.isStackSynced());
         return true;
       }
 
       case JSTRY_FINALLY: {
-        // See corresponding comment in ProcessTryNotes.
-        if (inForOfIterClose) {
-          break;
-        }
-
         PCMappingSlotInfo slotInfo;
         SettleOnTryNote(cx, tn, frame, ei, rfe, pc);
         rfe->kind = ResumeFromException::RESUME_FINALLY;
         rfe->target =
             script->baselineScript()->nativeCodeForPC(script, *pc, &slotInfo);
         MOZ_ASSERT(slotInfo.isStackSynced());
         // Drop the exception instead of leaking cross compartment data.
         if (!cx->getPendingException(
                 MutableHandleValue::fromMarkedLocation(&rfe->exception))) {
           rfe->exception = UndefinedValue();
         }
         cx->clearPendingException();
         return true;
       }
 
       case JSTRY_FOR_IN: {
-        // See corresponding comment in ProcessTryNotes.
-        if (inForOfIterClose) {
-          break;
-        }
-
         uint8_t* framePointer;
         uint8_t* stackPointer;
         BaselineFrameAndStackPointersFromTryNote(tn, frame, &framePointer,
                                                  &stackPointer);
         Value iterValue(*reinterpret_cast<Value*>(stackPointer));
         JSObject* iterObject = &iterValue.toObject();
         CloseIterator(iterObject);
         break;
       }
 
       case JSTRY_DESTRUCTURING: {
-        // See corresponding comment in ProcessTryNotes.
-        if (inForOfIterClose) {
-          break;
-        }
-
         uint8_t* framePointer;
         uint8_t* stackPointer;
         BaselineFrameAndStackPointersFromTryNote(tn, frame, &framePointer,
                                                  &stackPointer);
         RootedValue doneValue(cx, *(reinterpret_cast<Value*>(stackPointer)));
         bool done = ToBoolean(doneValue);
         if (!done) {
           Value iterValue(*(reinterpret_cast<Value*>(stackPointer) + 1));
@@ -479,27 +425,21 @@ static bool ProcessTryNotesBaseline(JSCo
           if (!IteratorCloseForException(cx, iterObject)) {
             SettleOnTryNote(cx, tn, frame, ei, rfe, pc);
             return false;
           }
         }
         break;
       }
 
-      case JSTRY_FOR_OF_ITERCLOSE:
-        inForOfIterClose = true;
-        break;
-
       case JSTRY_FOR_OF:
-        inForOfIterClose = false;
-        break;
-
       case JSTRY_LOOP:
         break;
 
+      // JSTRY_FOR_OF_ITERCLOSE is handled internally by the try note iterator.
       default:
         MOZ_CRASH("Invalid try note");
     }
   }
   return true;
 }
 
 static void HandleExceptionBaseline(JSContext* cx, const JSJitFrameIter& frame,
--- a/js/src/vm/Interpreter.cpp
+++ b/js/src/vm/Interpreter.cpp
@@ -1115,153 +1115,90 @@ class TryNoteIterInterpreter
       : TryNoteIter(cx, regs.fp()->script(), regs.pc,
                     InterpreterFrameStackDepthOp(regs)) {}
 };
 
 static void UnwindIteratorsForUncatchableException(
     JSContext* cx, const InterpreterRegs& regs) {
   // c.f. the regular (catchable) TryNoteIterInterpreter loop in
   // ProcessTryNotes.
-  bool inForOfIterClose = false;
   for (TryNoteIterInterpreter tni(cx, regs); !tni.done(); ++tni) {
     const JSTryNote* tn = *tni;
     switch (tn->kind) {
       case JSTRY_FOR_IN: {
-        // See corresponding comment in ProcessTryNotes.
-        if (inForOfIterClose) {
-          break;
-        }
-
         Value* sp = regs.spForStackDepth(tn->stackDepth);
         UnwindIteratorForUncatchableException(&sp[-1].toObject());
         break;
       }
-
-      case JSTRY_FOR_OF_ITERCLOSE:
-        inForOfIterClose = true;
-        break;
-
-      case JSTRY_FOR_OF:
-        inForOfIterClose = false;
-        break;
-
       default:
         break;
     }
   }
 }
 
 enum HandleErrorContinuation {
   SuccessfulReturnContinuation,
   ErrorReturnContinuation,
   CatchContinuation,
   FinallyContinuation
 };
 
 static HandleErrorContinuation ProcessTryNotes(JSContext* cx,
                                                EnvironmentIter& ei,
                                                InterpreterRegs& regs) {
-  bool inForOfIterClose = false;
   for (TryNoteIterInterpreter tni(cx, regs); !tni.done(); ++tni) {
     const JSTryNote* tn = *tni;
 
     switch (tn->kind) {
       case JSTRY_CATCH:
         /* 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;
 
       case JSTRY_FOR_IN: {
-        // Don't let (extra) values pushed on the stack while closing a
-        // for-of iterator confuse us into thinking we still have to close
-        // an inner for-in iterator.
-        if (inForOfIterClose) {
-          break;
-        }
-
         /* This is similar to JSOP_ENDITER in the interpreter loop. */
         DebugOnly<jsbytecode*> pc =
             regs.fp()->script()->offsetToPC(tn->start + tn->length);
         MOZ_ASSERT(JSOp(*pc) == JSOP_ENDITER);
         Value* sp = regs.spForStackDepth(tn->stackDepth);
         JSObject* obj = &sp[-1].toObject();
         CloseIterator(obj);
         break;
       }
 
       case JSTRY_DESTRUCTURING: {
-        // See note above.
-        if (inForOfIterClose) {
-          break;
-        }
-
         // Whether the destructuring iterator is done is at the top of the
         // stack. The iterator object is second from the top.
         MOZ_ASSERT(tn->stackDepth > 1);
         Value* sp = regs.spForStackDepth(tn->stackDepth);
         RootedValue doneValue(cx, sp[-1]);
         bool done = ToBoolean(doneValue);
         if (!done) {
           RootedObject iterObject(cx, &sp[-2].toObject());
           if (!IteratorCloseForException(cx, iterObject)) {
             SettleOnTryNote(cx, tn, ei, regs);
             return ErrorReturnContinuation;
           }
         }
         break;
       }
 
-      case JSTRY_FOR_OF_ITERCLOSE:
-        inForOfIterClose = true;
-        break;
-
       case JSTRY_FOR_OF:
-        inForOfIterClose = false;
-        break;
-
       case JSTRY_LOOP:
         break;
 
+      // JSTRY_FOR_OF_ITERCLOSE is handled internally by the try note iterator.
       default:
         MOZ_CRASH("Invalid try note");
     }
   }
 
   return SuccessfulReturnContinuation;
 }
 
--- a/js/src/vm/Interpreter.h
+++ b/js/src/vm/Interpreter.h
@@ -315,21 +315,56 @@ class MOZ_STACK_CLASS TryNoteIter {
   uint32_t pcOffset_;
   StackDepthOp getStackDepth_;
 
   const JSTryNote* tn_;
   const JSTryNote* tnEnd_;
 
   void settle() {
     for (; tn_ != tnEnd_; ++tn_) {
-      /* If pc is out of range, try the next one. */
-      if (pcOffset_ - tn_->start >= tn_->length) {
+      if (!pcInRange()) {
         continue;
       }
 
+      /*  Try notes cannot be disjoint. That is, we can't have
+       *  multiple notes with disjoint pc ranges jumping to the same
+       *  catch block. This interacts awkwardly with for-of loops, in
+       *  which calls to 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. If IteratorClose throws, we don't want to
+       *  catch it here.
+       *
+       *  To make this work, we use JSTRY_FOR_OF_ITERCLOSE try-notes,
+       *  which cover the range of the abnormal completion. When
+       *  looking up trynotes, a for-of iterclose note indicates that
+       *  the enclosing for-of has just been terminated. As a result,
+       *  trynotes within that for-of are no longer active. When we
+       *  see a for-of-iterclose, we skip ahead in the trynotes list
+       *  until we see the matching for-of.
+       */
+      if (tn_->kind == JSTRY_FOR_OF_ITERCLOSE) {
+        do {
+          ++tn_;
+          MOZ_ASSERT(tn_ != tnEnd_);
+          MOZ_ASSERT_IF(pcInRange(), tn_->kind != JSTRY_FOR_OF_ITERCLOSE);
+        } while (!(pcInRange() && tn_->kind == JSTRY_FOR_OF));
+
+        // Advance to trynote following the enclosing for-of.
+        ++tn_;
+      }
+
       /*
        * We have a note that covers the exception pc but we must check
        * whether the interpreter has already executed the corresponding
        * handler. This is possible when the executed bytecode implements
        * break or return from inside a for-in loop.
        *
        * In this case the emitter generates additional [enditer] and [gosub]
        * opcodes to close all outstanding iterators and execute the finally
@@ -339,18 +374,18 @@ class MOZ_STACK_CLASS TryNoteIter {
        * invoked the finally blocks.
        *
        * To address this, we make [enditer] always decrease the stack even
        * when its implementation throws an exception. Thus already executed
        * [enditer] and [gosub] opcodes will have try notes with the stack
        * depth exceeding the current one and this condition is what we use to
        * filter them out.
        */
-      if (tn_->stackDepth <= getStackDepth_()) {
-        break;
+      if (tn_ == tnEnd_ || tn_->stackDepth <= getStackDepth_()) {
+        return;
       }
     }
   }
 
  public:
   TryNoteIter(JSContext* cx, JSScript* script, jsbytecode* pc,
               StackDepthOp getStackDepth)
       : script_(cx, script),
@@ -368,16 +403,25 @@ class MOZ_STACK_CLASS TryNoteIter {
     settle();
   }
 
   void operator++() {
     ++tn_;
     settle();
   }
 
+  bool pcInRange() const {
+    // This checks both ends of the range at once
+    // because unsigned integers wrap on underflow.
+    uint32_t offset = pcOffset_;
+    uint32_t start = tn_->start;
+    uint32_t length = tn_->length;
+    return offset - start < length;
+
+  }
   bool done() const { return tn_ == tnEnd_; }
   const JSTryNote* operator*() const { return tn_; }
 };
 
 bool HandleClosingGeneratorReturn(JSContext* cx, AbstractFramePtr frame,
                                   bool ok);
 
 /************************************************************************/