Closed Bug 762764 Opened 9 years ago Closed 9 years ago

"ASSERTION: Unexpected children: 'aRoot->GetChildCount() <= 2'" with forwardDelete

Categories

(Core :: DOM: Editor, defect)

x86_64
macOS
defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla16
Tracking Status
firefox16 + fixed

People

(Reporter: jruderman, Assigned: ehsan)

References

(Depends on 1 open bug, Blocks 1 open bug)

Details

(Keywords: assertion, testcase, Whiteboard: [fuzzblocker])

Attachments

(4 files)

Attached file testcase
###!!! ASSERTION: We should have one text node and one mozBR at most: 'length <= 2', file layout/forms/nsTextControlFrame.cpp, line 995
Attached file stack trace
Is this a very recent regression, perhaps from bug 757771?
(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?)
The testcase isn't hitting any assertions now.  Maybe because https://hg.mozilla.org/mozilla-central/rev/8f5c42f0dcb3 removed the assertion.
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.
Never mind, the assertion still triggers when closing the testcase.
Attached file clearer testcase
The <input> ends up inside the <textarea> editor!
###!!! ASSERTION: Unexpected children: 'aRoot->GetChildCount() <= 2', file content/base/src/nsContentUtils.cpp, line 6881
Summary: "ASSERTION: We should have one text node and one mozBR at most" with forwardDelete → "ASSERTION: Unexpected children: 'aRoot->GetChildCount() <= 2'" with forwardDelete
This can lead to all kinds of badness, including "invalid offset" and "substring out of range".
Whiteboard: [fuzzblocker]
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.
Assignee: nobody → ayg
Status: NEW → ASSIGNED
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.
Assignee: ayg → nobody
Status: ASSIGNED → NEW
Attached patch Patch (v1)Splinter Review
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).
Assignee: nobody → ehsan
Status: NEW → ASSIGNED
Attachment #634654 - Flags: review?(roc)
https://hg.mozilla.org/integration/mozilla-inbound/rev/96303b557d55
Flags: in-testsuite+
Target Milestone: --- → mozilla16
https://hg.mozilla.org/mozilla-central/rev/96303b557d55
Status: ASSIGNED → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
(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).
(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!
Depends on: 1328030
Depends on: 1362873
You need to log in before you can comment on or make changes to this bug.