Issue 251723003: Check the traversal state in CompositeEditCommand::cloneParagraphUnderNewElement

Issue 251723003: Check the traversal state in CompositeEditCommand::cloneParagraphUnderNewElement (Closed)

Created:
6 years, 8 months ago by yoichio

Modified:
6 years, 7 months ago

Reviewers:
Yuta Kitamura, yosin_UTC9

CC:
blink-reviews

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

Visibility:
Public.

More Reviews

Description

Check the traversal state in CompositeEditCommand::cloneParagraphUnderNewElement. CompositeEditCommand::cloneParagraphUnderNewElement(const Position& start, const Position& end, Node* passedOuterNode, Element* blockElement) copies target elements between |start| and |end| to |blockElement|. This function can crash if it traverses on a strange tree. The root issue of issue 338558 is that it inserts a node after a node which has no parent. BUG=338558 Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=172967

Patch Set 1 #

Total comments: 8

Patch Set 2 : Update test #

Total comments: 8

Patch Set 3 : Nit pick #

Created: 6 years, 7 months ago

Download [raw] [tar.bz2]

	Unified diffs	Side-by-side diffs	Delta from patch set	Stats (+31 lines, -1 line)			Patch
A	LayoutTests/editing/execCommand/indent-nested-blockquotes-crash.html	View	1 2	1 chunk	+22 lines, -0 lines	0 comments	Download
A +	LayoutTests/editing/execCommand/indent-nested-blockquotes-crash-expected.txt	View		0 chunks	+-1 lines, --1 lines	0 comments	Download
M	Source/core/editing/CompositeEditCommand.cpp	View	1	2 chunks	+10 lines, -2 lines	0 comments	Download

Messages

Total messages: 10 (0 generated)

Expand Messages | Collapse Messages

yosin_UTC9

ACK for code changes https://codereview.chromium.org/251723003/diff/1/LayoutTests/editing/execCommand/indent-nested-blockquotes-crash.html File LayoutTests/editing/execCommand/indent-nested-blockquotes-crash.html (right): https://codereview.chromium.org/251723003/diff/1/LayoutTests/editing/execCommand/indent-nested-blockquotes-crash.html#newcode1 LayoutTests/editing/execCommand/indent-nested-blockquotes-crash.html:1: <html id="html"> nit: Could you ...

6 years, 7 months ago (2014-04-28 01:48:27 UTC) #2

Yuta Kitamura

https://codereview.chromium.org/251723003/diff/1/Source/core/editing/CompositeEditCommand.cpp File Source/core/editing/CompositeEditCommand.cpp (right): https://codereview.chromium.org/251723003/diff/1/Source/core/editing/CompositeEditCommand.cpp#newcode1014 Source/core/editing/CompositeEditCommand.cpp:1014: if (!outerNode) In what situation this could happen? (I ...

6 years, 7 months ago (2014-04-28 02:56:41 UTC) #3

yoichio

cloneParagraphUnderNewElement(const Position& start, const Position& end, Node* passedOuterNode, Element* blockElement) copies nodes between |start| and ...

6 years, 7 months ago (2014-04-30 07:45:16 UTC) #4

cloneParagraphUnderNewElement(const Position& start, const Position& end, Node*
passedOuterNode, Element* blockElement) copies nodes between |start| and |end|
under |passedOuterNode| to |blockElement| as a substep of execCommand('indent').
For example, image following DOM tree:
div
-div
--|blockElement|
-|outerNode|
--|start|
--node0
--|end|
--node1

cloneParagraphUnderNewElement did:
div
-div
--|blockElement|
---cloned|start|
---clonednode0
---cloned|end|
-|outerNode|
--|start|
--node0
--|end|
--node1

I think current algorithm is something wrong and we need re-write whole.
Since it take a time, let me fix the crash issue.

https://codereview.chromium.org/251723003/diff/1/LayoutTests/editing/execComm...
File LayoutTests/editing/execCommand/indent-nested-blockquotes-crash.html
(right):

https://codereview.chromium.org/251723003/diff/1/LayoutTests/editing/execComm...
LayoutTests/editing/execCommand/indent-nested-blockquotes-crash.html:1: <html
id="html">
On 2014/04/28 01:48:27, Yoshi wrote:
> nit: Could you add "<!DOCTYPE html>" to make Blink parser in strict mode?

Done.

https://codereview.chromium.org/251723003/diff/1/LayoutTests/editing/execComm...
LayoutTests/editing/execCommand/indent-nested-blockquotes-crash.html:12:
document.designMode = "on";
On 2014/04/28 01:48:27, Yoshi wrote:
> nit: Could you use single quote for string literal?
> http://google-styleguide.googlecode.com/svn/trunk/javascriptguide.xml#Strings

Done.

https://codereview.chromium.org/251723003/diff/1/Source/core/editing/Composit...
File Source/core/editing/CompositeEditCommand.cpp (right):

https://codereview.chromium.org/251723003/diff/1/Source/core/editing/Composit...
Source/core/editing/CompositeEditCommand.cpp:1014: if (!outerNode)
On 2014/04/28 02:56:42, Yuta Kitamura wrote:
> In what situation this could happen? (I really couldn't trace
> what this function is doing...)
It takes place when |passedOuterNode| and |end| are not in a same DOM tree.

https://codereview.chromium.org/251723003/diff/1/Source/core/editing/Composit...
Source/core/editing/CompositeEditCommand.cpp:1027: if (!lastNode ||
!lastNode->parentNode())
On 2014/04/28 02:56:42, Yuta Kitamura wrote:
> What's the point of checking the non-nullness of
> lastNode->parentNode()?
It is for L1031:insertNodeAfter(clonedNode, lastNode).
insertNodeAfter inserts |clonedNode| to the parent of |lastNode| as a last
child.

Yuta Kitamura

Okay, I'm not 100% sure but the change looks reasonable. LGTM with nits. https://codereview.chromium.org/251723003/diff/20001/LayoutTests/editing/execCommand/indent-nested-blockquotes-crash.html File ...

6 years, 7 months ago (2014-04-30 08:42:03 UTC) #6

yoichio

https://codereview.chromium.org/251723003/diff/20001/LayoutTests/editing/execCommand/indent-nested-blockquotes-crash.html File LayoutTests/editing/execCommand/indent-nested-blockquotes-crash.html (right): https://codereview.chromium.org/251723003/diff/20001/LayoutTests/editing/execCommand/indent-nested-blockquotes-crash.html#newcode2 LayoutTests/editing/execCommand/indent-nested-blockquotes-crash.html:2: <html id="html"> On 2014/04/30 08:42:03, Yuta Kitamura wrote: > ...

6 years, 7 months ago (2014-04-30 08:56:07 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/251723003/40001

6 years, 7 months ago (2014-04-30 08:56:20 UTC) #9

Message was sent while issue was closed.

Change committed as 172967

Expand Messages | Collapse Messages