Fixing crash when executing ApplyStyle command.

Fixing crash when executing InsertOrderedList execCommand.

In applyInlineStyleToNodeRange(), a call to removeConflictingInlineStyleFromRun() can modify the start and end Nodes of InlineRunToApplyStyle.

For the crash scenario, the start node being set in removeConflictingInlineStyleFromRun() is invalid and further use of it when calling positionToComputeInlineStyleChange() causes a crash.

Thus adding a check for startAndEndAreStillInDocument() fixes the crash. However, since for the invalid scenario positionForStyleComputation shall not be set, we should check for it's validity before using the same for creating the StyleChange object.

Have added a testcase based on the simplified clusterfuzz testcase added by yoichio in http://crbug.com/343799 which verifies the crash.

BUG=343799
Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=168278

[yosin_UTC9, 2014-02-27 01:38:39]

ACK w/ small nit for test case.

Thanks for fixing!

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

https://codereview.chromium.org/181103004/diff/1/LayoutTests/editing/execComm...
LayoutTests/editing/execCommand/inserting-ordered-list-crash.html:14:
document.execCommand('InsertOrderedList', false);
nit: We don't need to have |false|, default value of second parameter of
|execCommand| is |undefined|, which means |false| for |execCommand|.

https://codereview.chromium.org/181103004/diff/1/LayoutTests/editing/execComm...
File LayoutTests/editing/execCommand/selection-after-insert-list-2.html (right):

https://codereview.chromium.org/181103004/diff/1/LayoutTests/editing/execComm...
LayoutTests/editing/execCommand/selection-after-insert-list-2.html:14:
document.execCommand('InsertOrderedList', false, true);
We don't need to have |false| and |true|. "InsertOrderedList" command doesn't
use them.

[arpitab_, 2014-02-27 06:56:10]

Thanks for the review Yoshi, have uploaded another patch fixing the nits.

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

https://codereview.chromium.org/181103004/diff/1/LayoutTests/editing/execComm...
LayoutTests/editing/execCommand/inserting-ordered-list-crash.html:14:
document.execCommand('InsertOrderedList', false);
On 2014/02/27 01:38:40, Yoshi wrote:
> nit: We don't need to have |false|, default value of second parameter of
> |execCommand| is |undefined|, which means |false| for |execCommand|.

Done.

https://codereview.chromium.org/181103004/diff/1/LayoutTests/editing/execComm...
File LayoutTests/editing/execCommand/selection-after-insert-list-2.html (right):

https://codereview.chromium.org/181103004/diff/1/LayoutTests/editing/execComm...
LayoutTests/editing/execCommand/selection-after-insert-list-2.html:14:
document.execCommand('InsertOrderedList', false, true);
On 2014/02/27 01:38:40, Yoshi wrote:
> We don't need to have |false| and |true|. "InsertOrderedList" command doesn't
> use them.

Done.

[Yuta Kitamura, 2014-02-27 07:48:14]

https://codereview.chromium.org/181103004/diff/20001/LayoutTests/editing/exec...
File LayoutTests/editing/execCommand/inserting-ordered-list-crash.html (right):

https://codereview.chromium.org/181103004/diff/20001/LayoutTests/editing/exec...
LayoutTests/editing/execCommand/inserting-ordered-list-crash.html:6: >
What's up with this bracket? Is this necessary?

https://codereview.chromium.org/181103004/diff/20001/LayoutTests/editing/exec...
File LayoutTests/editing/execCommand/selection-after-insert-list-expected.txt
(right):

https://codereview.chromium.org/181103004/diff/20001/LayoutTests/editing/exec...
LayoutTests/editing/execCommand/selection-after-insert-list-expected.txt:9: "
Could you elaborate on this expectation update?

I think your argument of "no visual change" of this test
is too weak to support this diff, since almost nothing is
rendered from the beginning.

And certainly the behavior has changed as id attribute
is gone and a new ul is created. You probably need to
explain either (a) this is not a regression but a progression,
or (b) this change is harmless and won't affect real web pages
in a bad way.

At least I would like to understand how this behavior change
happened.

[arpitab_, 2014-02-28 11:49:41]

Separating out the crash issue [http://crbug.com/343799] and the incorrect
selection after executing the InsertOrderedList command over the selected
content [http://crbug.com/345935].

Patch for resolving the crash is posted here. 
Have modified the changelog and the title to reflect the same.

PTAL

[arpitab_, 2014-02-28 14:02:55]

https://codereview.chromium.org/181103004/diff/40001/LayoutTests/editing/exec...
File LayoutTests/editing/execCommand/inserting-ordered-list-crash.html (right):

https://codereview.chromium.org/181103004/diff/40001/LayoutTests/editing/exec...
LayoutTests/editing/execCommand/inserting-ordered-list-crash.html:6: >
Required for reproducing the crash.

[yosin_UTC9, 2014-03-03 02:06:45]

https://codereview.chromium.org/181103004/diff/40001/Source/core/editing/Appl...
File Source/core/editing/ApplyStyleCommand.cpp (right):

https://codereview.chromium.org/181103004/diff/40001/Source/core/editing/Appl...
Source/core/editing/ApplyStyleCommand.cpp:822: if
(!runs[i].positionForStyleComputation.isNull())
nit: We can use |Position::isNotNull()| here.

[arpitab_, 2014-03-03 05:59:59]

https://codereview.chromium.org/181103004/diff/40001/Source/core/editing/Appl...
File Source/core/editing/ApplyStyleCommand.cpp (right):

https://codereview.chromium.org/181103004/diff/40001/Source/core/editing/Appl...
Source/core/editing/ApplyStyleCommand.cpp:822: if
(!runs[i].positionForStyleComputation.isNull())
On 2014/03/03 02:06:45, Yoshi wrote:
> nit: We can use |Position::isNotNull()| here.

Done.

[Yuta Kitamura, 2014-03-03 06:04:52]

lgtm

[arpitab_, 2014-03-03 06:08:46]

The CQ bit was checked by a.bah@samsung.com

[commit-bot: I haz the power, 2014-03-03 06:09:00]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/a.bah@samsung.com/181103004/60001

[commit-bot: I haz the power, 2014-03-03 08:44:15]

Change committed as 168278