Last Comment Bug 664444 - Unlink nsGlobalChromeWindow
: Unlink nsGlobalChromeWindow
Status: RESOLVED FIXED
:
Product: Core
Classification: Components
Component: DOM (show other bugs)
: unspecified
: x86_64 Linux
: -- normal with 1 vote (vote)
: ---
Assigned To: Olli Pettay [:smaug] (high review load, please consider other reviewers)
:
Mentors:
Depends on:
Blocks: strongparent
  Show dependency treegraph
 
Reported: 2011-06-15 08:33 PDT by Olli Pettay [:smaug] (high review load, please consider other reviewers)
Modified: 2011-06-21 09:58 PDT (History)
4 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
patch (3.31 KB, patch)
2011-06-15 08:33 PDT, Olli Pettay [:smaug] (high review load, please consider other reviewers)
peterv: review+
Details | Diff | Review

Description Olli Pettay [:smaug] (high review load, please consider other reviewers) 2011-06-15 08:33:47 PDT
Created attachment 539541 [details] [diff] [review]
patch

The patch does also unlink mDoc
and calls ELM->Disconnect() properly.
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] (high review load, please consider other reviewers) 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] (high review load, please consider other reviewers) 2011-06-21 08:21:22 PDT
http://hg.mozilla.org/mozilla-central/rev/629353683b7f

Note You need to log in before you can comment on or make changes to this bug.