Editing: Null-check in FrameSelection::updteAppearancea

Editing: Null-check in FrameSelection::updateAppearance

This uncheck causes crash when selecting inside HTMLTextFormElement and either start 
or end node element doesn't have a renderer.

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

[yoichio, 2014-09-29 08:34:01]

yoichio@chromium.org changed reviewers:
+ yosin@chromium.org

[yosin_UTC9, 2014-09-29 09:40:48]

yosin@chromium.org changed reviewers:
+ tkent@chromium.org

[yosin_UTC9, 2014-09-29 09:40:49]

LGTM

Is it possible to write layout test? If so, please do so.
If not please, write reason for missing layout test.

Thanks!

tkent@, WDYT? It seems it is hard to write layout test. The issue doesn't come
from ClusterFuzz. It is from crash report.

[tkent, 2014-09-30 01:29:04]

Can you make a C++ unit test?

[yoichio, 2014-10-01 07:44:56]

yoichio@chromium.org changed reviewers:
+ yutak@chromium.org

[yoichio, 2014-10-01 07:45:49]

Add yutak@ as an owner.
PTAL?

[Yuta Kitamura, 2014-10-01 08:19:48]

https://codereview.chromium.org/610323003/diff/20001/Source/core/editing/Fram...
File Source/core/editing/FrameSelectionTest.cpp (right):

https://codereview.chromium.org/610323003/diff/20001/Source/core/editing/Fram...
Source/core/editing/FrameSelectionTest.cpp:151: node->setRenderer(nullptr);
I'm almost certain that you shouldn't simply call setRenderer() directly.
Doing so would break numerous assumptions around renderers; at the very
least the render object will leak, but other bad things could happen.

What's the point of this setRenderer() call? You should probably call
only public or public-ish APIs to achieve the similar result safely.

[yoichio, 2014-10-01 08:26:35]

On 2014/10/01 08:19:48, Yuta Kitamura wrote:
>
https://codereview.chromium.org/610323003/diff/20001/Source/core/editing/Fram...
> File Source/core/editing/FrameSelectionTest.cpp (right):
> 
>
https://codereview.chromium.org/610323003/diff/20001/Source/core/editing/Fram...
> Source/core/editing/FrameSelectionTest.cpp:151: node->setRenderer(nullptr);
> I'm almost certain that you shouldn't simply call setRenderer() directly.
> Doing so would break numerous assumptions around renderers; at the very
> least the render object will leak, but other bad things could happen.
> 
> What's the point of this setRenderer() call? You should probably call
> only public or public-ish APIs to achieve the similar result safely.

I would like to create a textarea element which child nodes don't have renderer
even layout is called.

[Yuta Kitamura, 2014-10-01 08:49:32]

https://codereview.chromium.org/610323003/diff/20001/Source/core/editing/Fram...
File Source/core/editing/FrameSelectionTest.cpp (right):

https://codereview.chromium.org/610323003/diff/20001/Source/core/editing/Fram...
Source/core/editing/FrameSelectionTest.cpp:151: node->setRenderer(nullptr);
I have little knowledge around this, but you could use some CSS
like "display: none" to disable renderer creation?

[yoichio, 2014-10-02 05:38:19]

On 2014/10/01 08:49:32, Yuta Kitamura wrote:
>
https://codereview.chromium.org/610323003/diff/20001/Source/core/editing/Fram...
> File Source/core/editing/FrameSelectionTest.cpp (right):
> 
>
https://codereview.chromium.org/610323003/diff/20001/Source/core/editing/Fram...
> Source/core/editing/FrameSelectionTest.cpp:151: node->setRenderer(nullptr);
> I have little knowledge around this, but you could use some CSS
> like "display: none" to disable renderer creation?

I missed information. What I want is a such textarea:
<div id="inner-ditor">
"text"
<br/>
"hidden"
</div>

A TextNode "text" has a renderer and "hidden" doesn't have.

I don't get how such a situation happens in real web.

[yosin_UTC9, 2014-10-02 06:28:16]

On 2014/10/02 05:38:19, yoichio wrote:
> On 2014/10/01 08:49:32, Yuta Kitamura wrote:
> >
>
https://codereview.chromium.org/610323003/diff/20001/Source/core/editing/Fram...
> > File Source/core/editing/FrameSelectionTest.cpp (right):
> > 
> >
>
https://codereview.chromium.org/610323003/diff/20001/Source/core/editing/Fram...
> > Source/core/editing/FrameSelectionTest.cpp:151: node->setRenderer(nullptr);
> > I have little knowledge around this, but you could use some CSS
> > like "display: none" to disable renderer creation?
> 
> I missed information. What I want is a such textarea:
> <div id="inner-ditor">
> "text"
> <br/>
> "hidden"
> </div>
> 
> A TextNode "text" has a renderer and "hidden" doesn't have.
> 
> I don't get how such a situation happens in real web.

How about set "display:none" then add "hidden" to text area by "insertText"
command?

[Yuta Kitamura, 2014-10-06 09:41:30]

At the very least, the current test looks wrong to me, so
I cannot approve this patchset as is. I can imagine that
you'd get assigned to new leak bugs against this test.

Having a wrong test is worse than no test, so, if you can't
reproduce the issue with a layout test or a unit test, then
it is okay for you to commit the change without tests,
as long as you note that fact in the change description
(but you should make a reasonable amount of efforts before
actually doing so).

[yoichio, 2014-10-16 04:22:11]

Finally I found the layout case.
PTAL.

[tkent, 2014-10-16 04:34:38]

lgtm

[yoichio, 2014-10-16 04:36:32]

The CQ bit was checked by yoichio@chromium.org

[commit-bot: I haz the power, 2014-10-16 04:36:55]

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

[commit-bot: I haz the power, 2014-10-16 05:56:52]

Committed patchset #3 (id:40001) as 183784