151803 - Debugger is leaking JSDValues

Status: RESOLVED FIXED

Description

[Robert Ginda]

After stopping at a breakpoint, all functions in the call stack remained around
for the lifetime of the mozilla process.

The problem involves the fact that debugger service depends on the XPCOM
JSDValue wrapper, jsdIValue, to call JSD_DropValue.  It also tried to avoid
creating new wrappers by maintaining a list of live values.

If a client asked to wrap a JSDValue for which we already had a jsdIValue for
the *underlying jsval*, the existing wrapper would be returned.  The unwrapped
JSDValue would never be dropped, leaking anything connected to it.

I think the whole idea is probably overkill.  It's pretty easy to create
thousands of these wrappers browsing around some of the larger objects on the
window, and the exponential cost in search time starts to outweigh the memory
savings (5 pointers and a PRBool per jsdIValue.)  Normal usage seems to point to
brief bursts of wrappers, destroyed when the user resumes execution.  My best
SWAG is that the memory overhead due to duplicate wrappers is under 1k in
"likely" heavy use cases.

At the time I did this, the big advantage was being able to test for equality
among wrappers.  These days I can unwrap the object and compare two jsvals directly.

I also took this opportunity to fix a few other issues...

* Convert a few raw pointers to nsCOMPtrs
* Fix a bug where removing the last filter did not null out the list head,
causing a crash the next time filters were used.
* Track live jsdStackFrames, so we can invalidate them all when execution
continues.  Without this, only the top frame is properly invalidated, and any
other frame accessed after a continue will do Bad Things.
* Add some debugging prints to GetInitAtService, which seems to be failing at
random times.

Comment 1

[Robert Ginda]

Created attachment 87674 [details] [diff] [review]
proposed patch

looking for r/sr, any takers?

Comment 2

[Peter Van der Beken [:peterv]]

Comment on attachment 87674 [details] [diff] [review]
proposed patch

>Index: jsd_xpc.cpp
>===================================================================
>RCS file: /cvsroot/mozilla/js/jsd/jsd_xpc.cpp,v
>retrieving revision 1.46.2.3
>diff -u -r1.46.2.3 jsd_xpc.cpp
>--- jsd_xpc.cpp	16 May 2002 23:56:45 -0000	1.46.2.3
>+++ jsd_xpc.cpp	14 Jun 2002 14:08:29 -0000

...

>@@ -1582,6 +1587,71 @@
> /* Stack Frames */
> NS_IMPL_THREADSAFE_ISUPPORTS2(jsdStackFrame, jsdIStackFrame, jsdIEphemeral);
> 
>+jsdStackFrame::jsdStackFrame (JSDContext *aCx, JSDThreadState *aThreadState,

...

>+jsdStackFrame::~jsdStackFrame() 

...

>+jsdIStackFrame *
>+jsdStackFrame::FromPtr  (JSDContext *aCx, JSDThreadState *aThreadState,

Be consistent with spaces before the opening parens.

>+    /* value will be dropped by te jsdValue destructor. */

... by the jsdValue ...

>+    NS_WARN_IF_FALSE(NS_SUCCEEDED(rv), "couldn't get category manager");
>+    NS_ENSURE_SUCCESS(rv, rv);

No need for the NS_WARN_IF_FALSE, NS_ENSURE_SUCCESS will warn.

>+            NS_WARN_IF_FALSE(NS_SUCCEEDED(rv), "SetInitAtStartup failed");
>+            NS_ENSURE_SUCCESS(rv, rv);

Ditto.

Comment 3

[Peter Van der Beken [:peterv]]

Comment on attachment 87674 [details] [diff] [review]
proposed patch

r=peterv

Comment 4

[Robert Ginda]

I've fixed the whitespace and NS_ENSURE/NS_WARN issues locally.  Still looking
for an sr.  I'd like to get this on the 1.0.1 branch real soon.

Comment 5

[Johnny Stenback (:jst)]

Comment on attachment 87674 [details] [diff] [review]
proposed patch

+    jsdIStackFrame *rv;
+    nsCOMPtr<jsdIEphemeral> eph =
+	 jsds_FindEphemeral (&gLiveStackFrames,
+			     NS_REINTERPRET_CAST(void *, aStackFrameInfo));
+    if (eph)
+    {
+	 nsCOMPtr<jsdIStackFrame> frame = do_QueryInterface(eph);
+	 rv = frame;
+    }
+    else
+    {
	 rv = new jsdStackFrame (aCx, aThreadState, aStackFrameInfo);
     }
+
+    NS_IF_ADDREF(rv);

|rv| could possibly point to garbage here depending on the implementation of
what |eph| points to. If the implementation handed back an XPCOM tearoff object
the tearoff could go away when |frame| goes out of scope, so to be more correct
you should declare |frame| in the same scope that |rv| is declared in. This
probably doesn't matter with the current code, but it doesn't cost much to do
the right thing here either... Given that do_QueryInterface() is null safe you
could simply do:

+    nsCOMPtr<jsdIEphemeral> eph = ...
+
+    nsCOMPtr<jsdIStackFrame> frame = do_QueryInterface(eph);
+    rv = frame;
+
+    if (!rv)
+    {
	 rv = new jsdStackFrame (aCx, aThreadState, aStackFrameInfo);
     }

sr=jst

Comment 6

[Judson Valeski]

please checkin to the 1.0.1 branch. once there, remove the "mozilla1.0.1+"
keyword and add the "fixed1.0.1" keyword.

Comment 7

[Robert Ginda]

checked in to branch and trunk