The default bug view has changed. See this FAQ.

Unlink nsGlobalChromeWindow

RESOLVED FIXED

Status

()

Core
DOM
RESOLVED FIXED
6 years ago
6 years ago

People

(Reporter: smaug, Assigned: smaug)

Tracking

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment)

Created attachment 539541 [details] [diff] [review]
patch

The patch does also unlink mDoc
and calls ELM->Disconnect() properly.
Attachment #539541 - Flags: review?(peterv)
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.
> @@ +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.
(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?
Attachment #539541 - Flags: review?(peterv) → review+
http://hg.mozilla.org/mozilla-central/rev/629353683b7f
Status: NEW → RESOLVED
Last Resolved: 6 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.