55117 - Add debug warning for leaked GC roots at shutdown.

Status: VERIFIED FIXED

(Core :: JavaScript Engine, defect, P3)

Description

[Mike McCabe]

The attachment adds a debug diagnostic to the JS engine warning of leaked GC
roots at shutdown; should make it a little easier to use the JS engine safely.

I've also made the JSExceptionState exception root a named exception, which
should help catch cases of un(restore|drop)ed JSExceptionStates.

review sought.

Comment 1

[Mike McCabe]

Attachment: add leaked gc-root warning; name a root.

Comment 2

[Brendan Eich [:brendan]]

Yikes -- that patch looks like a diff of jscntxt.c against jsapi.c?

/be

Comment 3

[rogerl (gone)]

Wow, you plan to replace all of jscntxt.c with jsapi.c? ok, you must know what 
you're doing, r=rogerl :-)

Comment 4

[Mike McCabe]

Attachment: revised diff for the folks in the peanut gallery

Comment 5

[Brendan Eich [:brendan]]

Looks good, a=[e-mail] conditional on r= from someone, and trying it 
out in Mozilla (of course -- what number of leaked roots does it print)?

/be

Comment 6

[rogerl (gone)]

r=rogerl

Comment 7

[John Bandhauer]

I have code from David Baron (can't find a bug or the original email) to add 
this dump in xpconnect shutdown rather than in the core engine. I ran with it 
for quite a while. I'm not sure one is better than the other. I'm iffy about 
having this on for all debug builds of the engine without a special #define.

My experience was that we often shutdown cleanly (unless you run mail/news or 
editor :(  The thing is that leaking one *real* root (e.g. an nsJSContext) 
often shows tons of JS roots. I'm not so sure it highlights the real problem 
well.

Comment 8

[Mike McCabe]

I was imagining that it might be useful for external cients of the engine, e.g.
for detecting misused JSExceptionStates.  I think it's better to flag these as
they occur, rather than just when looking for them.  But I'm open to discussion.

Sure enough, the browser shuts down cleanly, except when composer or mail runs:

JS engine warning: leaking GC root 'nsXPCWrappedJS::mJSObj' at location
0x8812ab8
JS engine warning: 1 GC roots remain after engine shutdown.  Any objects
reachable through these roots will not be finalized.

(composer is worse.)

Comment 9

[Brendan Eich [:brendan]]

Argh, I forgot about dbaron's patch completely -- recycling memory too fast 
these days.  Sorry.

I still think it's worthwhile having this in the core engine, because anyone can 
misuse the API, not just Mozilla-the-client or another app that uses XPConnect 
too.  We could make it GC_MARK_DEBUG rather than DEBUG only, but is DEBUG that 
much of a hardship?  It may shed more heat than light, but we hope to catch such 
bugs early, next time they appear (and other embeddings should too -- rathar 
than wait till it's hard and late for a fix).

/be

Comment 10

[Brendan Eich [:brendan]]

Nominating for js1.5 (meaning rtm?  Let's discuss).  This patch would have saved 
at least one JS embedder I'm helping some time and confusion.  That embedding 
had a GC root stuck in a cycle, and was destroying and recreating the last or 
first context (which frees atom state).

/be

Comment 11

[Mike McCabe]

Sounds good to me.  I don't think it's an RTM candidate, quite, but I do think
it fits in some qualified JS release.  I'll check it into the trunk, and may
change some wording to highlight possible usage problems.

Comment 12

[Mike McCabe]

Attachment: Plural-correctness; mention potential dangling nature.

Comment 13

[Brendan Eich [:brendan]]

%lu rather than %d for the uint32 format spec, cast to (unsigned long), and
a=brendan.

/be

Comment 14

[Mike McCabe]

Fix checked in to trunk.

Comment 15

[Phil Schwartau]

Marking Verified -