Closed Bug 630947 Opened 9 years ago Closed 9 years ago

reloading about:home builds up 50+ DOMWindows

Categories

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

defect
Not set

Tracking

()

RESOLVED DUPLICATE of bug 630932
Tracking Status
blocking2.0 --- betaN+

People

(Reporter: sayrer, Assigned: smaug)

References

Details

(Whiteboard: [hardblocker][has patch])

Attachments

(1 file)

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.
blocking2.0: --- → betaN+
OS: Windows CE → All
Whiteboard: [hardblocker]
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....
does your build have this changeset?

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

?
No.  My build is from Jan 28 with some local changes.
Sigh of relief ...
Is this related to the expando leak peterv just fixed?
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
Cc:ing smaug in case the event listener stuff above rings any bells.
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)
Ccing some of the people from the recent about:home changes.
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.
(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.
see BrowserOnAboutPageLoad
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.
Perhaps page loading should more likely call gc+cc (similarly to what happened
before bug 624549). Testing a patch...
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?
I can reproduce and GC and CC run periodically, we just don't collect because of missing edges.
Looks like the problem is that XPCJSRuntime::SuspectWrappedNative calls GetFlatJSObjectAndMark instead of GetFlatJSObjectNoMark :-/. I'll verify in the morning.
Assignee: nobody → 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.
Blocks: 624549
Peter, could you try attachment 509451 [details] [diff] [review]?
Reloading would be a perfect time for GC but it might slow town page transitions :(
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.
From a quick test it looks like attachment 509451 [details] [diff] [review] helps here. Smaug, want to take the bug?
For the record the marking problem (comment 17) will be fixed with bug 614347.
Assignee: peterv → Olli.Pettay
Attachment #509451 - Flags: review?(gal)
Attachment #509451 - Flags: review?(gal) → review+
Hardware: x86 → All
Whiteboard: [hardblocker] → [hardblocker][has patch][needs landing]
Version: unspecified → Trunk
sayrer, can you turn this into a testcase? we should not have such massive leaks and none of our tests tell us
Depends on: 614347
http://hg.mozilla.org/mozilla-central/rev/0a2e06927c31
but keeping this still open because of comment 18 (about bug 614347)
Whiteboard: [hardblocker][has patch][needs landing] → [hardblocker][has patch]
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
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.
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.
Status: NEW → RESOLVED
Closed: 9 years ago
Resolution: --- → DUPLICATE
Duplicate of bug: 630932
No longer blocks: mlk2.0
Component: DOM → DOM: Core & HTML
You need to log in before you can comment on or make changes to this bug.