Issue 181283002: Fixing a crash in InsertListCommand.

Issue 181283002: Fixing a crash in InsertListCommand. (Closed)

Created:
6 years, 10 months ago by yoichio

Modified:
6 years, 9 months ago

Reviewers:
Yuta Kitamura

CC:
blink-reviews, sof, eae+blinkwatch, dglazkov+blink, adamk+blink_chromium.org, Inactive, rwlbuis

Base URL:
https://chromium.googlesource.com/chromium/blink.git@master

Visibility:
Public.

More Reviews

Description

Fixing a crash in InsertListCommand. There are two crash causes: 1.Range and 2.InsertListCommand. 1. Range: In processAncestorsAndThierSiblings, it iterates siblings and remove each sibling. Because removeChild might kick a “SubtreeModified” event which might change the tree structure, ancestor might be not a parent of child at a next iteration. We should check it. 2. InsertListCommand: The doApply function does nothing when endingSelection().rootEditableElement() is Null. Because L124 setEndingSelection might change endingSelection(), we should check again. BUG=344280 Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=168006

Patch Set 1 #

Total comments: 10

Patch Set 2 : Update #

Created: 6 years, 10 months ago

Download [raw] [tar.bz2]

	Unified diffs	Side-by-side diffs	Delta from patch set	Stats (+53 lines, -7 lines)			Patch
A	LayoutTests/editing/execCommand/insert-ordered-list-crash.html	View	1	1 chunk	+39 lines, -0 lines	0 comments	Download
A +	LayoutTests/editing/execCommand/insert-ordered-list-crash-expected.txt	View	1	1 chunk	+6 lines, -5 lines	0 comments	Download
M	Source/core/dom/Range.cpp	View	1	1 chunk	+4 lines, -1 line	0 comments	Download
M	Source/core/editing/InsertListCommand.cpp	View		1 chunk	+4 lines, -1 line	0 comments	Download

Messages

Total messages: 11 (0 generated)

Expand Messages | Collapse Messages

Yuta Kitamura

https://codereview.chromium.org/181283002/diff/1/LayoutTests/editing/execCommand/insert-ordered-list-crash.html File LayoutTests/editing/execCommand/insert-ordered-list-crash.html (right): https://codereview.chromium.org/181283002/diff/1/LayoutTests/editing/execCommand/insert-ordered-list-crash.html#newcode12 LayoutTests/editing/execCommand/insert-ordered-list-crash.html:12: <script> Technically, <script> should not appear after <body> is ...

6 years, 10 months ago (2014-02-26 10:41:15 UTC) #2

https://codereview.chromium.org/181283002/diff/1/LayoutTests/editing/execComm...
File LayoutTests/editing/execCommand/insert-ordered-list-crash.html (right):

https://codereview.chromium.org/181283002/diff/1/LayoutTests/editing/execComm...
LayoutTests/editing/execCommand/insert-ordered-list-crash.html:12: <script>
Technically, <script> should not appear after
<body> is closed.

https://codereview.chromium.org/181283002/diff/1/LayoutTests/editing/execComm...
LayoutTests/editing/execCommand/insert-ordered-list-crash.html:13:
document.addEventListener("DOMContentLoaded", function () {
You need to tell the layout test harness that
this test is asynchronous. To do that, you can make
use of js-test.js (located at LayoutTests/resouces).
You need to load that file, set |window.jsTestIsAsync|
to true, and call |finishJSTest()| when the test
really finishes.

(Instead, you can manually call methods of testRunner,
 but I don't personally recommend this.)

Take a look at a test I wrote yesterday
<https://codereview.chromium.org/176863002/>.
It uses iframe to load the HTML causing a crash,
which I think is a little bit overkill, but at least
should serve as a reference.

https://codereview.chromium.org/181283002/diff/1/LayoutTests/editing/execComm...
LayoutTests/editing/execCommand/insert-ordered-list-crash.html:14: var
oSelection = window.getSelection();
nit: "oSelection" is awkward naming convention
and uncommon in Blink.

https://codereview.chromium.org/181283002/diff/1/Source/core/dom/Range.cpp
File Source/core/dom/Range.cpp (right):

https://codereview.chromium.org/181283002/diff/1/Source/core/dom/Range.cpp#ne...
Source/core/dom/Range.cpp:906: // Then we should validate again.
nit: The first sentence sounds a little bit strange
(what's "child before this child"?).

Also, in the last sentence "validate" sounds a little
bit vague under this context.

With that in mind I'd suggest something like:

    // Prior call of ancestor->removeChild() may cause a tree change due to
DOMSubtreeModified event.
    // Therefore, we need to make sure |ancestor| is still |child|'s parent.

https://codereview.chromium.org/181283002/diff/1/Source/core/dom/Range.cpp#ne...
Source/core/dom/Range.cpp:907: if (ancestor != child->parentNode())
Ideally, we could have bulk-removal functionality
to avoid this kind of mistake, but your change seems
acceptable at this moment.

You can make the code cleaner by replacing the lines
907-909 with:

    if (ancestor == child->parentNode())
        ancestor->removeChild(child, exceptionState);

yoichio

https://codereview.chromium.org/181283002/diff/1/LayoutTests/editing/execCommand/insert-ordered-list-crash.html File LayoutTests/editing/execCommand/insert-ordered-list-crash.html (right): https://codereview.chromium.org/181283002/diff/1/LayoutTests/editing/execCommand/insert-ordered-list-crash.html#newcode12 LayoutTests/editing/execCommand/insert-ordered-list-crash.html:12: <script> On 2014/02/26 10:41:16, Yuta Kitamura wrote: > Technically, ...

6 years, 10 months ago (2014-02-27 04:00:03 UTC) #3

https://codereview.chromium.org/181283002/diff/1/LayoutTests/editing/execComm...
File LayoutTests/editing/execCommand/insert-ordered-list-crash.html (right):

https://codereview.chromium.org/181283002/diff/1/LayoutTests/editing/execComm...
LayoutTests/editing/execCommand/insert-ordered-list-crash.html:12: <script>
On 2014/02/26 10:41:16, Yuta Kitamura wrote:
> Technically, <script> should not appear after
> <body> is closed.

Done.

https://codereview.chromium.org/181283002/diff/1/LayoutTests/editing/execComm...
LayoutTests/editing/execCommand/insert-ordered-list-crash.html:13:
document.addEventListener("DOMContentLoaded", function () {
On 2014/02/26 10:41:16, Yuta Kitamura wrote:
> You need to tell the layout test harness that
> this test is asynchronous. To do that, you can make
> use of js-test.js (located at LayoutTests/resouces).
> You need to load that file, set |window.jsTestIsAsync|
> to true, and call |finishJSTest()| when the test
> really finishes.
> 
> (Instead, you can manually call methods of testRunner,
>  but I don't personally recommend this.)
> 
> Take a look at a test I wrote yesterday
> <https://codereview.chromium.org/176863002/>.
> It uses iframe to load the HTML causing a crash,
> which I think is a little bit overkill, but at least
> should serve as a reference.

Done.

https://codereview.chromium.org/181283002/diff/1/LayoutTests/editing/execComm...
LayoutTests/editing/execCommand/insert-ordered-list-crash.html:14: var
oSelection = window.getSelection();
On 2014/02/26 10:41:16, Yuta Kitamura wrote:
> nit: "oSelection" is awkward naming convention
> and uncommon in Blink.

Done.

https://codereview.chromium.org/181283002/diff/1/Source/core/dom/Range.cpp
File Source/core/dom/Range.cpp (right):

https://codereview.chromium.org/181283002/diff/1/Source/core/dom/Range.cpp#ne...
Source/core/dom/Range.cpp:906: // Then we should validate again.
On 2014/02/26 10:41:16, Yuta Kitamura wrote:
> nit: The first sentence sounds a little bit strange
> (what's "child before this child"?).
> 
> Also, in the last sentence "validate" sounds a little
> bit vague under this context.
> 
> With that in mind I'd suggest something like:
> 
>     // Prior call of ancestor->removeChild() may cause a tree change due to
> DOMSubtreeModified event.
>     // Therefore, we need to make sure |ancestor| is still |child|'s parent.

Done.

https://codereview.chromium.org/181283002/diff/1/Source/core/dom/Range.cpp#ne...
Source/core/dom/Range.cpp:907: if (ancestor != child->parentNode())
On 2014/02/26 10:41:16, Yuta Kitamura wrote:
> Ideally, we could have bulk-removal functionality
> to avoid this kind of mistake, but your change seems
> acceptable at this moment.
> 
> You can make the code cleaner by replacing the lines
> 907-909 with:
> 
>     if (ancestor == child->parentNode())
>         ancestor->removeChild(child, exceptionState);

Done.

commit-bot: I haz the power

CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/yoichio@chromium.org/181283002/20001

6 years, 10 months ago (2014-02-27 04:07:16 UTC) #6

commit-bot: I haz the power

The CQ bit was unchecked by commit-bot@chromium.org

6 years, 9 months ago (2014-02-27 08:50:38 UTC) #7

commit-bot: I haz the power

Retried try job too often on linux_blink for step(s) webkit_tests http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_blink&number=14355

6 years, 9 months ago (2014-02-27 08:50:39 UTC) #8

commit-bot: I haz the power

CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/yoichio@chromium.org/181283002/20001

6 years, 9 months ago (2014-02-27 09:19:48 UTC) #10

Message was sent while issue was closed.

Change committed as 168006

Expand Messages | Collapse Messages