1349940 - DeleteRangeTransaction should not hold on to its range once it no longer needs it

Status: RESOLVED FIXED

(Core :: DOM: Editor, enhancement)

Description

[Boris Zbarsky [:bzbarsky]]

DeleteRangeTransaction stores a range to delete.  In DoTransaction, it converts that range into child transactions, but leaves the range alive.  Unfortunately, ranges are mutation observers, and keeping it alive forever means all mutations get slower and slower if we have a lot of DeleteRangeTransactions around.

The fix is simple: once we convert into child transactions, drop the range.  This gives me a nice 2x speedup on the testcase in bug 1346723.

Comment 1

[Boris Zbarsky [:bzbarsky]]

Attachment: DeleteRangeTransaction should not keep its range alive longer than it needs to

MozReview-Commit-ID: 9c1ZBPvDrLk

Comment 2

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Comment on attachment 8850531 [details] [diff] [review]
DeleteRangeTransaction should not keep its range alive longer than it needs to

> NS_IMETHODIMP
> DeleteRangeTransaction::DoTransaction()
> {
>-  MOZ_ASSERT(mRangeToDelete);
>+  // People probably shouldn't be calling DoTransaction more than once, which is
>+  // what a null mRangeToDelete would indicate.
>+  NS_ENSURE_TRUE(mRangeToDelete, NS_ERROR_UNEXPECTED);

Sorry, here is conflict with the fix of bug 1349138 (it's in mozilla-inbound now).

>+  // Swap mRangeToDelete out into a stack variable, so we make sure to null it
>+  // out on return from this function.  Once this function returns, we no longer
>+  // need mRangeToDelete.
>+  RefPtr<nsRange> rangeToDelete;
>+  rangeToDelete.swap(mRangeToDelete);

Sigh, then, we need the room to store RefPtr<nsRange> only during a call of the constructor and the call of DoTransaction(). I guess that following if/else should be moved to the constructor, but perhaps, I should do that in another bug.

>@@ -87,26 +95,22 @@ DeleteRangeTransaction::DoTransaction()
>   // else do nothing - dom range gravity will adjust selection
> 
>   return NS_OK;
> }
> 
> NS_IMETHODIMP
> DeleteRangeTransaction::UndoTransaction()
> {
>-  MOZ_ASSERT(mRangeToDelete);
>-

So, here is conflict with current mozilla-inbound, so, be careful at landing.

>   return EditAggregateTransaction::UndoTransaction();
> }
> 
> NS_IMETHODIMP
> DeleteRangeTransaction::RedoTransaction()
> {
>-  MOZ_ASSERT(mRangeToDelete);
>-

Same.

Comment 3

[Boris Zbarsky [:bzbarsky]]

> Sorry, here is conflict with the fix of bug 1349138 (it's in mozilla-inbound now).

Not a big deal.  I've merged with that; it more or less subsumed my first change to DoTransaction.  ;)

> we need the room to store RefPtr<nsRange> only during a call of the constructor and
> the call of DoTransaction()

Yes.

> I guess that following if/else should be moved to the constructor

We could try, but nominally it's fallible.  So we would need to deal with that somehow.

Comment 4

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/2c670b3e33db
DeleteRangeTransaction should not keep its range alive longer than it needs to.  r=masayuki, a=kwierso

Comment 5

[Boris Zbarsky [:bzbarsky]]

Doesn't seem to have affected speedometer much.

Comment 6

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/2c670b3e33db