766845 - "ASSERTION: bad aEndOffset" (and crash) when inserthtml deletes multiple ranges

Status: RESOLVED FIXED

(Core :: DOM: Editor, defect)

Description

[Jesse Ruderman]

Attachment: non-crashing testcase

This testcase trips an assert in DeleteRangeTxn::CreateTxnsToDeleteBetween:

###!!! ASSERTION: bad aEndOffset: 'aEndOffset <= startParent->GetChildCount()', file editor/libeditor/base/DeleteRangeTxn.cpp, line 226

A variant also crashes later in the function, with null |child| at:

234	    result = txn->Init(mEditor, child->AsDOMNode(), mRangeUpdater);

Comment 1

[Aryeh Gregor (:ayg) (no longer with Mozilla)]

Attachment: Patch part 1, v1 -- Clean up DeleteRangeTxn

One thing makes me nervous about this patch -- bug 574558 seems to have deliberately returned NS_OK if GetSelection() returned an error, but returned NS_ERROR_NULL_POINTER if GetSelection() returned NS_OK but a null pointer.  This patch changes it so that it will always return NS_ERROR_NULL_POINTER.  Should I change it to NS_ENSURE_TRUE(selection, NS_OK)?  I'm not clear why NS_OK had to be returned here anyway; I didn't get why returning an error isn't fine here.  This patch passes bug 574558's test, obviously.

Comment 2

[Aryeh Gregor (:ayg) (no longer with Mozilla)]

Attachment: Patch part 2, v1 -- Make CloneRange() return already_AddRefed<nsRange>

It always returns NS_OK unless it's passed a null pointer, so it doesn't need to return nsresult -- just return the pointer directly.

Comment 3

[Aryeh Gregor (:ayg) (no longer with Mozilla)]

Attachment: Patch part 3, v1 -- Fix assertion in DeleteRangeTxn

The problem in this case is that with a multi-range selection, we make multiple DeleteRangeTxns, then run them in sequence.  The DeleteRangeTxn stores the start/end nodes and offsets directly in Init, so deleting the first range in the selection might make the offsets invalid by the time DoTransaction runs.  This patch has it store only a clone of the range and retrieve the endpoints at DoTransaction time, so they'll have been adjusted by range gravity and will still be valid.  This actually cuts down on code a bunch too.

Try: https://tbpl.mozilla.org/?tree=Try&rev=3d42b77fab79

Comment 4

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 636269 [details] [diff] [review]
Patch part 2, v1 -- Make CloneRange() return already_AddRefed<nsRange>


> NS_IMETHODIMP
> nsRange::CloneRange(nsIDOMRange** aReturn)
> {
>-  nsRefPtr<nsRange> range;
>-  nsresult rv = CloneRange(getter_AddRefs(range));
>+  nsRefPtr<nsRange> range = CloneRange();
>   range.forget(aReturn);
>-  return rv;
>+  return NS_OK;
> }

I think the following would be simpler.
NS_IMETHODIMP
nsRange::CloneRange(nsIDOMRange** aReturn)
{
  *aReturn = CloneRange().get()
  return NS_OK;
}

Comment 5

[Aryeh Gregor (:ayg) (no longer with Mozilla)]

The third patch here causes an unexpected fail in test_fromUserInput.html, which was added by bug 686909:

4888 ERROR TEST-UNEXPECTED-FAIL | chrome://mochitests/content/a11y/accessible/events/test_fromUserInput.html | Wrong length for 'div' - got 2, expected 3
4890 ERROR TEST-UNEXPECTED-FAIL | chrome://mochitests/content/a11y/accessible/events/test_fromUserInput.html | Wrong removed text for 'div' - got he, expected hel

Looking at the test source code, I find:

  var selection = window.getSelection();
  var range = document.createRange();
  range.setStart(this.textNode, aStart);
  range.setEnd(this.textNode, aEnd-1);
  selection.addRange(range);

  synthesizeKey("VK_DELETE", {});

I.e., it selects the text from aStart to *aEnd - 1*, and then synthesizes delete, then expects that the text from aStart to *aEnd* is deleted.  Does anyone know why this ever worked?  And why would this patch change anything?  If I change it from aEnd - 1 to aEnd, it passes, but it fails without this patch -- which suggests I'm unintentionally changing behavior somehow.

Comment 6

[Trevor Saunders (:tbsaunde)]

(In reply to Aryeh Gregor from comment #5)
> The third patch here causes an unexpected fail in test_fromUserInput.html,
> which was added by bug 686909:
> 
> 4888 ERROR TEST-UNEXPECTED-FAIL |
> chrome://mochitests/content/a11y/accessible/events/test_fromUserInput.html |
> Wrong length for 'div' - got 2, expected 3
> 4890 ERROR TEST-UNEXPECTED-FAIL |
> chrome://mochitests/content/a11y/accessible/events/test_fromUserInput.html |
> Wrong removed text for 'div' - got he, expected hel
> 
> Looking at the test source code, I find:
> 
>   var selection = window.getSelection();
>   var range = document.createRange();
>   range.setStart(this.textNode, aStart);
>   range.setEnd(this.textNode, aEnd-1);
>   selection.addRange(range);
> 
>   synthesizeKey("VK_DELETE", {});
> 
> I.e., it selects the text from aStart to *aEnd - 1*, and then synthesizes
> delete, then expects that the text from aStart to *aEnd* is deleted.  Does
> anyone know why this ever worked?  And why would this patch change anything?

I suspect it worked because we passed 0, 3 as the start and end, so 0, 2 was selected which seems reasonable though a little conviluted.

> If I change it from aEnd - 1 to aEnd, it passes, but it fails without this
> patch -- which suggests I'm unintentionally changing behavior somehow.

it seems correct that selecting 0, 3 is wrong, but I'm not sure what you changed to cause this change.

Comment 7

[Aryeh Gregor (:ayg) (no longer with Mozilla)]

What do you mean by "we passed 0, 3 as the start and end, so 0, 2 was selected"?  If aStart is 0, and aEnd is 3, then the code I quoted will select the two characters at indices 0 and 1.  But textChangeChecker expects the three characters at indices 0, 1, and 2 to be deleted.

Perhaps there's a bug somewhere that caused VK_DELETE to first delete the selection and *then* delete the next character?  It's supposed to delete the next character if the selection is collapsed, and delete the selection if it's not collapsed, but maybe it was doing both somehow.  I don't know why my patch would fix that, though.

Comment 8

[Mark Capella [:capella]]

I remember that when I finished that patch, the end values involved seemed inconsistent. We may have created the test to pass, based on buggy code in the delete routine, which your patch now corrects. In that case, our test just needs updating?

Comment 9

[(no longer active)]

Comment on attachment 636268 [details] [diff] [review]
Patch part 1, v1 -- Clean up DeleteRangeTxn

Review of attachment 636268 [details] [diff] [review]:
-----------------------------------------------------------------

::: editor/libeditor/base/DeleteRangeTxn.cpp
@@ +110,5 @@
> +    nsRefPtr<Selection> selection = mEditor->GetSelection();
> +    // At this point, it is possible that the frame for our root element might
> +    // have been destroyed, in which case, the above call returns an error.  We
> +    // eat that error here intentionally.  See bug 574558 for a sample case
> +    // where this happens.

This comment is no longer valid.

Comment 10

[Aryeh Gregor (:ayg) (no longer with Mozilla)]

Attachment: Test fix (to be folded into patch 3)

I don't know why this test passed before patch 3 of this series, but I don't think it should have.

Comment 11

[Aryeh Gregor (:ayg) (no longer with Mozilla)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/225e51b07b6a
https://hg.mozilla.org/integration/mozilla-inbound/rev/281b41001b4a
https://hg.mozilla.org/integration/mozilla-inbound/rev/794245a8a852

Comment 12

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/225e51b07b6a
https://hg.mozilla.org/mozilla-central/rev/281b41001b4a
https://hg.mozilla.org/mozilla-central/rev/794245a8a852