Closed Bug 805668 Opened 8 years ago Closed 6 years ago

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

Categories

(Core :: DOM: Editor, defect, critical)

x86_64
macOS
defect
Not set
critical

Tracking

()

RESOLVED FIXED
mozilla31

People

(Reporter: jruderman, Assigned: ayg)

References

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

Details

(Keywords: assertion, testcase)

Attachments

(3 files, 1 obsolete file)

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.
Attached file stack
Hmm, this is bad.  Aryeh, can you please take a look at this?
Assignee: nobody → ayg
See Also: → 803410
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.
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?
Blocks: 700538
(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...
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.
Status: NEW → ASSIGNED
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.)
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 ?
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...
(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?
Because we can't properly clear it, for one thing...
(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...
(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
Hmm, what if we just return null for anonymous content from GetActiveEditingHost?
The selection being inside the native anon content here sounds pretty weird to me too...
Well, <input> and <textarea> do have selection inside them, but in this case...
(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...
(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.
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.)
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.
Assignee: ayg → nobody
Status: ASSIGNED → NEW
(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.
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.
Assignee: nobody → ayg
Status: NEW → ASSIGNED
Flags: needinfo?(ehsan)
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.
Flags: needinfo?(ehsan)
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?
Flags: needinfo?(ehsan)
I'm not sure what the best way of sending trusted key events from tests is...  Maybe Boris or Olli know?
Flags: needinfo?(ehsan)
Flags: needinfo?(carlosmaug)
Flags: needinfo?(bzbarsky)
(Sorry, wrong smaug!)
Flags: needinfo?(carlosmaug) → needinfo?(bugs)
synthesizeKey should be it, as long as the relevant thing is focused.
Flags: needinfo?(bzbarsky)
Flags: needinfo?(bugs)
Attached patch Patch that causes regressions (obsolete) — Splinter Review
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 (!=)
Attachment #734555 - Flags: feedback?(ehsan)
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...
Attachment #734555 - Flags: feedback?(ehsan) → feedback+
(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.
Flags: needinfo?(ehsan)
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);
}
Flags: needinfo?(ehsan)
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?
Attachment #734555 - Attachment is obsolete: true
Attachment #8402619 - Flags: review?(ehsan)
Flags: needinfo?(jruderman)
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. :/
Attachment #8402619 - Flags: review?(ehsan) → review+
Flags: needinfo?(jruderman)
(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?
Flags: needinfo?(ehsan)
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.
Flags: needinfo?(ehsan)
Depends on: 999416
https://hg.mozilla.org/mozilla-central/rev/76d97ee6133f
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla31
See Also: → 1511563
You need to log in before you can comment on or make changes to this bug.