Last Comment Bug 762183 - "ASSERTION: bad arg, numCharsToDelete. Not enough characters in node" with forwardDelete
: "ASSERTION: bad arg, numCharsToDelete. Not enough characters in node" with f...
Status: RESOLVED FIXED
: assertion, testcase
Product: Core
Classification: Components
Component: Editor (show other bugs)
: Trunk
: x86_64 Mac OS X
: -- normal (vote)
: mozilla16
Assigned To: Aryeh Gregor (:ayg) (next working March 28-April 26)
:
: Makoto Kato [:m_kato]
Mentors:
Depends on: 766413
Blocks: 336383 590640
  Show dependency treegraph
 
Reported: 2012-06-06 12:12 PDT by Jesse Ruderman
Modified: 2012-06-20 00:57 PDT (History)
4 users (show)
ayg: in‑testsuite+
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
testcase (438 bytes, text/html)
2012-06-06 12:12 PDT, Jesse Ruderman
no flags Details
stack trace+ (15.75 KB, text/plain)
2012-06-06 12:13 PDT, Jesse Ruderman
no flags Details
Patch part 1, v1 -- Clean up DeleteTextTxn (11.88 KB, patch)
2012-06-12 08:50 PDT, Aryeh Gregor (:ayg) (next working March 28-April 26)
ehsan: review+
Details | Diff | Splinter Review
Patch part 2, v1 -- Publicize nsSelectionIterator (2.90 KB, patch)
2012-06-12 08:50 PDT, Aryeh Gregor (:ayg) (next working March 28-April 26)
ehsan: review+
Details | Diff | Splinter Review
Patch part 3, v1 -- Clean up some nsEditor methods (25.89 KB, patch)
2012-06-12 08:57 PDT, Aryeh Gregor (:ayg) (next working March 28-April 26)
ehsan: review+
Details | Diff | Splinter Review
Patch part 4, v1 -- Fix assertion in DeleteTextTxn::Init (3.03 KB, patch)
2012-06-12 09:06 PDT, Aryeh Gregor (:ayg) (next working March 28-April 26)
ehsan: review+
Details | Diff | Splinter Review
diff -w for patch part 3, v1 (22.07 KB, patch)
2012-06-13 02:39 PDT, Aryeh Gregor (:ayg) (next working March 28-April 26)
no flags Details | Diff | Splinter Review
Patch part 4, v2 -- Fix assertion in DeleteTextTxn::Init (3.04 KB, patch)
2012-06-13 02:53 PDT, Aryeh Gregor (:ayg) (next working March 28-April 26)
ehsan: review+
Details | Diff | Splinter Review
Patch part 5, v1 -- Fix misleading comments/variable names (7.14 KB, patch)
2012-06-13 02:57 PDT, Aryeh Gregor (:ayg) (next working March 28-April 26)
ehsan: review+
Details | Diff | Splinter Review

Description Jesse Ruderman 2012-06-06 12:12:44 PDT
Created attachment 630661 [details]
testcase

###!!! ASSERTION: bad arg, numCharsToDelete.  Not enough characters in node: 'count>=aNumCharsToDelete', file editor/libeditor/base/DeleteTextTxn.cpp, line 59
Comment 1 Jesse Ruderman 2012-06-06 12:13:09 PDT
Created attachment 630662 [details]
stack trace+
Comment 2 Aryeh Gregor (:ayg) (next working March 28-April 26) 2012-06-12 06:49:18 PDT
Simpler test-case:

data:text/html,<body contenteditable=true>x   y
<script>
document.body.firstChild.splitText(2).splitText(1).splitText(1);
getSelection().collapse(document.body, 1);
document.execCommand("forwardDelete", false, null);
</script>

You can reproduce with no execCommand() if you like: go to the same URL without the execCommand() and hit the delete key yourself.  I'm guessing what's happening here is we're trying to delete a character from a text node, but before that happens, we collapse the consecutive whitespace, or normalize adjacent text nodes, or something like that.  Whatever normalization we're doing should be done before we create the transaction.
Comment 3 Aryeh Gregor (:ayg) (next working March 28-April 26) 2012-06-12 08:50:05 PDT
Created attachment 632269 [details] [diff] [review]
Patch part 1, v1 -- Clean up DeleteTextTxn
Comment 4 Aryeh Gregor (:ayg) (next working March 28-April 26) 2012-06-12 08:50:40 PDT
Created attachment 632270 [details] [diff] [review]
Patch part 2, v1 -- Publicize nsSelectionIterator

This allows it to be used on the stack in the next patch.
Comment 5 Aryeh Gregor (:ayg) (next working March 28-April 26) 2012-06-12 08:57:46 PDT
Created attachment 632273 [details] [diff] [review]
Patch part 3, v1 -- Clean up some nsEditor methods

This one is, um, somewhat nontrivial.  Because the functions were such a mess.  :)  Viewing with diff -w helps (don't know if Splinter will do that for you).  Some notable points that occur to me:

* I replace lots of if (NS_SUCCEEDED(res)) in CreateTxnForDeleteInsertionPoint with NS_ENSURE_SUCCESS(res, res).  I don't think this changes the logic substantially, because generally what happens if the condition fails is it breaks out of all the if statements and falls all the way to "return res;" at the end, so it works out to the same.  There are probably at least one or two points where there was an if (foo) that I replaced with NS_ENSURE_STATE(foo), which won't work quite the same -- previously it would bail out with NS_OK and now will bail out with NS_ERROR_UNEXPECTED.  Assuming it was even possible in those cases for the value to be null but NS_OK to be returned.

In the future I think I'll separate out obviously-correct stylistic changes from logic changes to make your job easier, for patches like this . . .
Comment 6 Aryeh Gregor (:ayg) (next working March 28-April 26) 2012-06-12 09:06:56 PDT
Created attachment 632280 [details] [diff] [review]
Patch part 4, v1 -- Fix assertion in DeleteTextTxn::Init

This fixes the assertion.  I don't know if it fixes it the right way, but if this is the most realistic case anyone can come up with where it got rid, I don't think it matters much how we fix it.  :)  This will have it skip over empty CharacterData nodes, which is about as right as anything.  It probably makes sense to delete the empty text node entirely here, just for tidiness, but that would be more complicated (more transactions yay!).

https://tbpl.mozilla.org/?tree=Try&rev=6c12b36c7cab
Comment 7 :Ms2ger (⌚ UTC+1/+2) 2012-06-12 09:42:59 PDT
Comment on attachment 632270 [details] [diff] [review]
Patch part 2, v1 -- Publicize nsSelectionIterator

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

::: layout/generic/Selection.h
@@ +222,5 @@
>  
>  } // namespace mozilla
>  
> +class nsSelectionIterator : public nsIBidirectionalEnumerator
> +{

How much work would it be to make this mozilla::SelectionIterator? :)

@@ +225,5 @@
> +class nsSelectionIterator : public nsIBidirectionalEnumerator
> +{
> +public:
> +/*BEGIN nsIEnumerator interfaces
> +see the nsIEnumerator for more details*/

These comments are useless, IMO.
Comment 8 :Ehsan Akhgari 2012-06-12 17:24:53 PDT
Comment on attachment 632270 [details] [diff] [review]
Patch part 2, v1 -- Publicize nsSelectionIterator

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

::: layout/generic/Selection.h
@@ +222,5 @@
>  
>  } // namespace mozilla
>  
> +class nsSelectionIterator : public nsIBidirectionalEnumerator
> +{

That can happen in another bug.
Comment 9 :Ehsan Akhgari 2012-06-12 17:27:27 PDT
Comment on attachment 632273 [details] [diff] [review]
Patch part 3, v1 -- Clean up some nsEditor methods

Well, can you please submit the diff -w version? :-)
Comment 10 Aryeh Gregor (:ayg) (next working March 28-April 26) 2012-06-13 02:39:56 PDT
Created attachment 632614 [details] [diff] [review]
diff -w for patch part 3, v1

Sure -- sorry about that.  Unfortunately, it's still not super-readable, but at least the chunks of changed content are smaller.  I spent a whole bunch of time trying to break it up into multiple diffs, but I didn't wind up with anything submittable.

The basic problem is that I'm switching a lot of things that return nsresult and an out param to newer methods that just return the thing, and return null on error.  Previously these methods used deeply nested if statements that had "return result;" at the end of the method, so they'd just fall through to that if there was an error.  But now the result variable is mostly unused, and the important thing is whether certain variables are null.  I can't switch to using the new methods without also changing the error-handling logic, which means changing lots of indentation and logic flow, which is hard to follow.

Anyway, if it's too hard for you to review, I can try breaking it up again.  Perhaps a better strategy would be to write one patch that only changes whitespace and variable names and comments and such, and a second patch that switches to the newer methods.
Comment 11 Aryeh Gregor (:ayg) (next working March 28-April 26) 2012-06-13 02:53:12 PDT
Created attachment 632615 [details] [diff] [review]
Patch part 4, v2 -- Fix assertion in DeleteTextTxn::Init

Oops -- there was a bug in my last patch.  Interdiff:

-    while (selectedNode->IsNodeOfType(nsINode::eCONTENT) &&
+    while (selectedNode->IsNodeOfType(nsINode::eDATA_NODE) &&
            !selectedNode->Length()) {
-      // Can't delete an empty node (bug 762183)
+      // Can't delete an empty chardata node (bug 762183)

I didn't mean to change behavior for non-chardata nodes.  Checking for eCONTENT doesn't make a lot of sense here -- I meant eDATA_NODE.  Sorry about that!
Comment 12 Aryeh Gregor (:ayg) (next working March 28-April 26) 2012-06-13 02:57:42 PDT
Created attachment 632617 [details] [diff] [review]
Patch part 5, v1 -- Fix misleading comments/variable names

As I was redoing part 3, I noticed that we're QIing to CharacterData, but all the comments/variable names talk about text as though they're the same, which they're not.  Here's one more cleanup patch that changes the comments and variable names to talk about chardata instead of text.  FWIW, our behavior here is probably wrong -- we actually do delete characters from comments when the user hits delete, not the next text node!  But that's a separate bug.

New try run: https://tbpl.mozilla.org/?tree=Try&rev=d241fee1e417
Comment 13 :Ehsan Akhgari 2012-06-13 06:32:31 PDT
(In reply to Aryeh Gregor from comment #12)
> Created attachment 632617 [details] [diff] [review]
> Patch part 5, v1 -- Fix misleading comments/variable names
> 
> As I was redoing part 3, I noticed that we're QIing to CharacterData, but
> all the comments/variable names talk about text as though they're the same,
> which they're not.  Here's one more cleanup patch that changes the comments
> and variable names to talk about chardata instead of text.  FWIW, our
> behavior here is probably wrong -- we actually do delete characters from
> comments when the user hits delete, not the next text node!  But that's a
> separate bug.

If you have steps to reproduce, would you please file a bug about that?  This is embarrassing!
Comment 14 Aryeh Gregor (:ayg) (next working March 28-April 26) 2012-06-13 07:52:07 PDT
(In reply to Ehsan Akhgari [:ehsan] from comment #13)
> If you have steps to reproduce, would you please file a bug about that? 
> This is embarrassing!

Looks like I was mistaken -- if we hit a comment, we tend to either delete the whole thing or just give up and do nothing.  There are a bunch of tests with comments in the editing test suite, though, if you're interested:

http://dvcs.w3.org/hg/editing/raw-file/tip/conformancetest/splitruntest.html?delete
Comment 15 :Ehsan Akhgari 2012-06-13 08:15:04 PDT
(In reply to Aryeh Gregor from comment #14)
> (In reply to Ehsan Akhgari [:ehsan] from comment #13)
> > If you have steps to reproduce, would you please file a bug about that? 
> > This is embarrassing!
> 
> Looks like I was mistaken -- if we hit a comment, we tend to either delete
> the whole thing or just give up and do nothing.  There are a bunch of tests
> with comments in the editing test suite, though, if you're interested:
> 
> http://dvcs.w3.org/hg/editing/raw-file/tip/conformancetest/splitruntest.
> html?delete

OK, cool!

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