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

Status: RESOLVED FIXED

(Core :: DOM: Editor, defect)

Description

[Jesse Ruderman]

Attachment: 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]

Attachment: stack trace

Comment 2

[(no longer active)]

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

Comment 3

[(no longer active)]

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

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 (:ayg) (no longer with Mozilla)]

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]

Never mind, the assertion still triggers when closing the testcase.

Comment 7

[Jesse Ruderman]

Attachment: clearer testcase

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

Comment 8

[Jesse Ruderman]

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

Comment 9

[Jesse Ruderman]

This can lead to all kinds of badness, including "invalid offset" and "substring out of range".

Comment 10

[Aryeh Gregor (:ayg) (no longer with Mozilla)]

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 (:ayg) (no longer with Mozilla)]

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

[(no longer active)]

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

[(no longer active)]

http://tbpl.mozilla.org/?tree=Try&rev=f282b68ad955

Comment 14

[(no longer active)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/96303b557d55

Comment 15

[(no longer active)]

https://hg.mozilla.org/mozilla-central/rev/96303b557d55

Comment 16

[[:Aleksej]]

(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

[(no longer active)]

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