shentry -> docviewer -> presshell -> mSelection link leads to need for multiple cycles of cycle collection

RESOLVED FIXED in mozilla1.9.1b4

Status

()

P2
normal
RESOLVED FIXED
10 years ago
10 years ago

People

(Reporter: dbaron, Assigned: dbaron)

Tracking

({fixed1.9.1, memory-leak})

Trunk
mozilla1.9.1b4
fixed1.9.1, memory-leak
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(2 attachments)

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.
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. :-)
Assignee: nobody → dbaron
Status: NEW → ASSIGNED
Attachment #373010 - Flags: superreview?(bzbarsky)
Attachment #373010 - Flags: review?(bzbarsky)
Attachment #373010 - Flags: superreview?(bzbarsky)
Attachment #373010 - Flags: superreview+
Attachment #373010 - Flags: review?(bzbarsky)
Attachment #373010 - Flags: review+
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.
http://hg.mozilla.org/mozilla-central/rev/4905b995a365
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+
You need to log in before you can comment on or make changes to this bug.