61471 - big leak main nsXULDocument if sidebar is closed

Status: RESOLVED FIXED

(Core :: XUL, defect, P1)

Description

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

If I have my sidebar closed (View | My Sidebar), and start Mozilla and exit, it
leaks the main nsXULDocument (around 850K).  If the sidebar is open, this
doesn't happen.

This is a regression sometime since Sunday, when I last pulled.  The only reason
it didn't show up on tinderbox is that I assume tinderbox runs the tests with
the sidebar open (the default).

Comment 1

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

Could this be something like bug 60729?

Comment 2

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

Attachment: GC_MARK_DEBUG output and leaked roots

Comment 3

[Peter Trudelle]

->pavlov, p1/moz0.8.  cc bryner

Comment 4

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

The two leaked nsXPCWrappedNative roots were both created within
nsXBLBinding::InstallProperties

Comment 5

[David Hyatt]

Let's narrow down the checkin to a specific day.  It's probably someone changing 
some XBL to create a cycle.  Look for checkins to XML files on the day the leak 
began.

Comment 6

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

We should test if jag's checkin for bug 61886 did anything to help this:
http://bonsai.mozilla.org/cvsview2.cgi?diff_mode=context&whitespace_mode=show&subdir=mozilla/xpfe/browser/resources/content&command=DIFF_FRAMESET&file=navigator.js&rev1=1.261&rev2=1.262&root=/cvsroot

Comment 7

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

Nope, didn't help.

Comment 8

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

The bug did exist at 2000-11-28 08:00 PST.

Comment 9

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

FWIW, the leak seems to depend on the state of the sidebar when the window is
closed, rather than the state when the window is opened.

Comment 10

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

This bug did exist at 2000-11-27 08:00 PST.

Comment 11

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

This bug did exist at 2000-11-26 08:00 PST.

Comment 12

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

This bug did NOT exist at 2000-11-24 08:00 PST.

Comment 13

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

This leak was triggered by jag's checkin moving navigator.js from revision 1.255
to revision 1.257.

Comment 14

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

Attachment: JS changes jag found to fix (work around) the leak

Comment 15

[Peter "jag" Annema]

With much help from dbaron. Thanks!

So it appears xbl properties (sometimes?) leak when they refer to eachother,
like in this case, where webNavigation refers to this.docShell:

<property name="docShell" readonly="true"
  onget="return this.boxObject
                    .QueryInterface(Components.interfaces.nsIBrowserBoxObject)
                    .docShell;"/>
<property name="webNavigation" readonly="true"
  onget="return this.docShell
                    .QueryInterface(Components.interfaces.nsIWebNavigation);"/>

This seems to be the same cause of the leak in bug 61821 for which a similar
work-around was provided.

Comment 16

[Brendan Eich [:brendan]]

hyatt, can you attach that JSPROP_SHARED patch from your tree?  If so, jag
please test it: it may help here.

/be

Comment 17

[David Hyatt]

yup

Comment 18

[David Hyatt]

fixed w the shared thing i already added

Comment 19

[Peter "jag" Annema]

Yep, seems fixed.

David, can you verify?

Comment 20

[Peter "jag" Annema]

Actually, this is back with a vengeance.

If I start Mozilla with the sidebar closed, and load ftp://ftp.mozilla.org, I
still leak.

Apparantly, opening the sidebar (or having it open to begin with) makes the leak
hide somewhere.

David, I recall you saying something similar for another leak? "browsing for a
while makes the leak go away"...

Comment 21

[Brendan Eich [:brendan]]

Can someone say more about how to reproduce the leak jag says is "back with a
vengeance"?

Hyatt taught XBL to use JSPROP_SHARED for getters and setters, eliminating major
garbage entrainment.  I believe that attribute was responsible for the verified
fix where getBrowser().webNavigation used to leak, and after the JSPROP_SHARED
patch was checked in, did not.

(My belief is based on the workaround jag found, which dbaron attached to this
bug: that workaround inline-expanded the gette r for webNavigation, replacing a
property get with a function call.  Function calls return values via the JS
stack, which is then popped; without JSPROP_SHARED, getters return values the
same way but cache the last r.v. in the property's slot, entraining potential
garbage.  JSPROP_SHARED eliminates the slot-cache entirely.)

If the symptom is back, it must be for a different reason, now that XBL uses
JSPROP_SHARED.  Should a new bug be opened?

/be