cycle collect editor transactions

RESOLVED FIXED in mozilla1.9.1

Status

()

defect
P3
normal
RESOLVED FIXED
10 years ago
10 years ago

People

(Reporter: dbaron, Assigned: dbaron)

Tracking

({fixed1.9.1})

Trunk
mozilla1.9.1
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(3 attachments, 1 obsolete attachment)

I have a series of patches to cycle collect editor transactions.  This seems important since content nodes can own editors, and editors, and transactions, can own content nodes.

Along with the patch in bug 423233 this seems to fix bug 420670 (although I thought the other patch alone did in some cases when I was testing a few days ago, but I can no longer reproduce any case where it helps).
Posted patch patch (obsolete) β€” β€” Splinter Review
Attachment #373262 - Flags: superreview?(peterv)
Attachment #373262 - Flags: review?(peterv)
Attachment #373259 - Flags: superreview?(peterv)
Attachment #373259 - Flags: superreview+
Attachment #373259 - Flags: review?(peterv)
Attachment #373259 - Flags: review+
Comment on attachment 373260 [details] [diff] [review]
pre-patch 2: Convert children of EditAggregateTxn from nsISupportsArray to nsTArray

> NS_IMETHODIMP EditAggregateTxn::GetTxnAt(PRInt32 aIndex, EditTxn **aTxn)

>   // ugh, this is all wrong - what a mess we have with editor transaction interfaces
>-  mChildren->QueryElementAt(aIndex, EditTxn::GetCID(), (void**)aTxn);
>+  *aTxn = mChildren[aIndex];
>   if (!*aTxn)
>     return NS_ERROR_UNEXPECTED;
>   return NS_OK;

Doesn't this need to AddRef?
Attachment #373260 - Flags: superreview?(peterv)
Attachment #373260 - Flags: superreview+
Attachment #373260 - Flags: review?(peterv)
Attachment #373260 - Flags: review+
Posted patch patch β€” β€” Splinter Review
I just noticed I missed two transactions; this should be the same as the previous patch except that the changes in nsStyleSheetTxns.{h,cpp} are added.
Attachment #373262 - Attachment is obsolete: true
Attachment #374316 - Flags: superreview?(peterv)
Attachment #374316 - Flags: review?(peterv)
Attachment #373262 - Flags: superreview?(peterv)
Attachment #373262 - Flags: review?(peterv)
Landed the 2 pre-patches:

http://hg.mozilla.org/mozilla-central/rev/a95a2fe899a9
http://hg.mozilla.org/mozilla-central/rev/30936df0271e

I fixed GetTxnAt to AddRef... although I now realize I should have just removed it.  (I meant to check that earlier.)  It's not called.
Attachment #374316 - Flags: superreview?(peterv)
Attachment #374316 - Flags: superreview+
Attachment #374316 - Flags: review?(peterv)
Attachment #374316 - Flags: review+
Comment on attachment 374316 [details] [diff] [review]
patch

>diff --git a/editor/libeditor/base/ChangeAttributeTxn.cpp b/editor/libeditor/base/ChangeAttributeTxn.cpp

>+NS_IMPL_CYCLE_COLLECTION_CLASS(ChangeAttributeTxn)
>+
>+NS_IMPL_CYCLE_COLLECTION_UNLINK_BEGIN_INHERITED(ChangeAttributeTxn, EditTxn)
>+  NS_IMPL_CYCLE_COLLECTION_UNLINK_NSCOMPTR(mElement)
>+NS_IMPL_CYCLE_COLLECTION_UNLINK_END
>+
>+NS_IMPL_CYCLE_COLLECTION_TRAVERSE_BEGIN_INHERITED(ChangeAttributeTxn, EditTxn)
>+  NS_IMPL_CYCLE_COLLECTION_TRAVERSE_NSCOMPTR(mElement)
>+NS_IMPL_CYCLE_COLLECTION_TRAVERSE_END

Some of these could use NS_IMPL_CYCLE_COLLECTION_*, up to you.

>diff --git a/editor/libeditor/base/EditAggregateTxn.cpp b/editor/libeditor/base/EditAggregateTxn.cpp

>+NS_IMPL_CYCLE_COLLECTION_UNLINK_BEGIN_INHERITED(EditAggregateTxn, EditTxn)
>+  tmp->mChildren.Clear();
>+NS_IMPL_CYCLE_COLLECTION_UNLINK_END
>+
>+NS_IMPL_CYCLE_COLLECTION_TRAVERSE_BEGIN_INHERITED(EditAggregateTxn, EditTxn)
>+  for (PRUint32 i = 0; i < tmp->mChildren.Length(); ++i) {
>+    NS_CYCLE_COLLECTION_NOTE_EDGE_NAME(cb, "mChildren[i]");
>+    cb.NoteXPCOMChild(static_cast<nsITransaction*>(tmp->mChildren[i]));
>+  }
>+NS_IMPL_CYCLE_COLLECTION_TRAVERSE_END

And this could use NS_IMPL_CYCLE_COLLECTION_UNLINK_NSCOMARRAY and NS_IMPL_CYCLE_COLLECTION_TRAVERSE_NSCOMARRAY I think, although I guess that might be confusing as this is a nsTArray< nsRefPtr<...> >.

I'd drop the Do prefixes wherever you have DoTraverse/DoUnlink, but up to you.
(In reply to comment #7)
> And this could use NS_IMPL_CYCLE_COLLECTION_UNLINK_NSCOMARRAY and
> NS_IMPL_CYCLE_COLLECTION_TRAVERSE_NSCOMARRAY I think, although I guess that
> might be confusing as this is a nsTArray< nsRefPtr<...> >.

That was why I didn't want to use it.  I think it also might not quite work, e.g., for Length() vs. Count().

> I'd drop the Do prefixes wherever you have DoTraverse/DoUnlink, but up to you.

I wanted something to point out that the object with that method is not actually a participant in cycle collection, but is fully owned by an object that does, so this is a helper method for the owner object.  I thought we'd use Do as a prefix for that elsewhere, although maybe that was just me, and I'd be open to better suggestions, although I do want some distinguishing feature in the name.
http://hg.mozilla.org/mozilla-central/rev/c7fb0978d1ee
http://hg.mozilla.org/mozilla-central/rev/c3b2c4b8dcd6
Status: ASSIGNED → RESOLVED
Closed: 10 years ago
Priority: -- → P3
Resolution: --- → FIXED
Comment on attachment 373259 [details] [diff] [review]
pre-patch 1: convert nsTransactionManager's mListeners from nsVoidArray* to nsCOMArray

These patches make leak-monitor and DEBUG_CC a good bit less noisy, and may well fix some more permanent leaks.
Attachment #373259 - Flags: approval1.9.1?
Attachment #373260 - Flags: approval1.9.1?
Comment on attachment 374316 [details] [diff] [review]
patch

These patches make leak-monitor and DEBUG_CC a good bit less noisy, and may well fix some more permanent leaks.
Attachment #374316 - Flags: approval1.9.1?
Comment on attachment 373259 [details] [diff] [review]
pre-patch 1: convert nsTransactionManager's mListeners from nsVoidArray* to nsCOMArray

Actually, these don't need approval since they block a blocking1.9.1+ bug.
Attachment #373259 - Flags: approval1.9.1?
Attachment #373260 - Flags: approval1.9.1?
Attachment #374316 - Flags: approval1.9.1?
You need to log in before you can comment on or make changes to this bug.