parser-related leak when loading DOM inspector in Firefox

RESOLVED FIXED in mozilla1.9alpha1

Status

()

Core
HTML: Parser
P2
normal
RESOLVED FIXED
12 years ago
12 years ago

People

(Reporter: dbaron, Assigned: dbaron)

Tracking

({memory-leak})

Trunk
mozilla1.9alpha1
x86
Linux
memory-leak
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [patch])

Attachments

(2 attachments)

(Assignee)

Description

12 years ago
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
(Assignee)

Comment 1

12 years ago
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.
(Assignee)

Comment 2

12 years ago
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
(Assignee)

Comment 3

12 years ago
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?
(Assignee)

Comment 4

12 years ago
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.
(Assignee)

Comment 5

12 years ago
Created attachment 205889 [details] [diff] [review]
patch

I'll do a followup patch after this lands to refactor this to reduce code duplication.
Assignee: mrbkap → dbaron
Status: NEW → ASSIGNED
Attachment #205889 - Flags: superreview?(jst)
Attachment #205889 - Flags: review?(mrbkap)
(Assignee)

Updated

12 years ago
Priority: -- → P2
Whiteboard: [patch]
Target Milestone: --- → mozilla1.9alpha
(Assignee)

Updated

12 years ago
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+
(Assignee)

Comment 8

12 years ago
Fix checked in to trunk.
Status: ASSIGNED → RESOLVED
Last Resolved: 12 years ago
Resolution: --- → FIXED
(Assignee)

Comment 9

12 years ago
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).
Attachment #206149 - Flags: superreview?(jst)
Attachment #206149 - Flags: review?(mrbkap)

Updated

12 years ago
Blocks: 320652
(Assignee)

Updated

12 years ago
No longer blocks: 320652
Depends on: 320652
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+
(Assignee)

Comment 12

12 years ago
Consolidation checked in to trunk.
You need to log in before you can comment on or make changes to this bug.