Last Comment Bug 664452 - Unlink content/xul more
: Unlink content/xul more
Status: RESOLVED FIXED
:
Product: Core
Classification: Components
Component: XUL (show other bugs)
: unspecified
: x86_64 Linux
: -- normal with 1 vote (vote)
: ---
Assigned To: Olli Pettay [:smaug] (vacation Aug 25-28)
:
Mentors:
Depends on:
Blocks: strongparent
  Show dependency treegraph
 
Reported: 2011-06-15 08:57 PDT by Olli Pettay [:smaug] (vacation Aug 25-28)
Modified: 2011-07-01 15:02 PDT (History)
2 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
patch (14.27 KB, patch)
2011-06-15 08:57 PDT, Olli Pettay [:smaug] (vacation Aug 25-28)
no flags Details | Diff | Splinter Review
patch (13.05 KB, patch)
2011-06-29 06:31 PDT, Olli Pettay [:smaug] (vacation Aug 25-28)
peterv: review+
Details | Diff | Splinter Review

Description Olli Pettay [:smaug] (vacation Aug 25-28) 2011-06-15 08:57:49 PDT
Created attachment 539552 [details] [diff] [review]
patch
Comment 1 Peter Van der Beken [:peterv] 2011-06-21 06:44:54 PDT
Comment on attachment 539552 [details] [diff] [review]
patch

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

Looks great, except for nsXULDocument::Destroy.

::: content/xul/document/src/nsXULDocument.cpp
@@ +636,5 @@
>      OnPrototypeLoadDone(PR_TRUE);
>  }
>  
> +void
> +nsXULDocument::Destroy()

We should be removing Destroy, not add more to it. Why doesn't the existing unlinking code work here?

::: content/xul/document/src/nsXULPrototypeDocument.cpp
@@ +198,5 @@
>                                                      nsXULPrototypeElement)
>      cb.NoteXPCOMChild(static_cast<nsIScriptGlobalObject*>(tmp->mGlobalObject));
>      NS_IMPL_CYCLE_COLLECTION_TRAVERSE_NATIVE_MEMBER(mNodeInfoManager,
>                                                      nsNodeInfoManager)
> +    for (PRUint32 i = 0; i < tmp->mPrototypeWaiters.Length(); ++i) {

You need to add NS_CYCLE_COLLECTION_NOTE_EDGE_NAME here.
Comment 2 Olli Pettay [:smaug] (vacation Aug 25-28) 2011-06-21 06:59:56 PDT
(In reply to comment #1)
> > +void
> > +nsXULDocument::Destroy()
> 
> We should be removing Destroy, not add more to it. Why doesn't the existing
> unlinking code work here?
Actually, I think that is a leftover from some earlier state of the
patch. I'll re-test.
Comment 3 Olli Pettay [:smaug] (vacation Aug 25-28) 2011-06-29 06:31:13 PDT
Created attachment 542798 [details] [diff] [review]
patch
Comment 4 Peter Van der Beken [:peterv] 2011-06-29 07:41:31 PDT
Comment on attachment 542798 [details] [diff] [review]
patch

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

::: content/xul/document/src/nsXULPrototypeDocument.cpp
@@ +200,1 @@
>      cb.NoteXPCOMChild(static_cast<nsIScriptGlobalObject*>(tmp->mGlobalObject));

Might be able to use NS_IMPL_CYCLE_COLLECTION_TRAVERSE_NSCOMPTR_AMBIGUOUS here?

@@ +201,5 @@
>      NS_IMPL_CYCLE_COLLECTION_TRAVERSE_NATIVE_MEMBER(mNodeInfoManager,
>                                                      nsNodeInfoManager)
> +    for (PRUint32 i = 0; i < tmp->mPrototypeWaiters.Length(); ++i) {
> +        NS_CYCLE_COLLECTION_NOTE_EDGE_NAME(cb, "mPrototypeWaiters[i]");
> +        cb.NoteXPCOMChild(static_cast<nsINode*>(tmp->mPrototypeWaiters[i].get()));

Same here? Not sure, so no big deal if you can't.
Comment 5 Olli Pettay [:smaug] (vacation Aug 25-28) 2011-07-01 15:02:33 PDT
http://hg.mozilla.org/mozilla-central/rev/6cf983334e7a

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