Bug 221287

Created attachment 419060 [details]
Patch

rdar://73356955

Committed r272235: <https://trac.webkit.org/changeset/272235>

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 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 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.