Closed
Bug 303043
Opened 19 years ago
Closed 19 years ago
feedview causes leaks on balsa
Categories
(Firefox Graveyard :: RSS Discovery and Preview, defect, P2)
Firefox Graveyard
RSS Discovery and Preview
Tracking
(Not tracked)
RESOLVED
FIXED
Firefox1.5
People
(Reporter: myk, Assigned: dbaron)
References
Details
(Keywords: fixed1.8, memory-leak, Whiteboard: [patch][eta 8/17])
Attachments
(3 files, 1 obsolete file)
2.82 KB,
patch
|
Details | Diff | Splinter Review | |
6.03 KB,
text/plain; charset=utf-8
|
Details | |
22.65 KB,
patch
|
Biesinger
:
review+
darin.moz
:
superreview+
cbeard
:
approval1.8b4+
|
Details | Diff | Splinter Review |
Per bug 302121, comment 50, the feedview checkin causes leaks on balsa: Yikes, this patch just caused a major mlk on Linux balsa: From: RLk:1.08KB Lk:333KB MH:7.82MB A:219K To: RLk:49.8KB Lk:1.30MB MH:7.96MB A:226K
Comment 1•19 years ago
|
||
looks like we're keeping a global XSLTProcessor for every window...
OS: Linux → All
Hardware: PC → All
Comment 2•19 years ago
|
||
It looks to me like the XSLTProcessor should be initialized halfway through maybeLoadFeedview, once we've determined that it is indeed a feed, and not for every window. http://lxr.mozilla.org/seamonkey/source/browser/components/feedview/content/feedviewOverlay.js#65
Updated•19 years ago
|
Severity: normal → blocker
Updated•19 years ago
|
Flags: blocking1.8b4?
Reporter | ||
Comment 3•19 years ago
|
||
I'm still analyzing the logs, but this patch may fix the problem.
Assignee: nobody → myk
Status: NEW → ASSIGNED
Reporter | ||
Comment 4•19 years ago
|
||
Patch v1 was missing the piece for browser.js. This version includes it. (In reply to comment #2) > It looks to me like the XSLTProcessor should be initialized halfway through > maybeLoadFeedview, once we've determined that it is indeed a feed, and not for > every window. There's value in keeping the processor around, since it means we don't have to reinstantiate it every time the user loads a feed. But we might want to wait to instantiate until the user loads their first feed. Note: that's not what this patch does. It stops storing the stylesheet as a global variable, deletes the stylesheet's load event listener once load has finished, and resets/deletes the XSLT processor on shutdown.
Reporter | ||
Updated•19 years ago
|
Attachment #191350 -
Attachment is obsolete: true
Assignee | ||
Comment 6•19 years ago
|
||
The fundamental problem here, IMO, is that feedview involves itself with every page load, slowing it down, rather than depending on our existing content dispatching mechanisms. A separate bug should really be filed on that.
Comment 7•19 years ago
|
||
I filed bug 303400 on that. Adding back the blockage to bug 302121, which should root all the feedview bugs.
Blocks: feedview
Updated•19 years ago
|
Flags: blocking1.8b4? → blocking1.8b4+
Updated•19 years ago
|
Component: General → RSS Discovery and Preview
Reporter | ||
Updated•19 years ago
|
Whiteboard: looking at this in-depth tomorrow, Tuesday, August 9
Updated•19 years ago
|
Updated•19 years ago
|
Reporter | ||
Comment 8•19 years ago
|
||
It looks like the leaks happen when the stylesheet gets loaded: gFeedviewStylesheet = document.implementation.createDocument("", "", null); gFeedviewStylesheet.addEventListener("load", onLoadFeedviewStylesheet, false); gFeedviewStylesheet.load("chrome://browser/content/feedview/feedview.xsl"); Eliminating the global gFeedviewStylesheet variable, removing the event listener after the document finishes loading, and loading some other document besides the XSLT stylesheet don't make the leaks go away, but loading the stylesheet from a remote website does. When I changed the stylesheet URL to http://www.melez.com/feedview.xsl, the leaks disappeared. When I changed it back to the chrome URL, they reappeared. resource: and file: URLs also exhibit the leaks. What could be different about local URLs that would cause these leaks?
Reporter | ||
Updated•19 years ago
|
Whiteboard: looking at this in-depth tomorrow, Tuesday, August 9
Assignee | ||
Comment 9•19 years ago
|
||
jst told me the leaked document is the XSLT document. The leak is due to a cycle between document and channel. The channel owns the document because it's the JAR channel's notification callbacks, passed as the 5th argument to NS_NewChannel in nsXMLDocument::Load. The document owns the channel because nsXMLDocument::Load calls nsXMLDocument::StartDocumentLoad which calls nsDocument::StartDocumentLoad which assigns the channel to mChannel. Nothing breaks this cycle. What should?
Assignee | ||
Comment 10•19 years ago
|
||
Assignee | ||
Comment 11•19 years ago
|
||
Setting |mCallbacks = 0;| just before the end of nsJARChannel::OnStopRequest fixes the leak. That matches what the HTTP channel does, but other channels don't seem to do that. Should they?
Comment 12•19 years ago
|
||
yes, I think channels should break references to listeners, listener contexts and notification callbacks (and stuff gotten from notification callbacks, if any) in onStopRequest.
Comment 13•19 years ago
|
||
(In reply to comment #12) > yes, I think channels should break references to listeners, listener contexts > and notification callbacks (and stuff gotten from notification callbacks, if > any) in onStopRequest. Agreed... and make that after they call their listener's OnStopRequest method.
Assignee | ||
Comment 14•19 years ago
|
||
This makes as many channels as I could find follow that rule. I'm not sure how many of these we should really consider checking in. In particular, I'm a bit wary of touching nsMsgProtocol, LDAP, wyciwyg, wxEmbed, the mac icon channel, input stream channel, and external protocol handler.
Assignee | ||
Comment 15•19 years ago
|
||
Comment on attachment 192423 [details] [diff] [review] patch that makes as many channels as I could find follow that rule So, thoughts on this? My initial inclination would be not to touch nsLDAPChannel or nsMsgProtocol, but to keep the rest. However, you guys probably know this stuff better.
Attachment #192423 -
Flags: superreview?(darin)
Attachment #192423 -
Flags: review?(cbiesinger)
Assignee | ||
Updated•19 years ago
|
Assignee: myk → dbaron
Status: ASSIGNED → NEW
Priority: -- → P2
Whiteboard: [patch][eta 8/17]
Target Milestone: --- → Firefox1.1
Updated•19 years ago
|
Attachment #192423 -
Flags: superreview?(darin) → superreview+
Comment 16•19 years ago
|
||
Comment on attachment 192423 [details] [diff] [review] patch that makes as many channels as I could find follow that rule modules/libpr0n/decoders/icon/win/nsIconChannel.cpp modules/libpr0n/decoders/icon/os2/nsIconChannel.cpp + mCallbacks = 0; the context uses nsnull instead of 0 for the mListener assignment, maybe this code should be consistent that the mac icon channel implements asyncOpen synchronously is unfortunate :-/ I would probably check in the ldap changes, they seem low risk (note also that that file is not part of the default build, only of --enable-ldap-experimental ones)
Attachment #192423 -
Flags: review?(cbiesinger) → review+
Assignee | ||
Comment 17•19 years ago
|
||
Comment on attachment 192423 [details] [diff] [review] patch that makes as many channels as I could find follow that rule This patch should fix a number of leak cases dealing with lesser-used protocols. There is some risk, but it shouldn't be too bad.
Attachment #192423 -
Flags: approval1.8b4?
Updated•19 years ago
|
Attachment #192423 -
Flags: approval1.8b4? → approval1.8b4+
Comment 18•19 years ago
|
||
BTW: The leak on balsa went down to 333KB again, not sure due which checkin. There were some checkins of bsmedberg and ben and after those checkins Lk was down again (except for one cycle, which randomly(?) went up to 1,14MB). dbaron's patch didn't have any impact on leak numbers it seems.
Assignee | ||
Comment 19•19 years ago
|
||
The leaks went down because ben broke feedview, I think. In any case, the underlying leak fix has been checked in: * to the trunk, 2005-08-12 15:53 -0700 * to MOZILLA_1_8_BRANCH, 2005-08-13 12:34 -0700.
Status: NEW → RESOLVED
Closed: 19 years ago
Resolution: --- → FIXED
Comment 20•19 years ago
|
||
> that the mac icon channel implements asyncOpen synchronously is unfortunate :-/ on the upside, Bug 302569 is fixing that.
Updated•6 years ago
|
Product: Firefox → Firefox Graveyard
You need to log in
before you can comment on or make changes to this bug.
Description
•