Bug 219068 – canvas: drawImage should not raise IndexSizeError on empty sources

• Status: RESOLVED
• Resolution: FIXED
• Component: Canvas

[Lauro Moura]

Test: imported/w3c/web-platform-tests/html/canvas/offscreen/drawing-images-to-the-canvas/2d.drawImage.zerosource.html

Recently, this test was updated in WPT[1] to avoid raising IndexSizeError in drawImage for empty sources, as the spec at [2] does not specify it should be raised in this case.

In WebKit, this change was pulled in r269598 and the test has been flaky raising this exception as a console message (the exception is swallowed by the test helper code).

[1] https://github.com/web-platform-tests/wpt/issues/2835
[2] https://html.spec.whatwg.org/multipage/canvas.html#check-the-usability-of-the-image-argument

[Lauro Moura]

Created attachment 414413 [details]
Tentative patch

Moving the check for empty source to the start of drawImage(ImageBitmap). Tested locally with fast/canvas and imported/w3c/web-platform-tests/html/canvas without regressions.

[Lauro Moura]

Created attachment 414539 [details]
Updated patch

Although the WPT issue links the 'check usability of the image' section, the correct check this test is about is whether one of the dimensions of the src rect is zero.

[Noam Rosenthal]

Comment on attachment 414539 [details]
Updated patch

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

> Source/WebCore/html/canvas/CanvasRenderingContext2DBase.cpp:1668
> +    if (!srcRect.width() != !srcRect.height())

Shouldn't this be || instead of != ?

[Noam Rosenthal]

Comment on attachment 414539 [details]
Updated patch

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

>> Source/WebCore/html/canvas/CanvasRenderingContext2DBase.cpp:1668
>> +    if (!srcRect.width() != !srcRect.height())
> 
> Shouldn't this be || instead of != ?

Or even better, srcRect.isEmpty()

[Chris Lord]

(In reply to Noam Rosenthal from comment #4)
> Comment on attachment 414539 [details]
> Updated patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=414539&action=review
> 
> >> Source/WebCore/html/canvas/CanvasRenderingContext2DBase.cpp:1668
> >> +    if (!srcRect.width() != !srcRect.height())
> > 
> > Shouldn't this be || instead of != ?
> 
> Or even better, srcRect.isEmpty()

I thought this on initial read, but I think it's correct - it's checking if only one of the parameters is zero. If both are zero, you want the exception, which is checked in the next line (though it superfluously still has both dimensions in the check, so makes this read a bit confusingly to me).

Maybe this would read less confusingly as

if (srcRect.isEmpty())
  return (srcRect.width() || srcRect.height()) ? { } : { IndexSizeError };

[Noam Rosenthal]

Comment on attachment 414539 [details]
Updated patch

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

>>>> Source/WebCore/html/canvas/CanvasRenderingContext2DBase.cpp:1668
>>>> +    if (!srcRect.width() != !srcRect.height())
>>> 
>>> Shouldn't this be || instead of != ?
>> 
>> Or even better, srcRect.isEmpty()
> 
> I thought this on initial read, but I think it's correct - it's checking if only one of the parameters is zero. If both are zero, you want the exception, which is checked in the next line (though it superfluously still has both dimensions in the check, so makes this read a bit confusingly to me).
> 
> Maybe this would read less confusingly as
> 
> if (srcRect.isEmpty())
>   return (srcRect.width() || srcRect.height()) ? { } : { IndexSizeError };

Yes, it's confusing. I would suggest

if (srcRect.size().isZero())
  return { IndexSizeError };

if (srcRect.isEmpty())
  return { };

[Chris Lord]

(In reply to Noam Rosenthal from comment #6)
> Yes, it's confusing. I would suggest
> 
> if (srcRect.size().isZero())
>   return { IndexSizeError };
> 
> if (srcRect.isEmpty())
>   return { };

That's much nicer, will have to make a note of that for the future...

[Simon Fraser (smfr)]

Comment on attachment 414539 [details]
Updated patch

r- based on feedback

[Noam Rosenthal]

Comment on attachment 414539 [details]
Updated patch

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

> Source/WebCore/ChangeLog:9
> +

Can you put a link to a hash in the spec? I couldn't find this...

>>>>> Source/WebCore/html/canvas/CanvasRenderingContext2DBase.cpp:1668
>>>>> +    if (!srcRect.width() != !srcRect.height())
>>>> 
>>>> Shouldn't this be || instead of != ?
>>> 
>>> Or even better, srcRect.isEmpty()
>> 
>> I thought this on initial read, but I think it's correct - it's checking if only one of the parameters is zero. If both are zero, you want the exception, which is checked in the next line (though it superfluously still has both dimensions in the check, so makes this read a bit confusingly to me).
>> 
>> Maybe this would read less confusingly as
>> 
>> if (srcRect.isEmpty())
>>   return (srcRect.width() || srcRect.height()) ? { } : { IndexSizeError };
> 
> Yes, it's confusing. I would suggest
> 
> if (srcRect.size().isZero())
>   return { IndexSizeError };
> 
> if (srcRect.isEmpty())
>   return { };

btw, where in the standard does it say that if both are zero there should be an exception? The patch and the Changelog should be more explicit about this.

[Lauro Moura]

Comment on attachment 414539 [details]
Updated patch

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

>> Source/WebCore/ChangeLog:9
>> +
> 
> Can you put a link to a hash in the spec? I couldn't find this...

Sure.  https://html.spec.whatwg.org/multipage/canvas.html#drawing-images

I'll add it to the changelog.

>>>>>> Source/WebCore/html/canvas/CanvasRenderingContext2DBase.cpp:1668
>>>>>> +    if (!srcRect.width() != !srcRect.height())
>>>>> 
>>>>> Shouldn't this be || instead of != ?
>>>> 
>>>> Or even better, srcRect.isEmpty()
>>> 
>>> I thought this on initial read, but I think it's correct - it's checking if only one of the parameters is zero. If both are zero, you want the exception, which is checked in the next line (though it superfluously still has both dimensions in the check, so makes this read a bit confusingly to me).
>>> 
>>> Maybe this would read less confusingly as
>>> 
>>> if (srcRect.isEmpty())
>>>   return (srcRect.width() || srcRect.height()) ? { } : { IndexSizeError };
>> 
>> Yes, it's confusing. I would suggest
>> 
>> if (srcRect.size().isZero())
>>   return { IndexSizeError };
>> 
>> if (srcRect.isEmpty())
>>   return { };
> 
> btw, where in the standard does it say that if both are zero there should be an exception? The patch and the Changelog should be more explicit about this.

Yeah, this is another issue. At first I thought it was related to the "check usability of image", but IndexSizeError is not mentioned there, only InvalidStateError.

IndexSizeError is mentioned in the canvas page [1] only in the following contexts:

* arcTo(), arc(), ellipse()
* addColorStop(), createRadialGradient()
* createImageData(), ImageData(), getImageData()
* convertToBlob()

Testing a version that only checks if srcRect is empty and returns {}, I could not find any consistent regressions and even the number of flakies (Usually 10-15 upstream) is slightly less with the single check.

I'll submit an updated version.

[1] https://html.spec.whatwg.org/multipage/canvas.html

[Lauro Moura]

Created attachment 414649 [details]
Patch without exception

Updated patch removing the exception. Also updated another old createBitmap test that explicitly checked for IndexSizeError.

[Noam Rosenthal]

Comment on attachment 414649 [details]
Patch without exception

Much better. LGTM!

[EWS]

Committed r270126: <https://trac.webkit.org/changeset/270126>

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

[Radar WebKit Bug Importer]

<rdar://problem/71637171>