Bug 677386 - Fix jsdbg2 breakpoint GC rules. r=billm.
authorJason Orendorff <jorendorff@mozilla.com>
Wed, 24 Aug 2011 18:42:19 -0500
changeset 75862 db8de6cd0712f9651b39c46992fe07963f9a12a1
parent 75861 6daedd1baec4267352979c3e42d4e1ea64206d19
child 75863 dddbe27e21a817be7907727196654dd2b877747d
push id21064
push usermak77@bonardo.net
push dateThu, 25 Aug 2011 11:26:55 +0000
treeherdermozilla-central@f6b61dde6a15 [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewersbillm
bugs677386
milestone9.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 677386 - Fix jsdbg2 breakpoint GC rules. r=billm.
js/src/jit-test/tests/debug/breakpoint-gc-04.js
js/src/jit-test/tests/debug/breakpoint-gc-05.js
js/src/jscompartment.cpp
js/src/jscompartment.h
js/src/vm/Debugger.cpp
js/src/vm/Debugger.h
new file mode 100644
--- /dev/null
+++ b/js/src/jit-test/tests/debug/breakpoint-gc-04.js
@@ -0,0 +1,23 @@
+// Enabled debuggers keep breakpoint handlers alive.
+
+var g = newGlobal('new-compartment');
+g.eval("var line0 = Error().lineNumber;\n" +
+       "function f() {\n" +     // line0 + 1
+       "    return 2;\n" +      // line0 + 2
+       "}\n");
+var N = 4;
+var hits = 0;
+for (var i = 0; i < N; i++) {
+    var dbg = Debugger(g);
+    dbg.onDebuggerStatement = function (frame) {
+        var handler = {hit: function () { hits++; }};
+        var s = frame.eval("f").return.script;
+        var offs = s.getLineOffsets(g.line0 + 2);
+        for (var j = 0; j < offs.length; j++)
+            s.setBreakpoint(offs[j], handler);
+    };
+}
+g.eval('debugger;');
+gc({});
+assertEq(g.f(), 2);
+assertEq(hits, N);
new file mode 100644
--- /dev/null
+++ b/js/src/jit-test/tests/debug/breakpoint-gc-05.js
@@ -0,0 +1,25 @@
+// Disabled debuggers keep breakpoint handlers alive.
+
+var g = newGlobal('new-compartment');
+g.eval("var line0 = Error().lineNumber;\n" +
+       "function f() {\n" +     // line0 + 1
+       "    return 2;\n" +      // line0 + 2
+       "}\n");
+var N = 4;
+var hits = 0;
+for (var i = 0; i < N; i++) {
+    var dbg = Debugger(g);
+    dbg.onDebuggerStatement = function (frame) {
+        var handler = {hit: function () { hits++; }};
+        var s = frame.eval("f").return.script;
+        var offs = s.getLineOffsets(g.line0 + 2);
+        for (var j = 0; j < offs.length; j++)
+            s.setBreakpoint(offs[j], handler);
+    };
+}
+g.eval('debugger;');
+dbg.enabled = false;
+gc({});
+dbg.enabled = true;
+assertEq(g.f(), 2);
+assertEq(hits, N);
--- a/js/src/jscompartment.cpp
+++ b/js/src/jscompartment.cpp
@@ -799,17 +799,17 @@ JSCompartment::clearTraps(JSContext *cx,
     for (BreakpointSiteMap::Enum e(breakpointSites); !e.empty(); e.popFront()) {
         BreakpointSite *site = e.front().value;
         if (!script || site->script == script)
             site->clearTrap(cx, &e);
     }
 }
 
 bool
-JSCompartment::markBreakpointsIteratively(JSTracer *trc)
+JSCompartment::markTrapClosuresIteratively(JSTracer *trc)
 {
     bool markedAny = false;
     JSContext *cx = trc->context;
     for (BreakpointSiteMap::Range r = breakpointSites.all(); !r.empty(); r.popFront()) {
         BreakpointSite *site = r.front().value;
 
         // Mark jsdbgapi state if any. But if we know the scriptObject, put off
         // marking trap state until we know the scriptObject is live.
@@ -818,36 +818,16 @@ JSCompartment::markBreakpointsIterativel
         {
             if (site->trapClosure.isMarkable() &&
                 IsAboutToBeFinalized(cx, site->trapClosure.toGCThing()))
             {
                 markedAny = true;
             }
             MarkValue(trc, site->trapClosure, "trap closure");
         }
-
-        // Mark js::Debugger breakpoints. If either the debugger or the script is
-        // collected, then the breakpoint is collected along with it. So do not
-        // mark the handler in that case.
-        //
-        // If scriptObject is non-null, examine it to see if the script will be
-        // collected. If scriptObject is null, then site->script is an eval
-        // script on the stack, so it is definitely live.
-        //
-        if (!site->scriptObject || !IsAboutToBeFinalized(cx, site->scriptObject)) {
-            for (Breakpoint *bp = site->firstBreakpoint(); bp; bp = bp->nextInSite()) {
-                if (!IsAboutToBeFinalized(cx, bp->debugger->toJSObject()) &&
-                    bp->handler &&
-                    IsAboutToBeFinalized(cx, bp->handler))
-                {
-                    MarkObject(trc, *bp->handler, "breakpoint handler");
-                    markedAny = true;
-                }
-            }
-        }
     }
     return markedAny;
 }
 
 void
 JSCompartment::sweepBreakpoints(JSContext *cx)
 {
     for (BreakpointSiteMap::Enum e(breakpointSites); !e.empty(); e.popFront()) {
--- a/js/src/jscompartment.h
+++ b/js/src/jscompartment.h
@@ -606,17 +606,17 @@ struct JS_FRIEND_API(JSCompartment) {
                         js::GlobalObjectSet::Enum *debuggeesEnum = NULL);
     bool setDebugModeFromC(JSContext *cx, bool b);
 
     js::BreakpointSite *getBreakpointSite(jsbytecode *pc);
     js::BreakpointSite *getOrCreateBreakpointSite(JSContext *cx, JSScript *script, jsbytecode *pc,
                                                   JSObject *scriptObject);
     void clearBreakpointsIn(JSContext *cx, js::Debugger *dbg, JSScript *script, JSObject *handler);
     void clearTraps(JSContext *cx, JSScript *script);
-    bool markBreakpointsIteratively(JSTracer *trc);
+    bool markTrapClosuresIteratively(JSTracer *trc);
 
   private:
     void sweepBreakpoints(JSContext *cx);
 
   public:
     js::WatchpointMap *watchpointMap;
 };
 
--- a/js/src/vm/Debugger.cpp
+++ b/js/src/vm/Debugger.cpp
@@ -382,29 +382,40 @@ JSObject *
 Debugger::getHook(Hook hook) const
 {
     JS_ASSERT(hook >= 0 && hook < HookCount);
     const Value &v = object->getReservedSlot(JSSLOT_DEBUG_HOOK_START + hook);
     return v.isUndefined() ? NULL : &v.toObject();
 }
 
 bool
-Debugger::hasAnyLiveHooks() const
+Debugger::hasAnyLiveHooks(JSContext *cx) const
 {
     if (!enabled)
         return false;
 
     if (getHook(OnDebuggerStatement) ||
         getHook(OnExceptionUnwind) ||
         getHook(OnNewScript) ||
         getHook(OnEnterFrame))
+    {
         return true;
-
-    if (!JS_CLIST_IS_EMPTY(&breakpoints))
-        return true;
+    }
+
+    /* If any breakpoints are in live scripts, return true. */
+    for (Breakpoint *bp = firstBreakpoint(); bp; bp = bp->nextInDebugger()) {
+        /*
+         * If holder is non-null, examine it to see if the script will be
+         * collected. If holder is null, then bp->site->script is an eval
+         * script on the stack, so it is definitely live.
+         */
+        JSObject *holder = bp->site->getScriptObject();
+        if (!holder || !IsAboutToBeFinalized(cx, holder))
+            return true;
+    }
 
     for (FrameMap::Range r = frames.all(); !r.empty(); r.popFront()) {
         if (!r.front().value->getReservedSlot(JSSLOT_DEBUGFRAME_ONSTEP_HANDLER).isUndefined())
             return true;
     }
 
     return false;
 }
@@ -1071,63 +1082,86 @@ Debugger::markCrossCompartmentDebuggerOb
  * returns false.
  */
 bool
 Debugger::markAllIteratively(GCMarker *trc, JSGCInvocationKind gckind)
 {
     bool markedAny = false;
 
     /*
-     * We must find all Debugger objects in danger of GC. This code is a little
+     * Find all Debugger objects in danger of GC. This code is a little
      * convoluted since the easiest way to find them is via their debuggees.
      */
-    JSRuntime *rt = trc->context->runtime;
+    JSContext *cx = trc->context;
+    JSRuntime *rt = cx->runtime;
     JSCompartment *comp = rt->gcCurrentCompartment;
     for (JSCompartment **c = rt->compartments.begin(); c != rt->compartments.end(); c++) {
         JSCompartment *dc = *c;
 
-        /* If dc is being collected, mark breakpoint handlers in it. */
+        /* If dc is being collected, mark jsdbgapi.h trap closures in it. */
         if (!comp || dc == comp)
-            markedAny = markedAny | dc->markBreakpointsIteratively(trc);
+            markedAny = markedAny | dc->markTrapClosuresIteratively(trc);
 
         /*
          * If this is a single-compartment GC, no compartment can debug itself, so skip
-         * |comp|. If it's a global GC, then search every live compartment.
+         * |comp|. If it's a global GC, then search every compartment.
          */
         if (comp && dc == comp)
             continue;
 
         const GlobalObjectSet &debuggees = dc->getDebuggees();
         for (GlobalObjectSet::Range r = debuggees.all(); !r.empty(); r.popFront()) {
             GlobalObject *global = r.front();
 
             /*
              * Every debuggee has at least one debugger, so in this case
              * getDebuggers can't return NULL.
              */
             const GlobalObject::DebuggerVector *debuggers = global->getDebuggers();
             JS_ASSERT(debuggers);
             for (Debugger **p = debuggers->begin(); p != debuggers->end(); p++) {
                 Debugger *dbg = *p;
-                JSObject *obj = dbg->toJSObject();
 
                 /*
                  * dbg is a Debugger with at least one debuggee. Check three things:
                  *   - dbg is actually in a compartment being GC'd
                  *   - it isn't already marked
                  *   - it actually has hooks that might be called
-                 * IsAboutToBeFinalized covers the first two criteria.
                  */
-                if (IsAboutToBeFinalized(trc->context, obj) && dbg->hasAnyLiveHooks()) {
+                JSObject *dbgobj = dbg->toJSObject();
+                if (comp && comp != dbgobj->compartment())
+                    continue;
+
+                bool dbgMarked = !IsAboutToBeFinalized(cx, dbgobj);
+                if (!dbgMarked && dbg->hasAnyLiveHooks(cx)) {
                     /*
                      * obj could be reachable only via its live, enabled
                      * debugger hooks, which may yet be called.
                      */
-                    MarkObject(trc, *obj, "enabled Debugger");
+                    MarkObject(trc, *dbgobj, "enabled Debugger");
                     markedAny = true;
+                    dbgMarked = true;
+                }
+
+                if (dbgMarked) {
+                    /* Search for breakpoints to mark. */
+                    for (Breakpoint *bp = dbg->firstBreakpoint(); bp; bp = bp->nextInDebugger()) {
+                        JSObject *scriptObject = bp->site->getScriptObject();
+                        if (!scriptObject || !IsAboutToBeFinalized(cx, scriptObject)) {
+                            /*
+                             * The debugger and the script are both live.
+                             * Therefore the breakpoint handler is live.
+                             */
+                            JSObject *handler = bp->getHandler();
+                            if (IsAboutToBeFinalized(cx, handler)) {
+                                MarkObject(trc, *bp->getHandler(), "breakpoint handler");
+                                markedAny = true;
+                            }
+                        }
+                    }
                 }
             }
         }
     }
     return markedAny;
 }
 
 void
--- a/js/src/vm/Debugger.h
+++ b/js/src/vm/Debugger.h
@@ -221,17 +221,17 @@ class Debugger {
     static JSBool getDebuggees(JSContext *cx, uintN argc, Value *vp);
     static JSBool getNewestFrame(JSContext *cx, uintN argc, Value *vp);
     static JSBool clearAllBreakpoints(JSContext *cx, uintN argc, Value *vp);
     static JSBool construct(JSContext *cx, uintN argc, Value *vp);
     static JSPropertySpec properties[];
     static JSFunctionSpec methods[];
 
     JSObject *getHook(Hook hook) const;
-    bool hasAnyLiveHooks() const;
+    bool hasAnyLiveHooks(JSContext *cx) const;
 
     static void slowPathOnEnterFrame(JSContext *cx);
     static void slowPathOnLeaveFrame(JSContext *cx);
     static void slowPathOnNewScript(JSContext *cx, JSScript *script, JSObject *obj,
                                     NewScriptKind kind);
     static void slowPathOnDestroyScript(JSScript *script);
 
     static JSTrapStatus dispatchHook(JSContext *cx, js::Value *vp, Hook which);
@@ -283,19 +283,19 @@ class Debugger {
      *   * it is in the middle of dispatching an event (the event dispatching
      *     code roots it in this case); OR
      *   * it is enabled, and it is debugging at least one live compartment,
      *     and at least one of the following is true:
      *       - it has a debugger hook installed
      *       - it has a breakpoint set on a live script
      *       - it has a watchpoint set on a live object.
      *
-     * Debugger::mark handles the last case. If it finds any Debugger objects
-     * that are definitely live but not yet marked, it marks them and returns
-     * true. If not, it returns false.
+     * Debugger::markAllIteratively handles the last case. If it finds any
+     * Debugger objects that are definitely live but not yet marked, it marks
+     * them and returns true. If not, it returns false.
      */
     static void markCrossCompartmentDebuggerObjectReferents(JSTracer *tracer);
     static bool markAllIteratively(GCMarker *trc, JSGCInvocationKind gckind);
     static void sweepAll(JSContext *cx);
     static void detachAllDebuggersFromGlobal(JSContext *cx, GlobalObject *global,
                                              GlobalObjectSet::Enum *compartmentEnum);
 
     static inline void onEnterFrame(JSContext *cx);
@@ -426,25 +426,44 @@ class BreakpointSite {
 
     bool recompile(JSContext *cx, bool forTrap);
 
   public:
     BreakpointSite(JSScript *script, jsbytecode *pc);
     Breakpoint *firstBreakpoint() const;
     bool hasBreakpoint(Breakpoint *bp);
     bool hasTrap() const { return !!trapHandler; }
+    JSObject *getScriptObject() const { return scriptObject; }
 
     bool inc(JSContext *cx);
     void dec(JSContext *cx);
     bool setTrap(JSContext *cx, JSTrapHandler handler, const Value &closure);
     void clearTrap(JSContext *cx, BreakpointSiteMap::Enum *e = NULL,
                    JSTrapHandler *handlerp = NULL, Value *closurep = NULL);
     void destroyIfEmpty(JSRuntime *rt, BreakpointSiteMap::Enum *e);
 };
 
+/*
+ * Each Breakpoint is a member of two linked lists: its debugger's list and its
+ * site's list.
+ *
+ * GC rules:
+ *   - script is live, breakpoint exists, and debugger is enabled
+ *      ==> debugger is live
+ *   - script is live, breakpoint exists, and debugger is live
+ *      ==> retain the breakpoint and the handler object is live
+ *
+ * Debugger::markAllIteratively implements these two rules. It uses
+ * Debugger::hasAnyLiveHooks to check for rule 1.
+ *
+ * Nothing else causes a breakpoint to be retained, so if its script or
+ * debugger is collected, the breakpoint is destroyed during GC sweep phase,
+ * even if the debugger compartment isn't being GC'd. This is implemented in
+ * JSCompartment::sweepBreakpoints.
+ */
 class Breakpoint {
     friend class ::JSCompartment;
     friend class js::Debugger;
 
   public:
     Debugger * const debugger;
     BreakpointSite * const site;
   private: