I leak a lot of stuff (including the hidden window's global object, thanks to event listeners in XBL documents) when loading inspector due to some parser-related leak. The first nsDocument leaked has a reference from each of two nsXULDocument::ParserObserver objects, which are each owned by both the nsParser and its CParserContext. I haven't dug further, yet. Steps to reproduce: 1. debug build, XPCOM_MEM_LEAK_LOG=leak.log 2. mkdir deleteme 3. ./firefox -profile deleteme 4. Tools | DOM Inspector 5. close DOMI window 6. wait 3 seconds 7. close browser window
I confirmed that these are nsParser-XULContentSinkImpl cycles. The cycle is created in: nsParser::SetContentSink(nsIContentSink*) (/builds/trunk/mozilla/parser/htmlparser/src/nsParser.cpp:576) or XULContentSinkImpl::SetParser(nsIParser*) (/builds/trunk/mozilla/content/xul/document/src/nsXULContentSink.cpp:437) nsParser::SetContentSink(nsIContentSink*) (/builds/trunk/mozilla/parser/htmlparser/src/nsParser.cpp:577) within nsXULDocument::PrepareToLoadPrototype(nsIURI*, char const*, nsIPrincipal*, nsIParser**) (/builds/trunk/mozilla/content/xul/document/src/nnsXULDocument::ResumeWalk() (/builds/trunk/mozilla/content/xul/document/src/nsXULDocument.cpp:3036) nsXULDocument::EndLoad() (/builds/trunk/mozilla/content/xul/document/src/nsXULDocument.cpp:743) XULContentSinkImpl::DidBuildModel() (/builds/trunk/mozilla/content/xul/document/src/nsXULContentSink.cpp:407) nsExpatDriver::DidBuildModel(unsigned int, int, nsIParser*, nsIContentSink*) (/builds/trunk/mozilla/parser/htmlparser/src/nsExpatDriver.cnsParser::DidBuildModel(unsigned int) (/builds/trunk/mozilla/parser/htmlparser/src/nsParser.cpp:1198) nsParser::ResumeParse(int, int, int) (/builds/trunk/mozilla/parser/htmlparser/src/nsParser.cpp:1919) nsParser::OnStopRequest(nsIRequest*, nsISupports*, unsigned int) (/builds/trunk/mozilla/parser/htmlparser/src/nsParser.cpp:2587) nsJARChannel::OnStopRequest(nsIRequest*, nsISupports*, unsigned int) (/builds/trunk/mozilla/modules/libjar/nsJARChannel.cpp:719) There are two leaked parsers and two leaked content sinks for this one nsXULDocument.
May well be related to the warnings: Couldn't convert chrome URL: chrome://communicator/content/utilityOverlay.xul Couldn't convert chrome URL: chrome://communicator/content/tasksOverlay.xul
It looks like so, since that warning happens here: #0 nsChromeProtocolHandler::NewChannel (this=0x9504fe8, aURI=0x9bff758, aResult=0xbfc16d18) at /builds/trunk/mozilla/chrome/src/nsChromeProtocolHandler.cpp:573 #1 0x009c2e61 in nsIOService::NewChannelFromURI (this=0x9499eb0, aURI=0x9bff758, result=0xbfc16d18) at /builds/trunk/mozilla/netwerk/base/src/nsIOService.cpp:494 #2 0x012ae6b2 in NS_NewChannel (result=0xbfc16d68, uri=0x9bff758, ioService=0x9499eb0, loadGroup=0x9bd2c40, callbacks=0x0, loadFlags=0) at ../../../../dist/include/necko/nsNetUtil.h:171 #3 0x01531d0e in NS_OpenURI (listener=0x9c12c94, context=0x0, uri=0x9bff758, ioService=0x0, loadGroup=0x9bd2c40, callbacks=0x0, loadFlags=0) at ../../../../dist/include/necko/nsNetUtil.h:225 #4 0x0152db42 in nsXULDocument::ResumeWalk (this=0x9bd08e8) at /builds/trunk/mozilla/content/xul/document/src/nsXULDocument.cpp:3060 and this NS_OpenURI call (#3) fails after printing that warning, and the parser doesn't like Parse being called without OnStartRequest and OnStopRequest. Perhaps the stuff passed to Parse could be passed in the context parameter to OnStartRequest instead?
But we could probably just stick with the current ugly code and call nsParser::Terminate. But mrbkap might have a better patch in the works.
Created attachment 205889 [details] [diff] [review] patch I'll do a followup patch after this lands to refactor this to reduce code duplication.
Priority: -- → P2
Target Milestone: --- → mozilla1.9alpha
Summary: parser-related leak when loading DOM inspector → parser-related leak when loading DOM inspector in Firefox
Comment on attachment 205889 [details] [diff] [review] patch r=mrbkap
Attachment #205889 - Flags: review?(mrbkap) → review+
Comment on attachment 205889 [details] [diff] [review] patch sr=jst
Attachment #205889 - Flags: superreview?(jst) → superreview+
Fix checked in to trunk.
Status: ASSIGNED → RESOLVED
Last Resolved: 12 years ago
Resolution: --- → FIXED
Created attachment 206149 [details] [diff] [review] code consolidation This consolidates the overlay loading into LoadOverlayInternal. There was previously only one caller which passed aIsDynamic==PR_TRUE. There's now a second one with aIsDynamic==PR_FALSE, which seems to be the original use of aIsDynamic. The goal here was to have no change of behavior (except for the details of the error handling in the obscure case where we can't do_GetService the security manager, since getting it is no longer pulled out of the loop).
Comment on attachment 206149 [details] [diff] [review] code consolidation - In: nsXULDocument.cpp >- mIsWritingFastLoad = useXULCache; >+ if (aIsDynamic) >+ mIsWritingFastLoad = useXULCache; This seems arbitrary to me, but it's how the code looks now, so I won't question it. >+ if (failureFromContent) >+ // The failure |rv| was the result of a problem in the content >+ // rather than an unexpected problem in our implementation, so >+ // just continue with the next overlay. >+ continue; Nit: Please add braces around this. r=mrbkap, thanks for doing this.
Attachment #206149 - Flags: review?(mrbkap) → review+
Comment on attachment 206149 [details] [diff] [review] code consolidation sr=jst
Attachment #206149 - Flags: superreview?(jst) → superreview+
Consolidation checked in to trunk.
You need to log in before you can comment on or make changes to this bug.