488799 - cycle collect editor transactions

Status: RESOLVED FIXED

(Core :: DOM: Editor, defect, P3)

Description

[David Baron :dbaron:]

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).

Comment 1

[David Baron :dbaron:]

Attachment: pre-patch 1: convert nsTransactionManager's mListeners from nsVoidArray* to nsCOMArray

Comment 2

[David Baron :dbaron:]

Attachment: pre-patch 2: Convert children of EditAggregateTxn from nsISupportsArray to nsTArray

Comment 3

[David Baron :dbaron:]

Attachment: patch

Comment 4

[Peter Van der Beken [:peterv]]

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?

Comment 5

[David Baron :dbaron:]

Attachment: patch

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.

Comment 6

[David Baron :dbaron:]

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.

Comment 7

[Peter Van der Beken [:peterv]]

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.

Comment 8

[David Baron :dbaron:]

(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.

Comment 9

[David Baron :dbaron:]

http://hg.mozilla.org/mozilla-central/rev/c7fb0978d1ee
http://hg.mozilla.org/mozilla-central/rev/c3b2c4b8dcd6

Comment 10

[David Baron :dbaron:]

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.

Comment 11

[David Baron :dbaron:]

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.

Comment 12

[David Baron :dbaron:]

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.

Comment 13

[David Baron :dbaron:]

http://hg.mozilla.org/releases/mozilla-1.9.1/rev/f1bff6bb901e
http://hg.mozilla.org/releases/mozilla-1.9.1/rev/0383336d8477
http://hg.mozilla.org/releases/mozilla-1.9.1/rev/0d898991dbd9
http://hg.mozilla.org/releases/mozilla-1.9.1/rev/69b776e23172