Null check renderers consistently in StyleImage code
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.