DisplayList::ImageBuffer calls virtual function erroneously in destructor

[Kimmo Kinnunen]

ConcreteImageBuffer calls virtual function erroneously in destructor

Calls flushDrawingContext().

Virtual function call ends up calling the currently destructed class version of the function, not the final overridden function.

In case of all ConcreteImageBuffer inheritance chains, this is semi-surprising behavioral difference in case of RemoteImageBufferProxy which overrides flush.
The code that is produced is incorrect as per logic.
As it stands today, the code doesn't crash as the ~RemoteImageBufferProxy flushes and that causes early-out in ConcreteImageBuffer::flushDrawingContext().

However, if ~RemoteImageBufferProxy does not cause the early out, it appears that the ConcreteImageBuffer::flushDrawingContext() seems to crash in a non-trivial convoluted way.

As this is surprising and typically incorrect pattern, it should be mostly not done.

[Kimmo Kinnunen]

Correction: DisplayList::ImageBuffer, not ConcreteImageBuffer, calls the virtual function.

[Kimmo Kinnunen]

Created attachment 431309 [details]
Patch

[Radar WebKit Bug Importer]

<rdar://problem/79554863>

[Said Abou-Hallawa]

Comment on attachment 431309 [details]
Patch

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

> Source/WebCore/ChangeLog:27
> +        Make the destructor public because it'd be surprising that the instances
> +        are intended to be holdable via `ImageBuffer` reference and via the final class
> +        references but not via the intermediate class references.

I do not understand what this statement means. Also I think this destructor of DisplayList::ImageBuffer can be protected now.

> Source/WebCore/platform/graphics/PlatformImageBuffer.h:87
> +class DisplayListUnacceleratedImageBuffer final : public DisplayList::ImageBuffer<UnacceleratedImageBufferBackend> {
> +    using Base = DisplayList::ImageBuffer<UnacceleratedImageBufferBackend>;
> +    using Base::Base;
> +public:
> +    static auto create(const WebCore::FloatSize& size, float resolutionScale, const WebCore::DestinationColorSpace& colorSpace, WebCore::PixelFormat pixelFormat, const HostWindow* hostWindow = nullptr)
> +    {
> +        return Base::create<DisplayListUnacceleratedImageBuffer>(size, resolutionScale, colorSpace, pixelFormat, hostWindow);
> +    }
> +    ~DisplayListUnacceleratedImageBuffer() final
> +    {
> +        flushDrawingContext();
> +    }
> +};
> +
> +class DisplayListAcceleratedImageBuffer final : public DisplayList::ImageBuffer<AcceleratedImageBufferBackend> {
> +    using Base = DisplayList::ImageBuffer<AcceleratedImageBufferBackend>;
> +    using Base::Base;
> +public:
> +    static auto create(const WebCore::FloatSize& size, float resolutionScale, const WebCore::DestinationColorSpace& colorSpace, WebCore::PixelFormat pixelFormat, const HostWindow* hostWindow = nullptr)
> +    {
> +        return Base::create<DisplayListAcceleratedImageBuffer>(size, resolutionScale, colorSpace, pixelFormat, hostWindow);
> +    }
> +    ~DisplayListAcceleratedImageBuffer() final
> +    {
> +        flushDrawingContext();
> +    }
> +};

This repeated code can be simplified by using templates:

// maybe in a new file or in DisplayListImageBuffer.h
namespace WebCore {
namespace DisplayList {

template<typename BackendType>
class ConcreteImageBuffer final : public ImageBuffer<BackendType> {
    using Base = ImageBuffer<BackendType>;
    using Base::Base;
public:
    static auto create(const WebCore::FloatSize& size, float resolutionScale, const WebCore::DestinationColorSpace& colorSpace, WebCore::PixelFormat pixelFormat, const HostWindow* hostWindow = nullptr)
    {
        return Base::template create<ConcreteImageBuffer>(size, resolutionScale, colorSpace, pixelFormat, hostWindow);
    }
    
    ~ConcreteImageBuffer() final
    {
        Base::flushDrawingContext();
    }
};

} // DisplayList
} // namespace WebCore

// And in this file:
using DisplayListUnacceleratedImageBuffer = DisplayList::ConcreteImageBuffer<UnacceleratedImageBufferBackend>;
using DisplayListAcceleratedImageBuffer = DisplayList::ConcreteImageBuffer<AcceleratedImageBufferBackend>;

> Source/WebCore/platform/graphics/displaylists/DisplayListImageBuffer.h:63
> -    ~ImageBuffer()
> +    ~ImageBuffer() override

What 'override' adds here? I am not sure what is the webkit coding style for overriding virtual destructor. I found most of the classes add virtual before the overridden destructor, e.g. BitmapImage, RenderLayerModelObject, RenderImage, CachedImage. So following this pattern this code should be:

    virtual ~ImageBuffer() { ... }

I also think this destructor can be protected now.

> Source/WebKit/WebProcess/GPU/graphics/RemoteImageBufferProxy.h:66
> -    ~RemoteImageBufferProxy()
> +    ~RemoteImageBufferProxy() override

I think this should be 

virtual ~RemoteImageBufferProxy() { ... }

[Dean Jackson]

Comment on attachment 431309 [details]
Patch

Clearing old review flag. Please rebase and mark for review if necessary.