nsContentShellInfo holds tabbrowser webshells until parent window destroyed

RESOLVED FIXED

Status

()

RESOLVED FIXED
14 years ago
13 years ago

People

(Reporter: dbaron, Assigned: neil)

Tracking

({memory-leak})

Trunk
memory-leak
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(4 attachments)

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.)
Created attachment 174880 [details]
IRC discussion on developers
Flags: blocking-aviary1.1?
Keywords: mlk
And, FWIW, this WEBSHELL leak doesn't happen on the aviary 1.0 branch (although
a DOMWINDOW leak does).
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.
... a change made in revision 1.151 of nsXULWindow.cpp for bug 104532.
Created attachment 174889 [details]
IRC discussion on developers (2)
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?).
(Assignee)

Comment 7

14 years ago
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.
We could just get rid of GetContentShellById and replace the mapping with an
mPrimaryContentShell pointer.
While we're here, the concept of "primary" content shell is very ill-defined...
Could we try to define what it really means?
(Assignee)

Comment 10

14 years ago
Created attachment 174917 [details] [diff] [review]
Hold separate primary content shell reference

Strangely I had already started on this approach before seeing the last
comment.
Assignee: nobody → neil.parkwaycc.co.uk
Status: NEW → ASSIGNED
Attachment #174917 - Flags: superreview?(dbaron)
Attachment #174917 - Flags: review?(danm.moz)
Attachment #174917 - Flags: superreview?(dbaron) → superreview+
Blocks: 282877

Comment 11

14 years ago
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?
Attachment #174917 - Flags: review?(danm.moz) → review-
(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.
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?
> 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

14 years ago
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?)
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.
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.
Note: I filed bug 283056 on the question of what
nsWebShellWindow::LoadContentAreas is used for.

Comment 19

14 years ago
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
Attachment #174917 - Flags: review- → review+

Comment 20

14 years ago
> "content" is now the most recently focused tab,
most recent tab to have lost focus, that is
(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.
(Assignee)

Comment 22

14 years ago
Fix checked in.
Status: ASSIGNED → RESOLVED
Last Resolved: 14 years ago
Resolution: --- → FIXED
Created attachment 175260 [details] [diff] [review]
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...
Attachment #175260 - Flags: superreview?(bzbarsky)
Attachment #175260 - Flags: review?(bzbarsky)
Comment on attachment 175260 [details] [diff] [review]
use weak references too (checked in 2005-02-22 21:29 -0800)

r+sr=bzbarsky
Attachment #175260 - Flags: superreview?(bzbarsky)
Attachment #175260 - Flags: superreview+
Attachment #175260 - Flags: review?(bzbarsky)
Attachment #175260 - Flags: review+
Attachment #175260 - Attachment description: use weak references too → use weak references too (checked in 2005-02-22 21:29 -0800)

Comment 25

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

Comment 26

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

Comment 27

14 years ago
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?!?!?
If a bug was marked as duplicate incorrectly, reopen it or comment on it; don't
comment here.

Updated

14 years ago
No longer blocks: 282877

Comment 29

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

Updated

14 years ago
Flags: blocking-aviary1.1?
You need to log in before you can comment on or make changes to this bug.