When I load google maps and then close the tab, it takes two cycles for things to be cycle collected. At least one of the reasons for this (and there's a pretty decent chance it's the only one) is the link from the pres shell to its mSelection. In particular, my steps to reproduce, on a Linux mozilla-central DEBUG_CC build with some extra DEBUG_CC patches from my patch queue, are: 1. start firefox 2. press Ctrl+L to focus the URL bar 3. type "maps.google.com" and hit enter 4. Hit Ctrl+T to open a new tab 5. close the google maps tab by clicking on its [X] with the mouse 6. wait for the next cycle collection (and its DEBUG_CC output) In that DEBUG_CC output, I see a bunch of warnings that suggest that things aren't being collected because of traversal deficiencies. (My local DEBUG_CC patches make these warnings a little better.) In particular, one of the warnings is: nsCycleCollector: nsFrameSelection 0x3446140 in component 28104 which was reference counted during the root/unlink/unroot phase of the last collection was not collected due to failure to unlink (see other warnings) or deficiency in traverse that causes cycles referenced only from other cycles to require multiple rounds of cycle collection in which this object was likely the reachable object An object expected to be garbage could be reached from it by the path: nsFrameSelection 0x3446140 via mDomSelections[i] nsTypedSelection 0x34462a0 via mAnchorFocusRange nsRange 0x35ac5b0 via mRoot nsDocument 0x3431da0 I used nsTraceRefcnt to get a log of all AddRef/Release calls on nsFrameSelection to find who was responsible for that Release during the root/unlink/unroot phase of the cycle collection. I'll attach the stack; it shows that this object is the pres shell's mSelection.
Created attachment 372738 [details] stack showing Release of untraversed reference to nsFrameSelection
Note that the reference count is 9 because of a whole bunch of nsTypedSelection that refer to this nsFrameSelection. I think the cycle collector can figure that out, though, since in the next cycle of cycle collection 9 references were dropped inside nsTypedSelection::cycleCollection::Unlink's call to nsRefPtr<nsFrameSelection>::operator=.
Note that traversing the PresShell::mSelection link properly probably requires traversing docshell, document viewer, etc. That seems like a lot of work.
Assignee: dbaron → nobody
Summary: add nsPresShell to cycle collection so we can traverse to its mSelection → add nsPresShell (etc.) to cycle collection so we can traverse to its mSelection
Though maybe there's a simpler fix here: nsDocShell::Destroy calls mContentViewer->Destroy(), but now (post-bfcache), all of its session history entries have content viewers too, and we don't call destroy on them until they're released. Maybe nsDocShell::Destroy should go through the session history entries and explicitly destroy the content viewers?
Yes, that would be a very good idea. Just add a call to nsIShistoryInternal.evictAllContentViewers in nsDocShell::Destroy if it has a session history?
Summary: add nsPresShell (etc.) to cycle collection so we can traverse to its mSelection → shentry -> docviewer -> presshell -> mSelection link leads to need for multiple cycles of cycle collection
Created attachment 373010 [details] [diff] [review] patch Yep, that fixes it. (Thanks for letting me know the code already existed. :-)
(In reply to comment #0) > with some extra DEBUG_CC patches from my patch queue I'm attaching these patches to bug 488603.
Comment on attachment 373010 [details] [diff] [review] patch > Thanks for letting me know the code already existed I figured you might appreciate that. ;) This looks good.
Status: ASSIGNED → RESOLVED
Last Resolved: 10 years ago
Priority: -- → P2
Resolution: --- → FIXED
Target Milestone: --- → mozilla1.9.2a1
Comment on attachment 373010 [details] [diff] [review] patch Requesting 1.9.1 approval for this patch. I think there's a significant chance that what this is fixing can lead to substantial leaks in some cases. I'd be surprised if it didn't affect a whole bunch of other cases. It's also pretty safe given that it does to the stuff in the bfcache pretty much what we just did to the displayed document a few lines up.
Attachment #373010 - Flags: approval1.9.1?
Component: Layout: Misc Code → Document Navigation
QA Contact: layout.misc-code → docshell
Comment on attachment 373010 [details] [diff] [review] patch a191=beltzner
Attachment #373010 - Flags: approval1.9.1? → approval1.9.1+
Target Milestone: mozilla1.9.2a1 → mozilla1.9.1b4
You need to log in before you can comment on or make changes to this bug.