Bug 1130768 - Fix some issues with Baseline exception handler and onExceptionUnwind/onPop hooks. r=shu, a=lmandel
authorJan de Mooij <jdemooij@mozilla.com>
Thu, 12 Feb 2015 12:56:52 +0100
changeset 249800 69ce849146bfd00550183871a4d82661080615a5
parent 249799 74827b8f61ebeba3c6d0d543ef72aed59e84592a
child 249801 02451c7d1558e2967b7f63fa79074d2a2f8bd38e
push id4489
push userraliiev@mozilla.com
push dateMon, 23 Feb 2015 15:17:55 +0000
treeherdermozilla-beta@fd7c3dc24146 [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewersshu, lmandel
bugs1130768
milestone37.0a2
Bug 1130768 - Fix some issues with Baseline exception handler and onExceptionUnwind/onPop hooks. r=shu, a=lmandel
js/src/jit-test/tests/debug/bug1130768.js
js/src/jit/BaselineDebugModeOSR.cpp
js/src/jit/BaselineDebugModeOSR.h
js/src/jit/BaselineFrame.h
js/src/jit/JitFrames.cpp
new file mode 100644
--- /dev/null
+++ b/js/src/jit-test/tests/debug/bug1130768.js
@@ -0,0 +1,12 @@
+// |jit-test| error:foo
+var g = newGlobal();
+g.parent = this;
+g.eval("(" + function() {
+    var dbg = new Debugger(parent);
+    count = 0;
+    dbg.onExceptionUnwind = function(frame) {
+        frame.onPop = function() { if (count++ < 30) frame.eval("x = 3"); };
+    };
+} + ")()");
+Object.defineProperty(this, "x", {set: [].map});
+throw "foo";
--- a/js/src/jit/BaselineDebugModeOSR.cpp
+++ b/js/src/jit/BaselineDebugModeOSR.cpp
@@ -202,17 +202,17 @@ CollectJitStackScripts(JSContext *cx, co
                 // use the BaselineDebugModeOSRInfo on the frame directly to
                 // patch. Indeed, we cannot use iter.returnAddressToFp(), as
                 // it points into the debug mode OSR handler and cannot be
                 // used to look up a corresponding ICEntry.
                 //
                 // See cases F and G in PatchBaselineFramesForDebugMode.
                 if (!entries.append(DebugModeOSREntry(script, info)))
                     return false;
-            } else if (frame->isDebuggerHandlingException()) {
+            } else if (frame->isHandlingException()) {
                 // We are in the middle of handling an exception and the frame
                 // must have an override pc.
                 uint32_t offset = script->pcToOffset(frame->overridePc());
                 if (!entries.append(DebugModeOSREntry(script, offset)))
                     return false;
             } else {
                 // The frame must be settled on a pc with an ICEntry.
                 uint8_t *retAddr = iter.returnAddressToFp();
@@ -330,17 +330,17 @@ SpewPatchBaselineFrameFromExceptionHandl
             js_CodeName[(JSOp)*pc]);
 }
 
 static void
 SpewPatchStubFrame(ICStub *oldStub, ICStub *newStub)
 {
     JitSpew(JitSpew_BaselineDebugModeOSR,
             "Patch   stub %p -> %p on BaselineStub frame (%s)",
-            oldStub, newStub, ICStub::KindString(newStub->kind()));
+            oldStub, newStub, newStub ? ICStub::KindString(newStub->kind()) : "exception handler");
 }
 
 static void
 PatchBaselineFramesForDebugMode(JSContext *cx, const Debugger::ExecutionObservableSet &obs,
                                 const ActivationIterator &activation,
                                 DebugModeOSREntryVector &entries, size_t *start)
 {
     //
@@ -426,17 +426,17 @@ PatchBaselineFramesForDebugMode(JSContex
                 //
                 // We are recompiling on-stack scripts from inside the
                 // exception handler, by way of an onExceptionUnwind
                 // invocation, on a pc without an ICEntry. This means the
                 // frame must have an override pc.
                 //
                 // Patch the resume address to nullptr, to ensure the old
                 // address is not used anywhere.
-                MOZ_ASSERT(iter.baselineFrame()->isDebuggerHandlingException());
+                MOZ_ASSERT(iter.baselineFrame()->isHandlingException());
                 MOZ_ASSERT(iter.baselineFrame()->overridePc() == pc);
                 uint8_t *retAddr = nullptr;
                 SpewPatchBaselineFrameFromExceptionHandler(prev->returnAddress(), retAddr,
                                                            script, pc);
                 DebugModeOSRVolatileJitFrameIterator::forwardLiveIterators(
                     cx, prev->returnAddress(), retAddr);
                 prev->setReturnAddress(retAddr);
                 entryIndex++;
@@ -587,17 +587,17 @@ PatchBaselineFramesForDebugMode(JSContex
             // across recompiling the script, so we don't patch anything for
             // such stub frames. We will return to that handler, which takes
             // care of cleaning up the stub frame.
             //
             // Note that for stub pointers that are already on the C stack
             // (i.e. fallback calls), we need to check for recompilation using
             // DebugModeOSRVolatileStub.
             if (layout->maybeStubPtr()) {
-                MOZ_ASSERT(entry.newStub);
+                MOZ_ASSERT(entry.newStub || prevFrame->isHandlingException());
                 SpewPatchStubFrame(entry.oldStub, entry.newStub);
                 layout->setStubPtr(entry.newStub);
             }
 
             break;
           }
 
           case JitFrame_IonJS: {
@@ -704,31 +704,43 @@ CloneOldBaselineStub(JSContext *cx, Debu
 {
     DebugModeOSREntry &entry = entries[entryIndex];
     if (!entry.oldStub)
         return true;
 
     ICStub *oldStub = entry.oldStub;
     MOZ_ASSERT(ICStub::CanMakeCalls(oldStub->kind()));
 
+    if (entry.frameKind == ICEntry::Kind_Invalid) {
+        // The exception handler can modify the frame's override pc while
+        // unwinding scopes. This is fine, but if we have a stub frame, the code
+        // code below will get confused: the entry's pcOffset doesn't match the
+        // stub that's still on the stack. To prevent that, we just set the new
+        // stub to nullptr as we will never return to this stub frame anyway.
+        entry.newStub = nullptr;
+        return true;
+    }
+
     // Get the new fallback stub from the recompiled baseline script.
     ICFallbackStub *fallbackStub = entry.fallbackStub();
 
     // We don't need to clone fallback stubs, as they are guaranteed to
     // exist. Furthermore, their JitCode is cached and should be the same even
     // across the recompile.
     if (oldStub->isFallback()) {
         MOZ_ASSERT(oldStub->jitCode() == fallbackStub->jitCode());
         entry.newStub = fallbackStub;
         return true;
     }
 
-    // Check if we have already cloned the stub on a younger frame.
+    // Check if we have already cloned the stub on a younger frame. Ignore
+    // frames that entered the exception handler (entries[i].newStub is nullptr
+    // in that case, see above).
     for (size_t i = 0; i < entryIndex; i++) {
-        if (oldStub == entries[i].oldStub) {
+        if (oldStub == entries[i].oldStub && entries[i].frameKind != ICEntry::Kind_Invalid) {
             MOZ_ASSERT(entries[i].newStub);
             entry.newStub = entries[i].newStub;
             return true;
         }
     }
 
     // Some stubs are monitored, get the first stub in the monitor chain from
     // the new fallback stub if so.
--- a/js/src/jit/BaselineDebugModeOSR.h
+++ b/js/src/jit/BaselineDebugModeOSR.h
@@ -48,16 +48,17 @@ class DebugModeOSRVolatileStub
   public:
     DebugModeOSRVolatileStub(BaselineFrame *frame, ICFallbackStub *stub)
       : stub_(static_cast<T>(stub)),
         frame_(frame),
         pcOffset_(stub->icEntry()->pcOffset())
     { }
 
     bool invalid() const {
+        MOZ_ASSERT(!frame_->isHandlingException());
         ICEntry &entry = frame_->script()->baselineScript()->icEntryFromPCOffset(pcOffset_);
         return stub_ != entry.fallbackStub();
     }
 
     operator const T&() const { MOZ_ASSERT(!invalid()); return stub_; }
     T operator->() const { MOZ_ASSERT(!invalid()); return stub_; }
     T *address() { MOZ_ASSERT(!invalid()); return &stub_; }
     const T *address() const { MOZ_ASSERT(!invalid()); return &stub_; }
--- a/js/src/jit/BaselineFrame.h
+++ b/js/src/jit/BaselineFrame.h
@@ -72,21 +72,20 @@ class BaselineFrame
         //
         // There can also be an override pc if the frame has had its scope chain
         // unwound to a pc during exception handling that is different from its
         // current pc.
         //
         // This flag should never be set when we're executing JIT code.
         HAS_OVERRIDE_PC = 1 << 11,
 
-        // Frame has called out to Debugger code from
-        // HandleExceptionBaseline. This is set for debug mode OSR sanity
-        // checking when it handles corner cases which only arise during
-        // exception handling.
-        DEBUGGER_HANDLING_EXCEPTION = 1 << 12
+        // If set, we're handling an exception for this frame. This is set for
+        // debug mode OSR sanity checking when it handles corner cases which
+        // only arise during exception handling.
+        HANDLING_EXCEPTION = 1 << 12
     };
 
   protected: // Silence Clang warning about unused private fields.
     // We need to split the Value into 2 fields of 32 bits, otherwise the C++
     // compiler may add some padding between the fields.
 
     union {
         struct {
@@ -285,24 +284,24 @@ class BaselineFrame
     bool isDebuggee() const {
         return flags_ & DEBUGGEE;
     }
     void setIsDebuggee() {
         flags_ |= DEBUGGEE;
     }
     inline void unsetIsDebuggee();
 
-    bool isDebuggerHandlingException() const {
-        return flags_ & DEBUGGER_HANDLING_EXCEPTION;
+    bool isHandlingException() const {
+        return flags_ & HANDLING_EXCEPTION;
     }
-    void setIsDebuggerHandlingException() {
-        flags_ |= DEBUGGER_HANDLING_EXCEPTION;
+    void setIsHandlingException() {
+        flags_ |= HANDLING_EXCEPTION;
     }
-    void unsetIsDebuggerHandlingException() {
-        flags_ &= ~DEBUGGER_HANDLING_EXCEPTION;
+    void unsetIsHandlingException() {
+        flags_ &= ~HANDLING_EXCEPTION;
     }
 
     JSScript *evalScript() const {
         MOZ_ASSERT(isEvalFrame());
         return evalScript_;
     }
 
     bool hasPushedSPSFrame() const {
--- a/js/src/jit/JitFrames.cpp
+++ b/js/src/jit/JitFrames.cpp
@@ -512,57 +512,50 @@ HandleClosingGeneratorReturn(JSContext *
         if (frame.baselineFrame()->isDebuggee())
             frame.baselineFrame()->setOverridePc(unwoundScopeToPc);
         pc = unwoundScopeToPc;
     }
 
     ForcedReturn(cx, frame, pc, rfe, calledDebugEpilogue);
 }
 
-struct AutoDebuggerHandlingException
+struct AutoBaselineHandlingException
 {
     BaselineFrame *frame;
-    AutoDebuggerHandlingException(BaselineFrame *frame, jsbytecode *pc)
+    AutoBaselineHandlingException(BaselineFrame *frame, jsbytecode *pc)
       : frame(frame)
     {
-        frame->setIsDebuggerHandlingException();
-        frame->setOverridePc(pc); // Will be cleared in HandleException.
+        frame->setIsHandlingException();
+        frame->setOverridePc(pc);
     }
-    ~AutoDebuggerHandlingException() {
-        frame->unsetIsDebuggerHandlingException();
+    ~AutoBaselineHandlingException() {
+        frame->unsetIsHandlingException();
+        frame->clearOverridePc();
     }
 };
 
 static void
 HandleExceptionBaseline(JSContext *cx, const JitFrameIterator &frame, ResumeFromException *rfe,
-                        jsbytecode **unwoundScopeToPc, bool *calledDebugEpilogue)
+                        jsbytecode *pc, jsbytecode **unwoundScopeToPc, bool *calledDebugEpilogue)
 {
     MOZ_ASSERT(frame.isBaselineJS());
     MOZ_ASSERT(!*calledDebugEpilogue);
 
-    RootedScript script(cx);
-    jsbytecode *pc;
-    frame.baselineScriptAndPc(script.address(), &pc);
-
     // 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;
     }
 
     RootedValue exception(cx);
     if (cx->isExceptionPending() && cx->compartment()->isDebuggee() &&
         cx->getPendingException(&exception) && !exception.isMagic(JS_GENERATOR_CLOSING))
     {
-        // Set for debug mode OSR. See note concerning
-        // 'isDebuggerHandlingException' in CollectJitStackScripts.
-        AutoDebuggerHandlingException debuggerHandling(frame.baselineFrame(), pc);
-
         switch (Debugger::onExceptionUnwind(cx, frame.baselineFrame())) {
           case JSTRAP_ERROR:
             // Uncatchable exception.
             MOZ_ASSERT(!cx->isExceptionPending());
             break;
 
           case JSTRAP_CONTINUE:
           case JSTRAP_THROW:
@@ -573,16 +566,18 @@ HandleExceptionBaseline(JSContext *cx, c
             ForcedReturn(cx, frame, pc, rfe, calledDebugEpilogue);
             return;
 
           default:
             MOZ_CRASH("Invalid trap status");
         }
     }
 
+    RootedScript script(cx, frame.baselineFrame()->script());
+
     if (!script->hasTrynotes()) {
         HandleClosingGeneratorReturn(cx, frame, pc, *unwoundScopeToPc, rfe, calledDebugEpilogue);
         return;
     }
 
     JSTryNote *tn = script->trynotes()->vector;
     JSTryNote *tnEnd = tn + script->trynotes()->length;
 
@@ -671,23 +666,16 @@ HandleExceptionBaseline(JSContext *cx, c
 
 struct AutoDeleteDebugModeOSRInfo
 {
     BaselineFrame *frame;
     explicit AutoDeleteDebugModeOSRInfo(BaselineFrame *frame) : frame(frame) { MOZ_ASSERT(frame); }
     ~AutoDeleteDebugModeOSRInfo() { frame->deleteDebugModeOSRInfo(); }
 };
 
-struct AutoClearBaselineOverridePc
-{
-    BaselineFrame *frame;
-    explicit AutoClearBaselineOverridePc(BaselineFrame *frame) : frame(frame) { MOZ_ASSERT(frame); }
-    ~AutoClearBaselineOverridePc() { frame->clearOverridePc(); }
-};
-
 void
 HandleException(ResumeFromException *rfe)
 {
     JSContext *cx = GetJSContextFromJitCode();
     TraceLoggerThread *logger = TraceLoggerForMainThread(cx->runtime());
 
     rfe->kind = ResumeFromException::RESUME_ENTRY_FRAME;
 
@@ -769,36 +757,38 @@ HandleException(ResumeFromException *rfe
 
         } else if (iter.isBaselineJS()) {
             // It's invalid to call DebugEpilogue twice for the same frame.
             bool calledDebugEpilogue = false;
 
             // Remember the pc we unwound the scope to.
             jsbytecode *unwoundScopeToPc = nullptr;
 
-            // Clear the frame's override pc when we leave this block. This is
-            // fine because we're either:
+            // Set a flag on the frame to signal to DebugModeOSR that we're
+            // handling an exception. Also ensure the frame has an override
+            // pc. We clear the frame's override pc when we leave this block,
+            // this is fine because we're either:
+            //
             // (1) Going to enter a catch or finally block. We don't want to
             //     keep the old pc when we're executing JIT code.
             // (2) Going to pop the frame, either here or a forced return.
             //     In this case nothing will observe the frame's pc.
             // (3) Performing an exception bailout. In this case
             //     FinishBailoutToBaseline will set the pc to the resume pc
             //     and clear it before it returns to JIT code.
-            AutoClearBaselineOverridePc clearPc(iter.baselineFrame());
-
-            HandleExceptionBaseline(cx, iter, rfe, &unwoundScopeToPc, &calledDebugEpilogue);
+            jsbytecode *pc;
+            iter.baselineScriptAndPc(nullptr, &pc);
+            AutoBaselineHandlingException handlingException(iter.baselineFrame(), pc);
+
+            HandleExceptionBaseline(cx, iter, rfe, pc, &unwoundScopeToPc, &calledDebugEpilogue);
 
             // If we are propagating an exception through a frame with
             // on-stack recompile info, we should free the allocated
             // RecompileInfo struct before we leave this block, as we will not
             // be returning to the recompile handler.
-            //
-            // We cannot delete it immediately because of the call to
-            // iter.baselineScriptAndPc below.
             AutoDeleteDebugModeOSRInfo deleteDebugModeOSRInfo(iter.baselineFrame());
 
             if (rfe->kind != ResumeFromException::RESUME_ENTRY_FRAME)
                 return;
 
             TraceLogStopEvent(logger, TraceLogger_Baseline);
             TraceLogStopEvent(logger, TraceLogger_Scripts);