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

RESOLVED FIXED in Firefox 16

Status

()

Core
Editor
RESOLVED FIXED
5 years ago
4 months ago

People

(Reporter: Jesse Ruderman, Assigned: Ehsan)

Tracking

(Blocks: 1 bug, {assertion, testcase})

Trunk
mozilla16
x86_64
Mac OS X
assertion, testcase
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(firefox16+ fixed)

Details

(Whiteboard: [fuzzblocker])

Attachments

(4 attachments)

(Reporter)

Description

5 years ago
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
(Reporter)

Comment 1

5 years ago
Created attachment 631255 [details]
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?)
(Reporter)

Comment 4

5 years ago
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.
(Reporter)

Comment 6

5 years ago
Never mind, the assertion still triggers when closing the testcase.
(Reporter)

Comment 7

5 years ago
Created attachment 631895 [details]
clearer testcase

The <input> ends up inside the <textarea> editor!
(Reporter)

Comment 8

5 years ago
###!!! 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
(Reporter)

Comment 9

5 years ago
This can lead to all kinds of badness, including "invalid offset" and "substring out of range".
tracking-firefox16: --- → ?
(Reporter)

Updated

5 years ago
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
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).
Assignee: nobody → ehsan
Status: NEW → ASSIGNED
Attachment #634654 - Flags: review?(roc)
http://tbpl.mozilla.org/?tree=Try&rev=f282b68ad955
Attachment #634654 - Flags: review?(roc) → review+

Updated

5 years ago
tracking-firefox16: ? → +
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
Last Resolved: 5 years ago
Resolution: --- → FIXED

Comment 16

5 years ago
(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!
tracking-firefox16: + → -
status-firefox16: --- → fixed
tracking-firefox16: - → +
Blocks: 666618

Updated

4 months ago
Depends on: 1328030
You need to log in before you can comment on or make changes to this bug.