Closed Bug 151803 Opened 23 years ago Closed 23 years ago

Debugger is leaking JSDValues

Categories

(Other Applications Graveyard :: Venkman JS Debugger, defect)

x86
All
defect
Not set
normal

Tracking

(Not tracked)

RESOLVED FIXED

People

(Reporter: rginda, Assigned: rginda)

Details

Attachments

(1 file)

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.
Attached patch proposed patchSplinter Review
looking for r/sr, any takers?
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 on attachment 87674 [details] [diff] [review] proposed patch r=peterv
Attachment #87674 - Flags: review+
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 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
Attachment #87674 - Flags: superreview+
Attachment #87674 - Flags: approval+
please checkin to the 1.0.1 branch. once there, remove the "mozilla1.0.1+" keyword and add the "fixed1.0.1" keyword.
Keywords: mozilla1.0.1+
checked in to branch and trunk
Status: NEW → RESOLVED
Closed: 23 years ago
Resolution: --- → FIXED
Product: Core → Other Applications
Product: Other Applications → Other Applications Graveyard
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: