Closed Bug 1349940 Opened 3 years ago Closed 3 years ago

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

Categories

(Core :: DOM: Editor, enhancement)

enhancement
Not set

Tracking

()

RESOLVED FIXED
mozilla55
Tracking Status
firefox55 --- fixed

People

(Reporter: bzbarsky, Assigned: bzbarsky)

References

(Blocks 1 open bug)

Details

Attachments

(1 file)

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.
Blocks: 1346723
MozReview-Commit-ID: 9c1ZBPvDrLk
Attachment #8850531 - Flags: review?(masayuki)
Assignee: nobody → bzbarsky
Status: NEW → ASSIGNED
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.
Attachment #8850531 - Flags: review?(masayuki) → review+
> 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.
Pushed by bzbarsky@mozilla.com:
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
Doesn't seem to have affected speedometer much.
https://hg.mozilla.org/mozilla-central/rev/2c670b3e33db
Status: ASSIGNED → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla55
You need to log in before you can comment on or make changes to this bug.