Last Comment Bug 766845 - "ASSERTION: bad aEndOffset" (and crash) when inserthtml deletes multiple ranges
: "ASSERTION: bad aEndOffset" (and crash) when inserthtml deletes multiple ranges
Status: RESOLVED FIXED
: assertion, crash, testcase
Product: Core
Classification: Components
Component: Editor (show other bugs)
: Trunk
: x86_64 Mac OS X
: -- critical (vote)
: mozilla16
Assigned To: :Aryeh Gregor (away until August 15)
:
Mentors:
Depends on:
Blocks: 336383
  Show dependency treegraph
 
Reported: 2012-06-20 21:37 PDT by Jesse Ruderman
Modified: 2012-06-28 01:11 PDT (History)
8 users (show)
ayg: in‑testsuite+
See Also:
Crash Signature:
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
non-crashing testcase (623 bytes, application/xhtml+xml)
2012-06-20 21:37 PDT, Jesse Ruderman
no flags Details
Patch part 1, v1 -- Clean up DeleteRangeTxn (17.82 KB, patch)
2012-06-25 04:01 PDT, :Aryeh Gregor (away until August 15)
ehsan: review+
Details | Diff | Review
Patch part 2, v1 -- Make CloneRange() return already_AddRefed<nsRange> (3.56 KB, patch)
2012-06-25 04:03 PDT, :Aryeh Gregor (away until August 15)
bugs: review+
Details | Diff | Review
Patch part 3, v1 -- Fix assertion in DeleteRangeTxn (7.97 KB, patch)
2012-06-25 04:10 PDT, :Aryeh Gregor (away until August 15)
ehsan: review+
Details | Diff | Review
Test fix (to be folded into patch 3) (931 bytes, patch)
2012-06-25 07:45 PDT, :Aryeh Gregor (away until August 15)
surkov.alexander: review+
Details | Diff | Review

Description Jesse Ruderman 2012-06-20 21:37:36 PDT
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);
Comment 1 :Aryeh Gregor (away until August 15) 2012-06-25 04:01:46 PDT
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.
Comment 2 :Aryeh Gregor (away until August 15) 2012-06-25 04:03:21 PDT
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.
Comment 3 :Aryeh Gregor (away until August 15) 2012-06-25 04:10:34 PDT
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
Comment 4 Olli Pettay [:smaug] (high review load, please consider other reviewers) 2012-06-25 04:35:51 PDT
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 (away until August 15) 2012-06-25 06:03:36 PDT
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) 2012-06-25 06:31:28 PDT
(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 (away until August 15) 2012-06-25 06:45:30 PDT
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] 2012-06-25 06:47:21 PDT
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 :Ehsan Akhgari (busy, don't ask for review please) 2012-06-25 06:56:40 PDT
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 (away until August 15) 2012-06-25 07:45:19 PDT
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.

Note You need to log in before you can comment on or make changes to this bug.