282938 - nsContentShellInfo holds tabbrowser webshells until parent window destroyed

Status: RESOLVED FIXED

(Core :: XUL, defect)

Description

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

nsContentShellInfo holds webshells associated with browsers within a tabbrowser
until the parent window containing the tabbrowser is destroyed.

The registration occurs from nsFrameLoader::EnsureDocShell or
nsSubDocumentFrame::AttributeChanged (under conditions inconsistent with each
other) calling nsIDocShellTreeOwner::ContentShellAdded (which goes through one
or more of the implementations thereof -- nsDocShellTreeOwner forwards to
another one, nsContentTreeOwner and nsChromeTreeOwner call directly to...) which
calls nsXULWindow::ContentShellAdded, which creates a nsContentShellInfo that
keeps a strong reference to the docshell, not destroyed until the parent goes away:

nsWebShell::Release()
  (/builds/trunk/mozilla/docshell/base/nsWebShell.cpp:242)
nsCOMPtr<nsIDocShellTreeItem>::~nsCOMPtr()
  (../../../dist/include/xpcom/nsCOMPtr.h:583)
nsContentShellInfo::~nsContentShellInfo()
  (/builds/trunk/mozilla/xpfe/appshell/src/nsXULWindow.cpp:2098)
nsXULWindow::Destroy()
  (/builds/trunk/mozilla/xpfe/appshell/src/nsXULWindow.cpp:524)

There's no parallel removal mechanism.

(Note that if you actually want to see the WEBSHELL numbers go down, you
probably need the patch I have in my tree that makes the DOMWINDOW numbers go
down like they should.)

Comment 1

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

Attachment: IRC discussion on developers

Comment 2

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

And, FWIW, this WEBSHELL leak doesn't happen on the aviary 1.0 branch (although
a DOMWINDOW leak does).

Comment 3

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

It seems that the difference is that nsXULWindow::ContentShellAdded used to
maintain mContentShells as a list of nsContentShellInfo objects **for each id**,
but now it's a list **for each docshell**, so docshells never get removed when
their ID is reused.

Comment 4

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

... a change made in revision 1.151 of nsXULWindow.cpp for bug 104532.

Comment 5

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

Attachment: IRC discussion on developers (2)

Comment 6

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

That said, I don't see a reason for the change in that patch, since
mContentShells is still only used for two things, GetPrimaryContentShell (which
only needs the primary one), and GetContentShellById (which is exactly a map
from id to shell, and is also completely unused).

It seems like a whole lot of code could be removed if we had a better way of
implementing GetPrimaryContentShell (could we search through the tree?).

Comment 7

[[e-mail]]

Originally the XUL window wasn't tracking the primary content shell correctly so
that none of the tabs thought that they were the primary content shell. I'll see
if I can find a better way of tracking the primary content shell.

Comment 8

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

We could just get rid of GetContentShellById and replace the mapping with an
mPrimaryContentShell pointer.

Comment 9

[Boris Zbarsky [:bzbarsky]]

While we're here, the concept of "primary" content shell is very ill-defined...
Could we try to define what it really means?

Comment 10

[[e-mail]]

Attachment: Hold separate primary content shell reference

Strangely I had already started on this approach before seeing the last
comment.

Comment 11

[Dan M]

Comment on attachment 174917 [details] [diff] [review]
Hold separate primary content shell reference

Disagreeing with comment 6, GetContentShellById isn't unused; see
nsWebShellWindow.cpp. Or David are you saying that that caller function isn't
used any longer? I'm not certain, myself. Anyway y'all don't seem to be trying
to get rid of it, but you are trying to cut the strong refs on the content
shells, using instead the ID to identify the particular shell you want. But
that won't work.

IDs aren't unique among browser tabs. There should be one called
"primary-content" at any given time, and an arbitrary number called "content".
Even with just two tabs there must be a point while swapping the primary shell
(in response to the user selecting a previously unfocused tab, say) where both
have the same ID. The reason GetContentShellById (presumably) works using the
ID rather than a pointer to the specific shell is that it's probably used only
on simpler windows.

The lack of an nsIDocShellTreeOwner::contentShellRemoved seems an oversight. Or
barring that, what's wrong with making nsDocShell implement weakrefs?

Comment 12

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

(In reply to comment #11)
> (From update of attachment 174917 [details] [diff] [review] [edit])
> Disagreeing with comment 6, GetContentShellById isn't unused; see
> nsWebShellWindow.cpp. Or David are you saying that that caller function isn't
> used any longer? I'm not certain, myself. Anyway y'all don't seem to be trying
> to get rid of it, but you are trying to cut the strong refs on the content
> shells, using instead the ID to identify the particular shell you want. But
> that won't work.

Right, I missed that, but it doesn't have anything to do with this patch.

Comment 13

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

But I don't see any reason not to revert the change that made mContentShells a
pointer->id mapping instead of an id->pointer mapping, since the only thing it's
used for is mapping ids to pointers.  Adding ContentShellRemoved would be nice
too, but why maintain a linked list that can become *huge* (in the tabbrowser
case) when it's completely unused?

Comment 14

[Boris Zbarsky [:bzbarsky]]

> what's wrong with making nsDocShell implement weakrefs?

It already does.  That may be a good change to make too (in addition to this patch).

Comment 15

[Dan M]

DocShell implements weakrefs? Oh, through nsDocLoader. Missed that. So we could
fairly easily use weakrefs in the content shell array. That little array of
(weak)refs isn't much of a hit compared to the actual content, and it could be
pruned.

However I see you aren't saying so much that the content shell array is never
used, but that what it's used for isn't important? You may be right; I'm not
certain. Tell me you're certain and I'll waffle over to a +. (Does anyone know
all useful invocations of nsWebShellWindow::LoadContentAreas?)

Comment 16

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

I was wrong when I said it wasn't used.

What I'm saying is that it's used to map id -> pointer, so it's better off being
a map from id->pointer than pointer->id, since (due to lots of duplicate ids)
the map stays much smaller.  This also happens to fix most of the leaks.

Comment 17

[Boris Zbarsky [:bzbarsky]]

Dan, the current behavior of GetContentShellById is to return the first entry in
the array that has that ID.  Given that, there is no reason to store more than
one entry for a given ID.  Since the entries are never accessed directly (no
accesses to mContentShells outside nsXULWindow.cpp), it's enough to consider the
"public" api here -- GetContentShellById and GetPrimaryContentShell.  The patch
doesn't change behavior of either one.  So it doesn't change the behavior of
nsWebShellWindow::LoadContentAreas, in particular.

Comment 18

[Boris Zbarsky [:bzbarsky]]

Note: I filed bug 283056 on the question of what
nsWebShellWindow::LoadContentAreas is used for.

Comment 19

[Dan M]

Comment on attachment 174917 [details] [diff] [review]
Hold separate primary content shell reference

Alright, I'm seeing it now. Thanks for explaining. The new scheme has the
amusing side effect that the tabbed browser content shell answering to ID
"content" is now the most recently focused tab, rather than the first created.
Not that anybody cares. Put a comment in there somewhere that mContentShells is
no longer a list of all content shells at all, would you? You might even change
its name to mContentShellIDs. +=me

Comment 20

[Dan M]

> "content" is now the most recently focused tab,
most recent tab to have lost focus, that is

Comment 21

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

(In reply to comment #19)
> Not that anybody cares. Put a comment in there somewhere that mContentShells is
> no longer a list of all content shells at all, would you? You might even change
> its name to mContentShellIDs. +=me

Agreed, although it's worth nothing that "no longer" isn't really accurate --
it's only been that for a few days -- since bug 104532 landed.

Comment 22

[[e-mail]]

Fix checked in.

Comment 23

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

Attachment: use weak references too (checked in 2005-02-22 21:29 -0800)

This should cause memory to be freed earlier in some cases (when an ID goes
out-of-use, for example, when going from two tabs to one).

Not tested yet, but will shortly...

Comment 24

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 175260 [details] [diff] [review]
use weak references too (checked in 2005-02-22 21:29 -0800)

r+sr=bzbarsky

Comment 25

[R.K.Aa.]

*** Bug 282594 has been marked as a duplicate of this bug. ***

Comment 26

[R.K.Aa.]

*** Bug 282593 has been marked as a duplicate of this bug. ***

Comment 27

[Gulliver]

Very interesting!

https://bugzilla.mozilla.org/show_bug.cgi?id=283974 is a duplicate of
https://bugzilla.mozilla.org/show_bug.cgi?id=282593.
https://bugzilla.mozilla.org/show_bug.cgi?id=282593 itself is a duplicate of
https://bugzilla.mozilla.org/show_bug.cgi?id=282938.
https://bugzilla.mozilla.org/show_bug.cgi?id=282938 (this bug) has the status:
RESOLVED/FIXED since 2005-02-21. 

I wonder why we still have this problem?!?!?

Comment 28

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

If a bug was marked as duplicate incorrectly, reopen it or comment on it; don't
comment here.

Comment 29

[Sander]

*** Bug 282877 has been marked as a duplicate of this bug. ***