Bug 221287 - Null check renderers consistently in StyleImage code
Summary: Null check renderers consistently in StyleImage code
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Layout and Rendering (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Darin Adler
URL:
Keywords: InRadar
Depends on:
Blocks: 221434
  Show dependency treegraph
 
Reported: 2021-02-02 12:57 PST by Darin Adler
Modified: 2022-06-08 05:34 PDT (History)
12 users (show)

See Also:


Attachments
Patch (23.22 KB, patch)
2021-02-02 13:20 PST, Darin Adler
simon.fraser: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Darin Adler 2021-02-02 12:57:10 PST
Null check renderers consistently in StyleImage code
Comment 1 Darin Adler 2021-02-02 13:20:34 PST
Created attachment 419060 [details]
Patch
Comment 2 Darin Adler 2021-02-02 13:58:52 PST
rdar://73356955
Comment 3 Darin Adler 2021-02-02 14:00:40 PST
Committed r272235: <https://trac.webkit.org/changeset/272235>
Comment 4 Said Abou-Hallawa 2021-02-03 11:01:08 PST
Comment on attachment 419060 [details]
Patch

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

> Source/WebCore/rendering/style/StyleGeneratedImage.cpp:57
>      if (m_fixedSize) {

An early return could make the code look better.

if (!m_fixedSize)
    return m_containerSize;

> Source/WebCore/rendering/style/StyleGeneratedImage.cpp:59
> +        if (!renderer)
> +            return { };

This line below could also be simplified since we know after this if-statement render is not null.

    float deviceScaleFactor = renderer ? renderer->document().deviceScaleFactor() : 1;

> Source/WebCore/rendering/style/StyleGeneratedImage.cpp:106
>  bool StyleGeneratedImage::knownToBeOpaque(const RenderElement* renderer) const

I think this function can take const RenderElement& also.
Comment 5 Darin Adler 2021-02-03 15:34:18 PST
Comment on attachment 419060 [details]
Patch

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

>> Source/WebCore/rendering/style/StyleGeneratedImage.cpp:57
>>      if (m_fixedSize) {
> 
> An early return could make the code look better.
> 
> if (!m_fixedSize)
>     return m_containerSize;

I agree.

>> Source/WebCore/rendering/style/StyleGeneratedImage.cpp:59
>> +            return { };
> 
> This line below could also be simplified since we know after this if-statement render is not null.
> 
>     float deviceScaleFactor = renderer ? renderer->document().deviceScaleFactor() : 1;

Good point!

>> Source/WebCore/rendering/style/StyleGeneratedImage.cpp:106
>>  bool StyleGeneratedImage::knownToBeOpaque(const RenderElement* renderer) const
> 
> I think this function can take const RenderElement& also.

Yes, I didn’t see that before, but I looked carefully and see that now.
Comment 6 Antti Koivisto 2022-06-08 05:34:33 PDT
Comment on attachment 419060 [details]
Patch

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

> Source/WebCore/rendering/RenderImageResource.cpp:68
> -    ASSERT(m_renderer);
> -    if (m_cachedImage && m_cachedImageRemoveClientIsNeeded)
> -        m_cachedImage->removeClient(*renderer());
> +    if (m_cachedImage && m_renderer && m_cachedImageRemoveClientIsNeeded)
> +        m_cachedImage->removeClient(*m_renderer);
>      m_cachedImage = newImage;
>      m_cachedImageRemoveClientIsNeeded = true;
>      if (!m_cachedImage)

I don't think this sort of random sprinkling of null tests is a good idea. Here m_renderer owns the RenderImageResource and m_renderer is a WeakPtr solely to protect against security issues (it could be CheckedPtr but renderers are already CanMakeWeakPtr). In any case where m_renderer is null, RenderImageResource is itself also likely dead and we are in a bad state. Adding null checks allows code to proceed further in bad state and makes identifying root causes from crash stacks harder.