67763 – Crashes in WebCore::InsertNodeBeforeCommand constructor.

RESOLVED FIXED Bug 67763

Crashes in WebCore::InsertNodeBeforeCommand constructor.

https://bugs.webkit.org/show_bug.cgi?id=67763

Summary Crashes in WebCore::InsertNodeBeforeCommand constructor.

Shinya Kawanaka

Reported 2011-09-07 23:58:16 PDT

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>

Attachments
Patch (3.92 KB, patch) 2011-09-08 17:05 PDT, Annie Sullivan	no flags	Details Formatted Diff Diff
Patch (4.24 KB, patch) 2011-09-08 19:01 PDT, Annie Sullivan	no flags	Details Formatted Diff Diff
Patch (4.24 KB, patch) 2011-09-08 19:10 PDT, Annie Sullivan	no flags	Details Formatted Diff Diff
Show Obsolete (2) View All Add attachment proposed patch, testcase, etc.

Annie Sullivan

Comment 1 2011-09-08 13:03:12 PDT

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?

Ryosuke Niwa

Comment 2 2011-09-08 13:07:31 PDT

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

Annie Sullivan

Comment 3 2011-09-08 13:21:30 PDT

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

Ryosuke Niwa

Comment 4 2011-09-08 13:24:20 PDT

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

Annie Sullivan

Comment 5 2011-09-08 17:05:29 PDT

Created attachment 106815 [details] Patch

Annie Sullivan

Comment 6 2011-09-08 17:09:28 PDT

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

Ryosuke Niwa

Comment 7 2011-09-08 17:40:28 PDT

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?

Annie Sullivan

Comment 8 2011-09-08 19:01:50 PDT

Created attachment 106828 [details] Patch

Annie Sullivan

Comment 9 2011-09-08 19:04:24 PDT

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

Ryosuke Niwa

Comment 10 2011-09-08 19:07:02 PDT

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"

Annie Sullivan

Comment 11 2011-09-08 19:10:10 PDT

Created attachment 106831 [details] Patch

Annie Sullivan

Comment 12 2011-09-08 19:10:31 PDT

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

Ryosuke Niwa

Comment 13 2011-09-08 19:12:26 PDT

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

WebKit Review Bot

Comment 14 2011-09-08 21:02:29 PDT

Comment on attachment 106831 [details] Patch Clearing flags on attachment: 106831 Committed r94832: <http://trac.webkit.org/changeset/94832>

WebKit Review Bot

Comment 15 2011-09-08 21:02:34 PDT

All reviewed patches have been landed. Closing bug.

Note You need to log in before you can comment on or make changes to this bug.

Status RESOLVED

Resolution FIXED

Priority P2

Severity Normal

Classification Unclassified

Version 528+ (Nightly build)

Hardware Unspecified

OS Unspecified

Product WebKit

Component HTML Editing

Assignee

Annie Sullivan

Reported

2011-09-07 23:58 PDT

Modified

2011-09-08 21:02 PDT History

CC List

8 users Show

URL

Keywords

Depends on

Blocks

67668

Dependancies

tree graph