Closed Bug 762183 Opened 12 years ago Closed 12 years ago

"ASSERTION: bad arg, numCharsToDelete. Not enough characters in node" with forwardDelete

Categories

(Core :: DOM: Editor, defect)

Product:
Core
Component:
DOM: Editor
Platform:
x86_64
macOS
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla16

People

(Reporter: jruderman, Assigned: ayg)

References

Details

(Keywords: assertion, testcase)

Attachments

(8 files, 1 obsolete file)

testcase
438 bytes, text/html
Details
stack trace+
15.75 KB, text/plain
Details
Patch part 1, v1 -- Clean up DeleteTextTxn
11.88 KB, patch
ehsan.akhgari
: review+
Details | Diff | Splinter Review
Patch part 2, v1 -- Publicize nsSelectionIterator
2.90 KB, patch
ehsan.akhgari
: review+
Details | Diff | Splinter Review
Patch part 3, v1 -- Clean up some nsEditor methods
25.89 KB, patch
ehsan.akhgari
: review+
Details | Diff | Splinter Review
diff -w for patch part 3, v1
22.07 KB, patch
Details | Diff | Splinter Review
Patch part 4, v2 -- Fix assertion in DeleteTextTxn::Init
3.04 KB, patch
ehsan.akhgari
: review+
Details | Diff | Splinter Review
Patch part 5, v1 -- Fix misleading comments/variable names
7.14 KB, patch
ehsan.akhgari
: review+
Details | Diff | Splinter Review
Attached file testcase — 
###!!! ASSERTION: bad arg, numCharsToDelete.  Not enough characters in node: 'count>=aNumCharsToDelete', file editor/libeditor/base/DeleteTextTxn.cpp, line 59
Attached file stack trace+ — 

    
    

    
  
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.
Assignee: nobody → ayg
Status: NEW → ASSIGNED

    
    

    
  


  
This allows it to be used on the stack in the next patch.

        Attachment #632270 -
        Flags: review?(ehsan)

    
    

    
  


  
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 . . .

        Attachment #632273 -
        Flags: review?(ehsan)

    
    

    
  


  
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

        Attachment #632280 -
        Flags: review?(ehsan)

    
    

    
  
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.

    
  

        Attachment #632269 -
        Flags: review?(ehsan) → review+

    
    

    
  
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.

        Attachment #632270 -
        Flags: review?(ehsan) → review+

    
    

    
  
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? :-)

        Attachment #632273 -
        Flags: review?(ehsan)

    
  

        Attachment #632280 -
        Flags: review?(ehsan) → review+

    
    

    
  


  
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.

        Attachment #632273 -
        Flags: review?(ehsan)

    
    

    
  


  
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!

        Attachment #632280 -
        Attachment is obsolete: true

        Attachment #632615 -
        Flags: review?(ehsan)

    
    

    
  


  
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

        Attachment #632617 -
        Flags: review?(ehsan)

    
  

        Attachment #632615 -
        Flags: review?(ehsan) → review+

    
    

    
  
(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!

    
  

        Attachment #632617 -
        Flags: review?(ehsan) → review+

    
  

        Attachment #632273 -
        Flags: review?(ehsan) → review+

    
    

    
  
(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

    
    

    
  
(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!
Depends on: 766413

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






  

    
  







  

    

      
Attachment

      

      
      
      
      
    

    

      

        

          

            
General

            

              Creator: 



            

            
Created: 

            
Updated: 

            
Size: