Last Comment Bug 762764 - "ASSERTION: Unexpected children: 'aRoot->GetChildCount() <= 2'" with forwardDelete
: "ASSERTION: Unexpected children: 'aRoot->GetChildCount() <= 2'" with forwardD...
Status: RESOLVED FIXED
[fuzzblocker]
: assertion, testcase
Product: Core
Classification: Components
Component: Editor (show other bugs)
: Trunk
: x86_64 Mac OS X
: -- normal (vote)
: mozilla16
Assigned To: :Ehsan Akhgari
:
Mentors:
Depends on:
Blocks: 336383 666618
  Show dependency treegraph
 
Reported: 2012-06-07 19:57 PDT by Jesse Ruderman
Modified: 2012-09-18 13:22 PDT (History)
8 users (show)
ehsan: in‑testsuite+
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---
+
fixed


Attachments
testcase (577 bytes, text/html)
2012-06-07 19:57 PDT, Jesse Ruderman
no flags Details
stack trace (12.65 KB, text/plain)
2012-06-07 19:58 PDT, Jesse Ruderman
no flags Details
clearer testcase (395 bytes, text/html)
2012-06-11 07:58 PDT, Jesse Ruderman
no flags Details
Patch (v1) (2.27 KB, patch)
2012-06-19 16:35 PDT, :Ehsan Akhgari
roc: review+
Details | Diff | Splinter Review

Description Jesse Ruderman 2012-06-07 19:57:38 PDT
Created attachment 631254 [details]
testcase

###!!! ASSERTION: We should have one text node and one mozBR at most: 'length <= 2', file layout/forms/nsTextControlFrame.cpp, line 995
Comment 1 Jesse Ruderman 2012-06-07 19:58:09 PDT
Created attachment 631255 [details]
stack trace
Comment 2 :Ehsan Akhgari 2012-06-07 20:28:24 PDT
Is this a very recent regression, perhaps from bug 757771?
Comment 3 :Ehsan Akhgari 2012-06-07 20:29:47 PDT
(Aryeh, this is one of the buggy cases that I told you about!  I bet that the image is injected under the textarea.  Wanna take this?)
Comment 4 Jesse Ruderman 2012-06-10 18:32:23 PDT
The testcase isn't hitting any assertions now.  Maybe because https://hg.mozilla.org/mozilla-central/rev/8f5c42f0dcb3 removed the assertion.
Comment 5 :Aryeh Gregor (away until August 15) 2012-06-11 02:05:31 PDT
I didn't mean to remove any assertions.  The caller of DOMPointToOffset now calls nsContentUtils::GetSelectionInTextControl instead, and that contains

  NS_ASSERTION(aRoot->GetChildCount() <= 2, "Unexpected children");

which is the same assertion as here.  But it's possible that some other change in that patch is causing it to bail out before hitting the assertion now.
Comment 6 Jesse Ruderman 2012-06-11 07:56:54 PDT
Never mind, the assertion still triggers when closing the testcase.
Comment 7 Jesse Ruderman 2012-06-11 07:58:24 PDT
Created attachment 631895 [details]
clearer testcase

The <input> ends up inside the <textarea> editor!
Comment 8 Jesse Ruderman 2012-06-11 07:59:47 PDT
###!!! ASSERTION: Unexpected children: 'aRoot->GetChildCount() <= 2', file content/base/src/nsContentUtils.cpp, line 6881
Comment 9 Jesse Ruderman 2012-06-14 18:30:13 PDT
This can lead to all kinds of badness, including "invalid offset" and "substring out of range".
Comment 10 :Aryeh Gregor (away until August 15) 2012-06-17 02:24:12 PDT
Note that this is not a regression from forwardDelete.  It just makes it triggerable by script.  You can achieve the same effect by hitting the delete key on your keyboard -- that's all forwardDelete does.  (For Mac users: I mean the PC delete key, which deletes the next character.)

I'll look at this, though.
Comment 11 :Aryeh Gregor (away until August 15) 2012-06-17 06:38:28 PDT
So I looked at this for a while but don't really understand what's happening here.  Basically, the difference between delete and forwardDelete here is in nsPlaintextEditor::ExtendSelectionForDelete.  For delete (ePrevious), the method simply does nothing if the start node isn't a text node -- the call to nsFrameSelection::CharacterExtendForBackspace() is conditional.

For forwardDelete (eNext), it calls nsFrameSelection::CharacterExtendForDelete() unconditionally.  This calls MoveCaret(), which calls TakeFocus(), which places the selection into the textarea.  This wreaks havoc.  Any subsequent attempt to call nsHTMLEditor::GetActiveEditingHost() will return null, which causes the execCommand("forwardDelete") to error out and throw.  After that, the cursor is in some kind of weird nowhere-land, so calls to getSelection().getFocusNode() and such in the page's JS throw some weird exception, but insertHTML seems to work happily.

I tried changing ExtendSelectionForDelete() to not do anything for eNext if the focused node isn't a CharacterData, but unsurprisingly, that broke lots of tests.  I'm guessing we want to do two things here:

1) Fix DeleteSelection() so that it doesn't create this kind of bogus selection.

2) Fix insertHTML so it bails out if the selection is bogus.

However, I don't understand how any of this DOM-in-text-control stuff works, so I don't know how to do either of these things.  Ehsan, if you have any pointers, please let me know.  Otherwise I'm going to have to drop this bug.
Comment 12 :Ehsan Akhgari 2012-06-19 16:35:50 PDT
Created attachment 634654 [details] [diff] [review]
Patch (v1)

We should just disallow peeking into textboxes, cause that doesn't make any sense.  This doesn't seem to break any of the existing tests (which is good news).
Comment 13 :Ehsan Akhgari 2012-06-19 16:36:37 PDT
http://tbpl.mozilla.org/?tree=Try&rev=f282b68ad955
Comment 16 [:Aleksej] 2012-07-13 13:43:43 PDT
(In reply to Ehsan Akhgari [:ehsan] from comment #15)
> https://hg.mozilla.org/mozilla-central/rev/96303b557d55

This appears to have partially fixed bug 448424 (bug 448424 comment 16).
Comment 17 :Ehsan Akhgari 2012-07-16 21:25:56 PDT
(In reply to Aleksej [:Aleksej] from comment #16)
> (In reply to Ehsan Akhgari [:ehsan] from comment #15)
> > https://hg.mozilla.org/mozilla-central/rev/96303b557d55
> 
> This appears to have partially fixed bug 448424 (bug 448424 comment 16).

Nice!

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