Bug 986767 - Fix adjusting stepModeCount when removing a debuggee global from inside the onStep handler. (r=jimb)
authorShu-yu Guo <shu@rfrn.org>
Thu, 24 Apr 2014 01:59:37 -0700
changeset 180265 a1354a3e748efbbdf62fa2b93b95eb80a4857324
parent 180264 a19a7c0a4b04602d66c3ecf3c956a6cee8ba6559
child 180266 c92f83e9a8640d2db1f0db49879caad5a937055a
push id272
push userpvanderbeken@mozilla.com
push dateMon, 05 May 2014 16:31:18 +0000
reviewersjimb
bugs986767
milestone31.0a1
Bug 986767 - Fix adjusting stepModeCount when removing a debuggee global from inside the onStep handler. (r=jimb)
js/src/jsscript.cpp
js/src/jsscript.h
js/src/vm/Debugger.cpp
--- a/js/src/jsscript.cpp
+++ b/js/src/jsscript.cpp
@@ -3153,69 +3153,73 @@ JSScript::ensureHasDebugScript(JSContext
         if (iter.activation()->isInterpreter())
             iter.activation()->asInterpreter()->enableInterruptsIfRunning(this);
     }
 
     return true;
 }
 
 void
-JSScript::recompileForStepMode(FreeOp *fop)
+JSScript::setNewStepMode(FreeOp *fop, uint32_t newValue)
 {
-#ifdef JS_ION
-    if (hasBaselineScript())
-        baseline->toggleDebugTraps(this, nullptr);
-#endif
-}
-
-bool
-JSScript::tryNewStepMode(JSContext *cx, uint32_t newValue)
-{
-    JS_ASSERT(hasDebugScript_);
-
     DebugScript *debug = debugScript();
     uint32_t prior = debug->stepMode;
     debug->stepMode = newValue;
 
     if (!prior != !newValue) {
-        /* Step mode has been enabled or disabled. Alert the methodjit. */
-        recompileForStepMode(cx->runtime()->defaultFreeOp());
+#ifdef JS_ION
+        if (hasBaselineScript())
+            baseline->toggleDebugTraps(this, nullptr);
+#endif
 
         if (!stepModeEnabled() && !debug->numSites)
-            js_free(releaseDebugScript());
+            fop->free_(releaseDebugScript());
     }
-
-    return true;
 }
 
 bool
 JSScript::setStepModeFlag(JSContext *cx, bool step)
 {
     if (!ensureHasDebugScript(cx))
         return false;
 
-    return tryNewStepMode(cx, (debugScript()->stepMode & stepCountMask) |
-                               (step ? stepFlagMask : 0));
+    setNewStepMode(cx->runtime()->defaultFreeOp(),
+                   (debugScript()->stepMode & stepCountMask) |
+                   (step ? stepFlagMask : 0));
+    return true;
 }
 
 bool
-JSScript::changeStepModeCount(JSContext *cx, int delta)
+JSScript::incrementStepModeCount(JSContext *cx)
 {
+    assertSameCompartment(cx, this);
+    MOZ_ASSERT(cx->compartment()->debugMode());
+
     if (!ensureHasDebugScript(cx))
         return false;
 
-    assertSameCompartment(cx, this);
-    JS_ASSERT_IF(delta > 0, cx->compartment()->debugMode());
-
     DebugScript *debug = debugScript();
     uint32_t count = debug->stepMode & stepCountMask;
-    JS_ASSERT(((count + delta) & stepCountMask) == count + delta);
-    return tryNewStepMode(cx,
-                          (debug->stepMode & stepFlagMask) |
-                          ((count + delta) & stepCountMask));
+    MOZ_ASSERT(((count + 1) & stepCountMask) == count + 1);
+
+    setNewStepMode(cx->runtime()->defaultFreeOp(),
+                   (debug->stepMode & stepFlagMask) |
+                   ((count + 1) & stepCountMask));
+    return true;
+}
+
+void
+JSScript::decrementStepModeCount(FreeOp *fop)
+{
+    DebugScript *debug = debugScript();
+    uint32_t count = debug->stepMode & stepCountMask;
+
+    setNewStepMode(fop,
+                   (debug->stepMode & stepFlagMask) |
+                   ((count - 1) & stepCountMask));
 }
 
 BreakpointSite *
 JSScript::getOrCreateBreakpointSite(JSContext *cx, jsbytecode *pc)
 {
     if (!ensureHasDebugScript(cx))
         return nullptr;
 
--- a/js/src/jsscript.h
+++ b/js/src/jsscript.h
@@ -1499,24 +1499,18 @@ class JSScript : public js::gc::Barriere
         return JSOp(*pc) == JSOP_RETRVAL;
     }
 
     bool varIsAliased(uint32_t varSlot);
     bool formalIsAliased(unsigned argSlot);
     bool formalLivesInArgumentsObject(unsigned argSlot);
 
   private:
-    /*
-     * Recompile with or without single-stepping support, as directed
-     * by stepModeEnabled().
-     */
-    void recompileForStepMode(js::FreeOp *fop);
-
-    /* Attempt to change this->stepMode to |newValue|. */
-    bool tryNewStepMode(JSContext *cx, uint32_t newValue);
+    /* Change this->stepMode to |newValue|. */
+    void setNewStepMode(js::FreeOp *fop, uint32_t newValue);
 
     bool ensureHasDebugScript(JSContext *cx);
     js::DebugScript *debugScript();
     js::DebugScript *releaseDebugScript();
     void destroyDebugScript(js::FreeOp *fop);
 
   public:
     bool hasBreakpointsAt(jsbytecode *pc);
@@ -1544,18 +1538,21 @@ class JSScript : public js::gc::Barriere
      */
     bool setStepModeFlag(JSContext *cx, bool step);
 
     /*
      * Increment or decrement the single-step count. If the count is non-zero or
      * the flag (set by setStepModeFlag) is set, then the script is in
      * single-step mode. (JSD uses an on/off-style interface; Debugger uses a
      * count-style interface.)
+     *
+     * Only incrementing is fallible, as it could allocate a DebugScript.
      */
-    bool changeStepModeCount(JSContext *cx, int delta);
+    bool incrementStepModeCount(JSContext *cx);
+    void decrementStepModeCount(js::FreeOp *fop);
 
     bool stepModeEnabled() { return hasDebugScript_ && !!debugScript()->stepMode; }
 
 #ifdef DEBUG
     uint32_t stepModeCount() { return hasDebugScript_ ? (debugScript()->stepMode & stepCountMask) : 0; }
 #endif
 
     void finalize(js::FreeOp *fop);
--- a/js/src/vm/Debugger.cpp
+++ b/js/src/vm/Debugger.cpp
@@ -142,18 +142,18 @@ ValueToIdentifier(JSContext *cx, HandleV
     }
     return true;
 }
 
 /*
  * A range of all the Debugger.Frame objects for a particular AbstractFramePtr.
  *
  * FIXME This checks only current debuggers, so it relies on a hack in
- * Debugger::removeDebuggeeGlobal to make sure only current debuggers have Frame
- * objects with .live === true.
+ * Debugger::removeDebuggeeGlobal to make sure only current debuggers
+ * have Frame objects with .live === true.
  */
 class Debugger::FrameRange
 {
     AbstractFramePtr frame;
 
     /* The debuggers in |fp|'s compartment, or nullptr if there are none. */
     GlobalObject::DebuggerVector *debuggers;
 
@@ -549,16 +549,20 @@ Debugger::slowPathOnEnterFrame(JSContext
                 return status;
         }
     }
 
     return JSTRAP_CONTINUE;
 }
 
 static void
+DebuggerFrame_maybeDecrementFrameScriptStepModeCount(FreeOp *fop, AbstractFramePtr frame,
+                                                     JSObject *frameobj);
+
+static void
 DebuggerFrame_freeScriptFrameIterData(FreeOp *fop, JSObject *obj);
 
 /*
  * Handle leaving a frame with debuggers watching. |frameOk| indicates whether
  * the frame is exiting normally or abruptly. Set |cx|'s exception and/or
  * |cx->fp()|'s return value, and return a new success value.
  */
 bool
@@ -625,25 +629,19 @@ Debugger::slowPathOnLeaveFrame(JSContext
      * debugger's onPop handler could have caused another debugger to create its
      * own Debugger.Frame instance.
      */
     for (FrameRange r(frame, global); !r.empty(); r.popFront()) {
         RootedObject frameobj(cx, r.frontFrame());
         Debugger *dbg = r.frontDebugger();
         JS_ASSERT(dbg == Debugger::fromChildJSObject(frameobj));
 
-        DebuggerFrame_freeScriptFrameIterData(cx->runtime()->defaultFreeOp(), frameobj);
-
-        /* If this frame had an onStep handler, adjust the script's count. */
-        if (!frameobj->getReservedSlot(JSSLOT_DEBUGFRAME_ONSTEP_HANDLER).isUndefined() &&
-            !frame.script()->changeStepModeCount(cx, -1))
-        {
-            status = JSTRAP_ERROR;
-            /* Don't exit the loop; we must mark all frames as dead. */
-        }
+        FreeOp *fop = cx->runtime()->defaultFreeOp();
+        DebuggerFrame_freeScriptFrameIterData(fop, frameobj);
+        DebuggerFrame_maybeDecrementFrameScriptStepModeCount(fop, frame, frameobj);
 
         dbg->frames.remove(frame);
     }
 
     /*
      * If this is an eval frame, then from the debugger's perspective the
      * script is about to be destroyed. Remove any breakpoints in it.
      */
@@ -2283,17 +2281,17 @@ Debugger::addDebuggeeGlobal(JSContext *c
 }
 
 void
 Debugger::removeDebuggeeGlobal(FreeOp *fop, GlobalObject *global,
                                GlobalObjectSet::Enum *compartmentEnum,
                                GlobalObjectSet::Enum *debugEnum)
 {
     AutoDebugModeInvalidation invalidate(global->compartment());
-    return removeDebuggeeGlobal(fop, global, invalidate, compartmentEnum, debugEnum);
+    removeDebuggeeGlobal(fop, global, invalidate, compartmentEnum, debugEnum);
 }
 
 void
 Debugger::removeDebuggeeGlobal(FreeOp *fop, GlobalObject *global,
                                AutoDebugModeInvalidation &invalidate,
                                GlobalObjectSet::Enum *compartmentEnum,
                                GlobalObjectSet::Enum *debugEnum)
 {
@@ -2314,18 +2312,20 @@ Debugger::removeDebuggeeGlobal(FreeOp *f
      * Debugger objects that are no longer debugging the relevant global might
      * have live Frame objects. So we take the easy way out and kill them here.
      * This is a bug, since it's observable and contrary to the spec. One
      * possible fix would be to put such objects into a compartment-wide bag
      * which slowPathOnLeaveFrame would have to examine.
      */
     for (FrameMap::Enum e(frames); !e.empty(); e.popFront()) {
         AbstractFramePtr frame = e.front().key();
+        JSObject *frameobj = e.front().value();
         if (&frame.script()->global() == global) {
-            DebuggerFrame_freeScriptFrameIterData(fop, e.front().value());
+            DebuggerFrame_freeScriptFrameIterData(fop, frameobj);
+            DebuggerFrame_maybeDecrementFrameScriptStepModeCount(fop, frame, frameobj);
             e.removeFront();
         }
     }
 
     GlobalObject::DebuggerVector *v = global->getDebuggers();
     Debugger **p;
     for (p = v->begin(); p != v->end(); p++) {
         if (*p == this)
@@ -4038,16 +4038,25 @@ DebuggerFrame_freeScriptFrameIterData(Fr
 {
     AbstractFramePtr frame = AbstractFramePtr::FromRaw(obj->getPrivate());
     if (frame.isScriptFrameIterData())
         fop->delete_((ScriptFrameIter::Data *) frame.raw());
     obj->setPrivate(nullptr);
 }
 
 static void
+DebuggerFrame_maybeDecrementFrameScriptStepModeCount(FreeOp *fop, AbstractFramePtr frame,
+                                                     JSObject *frameobj)
+{
+    /* If this frame has an onStep handler, decrement the script's count. */
+    if (!frameobj->getReservedSlot(JSSLOT_DEBUGFRAME_ONSTEP_HANDLER).isUndefined())
+        frame.script()->decrementStepModeCount(fop);
+}
+
+static void
 DebuggerFrame_finalize(FreeOp *fop, JSObject *obj)
 {
     DebuggerFrame_freeScriptFrameIterData(fop, obj);
 }
 
 const Class DebuggerFrame_class = {
     "Frame", JSCLASS_HAS_PRIVATE | JSCLASS_HAS_RESERVED_SLOTS(JSSLOT_DEBUGFRAME_COUNT),
     JS_PropertyStub, JS_DeletePropertyStub, JS_PropertyStub, JS_StrictPropertyStub,
@@ -4441,22 +4450,24 @@ DebuggerFrame_setOnStep(JSContext *cx, u
     REQUIRE_ARGC("Debugger.Frame.set onStep", 1);
     THIS_FRAME(cx, argc, vp, "set onStep", args, thisobj, frame);
     if (!IsValidHook(args[0])) {
         JS_ReportErrorNumber(cx, js_GetErrorMessage, nullptr, JSMSG_NOT_CALLABLE_OR_UNDEFINED);
         return false;
     }
 
     Value prior = thisobj->getReservedSlot(JSSLOT_DEBUGFRAME_ONSTEP_HANDLER);
-    int delta = !args[0].isUndefined() - !prior.isUndefined();
-    if (delta != 0) {
-        /* Try to adjust this frame's script single-step mode count. */
+    if (!args[0].isUndefined() && prior.isUndefined()) {
+        // Single stepping toggled off->on.
         AutoCompartment ac(cx, frame.scopeChain());
-        if (!frame.script()->changeStepModeCount(cx, delta))
+        if (!frame.script()->incrementStepModeCount(cx))
             return false;
+    } else if (args[0].isUndefined() && !prior.isUndefined()) {
+        // Single stepping toggled on->off.
+        frame.script()->decrementStepModeCount(cx->runtime()->defaultFreeOp());
     }
 
     /* Now that the step mode switch has succeeded, we can install the handler. */
     thisobj->setReservedSlot(JSSLOT_DEBUGFRAME_ONSTEP_HANDLER, args[0]);
     args.rval().setUndefined();
     return true;
 }