Bug 227519 – [GPU Process] Canvas image rendering can render arbitrary DOM content in the GPU process, which is against policy (for now)

• Component: New Bugs
• Status: RESOLVED FIXED

[Myles C. Maxfield]

[GPU Process] Canvas image rendering can render arbitrary DOM content in the GPU process, which is against policy (for now)

[Myles C. Maxfield]

Created attachment 432568 [details]
WIP

[Myles C. Maxfield]

<rdar://problem/76678163>

[Myles C. Maxfield]

The test failure might be rdar://77527267

[Myles C. Maxfield]

Created attachment 432622 [details]
Patch

[Myles C. Maxfield]

Comment on attachment 432622 [details]
Patch

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

> Source/WebCore/svg/graphics/SVGImage.cpp:284
> +            return ImageDrawResult::DidDraw;

I should probably add

    if (imageObserver())
        imageObserver()->didDraw(*this);

[Myles C. Maxfield]

Created attachment 432957 [details]
Patch

[Darin Adler]

Comment on attachment 432957 [details]
Patch

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

> Source/WebCore/svg/graphics/SVGImage.cpp:278
> +    // Display list drawing can't handle arbitrary DOM content.
> +    // FIXME: Remove this when it can.
> +    if (!context.hasPlatformContext()) {

Would be nice if there was a way we we're guaranteed to find this when we go to do that later, but I don’t immediately have an idea of how.

> Source/WebCore/svg/graphics/SVGImage.cpp:281
> +            if (static_cast<ImageOrientation::Orientation>(options.orientation()) == ImageOrientation::Orientation::FromImage)

Writing this as a static_cast is ugly. Let’s find a way to do without that; the static_cast makes it look like there is an unsafe conversion between enumeration types. Simplest way is to use a local variable:

    ImageOrientation::Orientation orientation = options.orientation();

Another way would be to add a member function to ImageOrientation to fetch the orientation value or an operator== overload to compare with a specific orientation enumeration value. Generally speaking not sure why we store the orientation enumeration in a class, ImageOrientation, with no other members. That leads to this kind of confusing code. We could instead just have a set of functions that take an ImageOrientation enumeration value.

[Simon Fraser (smfr)]

Comment on attachment 432957 [details]
Patch

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

> Source/WebCore/svg/graphics/SVGImage.cpp:277
> +    // FIXME: Remove this when it can.

FIXME comments should come with bug numbers.

>> Source/WebCore/svg/graphics/SVGImage.cpp:281
>> +            if (static_cast<ImageOrientation::Orientation>(options.orientation()) == ImageOrientation::Orientation::FromImage)
> 
> Writing this as a static_cast is ugly. Let’s find a way to do without that; the static_cast makes it look like there is an unsafe conversion between enumeration types. Simplest way is to use a local variable:
> 
>     ImageOrientation::Orientation orientation = options.orientation();
> 
> Another way would be to add a member function to ImageOrientation to fetch the orientation value or an operator== overload to compare with a specific orientation enumeration value. Generally speaking not sure why we store the orientation enumeration in a class, ImageOrientation, with no other members. That leads to this kind of confusing code. We could instead just have a set of functions that take an ImageOrientation enumeration value.

Why do we ever need to consult ImageOrientation? SVG images don't have EXIF data.

> Source/WebCore/svg/graphics/SVGImage.cpp:284
> +            context.drawNativeImage(*nativeImage, size(), dstRect, srcRect, localImagePaintingOptions);

Will this cause blurriness if a small SVG images is scaled up in the canvas?

[Said Abou-Hallawa]

Comment on attachment 432957 [details]
Patch

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

>> Source/WebCore/svg/graphics/SVGImage.cpp:278
>> +    if (!context.hasPlatformContext()) {
> 
> Would be nice if there was a way we we're guaranteed to find this when we go to do that later, but I don’t immediately have an idea of how.

I think this can be done if we just move some of the code around:

1. Override DisplayList::Recorder::drawImage():

ImageDrawResult Recorder::drawImage(Image& image, const FloatRect& destination, const FloatRect& source, const ImagePaintingOptions& options)
{
    InterpolationQualityMaintainer interpolationQualityForThisScope(*this, options.interpolationQuality());
    if (image.isSVGImage())
        return downcast<SVGImage>(image).drawAsNativeImage(*this, destination, source, options);
    return image.draw(*this, destination, source, options);
}

Add SVG:: drawAsNativeImage():

ImageDrawResult SVGImage::drawAsNativeImage(GraphicsContext& context, const FloatRect& dstRect, const FloatRect& srcRect, const ImagePaintingOptions& options)
{
    auto nativeImage = this->nativeImage(nullptr);
    if (nativeImage)
        return ImageDrawResult::DidNothing;

    auto localImagePaintingOptions = options;
    if (static_cast<ImageOrientation::Orientation>(options.orientation()) == ImageOrientation::Orientation::FromImage)
        localImagePaintingOptions = ImagePaintingOptions(options, ImageOrientation::Orientation::None);

    context.drawNativeImage(*nativeImage, size(), dstRect, srcRect, localImagePaintingOptions);
    if (imageObserver())
        imageObserver()->didDraw(*this);

    return ImageDrawResult::DidDraw;
}

Then we can leave SVG::draw() untouched.

[Myles C. Maxfield]

Comment on attachment 432957 [details]
Patch

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

>>> Source/WebCore/svg/graphics/SVGImage.cpp:278
>>> +    if (!context.hasPlatformContext()) {
>> 
>> Would be nice if there was a way we we're guaranteed to find this when we go to do that later, but I don’t immediately have an idea of how.
> 
> I think this can be done if we just move some of the code around:
> 
> 1. Override DisplayList::Recorder::drawImage():
> 
> ImageDrawResult Recorder::drawImage(Image& image, const FloatRect& destination, const FloatRect& source, const ImagePaintingOptions& options)
> {
>     InterpolationQualityMaintainer interpolationQualityForThisScope(*this, options.interpolationQuality());
>     if (image.isSVGImage())
>         return downcast<SVGImage>(image).drawAsNativeImage(*this, destination, source, options);
>     return image.draw(*this, destination, source, options);
> }
> 
> Add SVG:: drawAsNativeImage():
> 
> ImageDrawResult SVGImage::drawAsNativeImage(GraphicsContext& context, const FloatRect& dstRect, const FloatRect& srcRect, const ImagePaintingOptions& options)
> {
>     auto nativeImage = this->nativeImage(nullptr);
>     if (nativeImage)
>         return ImageDrawResult::DidNothing;
> 
>     auto localImagePaintingOptions = options;
>     if (static_cast<ImageOrientation::Orientation>(options.orientation()) == ImageOrientation::Orientation::FromImage)
>         localImagePaintingOptions = ImagePaintingOptions(options, ImageOrientation::Orientation::None);
> 
>     context.drawNativeImage(*nativeImage, size(), dstRect, srcRect, localImagePaintingOptions);
>     if (imageObserver())
>         imageObserver()->didDraw(*this);
> 
>     return ImageDrawResult::DidDraw;
> }
> 
> Then we can leave SVG::draw() untouched.

Good idea! I'll do this.

Pretty cool how Recorder just inherits from GraphicsContext!!!

>>> Source/WebCore/svg/graphics/SVGImage.cpp:281
>>> +            if (static_cast<ImageOrientation::Orientation>(options.orientation()) == ImageOrientation::Orientation::FromImage)
>> 
>> Writing this as a static_cast is ugly. Let’s find a way to do without that; the static_cast makes it look like there is an unsafe conversion between enumeration types. Simplest way is to use a local variable:
>> 
>>     ImageOrientation::Orientation orientation = options.orientation();
>> 
>> Another way would be to add a member function to ImageOrientation to fetch the orientation value or an operator== overload to compare with a specific orientation enumeration value. Generally speaking not sure why we store the orientation enumeration in a class, ImageOrientation, with no other members. That leads to this kind of confusing code. We could instead just have a set of functions that take an ImageOrientation enumeration value.
> 
> Why do we ever need to consult ImageOrientation? SVG images don't have EXIF data.

drawNativeImage() in BitmapImage.cpp says if (options.orientation().usesWidthAsHeight())

usesWidthAsHeight() says ASSERT(isValidEXIFOrientation(m_orientation));

FromImage is the only non-EXIF orientation.

>> Source/WebCore/svg/graphics/SVGImage.cpp:284
>> +            context.drawNativeImage(*nativeImage, size(), dstRect, srcRect, localImagePaintingOptions);
> 
> Will this cause blurriness if a small SVG images is scaled up in the canvas?

Yes! Will fix.

[Myles C. Maxfield]

Created attachment 433072 [details]
Patch for committing

[Simon Fraser (smfr)]

Comment on attachment 433072 [details]
Patch for committing

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

> Source/WebCore/svg/graphics/SVGImage.cpp:337
> +    FloatRect rectInNativeImage(FloatPoint(), destination.size());

The cool kids write this as 
auto rectInNativeImage = FloatRect { { }, destination.size() };

> Source/WebCore/svg/graphics/SVGImage.cpp:343
> +    ImageOrientation::Orientation orientation = options.orientation();

auto?

[Myles C. Maxfield]

Comment on attachment 433072 [details]
Patch for committing

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

>> Source/WebCore/svg/graphics/SVGImage.cpp:343
>> +    ImageOrientation::Orientation orientation = options.orientation();
> 
> auto?

See earlier discussion with Darin.

[Myles C. Maxfield]

Created attachment 433075 [details]
Patch for committing

[Myles C. Maxfield]

Comment on attachment 432957 [details]
Patch

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

>>>> Source/WebCore/svg/graphics/SVGImage.cpp:278
>>>> +    if (!context.hasPlatformContext()) {
>>> 
>>> Would be nice if there was a way we we're guaranteed to find this when we go to do that later, but I don’t immediately have an idea of how.
>> 
>> I think this can be done if we just move some of the code around:
>> 
>> 1. Override DisplayList::Recorder::drawImage():
>> 
>> ImageDrawResult Recorder::drawImage(Image& image, const FloatRect& destination, const FloatRect& source, const ImagePaintingOptions& options)
>> {
>>     InterpolationQualityMaintainer interpolationQualityForThisScope(*this, options.interpolationQuality());
>>     if (image.isSVGImage())
>>         return downcast<SVGImage>(image).drawAsNativeImage(*this, destination, source, options);
>>     return image.draw(*this, destination, source, options);
>> }
>> 
>> Add SVG:: drawAsNativeImage():
>> 
>> ImageDrawResult SVGImage::drawAsNativeImage(GraphicsContext& context, const FloatRect& dstRect, const FloatRect& srcRect, const ImagePaintingOptions& options)
>> {
>>     auto nativeImage = this->nativeImage(nullptr);
>>     if (nativeImage)
>>         return ImageDrawResult::DidNothing;
>> 
>>     auto localImagePaintingOptions = options;
>>     if (static_cast<ImageOrientation::Orientation>(options.orientation()) == ImageOrientation::Orientation::FromImage)
>>         localImagePaintingOptions = ImagePaintingOptions(options, ImageOrientation::Orientation::None);
>> 
>>     context.drawNativeImage(*nativeImage, size(), dstRect, srcRect, localImagePaintingOptions);
>>     if (imageObserver())
>>         imageObserver()->didDraw(*this);
>> 
>>     return ImageDrawResult::DidDraw;
>> }
>> 
>> Then we can leave SVG::draw() untouched.
> 
> Good idea! I'll do this.
> 
> Pretty cool how Recorder just inherits from GraphicsContext!!!

Uh oh. isSVGImage() returns true on both SVGImageForContainer and SVGImage, and these are both distinct types.

[Myles C. Maxfield]

Comment on attachment 432957 [details]
Patch

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

>>>>> Source/WebCore/svg/graphics/SVGImage.cpp:278
>>>>> +    if (!context.hasPlatformContext()) {
>>>> 
>>>> Would be nice if there was a way we we're guaranteed to find this when we go to do that later, but I don’t immediately have an idea of how.
>>> 
>>> I think this can be done if we just move some of the code around:
>>> 
>>> 1. Override DisplayList::Recorder::drawImage():
>>> 
>>> ImageDrawResult Recorder::drawImage(Image& image, const FloatRect& destination, const FloatRect& source, const ImagePaintingOptions& options)
>>> {
>>>     InterpolationQualityMaintainer interpolationQualityForThisScope(*this, options.interpolationQuality());
>>>     if (image.isSVGImage())
>>>         return downcast<SVGImage>(image).drawAsNativeImage(*this, destination, source, options);
>>>     return image.draw(*this, destination, source, options);
>>> }
>>> 
>>> Add SVG:: drawAsNativeImage():
>>> 
>>> ImageDrawResult SVGImage::drawAsNativeImage(GraphicsContext& context, const FloatRect& dstRect, const FloatRect& srcRect, const ImagePaintingOptions& options)
>>> {
>>>     auto nativeImage = this->nativeImage(nullptr);
>>>     if (nativeImage)
>>>         return ImageDrawResult::DidNothing;
>>> 
>>>     auto localImagePaintingOptions = options;
>>>     if (static_cast<ImageOrientation::Orientation>(options.orientation()) == ImageOrientation::Orientation::FromImage)
>>>         localImagePaintingOptions = ImagePaintingOptions(options, ImageOrientation::Orientation::None);
>>> 
>>>     context.drawNativeImage(*nativeImage, size(), dstRect, srcRect, localImagePaintingOptions);
>>>     if (imageObserver())
>>>         imageObserver()->didDraw(*this);
>>> 
>>>     return ImageDrawResult::DidDraw;
>>> }
>>> 
>>> Then we can leave SVG::draw() untouched.
>> 
>> Good idea! I'll do this.
>> 
>> Pretty cool how Recorder just inherits from GraphicsContext!!!
> 
> Uh oh. isSVGImage() returns true on both SVGImageForContainer and SVGImage, and these are both distinct types.

I'm starting to think this isn't worth it. This design requires:

1. Adding isSVGImageForContainer() in Image and SVGImageForContainer

2. Recorder::drawImage() becomes:

ImageDrawResult Recorder::drawImage(Image& image, const FloatRect& destination, const FloatRect& source, const ImagePaintingOptions& options)
{
    if (is<SVGImage>(image)) {
        // Display list drawing can't handle arbitrary DOM content.
        // FIXME https://bugs.webkit.org/show_bug.cgi?id=227748: Remove this when it can.
        InterpolationQualityMaintainer interpolationQualityForThisScope(*this, options.interpolationQuality());
        if (is<SVGImageForContainer>(image))
            downcast<SVGImageForContainer>(image).drawAsNativeImage(*this, destination, source, options);
        else
            return downcast<SVGImage>(image).drawAsNativeImage(*this, destination, source, options);
    }
    return GraphicsContext::drawImage(image, destination, source, options);
}

3. SVGImageForContainer::drawAsNativeImage() needs to be added: 

ImageDrawResult SVGImageForContainer::drawAsNativeImage(GraphicsContext& context, const FloatRect& destination, const FloatRect& source, const ImagePaintingOptions& options)
{
    return m_image->drawAsNativeImageForContainer(context, m_containerSize, m_containerZoom, m_initialFragmentURL, dstRect, srcRect, options);
}

4. SVGImage::drawAsNativeImageForContainer() needs to be added, because SVGImage::drawForContainer() calls SVGImage::draw()

5. SVGImage::drawForContainer() needs to be refactored to share code with SVGImage::drawAsNativeImageForContainer()

In the end, it doesn't seem worth it to do all this just to move the FIXME from SVGImage::draw() to Recorder::drawImage().

[Myles C. Maxfield]

Created attachment 433121 [details]
Patch for committing

[EWS]

Committed r279722 (239510@main): <https://commits.webkit.org/239510@main>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 433121 [details].

[Cameron McCormack (:heycam)]

*** Bug 227824 has been marked as a duplicate of this bug. ***