Splitting text can leave |start| and |end| Positions without renderers

Splitting text can leave |start| and |end| Positions without renderers

When establishing new start and end positions in ApplyStyleCommand::applyInlineStyle()
be sure to return early if either of them end up null, just as we do if either of
them are null initially.

In this test case execCommand('CreateLink') adds an anchor HTML element in an SVG
namespace so the text underneath validly does not receive a renderer. Without a renderer
the text won't get a visible position value for the command so there's nothing to do but
bail.

BUG=339185, 397076
Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=184124

[rhogan, 2014-10-14 21:07:24]

robhogan@gmail.com changed reviewers:
+ leviw@chromium.org, yosin@chromium.org

[yosin_UTC9, 2014-10-15 01:26:41]

Can we prevent to apply execCommand inside SVG document? Creating A element in
SVG document isn't valid operation.

https://codereview.chromium.org/651303003/diff/1/LayoutTests/TestExpectations
File LayoutTests/TestExpectations (right):

https://codereview.chromium.org/651303003/diff/1/LayoutTests/TestExpectations...
LayoutTests/TestExpectations:1373: crbug.com/339185
editing/apply-inline-style-to-element-with-no-renderer-crash.html [
NeedsRebaseline ]
We don't need to do pixel test. Please try to implement with "js-test.js".

https://codereview.chromium.org/651303003/diff/1/LayoutTests/editing/apply-in...
File
LayoutTests/editing/apply-inline-style-to-element-with-no-renderer-crash.html
(right):

https://codereview.chromium.org/651303003/diff/1/LayoutTests/editing/apply-in...
LayoutTests/editing/apply-inline-style-to-element-with-no-renderer-crash.html:1:
<script>
Could you simplify this test case?
We can have static SVG document then apply "createLink" and "italic".

https://codereview.chromium.org/651303003/diff/1/Source/core/editing/ApplySty...
File Source/core/editing/ApplyStyleCommand.cpp (right):

https://codereview.chromium.org/651303003/diff/1/Source/core/editing/ApplySty...
Source/core/editing/ApplyStyleCommand.cpp:1272:
ASSERT(position.containerNode());
nit: It is better to use |ASSERT(position.isNotNull())| as of |comparePositions|

[rhogan, 2014-10-15 20:17:14]

On 2014/10/15 at 01:26:41, yosin wrote:
> Can we prevent to apply execCommand inside SVG document? Creating A element in
SVG document isn't valid operation.

I can't answer to whether that would be a good approach other than to say
execCommand is namespace-agnostic at present so it seems like a bigger-picture
change we might want to do (if feasible) in addition to guarding against null
position values.

[rhogan, 2014-10-15 20:20:30]

https://codereview.chromium.org/651303003/diff/1/LayoutTests/TestExpectations
File LayoutTests/TestExpectations (right):

https://codereview.chromium.org/651303003/diff/1/LayoutTests/TestExpectations...
LayoutTests/TestExpectations:1373: crbug.com/339185
editing/apply-inline-style-to-element-with-no-renderer-crash.html [
NeedsRebaseline ]
On 2014/10/15 at 01:26:41, Yosi_UTC9 wrote:
> We don't need to do pixel test. Please try to implement with "js-test.js".

I tried that with no luck - it won't crash for me in dumpAsText mode.

https://codereview.chromium.org/651303003/diff/1/LayoutTests/editing/apply-in...
File
LayoutTests/editing/apply-inline-style-to-element-with-no-renderer-crash.html
(right):

https://codereview.chromium.org/651303003/diff/1/LayoutTests/editing/apply-in...
LayoutTests/editing/apply-inline-style-to-element-with-no-renderer-crash.html:1:
<script>
On 2014/10/15 at 01:26:41, Yosi_UTC9 wrote:
> Could you simplify this test case?
> We can have static SVG document then apply "createLink" and "italic".

I've reduced it down to a more manageable size.

https://codereview.chromium.org/651303003/diff/1/Source/core/editing/ApplySty...
File Source/core/editing/ApplyStyleCommand.cpp (right):

https://codereview.chromium.org/651303003/diff/1/Source/core/editing/ApplySty...
Source/core/editing/ApplyStyleCommand.cpp:1272:
ASSERT(position.containerNode());
On 2014/10/15 at 01:26:41, Yosi_UTC9 wrote:
> nit: It is better to use |ASSERT(position.isNotNull())| as of
|comparePositions|

Done.

[rhogan, 2014-10-20 18:11:27]

yosin: ping. Got time to take another look?

[Stephen Chennney, 2014-10-21 19:23:40]

schenney@chromium.org changed reviewers:
+ schenney@chromium.org

[Stephen Chennney, 2014-10-21 19:23:40]

I know that reducing and cleaning up clusterfuzz tests is a horrible experience,
but I also know that in the future it would be nice to understand what this test
is doing.

I believe you can take the first few lines of the test js and reproduce them as
an <svg> tag with appropriate children, and ids etc. You might even be able to
reproduce the results of the first few editing operations in a static document.
Then the js just does the minimum ops to get the crash.

You can use the un-reduced output, and also debug render tree dumps, to infer
the static tree structure you need.

But otherwise I think you have the fix right.

[Stephen Chennney, 2014-10-21 20:34:43]

LGTM. Thanks for slogging through it.

[rhogan, 2014-10-21 21:06:41]

The CQ bit was checked by robhogan@gmail.com

[commit-bot: I haz the power, 2014-10-21 21:07:47]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/651303003/60001

[commit-bot: I haz the power, 2014-10-21 22:20:45]

Committed patchset #4 (id:60001) as 184124