Comment 1 Peter Van der Beken [:peterv] 2011-06-21 06:32:12 PDT

Comment on attachment 539541 [details] [diff] [review]
patch

Review of attachment 539541 [details] [diff] [review]:
-----------------------------------------------------------------

::: dom/base/nsGlobalWindow.cpp
@@ +1412,5 @@
>    NS_IMPL_CYCLE_COLLECTION_UNLINK_NSCOMPTR(mOpenerScriptPrincipal)
> +  if (tmp->mListenerManager) {
> +    tmp->mListenerManager->Disconnect();
> +    NS_IMPL_CYCLE_COLLECTION_UNLINK_NSCOMPTR(mListenerManager)
> +  }

I have to say I don't really like this. Either we let the CC unlink things or we do it manually, if we need to do both it means our CC traversal is still incomplete and we're just hiding that with manual unlinking.

@@ +10119,5 @@
> +                                                nsGlobalWindow)
> +  NS_IMPL_CYCLE_COLLECTION_UNLINK_NSCOMPTR(mBrowserDOMWindow)
> +  if (tmp->mMessageManager) {
> +    static_cast<nsFrameMessageManager*>(
> +      tmp->mMessageManager.get())->Disconnect();

Shouldn't this be done from the nsFrameMessageManager unlink code? What do the cycles that this is unlinking look like? And what are we missing to let the CC do the unlinking.

Comment 2 Olli Pettay [:smaug] (reviewing overload) 2011-06-21 06:40:50 PDT

> @@ +1412,5 @@
> >    NS_IMPL_CYCLE_COLLECTION_UNLINK_NSCOMPTR(mOpenerScriptPrincipal)
> > +  if (tmp->mListenerManager) {
> > +    tmp->mListenerManager->Disconnect();
> > +    NS_IMPL_CYCLE_COLLECTION_UNLINK_NSCOMPTR(mListenerManager)
> > +  }
> 
> I have to say I don't really like this. Either we let the CC unlink things
> or we do it manually, if we need to do both it means our CC traversal is
> still incomplete and we're just hiding that with manual unlinking.

Well, listenermanager asserts if it is deleted without disconnect.

And message manager should be disconnected when the owner doesn't have
pointer to it anymore.

Comment 3 Peter Van der Beken [:peterv] 2011-06-21 07:05:48 PDT

(In reply to comment #2)
> Well, listenermanager asserts if it is deleted without disconnect.

And right before the assert is a comment:

  // XXX azakai: Is there any reason to not just call Disconnect
  //             from right here, if not previously called?

So, any reason?

Comment 4 Olli Pettay [:smaug] (reviewing overload) 2011-06-21 08:21:22 PDT

http://hg.mozilla.org/mozilla-central/rev/629353683b7f