303043 - feedview causes leaks on balsa

Status: RESOLVED FIXED

(Firefox Graveyard :: RSS Discovery and Preview, defect, P2)

Description

[Myk Melez [:myk] [@mykmelez]]

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

[Asaf Romano (gone)]

looks like we're keeping a global XSLTProcessor for every window...

Comment 2

[:Gavin Sharp [email: [e-mail]]]

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

Comment 3

[Myk Melez [:myk] [@mykmelez]]

Attachment: patch v1: may fix problem

I'm still analyzing the logs, but this patch may fix the problem.

Comment 4

[Myk Melez [:myk] [@mykmelez]]

Attachment: patch v2: like v1 but including the necessary change to browser.js

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.

Comment 5

[Bill Gianopoulos [:WG9s]]

IMHO this should block branching.

Comment 6

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

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

[Axel Hecht]

I filed bug 303400 on that. Adding back the blockage to bug 302121, which should
root all the feedview bugs.

Comment 8

[Myk Melez [:myk] [@mykmelez]]

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?

Comment 9

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

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?

Comment 10

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

Attachment: stacks showing cycle

Comment 11

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

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

[Christian :Biesinger (don't email me, ping me on IRC)]

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

[Darin Fisher]

(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.

Comment 14

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

Attachment: patch that makes as many channels as I could find follow that rule

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.

Comment 15

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

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.

Comment 16

[Christian :Biesinger (don't email me, ping me on IRC)]

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)

Comment 17

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

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.

Comment 18

[Frank Wein [:mcsmurf]]

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.

Comment 19

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

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.

Comment 20

[Christian :Biesinger (don't email me, ping me on IRC)]

> that the mac icon channel implements asyncOpen synchronously is unfortunate :-/

on the upside, Bug 302569 is fixing that.

Comment 21

[Tanner M. Young [:tmyoung]]

Resetting QA Contact to default.