Bug 175444 – Followup (r220289): RenderImageResourceStyleImage code clean up

• Component: Images
• Status: RESOLVED FIXED

[Said Abou-Hallawa]

Adding and removing the m_renderer of the RenderImageResourceStyleImage as a client to its m_cachedImage should be done by the same class. 

-- If RenderImageResourceStyleImage has StyleImage of type StyleCachedImage, adding and removing m_renderer is done through StyleCachedImage::addClient() and removeClient() which are called from RenderImageResourceStyleImage::initialize() and shutdown() respectively.

-- If RenderImageResourceStyleImage has StyleImage of type StyleGeneratedImage, adding and removing m_renderer will be done through RenderImageResource::setCachedImage(). This function is called from ImageLoader::updateRenderer() when the CachedImage finishes loading. It adds m_renderer as a client of the CachedImage. To remove m_renderer from the set of the CachedImage clients, RenderImageResource::setCachedImage() will be called with nullptr form RenderImageResourceStyleImage::shutdown() via RenderImageResource::shutdown().

[Said Abou-Hallawa]

Created attachment 317856 [details]
Patch

[Said Abou-Hallawa]

Created attachment 317872 [details]
Patch

[Build Bot]

Comment on attachment 317872 [details]
Patch

Attachment 317872 [details] did not pass mac-debug-ews (mac):
Output: http://webkit-queues.webkit.org/results/4292649

New failing tests:
fast/canvas/webgl/css-webkit-canvas.html
fast/canvas/canvas-as-image-incremental-repaint.html
fast/canvas/canvas-as-image-hidpi.html
fast/canvas/canvas-as-image.html
fast/canvas/webgl/css-webkit-canvas-repaint.html

[Build Bot]

Created attachment 317896 [details]
Archive of layout-test-results from ews115 for mac-elcapitan

The attached test failures were seen while running run-webkit-tests on the mac-debug-ews.
Bot: ews115  Port: mac-elcapitan  Platform: Mac OS X 10.11.6

[Said Abou-Hallawa]

Created attachment 317950 [details]
Patch

[Said Abou-Hallawa]

(In reply to Build Bot from comment #3)
> Comment on attachment 317872 [details]
> Patch
> 
> Attachment 317872 [details] did not pass mac-debug-ews (mac):
> Output: http://webkit-queues.webkit.org/results/4292649
> 
> New failing tests:
> fast/canvas/webgl/css-webkit-canvas.html
> fast/canvas/canvas-as-image-incremental-repaint.html
> fast/canvas/canvas-as-image-hidpi.html
> fast/canvas/canvas-as-image.html
> fast/canvas/webgl/css-webkit-canvas-repaint.html

These crashes were happening because I put m_styleImage->removeClient() before calling RenderImageResource::shutdown(). RenderImageResource::shutdown() has to be last becasue CSSImageGeneratorValue::removeClient() will deref() itself if m_renderer is the last client. RenderImageResource::shutdown() calls StyleGeneratedImage::image() which calls CSSImageGeneratorValue::image(). This may end up calling a function of an object after it was freed.

[Darin Adler]

Comment on attachment 317950 [details]
Patch

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

Looks like a step in the right direction, but I think it might need a little more work.

> Source/WebCore/rendering/RenderImageResource.h:43
>      virtual void initialize(RenderElement*);

This function should take a reference, not a pointer.

> Source/WebCore/rendering/RenderImageResource.h:60
> -    virtual bool errorOccurred() const;
> +    virtual bool errorOccurred() const { return m_cachedImage && m_cachedImage->errorOccurred(); }
>  
>      virtual void setContainerSizeForRenderer(const IntSize&);
> -    virtual bool imageHasRelativeWidth() const;
> -    virtual bool imageHasRelativeHeight() const;
>  
> -    virtual LayoutSize imageSize(float multiplier) const;
> -    virtual LayoutSize intrinsicSize(float multiplier) const;
> +    virtual bool imageHasRelativeWidth() const { return m_cachedImage && m_cachedImage->imageHasRelativeWidth(); }
> +    virtual bool imageHasRelativeHeight() const { return m_cachedImage && m_cachedImage->imageHasRelativeHeight(); }
> +
> +    LayoutSize imageSize(float multiplier) const { return imageSize(multiplier, CachedImage::UsedSize); }
> +    LayoutSize intrinsicSize(float multiplier) const { return imageSize(multiplier, CachedImage::UsedSize); }

What’s the rationale for moving these function bodies into the class definition? Maybe we want inlining?

> Source/WebCore/rendering/RenderImageResource.h:65
>  protected:
> +    virtual LayoutSize imageSize(float multiplier, CachedImage::SizeType) const;

Why make this protected rather than private? The old version of this function was private.

As far as I can tell, the protected members of this class are all things that should be private. The strange code in this patch comes from setting m_cachedImage directly in a derived class, and in my opinion making this function protected instead of private adds (slightly) to that problem.

> Source/WebCore/rendering/RenderImageResourceStyleImage.cpp:55
> +    // Revert what initialize() did for m_cachedImage before calling shutdown() of the base class.
> +    if (m_styleImage->isCachedImage())
> +        m_cachedImage = nullptr;

I don’t think this comment is clear enough.

I think the issue here is that in RenderImageResourceStyleImage::initialize we set m_cachedImage, bypassing the setCachedImage function, and so we did not do an addClient and so we must not do a removeClient. Setting m_cachedImage to nullptr prevents that from happening. But this is dangerous for any call to setCachedImage, not just for the call inside the shutdown function. I think a better design requires the base class understanding that m_cachedImage is in this state so that setCachedImage will work properly. I see seven different call sites for setCachedImage and it’s not easy for me to prove to myself that none of them can be called on a RenderImageResourceStyleImage object.

Maybe we can make m_cachedImage private and use setCachedImage here? Would it be OK to call addClient/removeClient on the style image?

> Source/WebCore/rendering/RenderImageResourceStyleImage.h:53
> +    LayoutSize imageSize(float multiplier, CachedImage::SizeType) const override { return LayoutSize(m_styleImage->imageSize(m_renderer, multiplier)); }

Our coding style guideline stays we should use final instead of override, here and in the other functions in this class.

[Said Abou-Hallawa]

Created attachment 318056 [details]
Patch

[Said Abou-Hallawa]

Comment on attachment 317950 [details]
Patch

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

>> Source/WebCore/rendering/RenderImageResource.h:43
>>      virtual void initialize(RenderElement*);
> 
> This function should take a reference, not a pointer.

Done.

>> Source/WebCore/rendering/RenderImageResource.h:60
>> +    LayoutSize intrinsicSize(float multiplier) const { return imageSize(multiplier, CachedImage::UsedSize); }
> 
> What’s the rationale for moving these function bodies into the class definition? Maybe we want inlining?

Yes I think we want inlining for these functions.

>> Source/WebCore/rendering/RenderImageResource.h:65
>> +    virtual LayoutSize imageSize(float multiplier, CachedImage::SizeType) const;
> 
> Why make this protected rather than private? The old version of this function was private.
> 
> As far as I can tell, the protected members of this class are all things that should be private. The strange code in this patch comes from setting m_cachedImage directly in a derived class, and in my opinion making this function protected instead of private adds (slightly) to that problem.

I made all the members be private and I managed to change m_cachedImage only from the base class.

>> Source/WebCore/rendering/RenderImageResourceStyleImage.cpp:55
>> +        m_cachedImage = nullptr;
> 
> I don’t think this comment is clear enough.
> 
> I think the issue here is that in RenderImageResourceStyleImage::initialize we set m_cachedImage, bypassing the setCachedImage function, and so we did not do an addClient and so we must not do a removeClient. Setting m_cachedImage to nullptr prevents that from happening. But this is dangerous for any call to setCachedImage, not just for the call inside the shutdown function. I think a better design requires the base class understanding that m_cachedImage is in this state so that setCachedImage will work properly. I see seven different call sites for setCachedImage and it’s not easy for me to prove to myself that none of them can be called on a RenderImageResourceStyleImage object.
> 
> Maybe we can make m_cachedImage private and use setCachedImage here? Would it be OK to call addClient/removeClient on the style image?

I added two protected methods in the base class: initialize() and shutdown(). Each of them takes a pointer to CachedImage. If m_styleImage is of type StyleCachedImage, a pointer to the CachedImage will be passed to these new methods. RenderImageResource will not add or remove m_renderer to or from the clients of its CachedImage if the passed CachedImage is not null. This is will be handled by the calls m_styleImage->addClient) and m_styleImage->removeClient() which are invoked by this class.

>> Source/WebCore/rendering/RenderImageResourceStyleImage.h:53
>> +    LayoutSize imageSize(float multiplier, CachedImage::SizeType) const override { return LayoutSize(m_styleImage->imageSize(m_renderer, multiplier)); }
> 
> Our coding style guideline stays we should use final instead of override, here and in the other functions in this class.

Fixed.

[Darin Adler]

Comment on attachment 317950 [details]
Patch

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

>>> Source/WebCore/rendering/RenderImageResourceStyleImage.cpp:55
>>> +        m_cachedImage = nullptr;
>> 
>> I don’t think this comment is clear enough.
>> 
>> I think the issue here is that in RenderImageResourceStyleImage::initialize we set m_cachedImage, bypassing the setCachedImage function, and so we did not do an addClient and so we must not do a removeClient. Setting m_cachedImage to nullptr prevents that from happening. But this is dangerous for any call to setCachedImage, not just for the call inside the shutdown function. I think a better design requires the base class understanding that m_cachedImage is in this state so that setCachedImage will work properly. I see seven different call sites for setCachedImage and it’s not easy for me to prove to myself that none of them can be called on a RenderImageResourceStyleImage object.
>> 
>> Maybe we can make m_cachedImage private and use setCachedImage here? Would it be OK to call addClient/removeClient on the style image?
> 
> I added two protected methods in the base class: initialize() and shutdown(). Each of them takes a pointer to CachedImage. If m_styleImage is of type StyleCachedImage, a pointer to the CachedImage will be passed to these new methods. RenderImageResource will not add or remove m_renderer to or from the clients of its CachedImage if the passed CachedImage is not null. This is will be handled by the calls m_styleImage->addClient) and m_styleImage->removeClient() which are invoked by this class.

This does not resolve the potential problem I mentioned above. If a caller calls setCachedImage then we will call removeClient on m_cachedImage even though we did not call addClient when setting m_cachedImage.

We can fix this a variety of ways:

1) Record the fact that we did not add a client for a given image, perhaps with a boolean alongside m_cachedImage, and respect that boolean in setCachedImage.
2) Ensure setCachedImage can never be called in such cases, somehow.
3) Add the renderer as a client even though we don’t need to, if it would do no harm.

I’m sure there are other solutions as well.

[Said Abou-Hallawa]

Created attachment 318418 [details]
Patch

[Said Abou-Hallawa]

Comment on attachment 317950 [details]
Patch

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

>>>> Source/WebCore/rendering/RenderImageResourceStyleImage.cpp:55
>>>> +        m_cachedImage = nullptr;
>>> 
>>> I don’t think this comment is clear enough.
>>> 
>>> I think the issue here is that in RenderImageResourceStyleImage::initialize we set m_cachedImage, bypassing the setCachedImage function, and so we did not do an addClient and so we must not do a removeClient. Setting m_cachedImage to nullptr prevents that from happening. But this is dangerous for any call to setCachedImage, not just for the call inside the shutdown function. I think a better design requires the base class understanding that m_cachedImage is in this state so that setCachedImage will work properly. I see seven different call sites for setCachedImage and it’s not easy for me to prove to myself that none of them can be called on a RenderImageResourceStyleImage object.
>>> 
>>> Maybe we can make m_cachedImage private and use setCachedImage here? Would it be OK to call addClient/removeClient on the style image?
>> 
>> I added two protected methods in the base class: initialize() and shutdown(). Each of them takes a pointer to CachedImage. If m_styleImage is of type StyleCachedImage, a pointer to the CachedImage will be passed to these new methods. RenderImageResource will not add or remove m_renderer to or from the clients of its CachedImage if the passed CachedImage is not null. This is will be handled by the calls m_styleImage->addClient) and m_styleImage->removeClient() which are invoked by this class.
> 
> This does not resolve the potential problem I mentioned above. If a caller calls setCachedImage then we will call removeClient on m_cachedImage even though we did not call addClient when setting m_cachedImage.
> 
> We can fix this a variety of ways:
> 
> 1) Record the fact that we did not add a client for a given image, perhaps with a boolean alongside m_cachedImage, and respect that boolean in setCachedImage.
> 2) Ensure setCachedImage can never be called in such cases, somehow.
> 3) Add the renderer as a client even though we don’t need to, if it would do no harm.
> 
> I’m sure there are other solutions as well.

I added the bool RenderImageResource::m_cachedImageClientAddedExternally which is initialized to false. It will be set to true if RenderImageResource::initialize() is passed a valid styleCachedImage. In this case the StyleImage is of type StyleCachedImage.  RenderImageResourceStyleImage::initialize() and shutdown() will call StyleCachedImage::addClient() and removeClient() which will take care of adding and removing the renderer as a client to the CachedImage. RenderImageResource::setCachedImage() will respect this flag by not calling CachedImage::removeClient() unless this flag is false.

[Darin Adler]

Comment on attachment 318418 [details]
Patch

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

OK, thanks. That fixes the "setCachedImage is dangerous" issue. I’m not sure I love the name of “m_cachedImageClientAddedExternally” but I don’t have a better name to suggest at the moment. I also would have put the inline function bodies outside the class definitions to keep them cleaner, but that’s a style question upon which it seems various reasonable people differ.

Side question about the original code (not the new code). What guarantees that m_cachedImage->removeClient is called if a RenderImageResource object is destroyed without first calling setCachedImage(nullptr)?

> Source/WebCore/rendering/RenderImageResource.h:46
> +    RenderElement* renderer() const { return m_renderer; }

Can this be protected instead of public, please?

[Said Abou-Hallawa]

Created attachment 318525 [details]
Patch

[Said Abou-Hallawa]

(In reply to Darin Adler from comment #13)
> Comment on attachment 318418 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=318418&action=review
> 
> OK, thanks. That fixes the "setCachedImage is dangerous" issue. I’m not sure
> I love the name of “m_cachedImageClientAddedExternally” but I don’t have a
> better name to suggest at the moment. I also would have put the inline
> function bodies outside the class definitions to keep them cleaner, but
> that’s a style question upon which it seems various reasonable people differ.
> 

I renamed the boolean to m_cachedImageRemoveClientIsNeeded which has the opposite meaning of m_cachedImageClientAddedExternally.

> Side question about the original code (not the new code). What guarantees
> that m_cachedImage->removeClient is called if a RenderImageResource object
> is destroyed without first calling setCachedImage(nullptr)?
> 

The RenderImage class calls RenderImageResource::initialize() from RenderImage::styleWillChange() when a style is applied for the first time. It calls RenderImageResource::shutdown() from RenderImage::willBeDestroyed() is called.
-- So RenderImage::willBeDestroyed() will be called before the renderer is destroyed. That means RenderImageResource::shutdown() or RenderImageResourceStyleImage::shutdown() will be called before the renderer is destroyed.
If RenderImage has its ImageResource of type RenderImageResource, it will have its m_cachedImageRemoveClientIsNeeded = true since this is the default.
-- So CachedImage::removeClient() will be called in RenderImageResource::setCachedImage() only if m_cachedImage isn't nullptr. This can only happen if setCachedImage() is called directly from ImageLoader::updateRenderer().
If RenderImage has its ImageResource of type RenderImageResourceStyleImage and its m_styleImage of type CachedStyleImage, its m_cachedImageRemoveClientIsNeeded = false because RenderImageResource::initialize() will be called with a valid styleCachedImage.
-- So CachedImage::removeClient() won't be called in RenderImageResource::setCachedImage(). But CachedImage::removeClient() will be called form StyleCachedImage::removeClient() which is called from RenderImageResourceStyleImage::shutdown().
If RenderImage has its ImageResource of type RenderImageResourceStyleImage and its m_styleImage of type StyleGeneratedImage, its m_cachedImageRemoveClientIsNeeded = true because RenderImageResource::initialize() will be called with a null styleCachedImage.
-- So CachedImage::removeClient() will be called in RenderImageResource::setCachedImage() only if m_cachedImage isn't nullptr. This can only happen if setCachedImage() is called directly from ImageLoader::updateRenderer().

> > Source/WebCore/rendering/RenderImageResource.h:46
> > +    RenderElement* renderer() const { return m_renderer; }
> 
> Can this be protected instead of public, please?

Done.

[WebKit Commit Bot]

Comment on attachment 318525 [details]
Patch

Clearing flags on attachment: 318525

Committed r220934: <http://trac.webkit.org/changeset/220934>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Radar WebKit Bug Importer]

<rdar://problem/33970434>