392542 - leak nsXULPDGlobalObject starting up and quitting Firefox on Mac (RLk nonzero)

Status: RESOLVED FIXED

(Core :: XUL, defect)

Description

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Starting and shutting down firefox on Mac, we leak an nsXULPDGlobalObject.  So far I've found that it's due to unbalanced js_LockGCThing calls due to multiple calls to nsXULPrototypeCache::PutScript.  I see the following assertion fire once for each unbalanced js_LockGCThing (and it would cause an unbalanced js_LockGCThing due to the extra HoldScriptObject in that function):

NS_ASSERTION(!mScriptTable.Get(aURI, nsnull), "already have script in table");

Note that we're creating two scripts for the URI, and the first one put in the prototype cache is overwritten in the cache and thus leaked.

Comment 1

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Er, I should have been clearer that I added that assertion at the start of nsXULPrototypeCache::PutScript.

Comment 2

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Attachment: nsXULPrototypeCache::PutScript calls during startup

Comment 3

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

The problem seems to be that two different nsXULDocument objects are loading (and blocking on) the same sequence 5 scripts at the same time.  Not surprisingly, one of them is chrome://browser/content/hiddenWindow.xul and the other is chrome://browser/content/browser.xul .

Comment 4

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

It seems like what we should really be doing here is keeping a table of loading scripts in the XUL prototype cache, and adding additional listeners to existing loads.  (I think we'd then need to have one of the listeners compile the script and put it in the cache and the rest get a different notification.)

And we should add the assertion above...

Comment 5

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Then again, maybe I should just do the simple leak fix now and worry about the rest later.

Comment 6

[Boris Zbarsky [:bzbarsky]]

Note, by the way, that we have a similar issue with overlays.  They're placed in the cache much later than NewChannel(), so two NewChannel()s for the same overlay in quick succession will both try to start overlay loads, then in nsXULDocument::StartDocumentLoad we'll find the overlay in the cache and create a CachedChromeStreamListener which will assert in OnDataAvailable...

I don't think we clobber the cache in that case, though.

What I'd do here is do the simple leak fix, and a followup bug on adding the assert and making this code coalesce better.

Comment 7

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Attachment: simple leak fix

Here's the obvious leak fix; I filed bug 392650 on the underlying problem, which is a bit more work.

Comment 8

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 277141 [details] [diff] [review]
simple leak fix

Looks reasonable.

Comment 9

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Fix checked in to trunk.

Comment 10

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Hrm.  On tinderbox, all this fixed was:

--FIXED-LEAKS---------------------------------leaks------leaks%
nsSystemPrincipal                                 0    -100.00%
nsXULPDGlobalObject                               0    -100.00%

whereas on my machine it had fixed a good bit more (although I was just starting up and shutting down; not running the page cycler).  So there's still more investigation needed for the leaks on tinderbox.