Closed
Bug 664444
Opened 14 years ago
Closed 14 years ago
Unlink nsGlobalChromeWindow
Categories
(Core :: DOM: Core & HTML, defect)
Tracking
()
RESOLVED
FIXED
People
(Reporter: smaug, Assigned: smaug)
References
Details
Attachments
(1 file)
3.31 KB,
patch
|
peterv
:
review+
|
Details | Diff | Splinter Review |
The patch does also unlink mDoc
and calls ELM->Disconnect() properly.
Attachment #539541 -
Flags: review?(peterv)
Comment 1•14 years ago
|
||
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.
Assignee | ||
Comment 2•14 years ago
|
||
> @@ +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•14 years ago
|
||
(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?
Updated•14 years ago
|
Attachment #539541 -
Flags: review?(peterv) → review+
Assignee | ||
Comment 4•14 years ago
|
||
Status: NEW → RESOLVED
Closed: 14 years ago
Resolution: --- → FIXED
Updated•6 years ago
|
Component: DOM → DOM: Core & HTML
You need to log in
before you can comment on or make changes to this bug.
Description
•