Bug 1252464 - Remove FrameRange cray cray in favor of using GCVectors. (r=jimb)
authorShu-yu Guo <shu@rfrn.org>
Thu, 24 Mar 2016 15:42:39 -0700
changeset 290361 27e149c2ecdc284b8a53f7ef14ea744c41aa333d
parent 290360 39fb883bcd1c170c4fc5e80842273898ebb13e67
child 290362 f3339e6a451eeec0b0b1ea26cfa0e1610670f5f0
push id19656
push usergwagner@mozilla.com
push dateMon, 04 Apr 2016 13:43:23 +0000
treeherderb2g-inbound@e99061fde28a [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewersjimb
bugs1252464
milestone48.0a1
Bug 1252464 - Remove FrameRange cray cray in favor of using GCVectors. (r=jimb)
js/src/jit-test/tests/debug/bug1252464.js
js/src/vm/Debugger.cpp
js/src/vm/Debugger.h
new file mode 100644
--- /dev/null
+++ b/js/src/jit-test/tests/debug/bug1252464.js
@@ -0,0 +1,15 @@
+// |jit-test| error: ReferenceError
+
+g = newGlobal();
+dbg = Debugger(g);
+hits = 0;
+dbg.onNewScript = function () hits++;
+assertEq(g.eval("eval('2 + 3')"), 5);
+this.gczeal(hits,1);
+dbg = Debugger(g);
+g.h = function () {
+  var env = dbg.getNewestFrame().environment;
+  dbg =  0;
+  assertThrowsInstanceOf;
+}
+g.eval("h()");
--- a/js/src/vm/Debugger.cpp
+++ b/js/src/vm/Debugger.cpp
@@ -144,121 +144,16 @@ ValueToIdentifier(JSContext* cx, HandleV
         ReportValueErrorFlags(cx, JSREPORT_ERROR, JSMSG_UNEXPECTED_TYPE,
                               JSDVG_SEARCH_STACK, val, nullptr, "not an identifier",
                               nullptr);
         return false;
     }
     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.
- */
-class Debugger::FrameRange
-{
-    AbstractFramePtr frame;
-
-    /* The debuggers in |fp|'s compartment, or nullptr if there are none. */
-    GlobalObject::DebuggerVector* debuggers;
-
-    /*
-     * The index of the front Debugger.Frame's debugger in debuggers.
-     * nextDebugger < debuggerCount if and only if the range is not empty.
-     */
-    size_t debuggerCount, nextDebugger;
-
-    /*
-     * If the range is not empty, this is front Debugger.Frame's entry in its
-     * debugger's frame table.
-     */
-    FrameMap::Ptr entry;
-
-  public:
-    /*
-     * Return a range containing all Debugger.Frame instances referring to
-     * |fp|. |global| is |fp|'s global object; if nullptr or omitted, we
-     * compute it ourselves from |fp|.
-     *
-     * We keep an index into the compartment's debugger list, and a
-     * FrameMap::Ptr into the current debugger's frame map. Thus, if the set of
-     * debuggers in |fp|'s compartment changes, this range becomes invalid.
-     * Similarly, if stack frames are added to or removed from frontDebugger(),
-     * then the range's front is invalid until popFront is called.
-     */
-    explicit FrameRange(AbstractFramePtr frame, GlobalObject* global = nullptr)
-      : frame(frame)
-    {
-        nextDebugger = 0;
-
-        /* Find our global, if we were not given one. */
-        if (!global)
-            global = &frame.script()->global();
-
-        /* The frame and global must match. */
-        MOZ_ASSERT(&frame.script()->global() == global);
-
-        /* Find the list of debuggers we'll iterate over. There may be none. */
-        debuggers = global->getDebuggers();
-        if (debuggers) {
-            debuggerCount = debuggers->length();
-            findNext();
-        } else {
-            debuggerCount = 0;
-        }
-    }
-
-    bool empty() const {
-        return nextDebugger >= debuggerCount;
-    }
-
-    NativeObject* frontFrame() const {
-        MOZ_ASSERT(!empty());
-        return entry->value();
-    }
-
-    Debugger* frontDebugger() const {
-        MOZ_ASSERT(!empty());
-        return (*debuggers)[nextDebugger];
-    }
-
-    /*
-     * Delete the front frame from its Debugger's frame map. After this call,
-     * the range's front is invalid until popFront is called.
-     */
-    void removeFrontFrame() const {
-        MOZ_ASSERT(!empty());
-        frontDebugger()->frames.remove(entry);
-    }
-
-    void popFront() {
-        MOZ_ASSERT(!empty());
-        nextDebugger++;
-        findNext();
-    }
-
-  private:
-    /*
-     * Either make this range refer to the first appropriate Debugger.Frame at
-     * or after nextDebugger, or make it empty.
-     */
-    void findNext() {
-        while (!empty()) {
-            Debugger* dbg = (*debuggers)[nextDebugger];
-            entry = dbg->frames.lookup(frame);
-            if (entry)
-                break;
-            nextDebugger++;
-        }
-    }
-};
-
 class AutoRestoreCompartmentDebugMode
 {
     JSCompartment* comp_;
     unsigned bits_;
 
   public:
     explicit AutoRestoreCompartmentDebugMode(JSCompartment* comp)
       : comp_(comp), bits_(comp->debugModeBits)
@@ -741,54 +636,45 @@ DebuggerFrame_freeScriptFrameIterData(Fr
 /*
  * 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.
  */
 /* static */ bool
 Debugger::slowPathOnLeaveFrame(JSContext* cx, AbstractFramePtr frame, jsbytecode* pc, bool frameOk)
 {
-    Handle<GlobalObject*> global = cx->global();
+    mozilla::DebugOnly<Handle<GlobalObject*>> debuggeeGlobal = cx->global();
+
+    auto frameMapsGuard = MakeScopeExit([&] {
+        // Clean up all Debugger.Frame instances.
+        removeFromFrameMapsAndClearBreakpointsIn(cx, frame);
+    });
 
     // The onPop handler and associated clean up logic should not run multiple
     // times on the same frame. If slowPathOnLeaveFrame has already been
     // called, the frame will not be present in the Debugger frame maps.
-    FrameRange frameRange(frame, global);
-    if (frameRange.empty())
+    Rooted<DebuggerFrameVector> frames(cx, DebuggerFrameVector(cx));
+    if (!getDebuggerFrames(frame, &frames))
+        return false;
+    if (frames.empty())
         return frameOk;
 
-    auto frameMapsGuard = MakeScopeExit([&] {
-        // Clean up all Debugger.Frame instances. This call creates a fresh
-        // FrameRange, as one debugger's onPop handler could have caused another
-        // debugger to create its own Debugger.Frame instance.
-        removeFromFrameMapsAndClearBreakpointsIn(cx, frame);
-    });
-
     /* Save the frame's completion value. */
     JSTrapStatus status;
     RootedValue value(cx);
     Debugger::resultToCompletion(cx, frameOk, frame.returnValue(), &status, &value);
 
     // This path can be hit via unwinding the stack due to over-recursion or
     // OOM. In those cases, don't fire the frames' onPop handlers, because
     // invoking JS will only trigger the same condition. See
     // slowPathOnExceptionUnwind.
     if (!cx->isThrowingOverRecursed() && !cx->isThrowingOutOfMemory()) {
-        /* Build a list of the recipients. */
-        AutoObjectVector frames(cx);
-        for (; !frameRange.empty(); frameRange.popFront()) {
-            if (!frames.append(frameRange.frontFrame())) {
-                cx->clearPendingException();
-                return false;
-            }
-        }
-
         /* For each Debugger.Frame, fire its onPop handler, if any. */
-        for (JSObject** p = frames.begin(); p != frames.end(); p++) {
-            RootedNativeObject frameobj(cx, &(*p)->as<NativeObject>());
+        for (size_t i = 0; i < frames.length(); i++) {
+            HandleNativeObject frameobj = frames[i];
             Debugger* dbg = Debugger::fromChildJSObject(frameobj);
             EnterDebuggeeNoExecute nx(cx, *dbg);
 
             if (dbg->enabled &&
                 !frameobj->getReservedSlot(JSSLOT_DEBUGFRAME_ONPOP_HANDLER).isUndefined())
             {
                 RootedValue handler(cx, frameobj->getReservedSlot(JSSLOT_DEBUGFRAME_ONPOP_HANDLER));
 
@@ -808,17 +694,17 @@ Debugger::slowPathOnLeaveFrame(JSContext
                 RootedValue nextValue(cx);
                 JSTrapStatus nextStatus = dbg->parseResumptionValue(ac, hookOk, rval,
                                                                     frame, pc, &nextValue);
 
                 /*
                  * At this point, we are back in the debuggee compartment, and any error has
                  * been wrapped up as a completion value.
                  */
-                MOZ_ASSERT(cx->compartment() == global->compartment());
+                MOZ_ASSERT(cx->compartment() == debuggeeGlobal->compartment());
                 MOZ_ASSERT(!cx->isExceptionPending());
 
                 /* JSTRAP_CONTINUE means "make no change". */
                 if (nextStatus != JSTRAP_CONTINUE) {
                     status = nextStatus;
                     value = nextValue;
                 }
             }
@@ -1725,25 +1611,19 @@ Debugger::onSingleStep(JSContext* cx, Mu
             return JSTRAP_ERROR;
         cx->clearPendingException();
     }
 
     /*
      * Build list of Debugger.Frame instances referring to this frame with
      * onStep handlers.
      */
-    AutoObjectVector frames(cx);
-    for (FrameRange r(iter.abstractFramePtr()); !r.empty(); r.popFront()) {
-        NativeObject* frame = r.frontFrame();
-        if (!frame->getReservedSlot(JSSLOT_DEBUGFRAME_ONSTEP_HANDLER).isUndefined() &&
-            !frames.append(frame))
-        {
-            return JSTRAP_ERROR;
-        }
-    }
+    Rooted<DebuggerFrameVector> frames(cx, DebuggerFrameVector(cx));
+    if (!getDebuggerFrames(iter.abstractFramePtr(), &frames))
+        return JSTRAP_ERROR;
 
 #ifdef DEBUG
     /*
      * Validate the single-step count on this frame's script, to ensure that
      * we're not receiving traps we didn't ask for. Even when frames is
      * non-empty (and thus we know this trap was requested), do the check
      * anyway, to make sure the count has the correct non-zero value.
      *
@@ -1767,19 +1647,22 @@ Debugger::onSingleStep(JSContext* cx, Mu
                     }
                 }
             }
         }
         MOZ_ASSERT(stepperCount == trappingScript->stepModeCount());
     }
 #endif
 
-    /* Call all the onStep handlers we found. */
-    for (JSObject** p = frames.begin(); p != frames.end(); p++) {
-        RootedNativeObject frame(cx, &(*p)->as<NativeObject>());
+    // Call onStep for frames that have the handler set.
+    for (size_t i = 0; i < frames.length(); i++) {
+        HandleNativeObject frame = frames[i];
+        if (frame->getReservedSlot(JSSLOT_DEBUGFRAME_ONSTEP_HANDLER).isUndefined())
+            continue;
+
         Debugger* dbg = Debugger::fromChildJSObject(frame);
         EnterDebuggeeNoExecute nx(cx, *dbg);
 
         Maybe<AutoCompartment> ac;
         ac.emplace(cx, dbg->object);
 
         const Value& handler = frame->getReservedSlot(JSSLOT_DEBUGFRAME_ONSTEP_HANDLER);
         RootedValue rval(cx);
@@ -2188,18 +2071,17 @@ Debugger::updateExecutionObservabilityOf
                     oldestEnabledFrame = iter.abstractFramePtr();
                     oldestEnabledFrame.setIsDebuggee();
                 }
             } else {
 #ifdef DEBUG
                 // Debugger.Frame lifetimes are managed by the debug epilogue,
                 // so in general it's unsafe to unmark a frame if it has a
                 // Debugger.Frame associated with it.
-                FrameRange r(iter.abstractFramePtr());
-                MOZ_ASSERT(r.empty());
+                MOZ_ASSERT(!inFrameMaps(iter.abstractFramePtr()));
 #endif
                 iter.abstractFramePtr().unsetIsDebuggee();
             }
         }
     }
 
     // See comment in unsetPrevUpToDateUntil.
     if (oldestEnabledFrame) {
@@ -2314,16 +2196,41 @@ Debugger::updateExecutionObservabilityOf
     for (ZoneRange r = obs.zones()->all(); !r.empty(); r.popFront()) {
         if (!UpdateExecutionObservabilityOfScriptsInZone(cx, r.front(), obs, observing))
             return false;
     }
 
     return true;
 }
 
+template <typename FrameFn>
+/* static */ void
+Debugger::forEachDebuggerFrame(AbstractFramePtr frame, FrameFn fn)
+{
+    GlobalObject* global = &frame.script()->global();
+    if (GlobalObject::DebuggerVector* debuggers = global->getDebuggers()) {
+        for (auto p = debuggers->begin(); p != debuggers->end(); p++) {
+            Debugger* dbg = *p;
+            if (FrameMap::Ptr entry = dbg->frames.lookup(frame))
+                fn(entry->value());
+        }
+    }
+}
+
+/* static */ bool
+Debugger::getDebuggerFrames(AbstractFramePtr frame, MutableHandle<DebuggerFrameVector> frames)
+{
+    bool hadOOM = false;
+    forEachDebuggerFrame(frame, [&](NativeObject* frameobj) {
+        if (!hadOOM && !frames.append(frameobj))
+            hadOOM = true;
+    });
+    return !hadOOM;
+}
+
 /* static */ bool
 Debugger::updateExecutionObservability(JSContext* cx, ExecutionObservableSet& obs,
                                        IsObserving observing)
 {
     if (!obs.singleZone() && obs.zones()->empty())
         return true;
 
     // Invalidate scripts first so we can set the needsArgsObj flag on scripts
@@ -5784,75 +5691,75 @@ Debugger::observesScript(JSScript* scrip
 
 /* static */ bool
 Debugger::replaceFrameGuts(JSContext* cx, AbstractFramePtr from, AbstractFramePtr to,
                            ScriptFrameIter& iter)
 {
     auto removeFromDebuggerFramesOnExit = MakeScopeExit([&] {
         // Remove any remaining old entries on exit, as the 'from' frame will
         // be gone. On success, the range will be empty.
-        for (Debugger::FrameRange r(from); !r.empty(); r.popFront()) {
-            r.frontFrame()->setPrivate(nullptr);
-            r.removeFrontFrame();
-        }
+        Debugger::forEachDebuggerFrame(from, [&](NativeObject* frameobj) {
+            frameobj->setPrivate(nullptr);
+            Debugger::fromChildJSObject(frameobj)->frames.remove(from);
+        });
 
         // Rekey missingScopes to maintain Debugger.Environment identity and
         // forward liveScopes to point to the new frame.
         DebugScopes::forwardLiveFrame(cx, from, to);
     });
 
     // Forward live Debugger.Frame objects.
-    for (Debugger::FrameRange r(from); !r.empty(); r.popFront()) {
-        RootedNativeObject frameobj(cx, r.frontFrame());
-        Debugger* dbg = r.frontDebugger();
-        MOZ_ASSERT(dbg == Debugger::fromChildJSObject(frameobj));
+    Rooted<DebuggerFrameVector> frames(cx, DebuggerFrameVector(cx));
+    if (!getDebuggerFrames(from, &frames))
+        return false;
+
+    for (size_t i = 0; i < frames.length(); i++) {
+        HandleNativeObject frameobj = frames[i];
+        Debugger* dbg = Debugger::fromChildJSObject(frameobj);
 
         // Update frame object's ScriptFrameIter::data pointer.
         DebuggerFrame_freeScriptFrameIterData(cx->runtime()->defaultFreeOp(), frameobj);
         ScriptFrameIter::Data* data = iter.copyData();
         if (!data)
             return false;
         frameobj->setPrivate(data);
 
-        // Remove the old frame.
-        r.removeFrontFrame();
+        // Remove old frame.
+        dbg->frames.remove(from);
 
         // Add the frame object with |to| as key.
         if (!dbg->frames.putNew(to, frameobj)) {
             ReportOutOfMemory(cx);
             return false;
         }
     }
 
     return true;
 }
 
 /* static */ bool
 Debugger::inFrameMaps(AbstractFramePtr frame)
 {
-    FrameRange r(frame);
-    return !r.empty();
+    bool foundAny = false;
+    forEachDebuggerFrame(frame, [&](NativeObject* frameobj) { foundAny = true; });
+    return foundAny;
 }
 
 /* static */ void
 Debugger::removeFromFrameMapsAndClearBreakpointsIn(JSContext* cx, AbstractFramePtr frame)
 {
-    Handle<GlobalObject*> global = cx->global();
-
-    for (FrameRange r(frame, global); !r.empty(); r.popFront()) {
-        RootedNativeObject frameobj(cx, r.frontFrame());
-        Debugger* dbg = r.frontDebugger();
-        MOZ_ASSERT(dbg == Debugger::fromChildJSObject(frameobj));
+    forEachDebuggerFrame(frame, [&](NativeObject* frameobj) {
+        Debugger* dbg = Debugger::fromChildJSObject(frameobj);
 
         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.
      */
     if (frame.isEvalFrame()) {
         RootedScript script(cx, frame.script());
         script->clearBreakpointsIn(cx->runtime()->defaultFreeOp(), nullptr, nullptr);
--- a/js/src/vm/Debugger.h
+++ b/js/src/vm/Debugger.h
@@ -473,17 +473,16 @@ class Debugger : private mozilla::Linked
      */
 #ifdef NIGHTLY_BUILD
     uint32_t traceLoggerLastDrainedSize;
     uint32_t traceLoggerLastDrainedIteration;
 #endif
     uint32_t traceLoggerScriptedCallsLastDrainedSize;
     uint32_t traceLoggerScriptedCallsLastDrainedIteration;
 
-    class FrameRange;
     class ScriptQuery;
     class ObjectQuery;
 
     bool addDebuggeeGlobal(JSContext* cx, Handle<GlobalObject*> obj);
     void removeDebuggeeGlobal(FreeOp* fop, GlobalObject* global,
                               WeakGlobalObjectSet::Enum* debugEnum);
 
     /*
@@ -618,16 +617,28 @@ class Debugger : private mozilla::Linked
     static void removeFromFrameMapsAndClearBreakpointsIn(JSContext* cx, AbstractFramePtr frame);
     static bool updateExecutionObservabilityOfFrames(JSContext* cx, const ExecutionObservableSet& obs,
                                                      IsObserving observing);
     static bool updateExecutionObservabilityOfScripts(JSContext* cx, const ExecutionObservableSet& obs,
                                                       IsObserving observing);
     static bool updateExecutionObservability(JSContext* cx, ExecutionObservableSet& obs,
                                              IsObserving observing);
 
+    template <typename FrameFn /* void (NativeObject*) */>
+    static void forEachDebuggerFrame(AbstractFramePtr frame, FrameFn fn);
+
+    /*
+     * Return a vector containing all Debugger.Frame instances referring to
+     * |frame|. |global| is |frame|'s global object; if nullptr or omitted, we
+     * compute it ourselves from |frame|.
+     */
+    using DebuggerFrameVector = GCVector<NativeObject*>;
+    static bool getDebuggerFrames(AbstractFramePtr frame,
+                                  MutableHandle<DebuggerFrameVector> frames);
+
   public:
     static bool ensureExecutionObservabilityOfOsrFrame(JSContext* cx, InterpreterFrame* frame);
 
     // Public for DebuggerScript_setBreakpoint.
     static bool ensureExecutionObservabilityOfScript(JSContext* cx, JSScript* script);
 
     // Whether the Debugger instance needs to observe all non-AOT JS
     // execution of its debugees.