Bug 123013 - Bad cast with toRenderBoxModelObject in RenderBlock::updateFirstLetter()
Summary: Bad cast with toRenderBoxModelObject in RenderBlock::updateFirstLetter()
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Layout and Rendering (show other bugs)
Version: 528+ (Nightly build)
Hardware: All All
: P2 Normal
Assignee: Joone Hur
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2013-10-18 02:48 PDT by Joone Hur
Modified: 2013-10-21 17:28 PDT (History)
5 users (show)

See Also:


Attachments
Patch (2.54 KB, patch)
2013-10-18 10:14 PDT, Joone Hur
no flags Details | Formatted Diff | Diff
Patch (2.44 KB, patch)
2013-10-19 01:38 PDT, Joone Hur
no flags Details | Formatted Diff | Diff
Patch (2.55 KB, patch)
2013-10-21 02:21 PDT, Joone Hur
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Joone Hur 2013-10-18 02:48:37 PDT
There is a case that toRenderBoxModelObject cause a crash in RenderBlock::updateFirstLetter() due to bad cast.  (http://trac.webkit.org/changeset/156742)
Comment 1 Joone Hur 2013-10-18 10:14:16 PDT
Created attachment 214584 [details]
Patch
Comment 2 Andreas Kling 2013-10-18 13:48:03 PDT
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());
Comment 3 Joone Hur 2013-10-19 01:35:51 PDT
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.
Comment 4 Joone Hur 2013-10-19 01:38:22 PDT
Created attachment 214644 [details]
Patch
Comment 5 Andreas Kling 2013-10-19 03:37:19 PDT
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?
Comment 6 Joone Hur 2013-10-21 02:15:00 PDT
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.
Comment 7 Joone Hur 2013-10-21 02:21:05 PDT
Created attachment 214718 [details]
Patch
Comment 8 Andreas Kling 2013-10-21 02:31:40 PDT
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.
Comment 9 Joone Hur 2013-10-21 16:23:01 PDT
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]
Comment 10 Andreas Kling 2013-10-21 17:03:29 PDT
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 :)
Comment 11 Joone Hur 2013-10-21 17:14:41 PDT
(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! :)
Comment 12 WebKit Commit Bot 2013-10-21 17:28:06 PDT
Comment on attachment 214718 [details]
Patch

Clearing flags on attachment: 214718

Committed r157768: <http://trac.webkit.org/changeset/157768>
Comment 13 WebKit Commit Bot 2013-10-21 17:28:08 PDT
All reviewed patches have been landed.  Closing bug.