805668 - "Assertion failure: IsElement()" with arrow key, execCommand("removeFormat")

Status: RESOLVED FIXED

(Core :: DOM: Editor, defect)

Description

[Jesse Ruderman]

Attachment: testcase (see comment 0)

1. Install https://www.squarefree.com/extensions/domFuzzLite3.xpi
2. Load the testcase

Assertion failure: IsElement(), at Element.h:341

Like in bug 803410, arrow keys put the focus/selection in a place that the page can't normally reference.

Comment 1

[Jesse Ruderman]

Attachment: stack

Comment 2

[(no longer active)]

Hmm, this is bad.  Aryeh, can you please take a look at this?

Comment 3

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

I'll put it on my list, but I don't know if I'll be able to fix these as fast as you're assigning them.  I'll prioritize security first, then regressions, then crashes, then other stuff, unless you'd prefer a different priority.

Comment 4

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

So this is an nsTextNode with NODE_IS_EDITABLE set.  In bug 700538 part 2, I changed nsIContent::GetEditingHost to return dom::Element* instead of nsIContent*, and didn't account for the possibility that a detached text node could be editable because I assumed it made no sense.  From source code inspection, it seems like the text node child of a text control will have NODE_IS_EDITABLE set; is is perhaps not being cleared when the child is orphaned?

Comment 5

[(no longer active)]

(In reply to comment #4)
> So this is an nsTextNode with NODE_IS_EDITABLE set.  In bug 700538 part 2, I
> changed nsIContent::GetEditingHost to return dom::Element* instead of
> nsIContent*, and didn't account for the possibility that a detached text node
> could be editable because I assumed it made no sense.  From source code
> inspection, it seems like the text node child of a text control will have
> NODE_IS_EDITABLE set; is is perhaps not being cleared when the child is
> orphaned?

It should be, I think.  If not, that's a bug...

Comment 6

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

When I tried again, I found that it did have a parent (a select element), just the parent wasn't editable.  I'll try adding an assertion to SetFlags to check for the case where we're setting an editable flag on something that's not an element or document and whose parent isn't editable.  That won't catch the case where it's later detached, but maybe that's not what we're hitting.

Comment 7

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

Okay, so I banged my head against the wall a bit because what I saw in the debugger didn't seem to make sense, until I discovered this:

Program received signal SIGSEGV, Segmentation fault.
0xb3814817 in nsINode::AsElement (this=0x9f7e8880) at ../../dist/include/mozilla/dom/Element.h:341
341       MOZ_ASSERT(IsElement());
(gdb) p this
$30 = (nsTextNode *) 0x9f7e8880
(gdb) p this->mParent
$31 = (nsHTMLSelectElement *) 0xab9078b0
(gdb) p this->mParent->mFirstChild
$32 = (nsIContent *) 0x0

So the text node's parent has no children.  Any suggestions for what breakpoints to set to track this down?  Is this even an editor bug?  content/ should expose no entry points that allow this situation to ever happen, should it?

(For the curious, the way this bug manifests itself is that making the body no longer contenteditable calls MakeContentDescendantsEditable(), which recursively removes the editable flag from all descendants.  It skips the text node in question, since it's not the child of the select element.  But then nsIContent::GetEditingHost() is called on the text node, and finds that it's editable but its parent is not.)

Comment 8

[Olli Pettay [:smaug][[e-mail]]]

So is the textnode the mDisplayContent inside select?
mDisplayContent is native anonymous, so its parent doesn't have pointer to it.

Can you get to the textnode from its parent using
((nsComboboxControlFrame*)parent->GetPrimaryFrame())->mDisplayContent ?

Comment 9

[Boris Zbarsky [:bzbarsky]]

Hmm.  We should probably not inherit editable state into anonymous content.  That said, it's not clear why we're getting the editing host for anon content...

Comment 10

[(no longer active)]

(In reply to comment #9)
> Hmm.  We should probably not inherit editable state into anonymous content. 

I don't agree.  Why do you say that?

Comment 11

[Boris Zbarsky [:bzbarsky]]

Because we can't properly clear it, for one thing...

Comment 12

[(no longer active)]

(In reply to comment #11)
> Because we can't properly clear it, for one thing...

Well, yeah, I guess.  But in (almost) all places where we check for that flag, we don't care whether the content is under an anonymous subtree or not...

Comment 13

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

(In reply to Olli Pettay [:smaug] from comment #8)
> So is the textnode the mDisplayContent inside select?
> mDisplayContent is native anonymous, so its parent doesn't have pointer to
> it.
> 
> Can you get to the textnode from its parent using
> ((nsComboboxControlFrame*)parent->GetPrimaryFrame())->mDisplayContent ?

Yep, that's right.  So I'd be happy to write a fix, but I don't know what it should be.  It seems to me that setting an editable flag that we're not going to be able to clear when necessary is not the right solution, in any event.

(In reply to Boris Zbarsky (:bz) from comment #9)
> That said, it's not clear why we're getting the editing host for anon
> content...

The selection is inside the anonymous content, and editing code wants to know what the active editing host is, which is the editing host of the selection's focus.  Specifically, this is the top of the stack:

#0  0xb3814817 in nsINode::AsElement (this=0x9f7e8880) at ../../dist/include/mozilla/dom/Element.h:341
#1  0xb3cd2748 in nsIContent::GetEditingHost (this=0x9f7e8880)
    at /mnt/ssd/checkouts/central/content/base/src/FragmentOrElement.cpp:234
#2  0xb433086e in nsHTMLEditor::GetActiveEditingHost (this=0xa0e4fa00)
    at /mnt/ssd/checkouts/central/editor/libeditor/html/nsHTMLEditor.cpp:5232
#3  0xb432955b in nsHTMLEditor::GetPriorHTMLNode (this=0xa0e4fa00, inParent=0x9f7e88c0, inOffset=1, 
    outNode=0xbfffabb8, bNoBlockCrossing=true)
    at /mnt/ssd/checkouts/central/editor/libeditor/html/nsHTMLEditor.cpp:4117
#4  0xb436d295 in nsHTMLEditRules::CheckInterlinePosition (this=0xa1660000, aSelection=0x99b77500)
    at /mnt/ssd/checkouts/central/editor/libeditor/html/nsHTMLEditRules.cpp:7339
#5  0xb4341b6c in nsHTMLEditRules::AfterEditInner (this=0xa1660000, action=resetTextProperties, 
    aDirection=1) at /mnt/ssd/checkouts/central/editor/libeditor/html/nsHTMLEditRules.cpp:528
#6  0xb4340b8f in nsHTMLEditRules::AfterEdit (this=0xa1660000, action=resetTextProperties, aDirection=1)
    at /mnt/ssd/checkouts/central/editor/libeditor/html/nsHTMLEditRules.cpp:376
#7  0xb4326574 in nsHTMLEditor::EndOperation (this=0xa0e4fa00)
    at /mnt/ssd/checkouts/central/editor/libeditor/html/nsHTMLEditor.cpp:3518
#8  0xb424515f in nsAutoRules::~nsAutoRules (this=0xbfffae78, __in_chrg=<optimized out>)
    at /mnt/ssd/checkouts/central/editor/libeditor/base/nsEditorUtils.h:94
#9  0xb433aee0 in nsHTMLEditor::RemoveAllInlineProperties (this=0xa0e4fa00)
    at /mnt/ssd/checkouts/central/editor/libeditor/html/nsHTMLEditorStyle.cpp:1350
#10 0xb491800e in nsRemoveStylesCommand::DoCommand (this=0x9f7f4af0, 
    aCommandName=0xbfffb048 "cmd_removeStyles", refCon=0xa0e4fa00)
    at /mnt/ssd/checkouts/central/editor/composer/src/nsComposerCommands.cpp:1176

Comment 14

[(no longer active)]

Hmm, what if we just return null for anonymous content from GetActiveEditingHost?

Comment 15

[Boris Zbarsky [:bzbarsky]]

The selection being inside the native anon content here sounds pretty weird to me too...

Comment 16

[Olli Pettay [:smaug][[e-mail]]]

Well, <input> and <textarea> do have selection inside them, but in this case...

Comment 17

[(no longer active)]

(In reply to comment #16)
> Well, <input> and <textarea> do have selection inside them, but in this case...

They're handled by the editor's selection controller...

Comment 18

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

(In reply to Ehsan Akhgari [:ehsan] from comment #14)
> Hmm, what if we just return null for anonymous content from
> GetActiveEditingHost?

That hacks around the symptom in this case, but leaves the problem that the editable flag will be incorrectly set on the child.  That's not going to cause other problems?  Wouldn't it be better to somehow update the anonymous child's editable flag when the parent's changes?  Either that, or not set it in the first place.  Setting it and then not unsetting it doesn't sound like the right solution.

Comment 19

[(no longer active)]

Hmm, I'm pretty sure that not setting that flag on the anonymous children of the text boxes will break them.  Maybe we should just special case those and avoid setting the editable flags on other types of anonymous elements?

(Really, all of the possible solutions here suck for one reason or another.)

Comment 20

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

If you decide how we should fix it, let me know and I can take a shot, but until then, there's not much I can do here.

Comment 21

[(no longer active)]

(In reply to comment #20)
> If you decide how we should fix it, let me know and I can take a shot, but
> until then, there's not much I can do here.

I think we need to proceed with comment 19.

Comment 22

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

Okay -- could you give me some pointers on what code I need to look at changing?  I'm not familiar with this code at all, so maybe I'm not the best person to do this, but it sounds simple enough.

Comment 23

[(no longer active)]

I think nsIContent::UpdateEditableState is the best place to do what comment 19 suggests.  For the special casing, we may not even need to do anything special since the code in nsCSSFrameConstructor::GetAnonymousContent already makes sure to force-set the editable flag if the anonymous content creator requests it, and that may be enough here.  If you see that it's not, the way that you can get access to the parent element is to get the frame for the content if one exists, get its parent, and then get the content node off of that frame.

Comment 24

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

What's the best way to adapt this test-case as a mochitest or crashtest?  I tried doing something similar-looking using synthesizeKey, but it didn't trigger the bug.  The test-case sends a trusted key event.  Do I want to use something from nsIDOMWindowUtils?

Comment 25

[(no longer active)]

I'm not sure what the best way of sending trusted key events from tests is...  Maybe Boris or Olli know?

Comment 26

[(no longer active)]

(Sorry, wrong smaug!)

Comment 27

[Boris Zbarsky [:bzbarsky]]

synthesizeKey should be it, as long as the relevant thing is focused.

Comment 28

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

Attachment: Patch that causes regressions

So this fixes the test-case, but causes many regressions, including at least the ones I'm pasting here.  Is this what you meant when you said we might need to set the flag on the anonymous children of the text boxes?  If so, what should I do to fix it?  I could use a little more help than comment 23 provides, preferably including exact function names, because I have no clue how all this stuff with frames works.

(Also, won't this just mean the bug will remain with <textarea> rather than <select>?)


255 ERROR TEST-UNEXPECTED-FAIL | /tests/editor/libeditor/base/tests/test_bug795785.html | textarea was not scrolled by up key events - didn't expect 73, but got it
258 ERROR TEST-UNEXPECTED-FAIL | /tests/editor/libeditor/base/tests/test_bug795785.html | textarea was not scrolled by left key events - didn't expect 85, but got it
5669 ERROR TEST-UNEXPECTED-FAIL | /tests/editor/libeditor/html/tests/test_bug549262.html | Selection should be moved to the beginning - got 3, expected 0
5671 ERROR TEST-UNEXPECTED-FAIL | /tests/editor/libeditor/html/tests/test_bug549262.html | Page is not scrolled down - got 84, expected 27
5673 ERROR TEST-UNEXPECTED-FAIL | /tests/editor/libeditor/html/tests/test_bug549262.html | Selection should be moved to the end - got 3, expected 5
6654 ERROR TEST-UNEXPECTED-FAIL | /tests/editor/libeditor/html/tests/test_bug674770-2.html | failed to paste in contenteditable editor #2 when editor2 is clicked - got editor2:, expected editor2:pasted
11219 ERROR TEST-UNEXPECTED-FAIL | /tests/editor/libeditor/text/tests/test_bug596333.html | Correct number of misspellings and words. - got 0, expected 2
11220 ERROR TEST-UNEXPECTED-FAIL | /tests/editor/libeditor/text/tests/test_bug596333.html | All misspellings before editing are accounted for. - got false, expected true
11221 ERROR TEST-UNEXPECTED-FAIL | /tests/editor/libeditor/text/tests/test_bug596333.html | Correct number of misspellings and words. - got 0, expected 4
11222 ERROR TEST-UNEXPECTED-FAIL | /tests/editor/libeditor/text/tests/test_bug596333.html | All misspellings after typing are accounted for. - got false, expected true
11223 ERROR TEST-UNEXPECTED-FAIL | /tests/editor/libeditor/text/tests/test_bug596333.html | Correct number of misspellings and words. - got 0, expected 5
11224 ERROR TEST-UNEXPECTED-FAIL | /tests/editor/libeditor/text/tests/test_bug596333.html | All misspellings after blur are accounted for. - got false, expected true
11225 ERROR TEST-UNEXPECTED-FAIL | /tests/editor/libeditor/text/tests/test_bug596333.html | Correct number of misspellings and words. - got 0, expected 1
11226 ERROR TEST-UNEXPECTED-FAIL | /tests/editor/libeditor/text/tests/test_bug596333.html | Misspelling in the first entered word is accounted for. - got false, expected true
11227 ERROR TEST-UNEXPECTED-FAIL | /tests/editor/libeditor/text/tests/test_bug596333.html | Correct number of misspellings and words. - got 0, expected 1
11228 ERROR TEST-UNEXPECTED-FAIL | /tests/editor/libeditor/text/tests/test_bug596333.html | Misspelling is accounted for after pasting. - got false, expected true


 0:38.19 REFTEST TEST-UNEXPECTED-FAIL | file:///mnt/ssd/checkouts/central/editor/reftests/spellcheck-textarea-attr.html | image comparison (!=)
 0:38.19 REFTEST TEST-UNEXPECTED-FAIL | file:///mnt/ssd/checkouts/central/editor/reftests/spellcheck-textarea-nofocus.html | image comparison (!=)
 0:38.19 REFTEST TEST-UNEXPECTED-FAIL | file:///mnt/ssd/checkouts/central/editor/reftests/spellcheck-textarea-disabled.html | image comparison (!=)
 0:38.19 REFTEST TEST-UNEXPECTED-FAIL | file:///mnt/ssd/checkouts/central/editor/reftests/spellcheck-textarea-attr-inherit.html | image comparison (!=)
 0:38.19 REFTEST TEST-UNEXPECTED-FAIL | file:///mnt/ssd/checkouts/central/editor/reftests/spellcheck-textarea-attr-dynamic.html | image comparison (!=)
 0:38.19 REFTEST TEST-UNEXPECTED-FAIL | file:///mnt/ssd/checkouts/central/editor/reftests/spellcheck-textarea-attr-dynamic-inherit.html | image comparison (!=)
 0:38.19 REFTEST TEST-UNEXPECTED-FAIL | file:///mnt/ssd/checkouts/central/editor/reftests/spellcheck-textarea-property-dynamic.html | image comparison (!=)
 0:38.19 REFTEST TEST-UNEXPECTED-FAIL | file:///mnt/ssd/checkouts/central/editor/reftests/spellcheck-textarea-property-dynamic-inherit.html | image comparison (!=)
 0:38.19 REFTEST TEST-UNEXPECTED-FAIL | file:///mnt/ssd/checkouts/central/editor/reftests/spellcheck-textarea-attr-dynamic-override.html | image comparison (!=)
 0:38.19 REFTEST TEST-UNEXPECTED-FAIL | file:///mnt/ssd/checkouts/central/editor/reftests/spellcheck-textarea-attr-dynamic-override-inherit.html | image comparison (!=)
 0:38.19 REFTEST TEST-UNEXPECTED-FAIL | file:///mnt/ssd/checkouts/central/editor/reftests/spellcheck-textarea-property-dynamic-override.html | image comparison (!=)
 0:38.19 REFTEST TEST-UNEXPECTED-FAIL | file:///mnt/ssd/checkouts/central/editor/reftests/spellcheck-textarea-property-dynamic-override-inherit.html | image comparison (!=)
 0:38.19 REFTEST TEST-UNEXPECTED-FAIL | file:///mnt/ssd/checkouts/central/editor/reftests/spellcheck-hyphen-invalid.html | image comparison (!=)
 0:38.19 REFTEST TEST-UNEXPECTED-FAIL | file:///mnt/ssd/checkouts/central/editor/reftests/spellcheck-hyphen-multiple-invalid.html | image comparison (!=)
 0:38.19 REFTEST TEST-UNEXPECTED-FAIL | file:///mnt/ssd/checkouts/central/editor/reftests/spellcheck-superscript-2.html | image comparison (!=)

Comment 29

[(no longer active)]

So I was wrong in saying that we don't need the special casing code here, I think, since now we'll do the wrong thing everytime that something triggers a call to SetEditableState.

So, inside nsIContent::SetEditableState once you find an anonymous content, walk up the tree until you get to the root (until IsRootOfNativeAnonymousSubtree returns true), then grab the frame using nsIContent::GetPrimaryFrame, grab its parent and then get to the content using nsIFrame::GetContent, and then check to see if it's an input or textarea element...

Comment 30

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

(In reply to :Ehsan Akhgari (needinfo? me!) from comment #29)
> So, inside nsIContent::SetEditableState once you find an anonymous content,
> walk up the tree until you get to the root (until
> IsRootOfNativeAnonymousSubtree returns true), then grab the frame using
> nsIContent::GetPrimaryFrame, grab its parent and then get to the content
> using nsIFrame::GetContent, and then check to see if it's an input or
> textarea element...

I tried this, but it doesn't seem to do what I want at all.  I fiddled around a bunch and then gave up.  Could you maybe give some sample code for what UpdateEditableState() should look like?  I really don't know what I'm doing here when it comes to anonymous content or frames.

Comment 31

[(no longer active)]

Hmm, this is sort of what I had in mind:

void
nsIContent::UpdateEditableState(bool aNotify)
{
  // ...

  bool isUnknownNativeAnon = false;
  if (IsInAnonymousSubtree()) {
    isUnknownNativeAnon = true;
    nsCOMPtr<nsIContent> root = this;
    while (!root->IsRootOfNativeAnonymousSubtree()) {
      root = root->GetParent();
      // I don't think root should ever be null here, perhaps MOZ_ASSERT that?
    }
    nsIFrame* rootFrame = root->GetPrimaryFrame();
    if (rootFrame) {
      nsIFrame* parentFrame = rootFrame->GetParent();
      nsITextControlFrame* textCtrl = do_QueryFrame(parentFrame);
      isUnknownNativeAnon = !textCtrl;
    }
  }

  SetEditableFlag(... &&
                  !isUnknownNativeAnon);
}

Comment 32

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

Attachment: Patch that works but the test case doesn't reproduce the bug

I can verify manually that this fixes the bug, but the mochitest doesn't trigger the bug.  Jesse, do you know how I should reproduce the effects of fuzzPriv.trustedKeyEvent in a mochitest?  Failing that, Ehsan, what should I do about the test?

Comment 33

[(no longer active)]

Comment on attachment 8402619 [details] [diff] [review]
Patch that works but the test case doesn't reproduce the bug

Review of attachment 8402619 [details] [diff] [review]:
-----------------------------------------------------------------

I'm pretty sure that synthesizeKey sends a trusted key event.  If you can't make the test work though, we would need to check this in without a test. :/

Comment 34

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

https://tbpl.mozilla.org/?tree=Try&rev=042bdf7bf417

Comment 35

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

(In reply to :Ehsan Akhgari (lagging on bugmail, needinfo? me!) from comment #31)
>   if (IsInAnonymousSubtree()) {
>   ...
>     while (!root->IsRootOfNativeAnonymousSubtree()) {
>       root = root->GetParent();
>       // I don't think root should ever be null here, perhaps MOZ_ASSERT
> that?

I did, and it can.  See try run -- a test crashes because of this assert.  In a debugger, I find that "this" is a text node with contents

foo\245\245\245\245\245--> \245\245\245\245\n <--\245\245\245MochiKitStatus\245\245Passed: Failed: Todo: \245\245\n\n\nZZZZZZZZZZZZZfirefox

with mFlags 546 (evidently NODE_IS_IN_NATIVE_ANONYMOUS_SUBTREE | NODE_IS_EDITABLE), and its parent is a DocumentFragment with mFlags 2.  This is all gibberish to me.  What's the right thing to do, break from the loop if root is null?  File a bug?

Comment 36

[(no longer active)]

I bet this is coming from: http://mxr.mozilla.org/mozilla-central/source/dom/events/test/test_bug328885.html?force=1#88

It seems like in that test, the range object selects the native anonymous text node inside the input element and extracts it into a document fragment, which is what causes the GetParent() assertion to fail.  I _think_ the right thing to do is to make sure selectNodeContents() doesn't delve into the native anon subtree of input elements.  Please file a follow-up on that and get rid of the assertion for now (preferably with a comment pointing to the follow-up bug.)  Thanks.

Comment 37

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

http://hg.mozilla.org/integration/mozilla-inbound/rev/76d97ee6133f

Comment 38

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/76d97ee6133f