nsXULTemplateBuilder should unlink more

RESOLVED FIXED

Status

()

defect
RESOLVED FIXED
10 years ago
9 years ago

People

(Reporter: smaug, Assigned: smaug)

Tracking

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(2 attachments, 1 obsolete attachment)

Posted patch patch (obsolete) — Splinter Review
Split from bug 335998.
Attachment #406999 - Flags: review?(peterv)
Comment on attachment 406999 [details] [diff] [review]
patch

>diff --git a/content/xul/templates/src/nsXULTemplateBuilder.cpp b/content/xul/templates/src/nsXULTemplateBuilder.cpp

>+      tmp->mMatchMap.EnumerateRead(DestroyMatchList, &(tmp->mPool));
>+      tmp->mMatchMap.Clear();

Can we make DestroyMatchList return PL_DHASH_REMOVE instead of making every caller call Clear and effectively enumerating twice?

>diff --git a/content/xul/templates/src/nsXULTemplateQueryProcessorXML.cpp b/content/xul/templates/src/nsXULTemplateQueryProcessorXML.cpp

>     NS_IMPL_CYCLE_COLLECTION_UNLINK_NSCOMPTR(mTemplateBuilder)
>     NS_IMPL_CYCLE_COLLECTION_UNLINK_NSCOMPTR(mRequest)
>+    NS_IMPL_CYCLE_COLLECTION_UNLINK_NSCOMPTR(mRoot)
>+    NS_IMPL_CYCLE_COLLECTION_UNLINK_NSCOMPTR(mEvaluator)

Maybe use the same order that these are declared in.

Looks like we also need to hook up nsXMLBindingSet and nsXMLBinding, and mRuleToBindingsMap (both keys and values).
Attachment #406999 - Flags: review?(peterv) → review-
Comment on attachment 406999 [details] [diff] [review]
patch

New patch coming
Attachment #406999 - Attachment is obsolete: true
Posted patch patchSplinter Review
Unlink even more.
I took the nsDocument.cpp part from Bug 335998.
I could file a new bug for that if really needed.
Attachment #438744 - Flags: review?(peterv)
I posted the patch to tryserver, though I did run
template tests locally.
Comment on attachment 438744 [details] [diff] [review]
patch

>diff --git a/content/base/src/nsDocument.cpp b/content/base/src/nsDocument.cpp

>+  if (tmp->mBoxObjectTable) {
>+   tmp->mBoxObjectTable->EnumerateRead(ClearAllBoxObjects, nsnull);
>+   delete tmp->mBoxObjectTable;
>+   tmp->mBoxObjectTable = nsnull;
>+ }

My worry here was/is that maybe something needs their boxobject when they are unlinked or being destroyed from unlinking.

>diff --git a/content/xul/templates/src/nsXULTemplateQueryProcessorXML.cpp b/content/xul/templates/src/nsXULTemplateQueryProcessorXML.cpp

>+TraverseRuleToBindingsMap(nsISupports* aKey, nsXMLBindingSet* aMatch, void* aContext)

>+    cb->NoteXPCOMChild(aKey);
>+    cb->NoteNativeChild(aMatch, &NS_CYCLE_COLLECTION_NAME(nsXMLBindingSet));

Please add NS_CYCLE_COLLECTION_NOTE_EDGE_NAME for both.
Attachment #438744 - Flags: review?(peterv) → review+
I need to remember to land this
Posted patch up-to-dateSplinter Review
Attachment #463259 - Flags: approval2.0?
Attachment #463259 - Flags: approval2.0? → approval2.0+
http://hg.mozilla.org/mozilla-central/rev/b99eafbd3644
Status: NEW → RESOLVED
Last Resolved: 9 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.