3.74 KB, text/plain; charset=UTF-8
1.89 KB, text/plain; charset=UTF-8
5.33 KB, patch
|Details | Diff | Splinter Review|
5.93 KB, patch
|Details | Diff | Splinter Review|
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.)
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.
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?).
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?
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.
Attachment #174917 - Flags: superreview?(dbaron) → superreview+
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] ) > 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).
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 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+
> "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.
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...
Comment on attachment 175260 [details] [diff] [review] use weak references too (checked in 2005-02-22 21:29 -0800) r+sr=bzbarsky
Attachment #175260 - Attachment description: use weak references too → use weak references too (checked in 2005-02-22 21:29 -0800)
*** Bug 282594 has been marked as a duplicate of this bug. ***
*** Bug 282593 has been marked as a duplicate of this bug. ***
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.
*** Bug 282877 has been marked as a duplicate of this bug. ***
You need to log in before you can comment on or make changes to this bug.