Closed
Bug 320211
Opened 19 years ago
Closed 19 years ago
parser-related leak when loading DOM inspector in Firefox
Categories
(Core :: DOM: HTML Parser, defect, P2)
Tracking
()
RESOLVED
FIXED
mozilla1.9alpha1
People
(Reporter: dbaron, Assigned: dbaron)
References
Details
(Keywords: memory-leak, Whiteboard: [patch])
Attachments
(2 files)
3.81 KB,
patch
|
mrbkap
:
review+
jst
:
superreview+
|
Details | Diff | Splinter Review |
14.92 KB,
patch
|
mrbkap
:
review+
jst
:
superreview+
|
Details | Diff | Splinter Review |
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•19 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•19 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•19 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•19 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•19 years ago
|
||
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•19 years ago
|
Priority: -- → P2
Whiteboard: [patch]
Target Milestone: --- → mozilla1.9alpha
Assignee | ||
Updated•19 years ago
|
Summary: parser-related leak when loading DOM inspector → parser-related leak when loading DOM inspector in Firefox
Comment 6•19 years ago
|
||
Comment on attachment 205889 [details] [diff] [review]
patch
r=mrbkap
Attachment #205889 -
Flags: review?(mrbkap) → review+
Comment 7•19 years ago
|
||
Comment on attachment 205889 [details] [diff] [review]
patch
sr=jst
Attachment #205889 -
Flags: superreview?(jst) → superreview+
Assignee | ||
Comment 8•19 years ago
|
||
Fix checked in to trunk.
Status: ASSIGNED → RESOLVED
Closed: 19 years ago
Resolution: --- → FIXED
Assignee | ||
Comment 9•19 years ago
|
||
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)
Assignee | ||
Updated•19 years ago
|
Comment 10•19 years ago
|
||
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 11•19 years ago
|
||
Comment on attachment 206149 [details] [diff] [review]
code consolidation
sr=jst
Attachment #206149 -
Flags: superreview?(jst) → superreview+
Assignee | ||
Comment 12•19 years ago
|
||
Consolidation checked in to trunk.
You need to log in
before you can comment on or make changes to this bug.
Description
•