320211 - parser-related leak when loading DOM inspector in Firefox

Status: RESOLVED FIXED

(Core :: DOM: HTML Parser, defect, P2)

Description

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

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

Comment 1

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

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.

Comment 2

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

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

Comment 3

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

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?

Comment 4

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

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.

Comment 5

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

Attachment: patch

I'll do a followup patch after this lands to refactor this to reduce code duplication.

Comment 6

[Blake Kaplan (:mrbkap) (inactive)]

Comment on attachment 205889 [details] [diff] [review]
patch

r=mrbkap

Comment 7

[Johnny Stenback (:jst)]

Comment on attachment 205889 [details] [diff] [review]
patch

sr=jst

Comment 8

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

Fix checked in to trunk.

Comment 9

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

Attachment: 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 10

[Blake Kaplan (:mrbkap) (inactive)]

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.

Comment 11

[Johnny Stenback (:jst)]

Comment on attachment 206149 [details] [diff] [review]
code consolidation

sr=jst

Comment 12

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

Consolidation checked in to trunk.