Closed Bug 392542 Opened 13 years ago Closed 13 years ago

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


(Core :: XUL, defect)

Not set





(Reporter: dbaron, Assigned: dbaron)



(Keywords: memory-leak)


(2 files)

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.
Er, I should have been clearer that I added that assertion at the start of nsXULPrototypeCache::PutScript.
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 .
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...
Assignee: dbaron → nobody
Then again, maybe I should just do the simple leak fix now and worry about the rest later.
Assignee: nobody → dbaron
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.
Attached patch simple leak fixSplinter Review
Here's the obvious leak fix; I filed bug 392650 on the underlying problem, which is a bit more work.
Attachment #277141 - Flags: superreview?(bzbarsky)
Attachment #277141 - Flags: review?(enndeakin)
Comment on attachment 277141 [details] [diff] [review]
simple leak fix

Looks reasonable.
Attachment #277141 - Flags: superreview?(bzbarsky) → superreview+
Attachment #277141 - Flags: review?(enndeakin) → review+
Fix checked in to trunk.
Closed: 13 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla1.9 M8
Hrm.  On tinderbox, all this fixed was:

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.
Component: XP Toolkit/Widgets: XUL → XUL
QA Contact: xptoolkit.xul → xptoolkit.widgets
Depends on: 1244948
You need to log in before you can comment on or make changes to this bug.