The default bug view has changed. See this FAQ.

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

RESOLVED FIXED in mozilla16

Status

()

Core
Editor
--
critical
RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: Jesse Ruderman, Assigned: ayg)

Tracking

(Blocks: 1 bug, {assertion, crash, testcase})

Trunk
mozilla16
x86_64
Mac OS X
assertion, crash, testcase
Points:
---
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(Not tracked)

Details

(crash signature)

Attachments

(5 attachments)

(Reporter)

Description

5 years ago
Created attachment 635184 [details]
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);
Created attachment 636268 [details] [diff] [review]
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.
Assignee: nobody → ayg
Status: NEW → ASSIGNED
Attachment #636268 - Flags: review?(ehsan)
Created attachment 636269 [details] [diff] [review]
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.
Attachment #636269 - Flags: review?(bugs)
Created attachment 636270 [details] [diff] [review]
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
Attachment #636270 - Flags: review?(ehsan)

Comment 4

5 years ago
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;
}
Attachment #636269 - Flags: review?(bugs) → review+
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.
(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.
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.
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 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.
Attachment #636268 - Flags: review?(ehsan) → review+

Updated

5 years ago
Attachment #636270 - Flags: review?(ehsan) → review+

Updated

5 years ago
Keywords: regressionwindow-wanted
Created attachment 636306 [details] [diff] [review]
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.
Attachment #636306 - Flags: review?(surkov.alexander)

Updated

5 years ago
Keywords: regressionwindow-wanted

Updated

5 years ago
Attachment #636306 - Flags: review?(surkov.alexander) → review+
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
Flags: in-testsuite+
Target Milestone: --- → mozilla16
https://hg.mozilla.org/mozilla-central/rev/225e51b07b6a
https://hg.mozilla.org/mozilla-central/rev/281b41001b4a
https://hg.mozilla.org/mozilla-central/rev/794245a8a852
Status: ASSIGNED → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.