Bug 394709: fixing memory leak with watch handlers. r+a=brendan

js/src/jsdbgapi.c

@@ -386,7 +386,6 @@ DropWatchPointAndUnlock(JSContext *cx, JSWatchPoint *wp, uintN flag)
         }
     }
 
-    js_RemoveRoot(cx->runtime, &wp->closure);
     JS_free(cx, wp);
     return ok;
 }
@@ -397,7 +396,7 @@ DropWatchPointAndUnlock(JSContext *cx, JSWatchPoint *wp, uintN flag)
  * respect the request model).
  */
 void
-js_TraceWatchPoints(JSTracer *trc)
+js_TraceWatchPoints(JSTracer *trc, JSObject *obj)
 {
     JSRuntime *rt;
     JSWatchPoint *wp;
@@ -407,12 +406,46 @@ js_TraceWatchPoints(JSTracer *trc)
     for (wp = (JSWatchPoint *)rt->watchPointList.next;
          wp != (JSWatchPoint *)&rt->watchPointList;
          wp = (JSWatchPoint *)wp->links.next) {
+        if (wp->object == obj) {
             TRACE_SCOPE_PROPERTY(trc, wp->sprop);
-        if ((wp->sprop->attrs & JSPROP_SETTER) && wp->setter)
-            JS_CALL_OBJECT_TRACER(trc, (JSObject *)wp->setter, "wp->setter");
+            if ((wp->sprop->attrs & JSPROP_SETTER) && wp->setter) {
+                JS_CALL_OBJECT_TRACER(trc, (JSObject *)wp->setter,
+                                      "wp->setter");
+            }
+            JS_SET_TRACING_NAME(trc, "wp->closure");
+            js_CallValueTracerIfGCThing(trc, (jsval) wp->closure);
+        }
     }
 }
 
+void
+js_SweepWatchPoints(JSContext *cx)
+{
+    JSRuntime *rt;
+    JSWatchPoint *wp, *next;
+    uint32 sample;
+
+    rt = cx->runtime;
+    DBG_LOCK(rt);
+    for (wp = (JSWatchPoint *)rt->watchPointList.next;
+         wp != (JSWatchPoint *)&rt->watchPointList;
+         wp = next) {
+        next = (JSWatchPoint *)wp->links.next;
+        if (js_IsAboutToBeFinalized(cx, wp->object)) {
+            sample = rt->debuggerMutations;
+
+            /* Ignore failures. */
+            DropWatchPointAndUnlock(cx, wp, JSWP_LIVE);
+            DBG_LOCK(rt);
+            if (rt->debuggerMutations != sample + 1)
+                next = (JSWatchPoint *)rt->watchPointList.next;
+        }
+    }
+    DBG_UNLOCK(rt);
+}
+
+
+
 /*
  * NB: FindWatchPoint must be called with rt->debuggerLock acquired.
  */
@@ -734,11 +767,6 @@ JS_SetWatchPoint(JSContext *cx, JSObject *obj, jsval id,
         }
         wp->handler = NULL;
         wp->closure = NULL;
-        ok = js_AddRoot(cx, &wp->closure, "wp->closure");
-        if (!ok) {
-            JS_free(cx, wp);
-            goto out;
-        }
         wp->object = obj;
         JS_ASSERT(sprop->setter != js_watch_set || pobj != obj);
         wp->setter = sprop->setter;

js/src/jsdbgapi.h

@@ -99,7 +99,10 @@ JS_ClearAllWatchPoints(JSContext *cx);
  * header file "jsconfig.h" has been included.
  */
 extern void
-js_TraceWatchPoints(JSTracer *trc);
+js_TraceWatchPoints(JSTracer *trc, JSObject *obj);
+
+extern void
+js_SweepWatchPoints(JSContext *cx);
 
 extern JSScopeProperty *
 js_FindWatchPoint(JSRuntime *rt, JSScope *scope, jsid id);

js/src/jsgc.c

@@ -2088,7 +2088,6 @@ js_TraceRuntime(JSTracer *trc, JSBool allAtoms)
     if (rt->gcLocksHash)
         JS_DHashTableEnumerate(rt->gcLocksHash, gc_lock_traversal, trc);
     js_TraceAtomState(trc, allAtoms);
-    js_TraceWatchPoints(trc);
     js_TraceNativeIteratorStates(trc);
 
     iter = NULL;
@@ -2339,17 +2338,6 @@ restart:
 
     rt->gcMarkingTracer = NULL;
 
-    /* Finalize iterator states before the objects they iterate over. */
-    CloseNativeIterators(cx);
-
-#ifdef DUMP_CALL_TABLE
-    /*
-     * Call js_DumpCallTable here so it can meter and then clear weak refs to
-     * GC-things that are about to be finalized.
-     */
-    js_DumpCallTable(cx);
-#endif
-
     /*
      * Sweep phase.
      *
@@ -2366,6 +2354,20 @@ restart:
      */
     js_SweepAtomState(cx);
 
+    /* Finalize iterator states before the objects they iterate over. */
+    CloseNativeIterators(cx);
+
+    /* Finalize watch points associated with unreachable objects. */
+    js_SweepWatchPoints(cx);
+
+#ifdef DUMP_CALL_TABLE
+    /*
+     * Call js_DumpCallTable here so it can meter and then clear weak refs to
+     * GC-things that are about to be finalized.
+     */
+    js_DumpCallTable(cx);
+#endif
+
     /*
      * Here we need to ensure that JSObject instances are finalized before GC-
      * allocated JSString and jsdouble instances so object's finalizer can

js/src/jsobj.c

@@ -2791,9 +2791,6 @@ js_FinalizeObject(JSContext *cx, JSObject *obj)
                                    cx->debugHooks->objectHookData);
     }
 
-    /* Remove all watchpoints with weak links to obj. */
-    JS_ClearWatchPointsForObject(cx, obj);
-
     /* Finalize obj first, in case it needs map and slots. */
     GC_AWARE_GET_CLASS(cx, obj)->finalize(cx, obj);
 
@@ -4848,6 +4845,9 @@ js_TraceObject(JSTracer *trc, JSObject *obj)
         TRACE_SCOPE_PROPERTY(trc, sprop);
     }
 
+    if (!JS_CLIST_IS_EMPTY(&cx->runtime->watchPointList))
+        js_TraceWatchPoints(trc, obj);
+
     /* No one runs while the GC is running, so we can use LOCKED_... here. */
     clasp = LOCKED_OBJ_GET_CLASS(obj);
     if (clasp->mark) {