Bug 123013 – Bad cast with toRenderBoxModelObject in RenderBlock::updateFirstLetter()

• Status: RESOLVED
• Resolution: FIXED
• Component: Layout and Rendering

[Joone Hur]

There is a case that toRenderBoxModelObject cause a crash in RenderBlock::updateFirstLetter() due to bad cast.  (http://trac.webkit.org/changeset/156742)

[Joone Hur]

Created attachment 214584 [details]
Patch

[Andreas Kling]

Comment on attachment 214584 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=214584&action=review

Can we make a test for this?

> Source/WebCore/rendering/RenderBlock.cpp:5179
> +                RenderObject* oldRemainingText = oldFirstLetter->firstLetterRemainingText();

You should use a tighter type than RenderObject* here, since firstLetterRemainingText() returns a RenderTextFragment.
I would use auto, like so:

    auto oldRemainingText = oldFirstLetter->firstLetterRemainingText();

> Source/WebCore/rendering/RenderBlock.cpp:5180
> +                if (oldRemainingText && oldRemainingText->isText()) {

isText() here will always be true.

> Source/WebCore/rendering/RenderBlock.cpp:5183
> +                    toRenderText(oldRemainingText)->setText(toText(oldRemainingText->node())->data().impl());

If you use a tighter type like I suggested above, this line could become:
oldRemainingText->setText(oldRemainingText->textNode()->data());

[Joone Hur]

Comment on attachment 214584 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=214584&action=review

I don't know the exact case that can reproduce this crash because it happened in ClusterFuzz:
https://codereview.chromium.org/25713009/

>> Source/WebCore/rendering/RenderBlock.cpp:5179
>> +                RenderObject* oldRemainingText = oldFirstLetter->firstLetterRemainingText();
> 
> You should use a tighter type than RenderObject* here, since firstLetterRemainingText() returns a RenderTextFragment.
> I would use auto, like so:
> 
>     auto oldRemainingText = oldFirstLetter->firstLetterRemainingText();

Done.

>> Source/WebCore/rendering/RenderBlock.cpp:5180
>> +                if (oldRemainingText && oldRemainingText->isText()) {
> 
> isText() here will always be true.

Done.

>> Source/WebCore/rendering/RenderBlock.cpp:5183
>> +                    toRenderText(oldRemainingText)->setText(toText(oldRemainingText->node())->data().impl());
> 
> If you use a tighter type like I suggested above, this line could become:
> oldRemainingText->setText(oldRemainingText->textNode()->data());

Done.

[Joone Hur]

Created attachment 214644 [details]
Patch

[Andreas Kling]

Comment on attachment 214644 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=214644&action=review

> Source/WebCore/ChangeLog:10
> +        There is a case that toRenderBoxModelObject causes a crash in RenderBlock::updateFirstLetter() 
> +        due to bad cast, so we need to check whether the RenderObject is a RenderBoxModelObject 
> +        by running isBoxModelObject() before calling toRenderBoxModelObject.  

This needs a comment about why we don't have a test.

> Source/WebCore/rendering/RenderBlock.cpp:5178
> +            if (RenderBoxModelObject* oldFirstLetter = descendant->parent()->isBoxModelObject() ? toRenderBoxModelObject(descendant->parent()) : 0) { 

Can we use auto instead of spelling out RenderBoxModelObject twice here?
0 -> nullptr

> Source/WebCore/rendering/RenderBlock.cpp:5183
> +                    createFirstLetterRenderer(firstLetterBlock, toRenderText(remainingText));

We will now only call createFirstLetterRenderer() if oldFirstLetter is a RenderBoxModelObject and there was an oldRemainingText.
Is this intentional, or should it go after the two if blocks?

[Joone Hur]

Comment on attachment 214644 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=214644&action=review

>> Source/WebCore/ChangeLog:10
>> +        by running isBoxModelObject() before calling toRenderBoxModelObject.  
> 
> This needs a comment about why we don't have a test.

Done.

>> Source/WebCore/rendering/RenderBlock.cpp:5178
>> +            if (RenderBoxModelObject* oldFirstLetter = descendant->parent()->isBoxModelObject() ? toRenderBoxModelObject(descendant->parent()) : 0) { 
> 
> Can we use auto instead of spelling out RenderBoxModelObject twice here?
> 0 -> nullptr

Done.

>> Source/WebCore/rendering/RenderBlock.cpp:5183
>> +                    createFirstLetterRenderer(firstLetterBlock, toRenderText(remainingText));
> 
> We will now only call createFirstLetterRenderer() if oldFirstLetter is a RenderBoxModelObject and there was an oldRemainingText.
> Is this intentional, or should it go after the two if blocks?

This is intentional because we can remove the old first letter by running setText and then create a new one. These operations should be done together.

[Joone Hur]

Created attachment 214718 [details]
Patch

[Andreas Kling]

Comment on attachment 214718 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=214718&action=review

Ok, r=me.

> Source/WebCore/rendering/RenderBlock.cpp:5183
> +                    createFirstLetterRenderer(firstLetterBlock, toRenderText(remainingText));

You don't need the toRenderText() here.

[Joone Hur]

Comment on attachment 214718 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=214718&action=review

>> Source/WebCore/rendering/RenderBlock.cpp:5183
>> +                    createFirstLetterRenderer(firstLetterBlock, toRenderText(remainingText));
> 
> You don't need the toRenderText() here.

But, there is a build error:
../../Source/WebCore/rendering/RenderBlock.cpp:5183:78: error: invalid conversion from 'WebCore::RenderObject*' to 'WebCore::RenderText*' [-fpermissive]

[Andreas Kling]

Comment on attachment 214718 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=214718&action=review

>>> Source/WebCore/rendering/RenderBlock.cpp:5183
>>> +                    createFirstLetterRenderer(firstLetterBlock, toRenderText(remainingText));
>> 
>> You don't need the toRenderText() here.
> 
> But, there is a build error:
> ../../Source/WebCore/rendering/RenderBlock.cpp:5183:78: error: invalid conversion from 'WebCore::RenderObject*' to 'WebCore::RenderText*' [-fpermissive]

Oh, you're right! I confused it with oldRemainingText. Sorry about that :)

[Joone Hur]

(In reply to comment #10)
> (From update of attachment 214718 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=214718&action=review
> 
> >>> Source/WebCore/rendering/RenderBlock.cpp:5183
> >>> +                    createFirstLetterRenderer(firstLetterBlock, toRenderText(remainingText));
> >> 
> >> You don't need the toRenderText() here.
> > 
> > But, there is a build error:
> > ../../Source/WebCore/rendering/RenderBlock.cpp:5183:78: error: invalid conversion from 'WebCore::RenderObject*' to 'WebCore::RenderText*' [-fpermissive]
> 
> Oh, you're right! I confused it with oldRemainingText. Sorry about that :)

Thank you for the review! :)

[WebKit Commit Bot]

Comment on attachment 214718 [details]
Patch

Clearing flags on attachment: 214718

Committed r157768: <http://trac.webkit.org/changeset/157768>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.