443483 - Closed windows need two cycle collections to be collected

Status: RESOLVED FIXED

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

Description

[Peter Van der Beken [:peterv]]

Attachment: v1

I'm seeing windows stay alive through a cycle collection, even though they really can be collected. The cycle collector even considers them to be white (so they're in a cycle that has no outstanding references). I need to untangle my tree, but I think I have a fix: we're currently not collecting XPCWrappedJS objects that something holds a weak reference too. Fixing that seems to make the closed windows get collected by the next cycle collection.

Comment 1

[Peter Van der Beken [:peterv]]

Attachment: v2

Also need to collect nsTimeout objects.

Comment 2

[Ryan VanderMeulen [:RyanVM]]

Still valid, peterv?

Comment 3

[Peter Van der Beken [:peterv]]

Comment on attachment 328322 [details] [diff] [review]
v2

I'd forgotten about this, we should try to get this into 1.9.1.

Comment 4

[Ben Turner (not reading bugmail, use the needinfo flag!)]

Comment on attachment 328322 [details] [diff] [review]
v2

>+    cb.NoteNativeChild(timeout, &NS_CYCLE_COLLECTION_NAME(nsTimeout));
>+  }
>+
>+

Nit: one too many empty lines here.


> NS_IMPL_CYCLE_COLLECTION_ROOT_BEGIN(nsXPCWrappedJS)
>-    if(tmp->mRoot && !tmp->mRoot->HasWeakReferences() && tmp->IsValid())
>+    if(tmp->mRoot && tmp->IsValid())

It looks to me like mRoot can only ever be null if we've already called Unlink, in which case I wouldn't think it should be possible to end up in another call to Root. Can you remove that null check too?

r=me.

Comment 5

[Peter Van der Beken [:peterv]]

Comment on attachment 328322 [details] [diff] [review]
v2

This fixes some window leaks that are not shutdown leaks, just windows staying alive too long. Should be fairly safe, especially because it doesn't add unlink code (which is the part of cycle collection that causes the most crashes). Our current test infrastructure only allows us to test for shutdown leaks, so I can't write a test for this. Hopefully that'll change soon with the fix for bug 469779.