378987 - Don't try to collect documents, windows and elements of actively viewed documents

Status: RESOLVED FIXED

(Core :: DOM: Core & HTML, defect)

Description

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Attachment: Patch to fix

Patch coming up that makes us not traverse out of actively viewed elements, documents and windows.

I still haven't tested the performance winnings by this. I'll do that before requesting review

Comment 1

[Boris Zbarsky [:bzbarsky]]

>+  nsRefPtr<nsCCUncollectableMarker> marker = new nsCCUncollectableMarker;

Why not |nsCOMPtr<nsIObserver>| and save on the codesize hit?

I assume there's a reason you're not traversing the shentry tree?  I guess that might be ok while we don't bfcache subframe navigations....

>+  nsCOMPtr<nsIWindowMediator> med =
>+    do_GetService(NS_WINDOWMEDIATOR_CONTRACTID, &rv);

I think if this fails (as it will in embedding apps) you should get the list from the window watcher instead.  Yay dumb apis.  :(

Comment 2

[Boris Zbarsky [:bzbarsky]]

Actually, I'd check with Benjamin on whether in embedding builds the window mediator simply won't exist, or whether it'll exist but lie...

Also, XUL elements' GetCurrentDoc() lies, so you'll want to traverse those no matter what, no?

Comment 3

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

I did some testing on this on windows, and it doesn't look like we cyclecollect a lot through documents on windows at all. A breakpoint in nsDocuments traverse function doesn't get hit at all while i'm opening and closing tabs. Not sure if things are different on linux?

I did test that we mark the right documents though. Including the ones in the bfcache.

The reason i'm not traversing the shentry tree is that it doesn't look like for frameset documents we actually store the subframe documents in the shentry. Instead we in the top shentry store the list of sub-docshells. Traversing those sub-docshells seems to catch all the right documents. Even when we bfcache framesets (which it appears like we do).

Before going further I'd like to test on linux that we actually hit the relevant code.

Not sure what to do about XUL elements. They only lie when they get cloned, no? So possibly we could hope that such clones are usually inserted into the tree. I guess we could alternatively always traverse XUL elements, but that'll probably make this optimization mostly useless for chrome documents. Though I guess the real win is in not traversing the content documents anyway, so it might not be a big deal.

Comment 4

[Benjamin Smedberg]

windowmediator will exist and lie in embedding builds. In fact, you can't trust it to be complete in XUL apps either, because some windows may not be registered on purpose (e.g. the hiddenwindow)

Comment 5

[Boris Zbarsky [:bzbarsky]]

That's fine.  If we miss things, it just means we optimize less.  I'd say that if the window mediator gives you an empty enumerator, look at the window watcher itself.

> it doesn't look like we cyclecollect a lot through documents on windows at all

Try several windows where you mouse over one of them and open tabs in another one.  ;)  Though that will cycle-collect through the elements, mostly (the mouse move event targets).

> Instead we in the top shentry store the list of sub-docshells.

Right, because we don't bfcache subframe navigations yet.  If we ever fix that, we'll want to extend this code...

> They only lie when they get cloned, no?

Yes.  I wouldn't assume that they're inserted, esp. if someone is _trying_ to make us leak.

I think always traversing the chrome if it needs to be traversed should not be that bad; it's a lot smaller than typical content.

Comment 6

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

Maybe we should go back to relying on somebody calling GlobalWindowImpl(inner)::FreeInnerObjects or GlobalWindowImpl(outer)::SetDocShell(nsnull) (or something similar) to detect when documents are unneeded?  This ought to correspond to code that worked in the past for making sure we don't leak.

Comment 7

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Grr, missed the comment about the mediator existing but lying. Benjamin, how do I tell if I should use the mediator or the watcher? Can I somehow tell it's an embedding build? Or should I simply always walk both?

Comment 8

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Attachment: Iterate both

This fixes all comments so far. It always walks both the windowwatcher and the windowmediator. Not sure if this'll walk the windows twice in some builds, but performance isn't really an issue here since all this is much much faster than the cycle collection itself.

Would still like to get benjamins input on if this'll work.

Comment 9

[Boris Zbarsky [:bzbarsky]]

I think you can pretty much assume that if the mediator's enumerator is not empty, it's enough to walk that.  Walking both will basically walk the toplevel XUL windows in Firefox, then walk all the toplevel content windows again.  That said, I agree it's probably not really a performance issue either way, so in the interests of simplicity walking both is fine.

Comment 10

[Brendan Eich [:brendan]]

We need to fire up that RFC process for obsoleting APIs such as window mediator in Mozilla 2 (http://wiki.mozilla.org/Mozilla_2:Obsolete_APIs). Anyone volunteers to lead this, or perhaps in this case, just add this API to the wiki?

/be

Comment 11

[Boris Zbarsky [:bzbarsky]]

What do you plan to replace it with, exactly?

Comment 12

[Brendan Eich [:brendan]]

Window watcher was meant to replace it, IIRC (danm wrote window watcher). Why do we need two?

/be

Comment 13

[Boris Zbarsky [:bzbarsky]]

They enumerate different things, at the moment.  Window watcher enumerates toplevel non-chrome windows.  Window mediator enumerates toplevel chrome windows (in XUL apps only).

Comment 14

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 263827 [details] [diff] [review]
Iterate both

>Index: content/base/public/nsINode.h

> // Remaining bits are node type specific.
>-#define NODE_TYPE_SPECIFIC_BITS_OFFSET       0x0a
>+#define NODE_TYPE_SPECIFIC_BITS_OFFSET       0x0b

I assume you've verified that this doesn't affect any bits currently being used in a bad way, right?

>Index: content/base/src/nsCCUncollectableMarker.cpp
>+MarkDocShell(nsIDocShellTreeNode* aNode)
>+  nsCOMPtr<nsIDocShell> shell = do_QueryInterface(aNode);

I don't think you want to assume that that QI succeeds...  It might stop doing that once nsWebBrowser implements nsIDocShellTreeNode.  So rather bail on null |shell| instead of null |aNode|?

>+      nsCOMPtr<nsIHistoryEntry> historyEntry;
>+      history->GetEntryAtIndex(i, PR_FALSE, getter_AddRefs(historyEntry));
>+      nsCOMPtr<nsISHEntry> shEntry = do_QueryInterface(historyEntry);
>+
>+      MarkSHEntry(shEntry);

I wouldn't assume that GetEntryAtIndex didn't hand back null... Add a null-check, either here or in MarkSHEntry?  Maybe it's just my distrust of session history in general... :(

>Index: content/xul/content/src/nsXULElement.cpp

So... I just realized that when a node gets appended as a child to a lying XUL element, it'll end up lying too.  We should propagate the NODE_HAS_FAKED_INDOC bit when we propagate PARENT_BIT_INDOCUMENT in various BindToTree impls...

>Index: xpcom/base/nsCycleCollector.cpp
>+#define COLLECT_TIME_DEBUG

Don't check that in.

r=bzbarsky with the nits addressed, esp the NODE_HAS_FAKED_INDOC thing.

Comment 15

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

(In reply to comment #14)
> (From update of attachment 263827 [details] [diff] [review])
> >Index: content/base/public/nsINode.h
> 
> > // Remaining bits are node type specific.
> >-#define NODE_TYPE_SPECIFIC_BITS_OFFSET       0x0a
> >+#define NODE_TYPE_SPECIFIC_BITS_OFFSET       0x0b
> 
> I assume you've verified that this doesn't affect any bits currently being 
> used in a bad way, right?

Not sure what you mean. This is just the offset where some nodes store node-specific info. No-one needs more bits than will fit if that is what you mean.

> >Index: content/xul/content/src/nsXULElement.cpp
> 
> So... I just realized that when a node gets appended as a child to a lying XUL
> element, it'll end up lying too.  We should propagate the NODE_HAS_FAKED_INDOC
> bit when we propagate PARENT_BIT_INDOCUMENT in various BindToTree impls...

Doh, good catch. That also means I should change the comment for that flag since any node can fake its in-doc flag. Which further means that everywhere where we assume that only XUL elements have this quirk is wrong :( That's a different bug of course.

Comment 16

[Brendan Eich [:brendan]]

(In reply to comment #13)
> They enumerate different things, at the moment.  Window watcher enumerates
> toplevel non-chrome windows.  Window mediator enumerates toplevel chrome
> windows (in XUL apps only).

Does it seem worth having one interface to handle window enumeration? If not, at least the names could be better. Off topic, I'll follow up elsewhere but wanted to close this off with your thoughts (bz especially).

/be

Comment 17

[Boris Zbarsky [:bzbarsky]]

I'd be much happier with a single interface with two different clearly named methods, that's for sure.

Comment 18

[Boris Zbarsky [:bzbarsky]]

> No-one needs more bits than will fit if that is what you mean.

Yeah, that's what I meant.

By the way, I just checked in bug 377303, so you no longer need to QI to get from an nsIDocShellTreeItem to an nsIDocShellTreeNode.  ;)

Comment 19

[Boris Zbarsky [:bzbarsky]]

> Which further means that everywhere where we assume that only XUL elements have
> this quirk is wrong :(

Yes.  We have some bugs on the resulting asserts, actually.  :(

I still think that fixing the quirk is the way to go, frankly; it's easier than working around it. I would go ahead and check in this patch as-is, then fix the quirk.  I'm volunteering to do that part, if you care.

Comment 20

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Attachment: Addresses bzs review comments

This should do it.

Comment 21

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Leaks went through the roof with this landed so I backed out for now. I suspect it's some sort of shut-down issue, possibly we're keeping the documents alive past the last cycle collection.

I also want to make sure that we never accidentally end up with the generation matching, so i'll init it to 1 and then count from 0xffffffff to 1.

Will investigate more tomorrow.

Comment 22

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Attachment: Fixes leaks

This fixes the leaks on my machine. What seems to happen is that even during the final CC the windowmediator is alive and kickin' reporting windows which makes me mark the documents as uncollectable. This patch registeres a listener for "xpcom-shutdown" and kills the marking-observer at that point.

I also fixed a theoretical issue where documents could think they were marked if we never ran the nsCCUncollectableMarker or if we ran it exactly N*2^32 times.

Comment 23

[Johnny Stenback (:jst)]

Comment on attachment 264285 [details] [diff] [review]
Fixes leaks

r+sr=jst

Comment 24

[Boris Zbarsky [:bzbarsky]]

While you're there, s/effectivly/effectively/ ?

Comment 25

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

This is finally in. The last backout was bogus, apparently it was just a flaky tinderbox that turned orange.