| Summary: | Crashes in WebCore::InsertNodeBeforeCommand constructor. | ||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|
| Product: | WebKit | Reporter: | Shinya Kawanaka <shinyak> | ||||||||
| Component: | HTML Editing | Assignee: | Annie Sullivan <sullivan> | ||||||||
| Status: | RESOLVED FIXED | ||||||||||
| Severity: | Normal | CC: | dglazkov, dominicc, morrita, rniwa, rolandsteiner, shinyak, sullivan, webkit.review.bot | ||||||||
| Priority: | P2 | ||||||||||
| Version: | 528+ (Nightly build) | ||||||||||
| Hardware: | Unspecified | ||||||||||
| OS: | Unspecified | ||||||||||
| Bug Depends on: | |||||||||||
| Bug Blocks: | 67668 | ||||||||||
| Attachments: |
|
||||||||||
I took a quick look at this in the debugger and it's a pretty weird case with a <span> inside a contenteditable <meter> tag, which has shadow DOM.
When InsertParagraphSeparatorCommand::doApply() gets called, startingSelection() and endingSelection() both look like this:
BODY 0x10931ce80
#text 0x10930e380 ">"
SE METER 0x10931d9d0
#shadow-root 0x10931db80
DIV 0x10931da80
DIV 0x10931db00 STYLE=width: 0%;
SPAN 0x10931e670
#text 0x10931e4e0 ">"
SCRIPT 0x10931e700
#text 0x10931e900 "\nvar sel = window.getSelection();\nsel.setPosition(document.getElementById("wrapper"), 1);\ndocument.execCommand("InsertParagraph", false, null);\n"
start: before, offset:0
end: before, offset:0
So the code tries to insert a <br> before the <meter>. This causes the assertion to fail in InsertNodeBeforeCommand::InsertNodeBeforeCommand(). m_refChild is <meter> and m_refChild->parentNode() is <body>. <body> is not editable and it is attached.
Should <meter> be allowed to be contenteditable? If so, should the selection have been set to inside the <span> as the JavaScript says? If not, how should this case be handled?
(In reply to comment #1) > I took a quick look at this in the debugger and it's a pretty weird case with a <span> inside a contenteditable <meter> tag, which has shadow DOM. HTMLMeterElement::canContainRangeEndPoint returns false, so we shouldn't be inserting a node inside a meter element. > When InsertParagraphSeparatorCommand::doApply() gets called, startingSelection() and endingSelection() both look like this: We should bail out in that case because we're outside of the contenteditable area. > BODY 0x10931ce80 > #text 0x10930e380 ">" > SE METER 0x10931d9d0 > #shadow-root 0x10931db80 > DIV 0x10931da80 > DIV 0x10931db00 STYLE=width: 0%; > SPAN 0x10931e670 > #text 0x10931e4e0 ">" > SCRIPT 0x10931e700 > #text 0x10931e900 "\nvar sel = window.getSelection();\nsel.setPosition(document.getElementById("wrapper"), 1);\ndocument.execCommand("InsertParagraph", false, null);\n" > start: before, offset:0 > end: before, offset:0 Notice, it's before the meter element (i.e. at (body, 1)). We shouldn't be inserting any node here. (In reply to comment #2) > (In reply to comment #1) > > I took a quick look at this in the debugger and it's a pretty weird case with a <span> inside a contenteditable <meter> tag, which has shadow DOM. > > HTMLMeterElement::canContainRangeEndPoint returns false, so we shouldn't be inserting a node inside a meter element. So does that mean that the code setting the selection outside of the meter element is doing the right thing? > > When InsertParagraphSeparatorCommand::doApply() gets called, startingSelection() and endingSelection() both look like this: > > We should bail out in that case because we're outside of the contenteditable area. Sounds good to me. I'm going to work on a patch that bails out of InsertParagraphSeparatorCommand::doApply() if we're outside of the contenteditable area. Let me know if you think the bailout should go in a different place. When I'm writing the check, should I worry about the case where the selection is partially inside the contenteditable area? It seems like the code that handles that is already working fine; I can't actually reproduce any problems where startingSelection() is in the contenteditable area and endingSelection() is outside, or vice versa. It would make the check much simpler just to bail if either of them is outside of the editing area. (In reply to comment #3) > (In reply to comment #2) > > HTMLMeterElement::canContainRangeEndPoint returns false, so we shouldn't be inserting a node inside a meter element. > > So does that mean that the code setting the selection outside of the meter element is doing the right thing? Yes. > > We should bail out in that case because we're outside of the contenteditable area. > > Sounds good to me. I'm going to work on a patch that bails out of InsertParagraphSeparatorCommand::doApply() if we're outside of the contenteditable area. Let me know if you think the bailout should go in a different place. I'm surprised that we even call InsertParagraphSeparatorCommand::doApply. I thought we have a check in EditorCommand or Editor and don't even call apply when we're outside of the editable region. For example, I suspect that queryCommandEnabled('insertParagrpah') returns true in this case (it should return false instead!). > When I'm writing the check, should I worry about the case where the selection is partially inside the contenteditable area? It seems like the code that handles that is already working fine; I can't actually reproduce any problems where startingSelection() is in the contenteditable area and endingSelection() is outside, or vice versa. It would make the check much simpler just to bail if either of them is outside of the editing area. No, that should never happen as long as FrameSelection::validate is doing its work. Created attachment 106815 [details]
Patch
> I'm surprised that we even call InsertParagraphSeparatorCommand::doApply. I thought we have a check in EditorCommand or Editor and don't even call apply when we're outside of the editable region.
>
> For example, I suspect that queryCommandEnabled('insertParagrpah') returns true in this case (it should return false instead!).
deprecatedNode() strikes again!
The problem was that enabledInEditableText() calls editableRootForPosition() with a position that is anchored before the meter element. Because editableRootForPosition() was using deprecatedNode(), it was getting meter->rootEditableElement(), which is the meter node itself. It should have been getting meter->parentNode()->rootEditableElement().
I changed it to use containerNode, and surprisingly all the editing layout tests still pass for me.
There are other usages of deprecatedNode in htmlediting.cpp, but I'd prefer to fix them in separate patches.
Comment on attachment 106815 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=106815&action=review Perfect! > LayoutTests/editing/inserting/insert-paragraph-selection-outside-contenteditable-expected.txt:1 > + This test ensures that WebKit does not crash when the selection is outside of the contenteditable area. You should hide the meter element so that it won't add space in front of "This". I'd also like to see this test making sure the DOM is not changed by the editing command. > Source/WebCore/ChangeLog:10 > + root. Could you fit "root" in the previous line so that it's easier to read? Created attachment 106828 [details]
Patch
(In reply to comment #7) > > LayoutTests/editing/inserting/insert-paragraph-selection-outside-contenteditable-expected.txt:1 > > + This test ensures that WebKit does not crash when the selection is outside of the contenteditable area. > > You should hide the meter element so that it won't add space in front of "This". Done. > I'd also like to see this test making sure the DOM is not changed by the editing command. I did that by comparing meter.outerHTML before/after, is that okay? > > Source/WebCore/ChangeLog:10 > > + root. > > Could you fit "root" in the previous line so that it's easier to read? Done. Comment on attachment 106828 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=106828&action=review > LayoutTests/ChangeLog:6 > + Tests for crash when the selection is outside the contenteditable node. This line should appear after "Reviewed by" Created attachment 106831 [details]
Patch
(In reply to comment #10) > (From update of attachment 106828 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=106828&action=review > > > LayoutTests/ChangeLog:6 > > + Tests for crash when the selection is outside the contenteditable node. > > This line should appear after "Reviewed by" Oops, fixed. (In reply to comment #12) > (In reply to comment #10) > > (From update of attachment 106828 [details] [details]) > > View in context: https://bugs.webkit.org/attachment.cgi?id=106828&action=review > > > > > LayoutTests/ChangeLog:6 > > > + Tests for crash when the selection is outside the contenteditable node. > > > > This line should appear after "Reviewed by" > > Oops, fixed. You don't have to ask for another review for these trivial changes. In fact, as soon as I've r+ed, you can just do "webkit-patch land-safely" or make changes and commit locally. Comment on attachment 106831 [details] Patch Clearing flags on attachment: 106831 Committed r94832: <http://trac.webkit.org/changeset/94832> All reviewed patches have been landed. Closing bug. |
ASSERT(m_refChild->parentNode()->rendererIsEditable() || !m_refChild->parentNode()->attached()); fails. testcase2:: ><meter contenteditable><span id="wrapper">><script> var sel = window.getSelection(); sel.setPosition(document.getElementById("wrapper"), 1); document.execCommand("InsertParagraph", false, null); </script>