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

RESOLVED FIXED in Firefox 16



7 years ago
2 years ago


(Reporter: jruderman, Assigned: Ehsan)


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

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

Firefox Tracking Flags

(firefox16+ fixed)


(Whiteboard: [fuzzblocker])


(4 attachments)



7 years ago
Created attachment 631254 [details]

###!!! ASSERTION: We should have one text node and one mozBR at most: 'length <= 2', file layout/forms/nsTextControlFrame.cpp, line 995

Comment 1

7 years ago
Created attachment 631255 [details]
stack trace

Comment 2

7 years ago
Is this a very recent regression, perhaps from bug 757771?

Comment 3

7 years ago
(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

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

Comment 6

7 years ago
Never mind, the assertion still triggers when closing the testcase.

Comment 7

7 years ago
Created attachment 631895 [details]
clearer testcase

The <input> ends up inside the <textarea> editor!

Comment 8

7 years ago
###!!! ASSERTION: Unexpected children: 'aRoot->GetChildCount() <= 2', file content/base/src/nsContentUtils.cpp, line 6881


7 years ago
Summary: "ASSERTION: We should have one text node and one mozBR at most" with forwardDelete → "ASSERTION: Unexpected children: 'aRoot->GetChildCount() <= 2'" with forwardDelete

Comment 9

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


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

Comment 12

7 years ago
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
Attachment #634654 - Flags: review?(roc)
tracking-firefox16: ? → +

Comment 14

7 years ago
Flags: in-testsuite+
Target Milestone: --- → mozilla16

Comment 15

7 years ago
Last Resolved: 7 years ago
Resolution: --- → FIXED

Comment 16

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

Comment 17

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

tracking-firefox16: + → -


7 years ago
status-firefox16: --- → fixed
tracking-firefox16: - → +


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