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

Status: RESOLVED FIXED

(Core :: DOM: Navigation, defect, P2)

Description

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

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.

Comment 1

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

Attachment: stack showing Release of untraversed reference to nsFrameSelection

Comment 2

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

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=.

Comment 3

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

Note that traversing the PresShell::mSelection link properly probably requires traversing docshell, document viewer, etc.  That seems like a lot of work.

Comment 4

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

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?

Comment 5

[Boris Zbarsky [:bzbarsky]]

Yes, that would be a very good idea.  Just add a call to nsIShistoryInternal.evictAllContentViewers in nsDocShell::Destroy if it has a session history?

Comment 6

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

Attachment: patch

Yep, that fixes it.  (Thanks for letting me know the code already existed. :-)

Comment 7

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

(In reply to comment #0)
> with some extra DEBUG_CC patches from my patch queue

I'm attaching these patches to bug 488603.

Comment 8

[Boris Zbarsky [:bzbarsky]]

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.

Comment 9

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

http://hg.mozilla.org/mozilla-central/rev/4905b995a365

Comment 10

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

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.

Comment 11

[Mike Beltzner [:beltzner, not reading bugmail]]

Comment on attachment 373010 [details] [diff] [review]
patch

a191=beltzner

Comment 12

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

http://hg.mozilla.org/releases/mozilla-1.9.1/rev/8a764e1d8984