630947 - reloading about:home builds up 50+ DOMWindows

Status: RESOLVED DUPLICATE of bug 630932

(Core :: DOM: Core & HTML, defect)

Description

[Robert Sayre]

We seem to be hanging on to references. Nothing leaks at shutdown. Opening a new browser window and closing the old one does not collect these references.

Comment 1

[Boris Zbarsky [:bzbarsky]]

I just tried that (on a not-quite-tip mac build).

I load the page, hold down the reload key.  Get about 100 windows.  Open a new window.  Close the old one.  Not much change.  Load about:blank in the new window.  All the windows get GCed....

Comment 2

[Robert Sayre]

does your build have this changeset?

Andreas Gal - Enter compartment in AutoScriptEvaluate (bug 630243, r=jst, a=blocker). 

?

Comment 3

[Boris Zbarsky [:bzbarsky]]

No.  My build is from Jan 28 with some local changes.

Comment 4

[Andreas Gal :gal]

Sigh of relief ...

Comment 5

[Andreas Gal :gal]

Is this related to the expando leak peterv just fixed?

Comment 6

[Peter Van der Beken [:peterv]]

Doesn't look like it is.

There's an event listener held by a div element, we're missing an edge:

0x2046eb98 [nsXPCWrappedJS (nsIDOMEventListener)]

    0x20456060 [nsGenericElement (xhtml) div [1/2]]
        --[[via hash] mListenerManager]-> 0x2046ebd0 [nsEventListenerManager]
        --[mListeners[i] mListener]-> 0x2046eb98 [nsXPCWrappedJS (nsIDOMEventListener)]

        known edges:
            0x20455fe0 nsGenericElement (xhtml) div --[mAttrsAndChildren[i]]--> 0x20456060

Comment 7

[Johnny Stenback (:jst)]

Cc:ing smaug in case the event listener stuff above rings any bells.

Comment 8

[Olli Pettay [:smaug][[e-mail]]]

Who knows about about:home implementation?
It seems to be a mix of that xhtml+js page and some logic in browser.js


(And no, a missing edge to XPCWrappedJS DOMEventListener doesn't ring any bells)

Comment 9

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

Ccing some of the people from the recent about:home changes.

Comment 10

[Olli Pettay [:smaug][[e-mail]]]

Actually, I'm not sure I can reproduce the problem.
Sure, if I press ctrl+r, lots of new DOMWindows are created and kept alive some
time. But once GC and CC have both run, extra DOMWindows are deleted.

Though, I think I managed to reproduce the problem yesterday :/
Today I'm not getting that "restore session" thing in the page.
It is not clear to me when that is shown.

Comment 11

[Marco Bonardo [:mak]]

(In reply to comment #10)
> Though, I think I managed to reproduce the problem yesterday :/
> Today I'm not getting that "restore session" thing in the page.
> It is not clear to me when that is shown.

on load of the page browser checks if sessionstore canRestoreSession and flips hidden status of a div.

Comment 12

[Marco Bonardo [:mak]]

see BrowserOnAboutPageLoad

Comment 13

[Olli Pettay [:smaug][[e-mail]]]

Ok, now I know how to get that "Restore previous session" and that doesn't
seem to affect the behavior. DOMWindows are collected eventually, at least
on my debug linux build.
But that missing edge is still strange.

Comment 14

[Olli Pettay [:smaug][[e-mail]]]

Perhaps page loading should more likely call gc+cc (similarly to what happened
before bug 624549). Testing a patch...

Comment 15

[Olli Pettay [:smaug][[e-mail]]]

Attachment: more gc before cc

This should release DOMWindows earlier.
With the patch we end up calling GC more often;
morethe way we used to do it. If MaybeCC ends up calling CC, GC+CC is called 
after page loads and when user becomes inactive.


But I still wonder, can anyone actually reproduce possible leak?
What if you reload about:home 100 times?

Comment 16

[Peter Van der Beken [:peterv]]

I can reproduce and GC and CC run periodically, we just don't collect because of missing edges.

Comment 17

[Peter Van der Beken [:peterv]]

Looks like the problem is that XPCJSRuntime::SuspectWrappedNative calls GetFlatJSObjectAndMark instead of GetFlatJSObjectNoMark :-/. I'll verify in the morning.

Comment 18

[Peter Van der Beken [:peterv]]

So even with the change to XPCJSRuntime::SuspectWrappedNative (which will be done as part of bug 614347) we keep windows alive. It looks like we do need changes to GC scheduling. I've been able to reload about:home 50 times without any GC happening. We do run a bunch of CC's, but those aren't able to collect anything because anything held by the JS objects is not collectable. I think the current GC scheduling is having a really bad effect on the effectiveness of the CC.

Comment 19

[Olli Pettay [:smaug][[e-mail]]]

Peter, could you try attachment 509451 [details] [diff] [review]?

Comment 20

[Andreas Gal :gal]

Reloading would be a perfect time for GC but it might slow town page transitions :(

Comment 21

[Olli Pettay [:smaug][[e-mail]]]

We have had GC+CC after page load for ages. Bug 624549 made those less likely,
and attachment 509451 [details] [diff] [review] makes them again more likely.

Comment 22

[Peter Van der Beken [:peterv]]

From a quick test it looks like attachment 509451 [details] [diff] [review] helps here. Smaug, want to take the bug?

Comment 23

[Ben Turner (not reading bugmail, use the needinfo flag!)]

For the record the marking problem (comment 17) will be fixed with bug 614347.

Comment 24

[Andreas Gal :gal]

sayrer, can you turn this into a testcase? we should not have such massive leaks and none of our tests tell us

Comment 25

[Olli Pettay [:smaug][[e-mail]]]

http://hg.mozilla.org/mozilla-central/rev/0a2e06927c31
but keeping this still open because of comment 18 (about bug 614347)

Comment 26

[Mike]

I'm not sure if this is related, but load about:memory and hold F5
Easy to monitor some mem numbers, steady grow until GC, then grow bit more again, goto GC

PS. Mozilla/5.0 (X11; Linux i686; rv:2.0b12pre) Gecko/20110209 Firefox/4.0b12pre

Comment 27

[Chris Leary [:cdleary] (not checking bugmail)]

cdleary-bot mozilla-central merge info:
http://hg.mozilla.org/mozilla-central/rev/d07aab3b3b7c
Note: not marking as fixed because fixed-in-tracemonkey is not present on the whiteboard.

Comment 28

[Andreas Gal :gal]

I think we can mark this a dup of 630932 now. That bug will take care of GC and CC scheduling. That bug should land shortly. Please re-open if you disagree.