1614225 - createPattern should throw "InvalidStateError" when image's current request state is "broken"

Status: RESOLVED FIXED

(Core :: Graphics: Canvas2D, defect, P3)

Description

[guest271314]

Attachment: createPatternShouldThrowInvalidStateErrForImageSourceWidthAndHeightZero.html

User Agent: Mozilla/5.0 (X11; Linux i686; rv:74.0) Gecko/20100101 Firefox/74.0

Steps to reproduce:

1. Create <canvas> element
2. Create img element with width and height set to 0
3. Execute 2d context createPattern() with img as parameter

Actual results:

null is returned

Expected results:

A DOM exception should be thrown per HTML Standard

https://html.spec.whatwg.org/multipage/images.html#img-error

Broken
The user agent has obtained all of the image data that it can, but it cannot even decode the image enough to get the image dimensions (e.g. the image is corrupted, or the format is not supported, or no data could be obtained).

See https://github.com/web-platform-tests/wpt/issues/21685

Comment 1

[Andrew Osmond [:aosmond] (he/him)]

From the source this looks more like an issue the DOM for an image rather than canvas. Timothy, can you take a look? Thanks!

Comment 2

[Timothy Nikkel (:tnikkel)]

Firefox doesn't throw and returns a null CanvasPattern.
Chrome throws.
Safari doesn't throw and returns a CanvasPattern.

So before changing this I think we want some clarity on what the expected behaviour of this testcase is.

Comment 3

[Timothy Nikkel (:tnikkel)]

Also, it's not clear that the image should be considered "broken" in the spec sense:

1. it's produced as a result of canvas.toDataURL() on a valid (0 by 0) canvas
2. we decode the correct image dimensions.

Now there is the part of the spec that says "no data could be obtained": there is no data, but a 0x0 image has no data. And that wording is in the "eg" list, so does that mean it's not the actual spec but just comments designed to provide some help in understanding the previous sentence?

Comment 4

[guest271314]

Currently banned indefinitely from contributing to WHATWG which HTML Standard is under the umbrella of.

Take below observations FWIW.

Note, became aware of the behaviour following running local tests pursuant to reading https://github.com/web-platform-tests/wpt/issues/21683.

• data:, objectively has "no data".
  Thus an exception appears to be expected.

• null does not provide any meaningful output that is not unambiguous.
  null is not referenced in the specification relevant to this case, though is in other cases where there is room for ambiguous interpretation re canvas algorithms. Thus Firefox implementation is not in compliance with the specification.

• Chromium throws the exception clearly described in the specification
  Chromium, Chrome behaviour is consistent with, or otherwise in compliance with the specification.

• Safari result (do not currently have access to testing at Safari)
  Is there any technical possibility for image data to be set and retrieved from a 0X0 image (img or canvas or CanvasPattern)?
  Are there any known or conceivable use cases for a 0x0 (width x height) CanvasPattern?

The answers the the questions re Safari behaviour should provide the balance of the applicable and available data to draw at least a basic conclusion as to how to proceed.

Comment 5

[Timothy Nikkel (:tnikkel)]

I don't really care what we decide to do here as long as all the browsers agree on it. The spec can be updated to whatever is agreed upon by all parties.

Comment 6

[guest271314]

Do not care either. All browsers do not agree on many API's. Why is agreement by another party necessary where the specification is already clear at least on this matter?

Comment 7

[Timothy Nikkel (:tnikkel)]

It does not seem clear to me on the definition of broken image as it applies to this testcase for reasons I outlined in comment 3.

Comment 8

[guest271314]

Not sure how can be clearer as to the data URI data:, having no image data?

How would such an image considered to be not "Broken"?

What are the use cases for such an image?

However, as pointed out in WPT issue the image at the repository which led to attempting to re-write the test appeared to be designed to be "Broken" due to

Fatal error reading PNG image file: IDAT: CRC error

which none of the browsers tested at throw (meaning the test as written prints FAIL due to no exception being thrown where that is the expected result of the test) rather the native program Ristretto Image Viewer. The conclusion drawn here is that modern browsers' image decoding source code either ignores that apparently intentional fatal error, or is yet still able to decode the image.

If the language "Broken" is problematic to you re not being a technical term, then yes, change that language to only state 0 width and 0 height or "no data". Or, go further and re-do the entire technical write-up consistent with what you have determined to be an image that has no data.

Comment 10

[Boris Zbarsky [:bzbarsky]]

This is not a duplicate of a test bug. This is about a Gecko problem.

It looks like we used to throw InvalidStateError when the image had NS_EVENT_STATE_BROKEN, but that was removed (to return null instead) in bug 1565991.

Ignoring for the moment the poor test coverage here (of which bug 1016482 is a part), was there a specific reason that the fix was done the way it was, and for only <html:img>, not <svgi:img>? Our NS_EVENT_STATE_BROKEN can be set in multiple different cases, that have different per-spec behavior, as far as I can tell..

Comment 11

[Boris Zbarsky [:bzbarsky]]

The right fix here is probably to expose a getter for the equivalent of https://html.spec.whatwg.org/multipage/images.html#img-req-state on nsImageLoadingContent and use that getter in this code.

Comment 12

[guest271314]

Attachment: firefoxDoesNotThrowForBrokenImageSourcePassedToCanvasPattern.html

Comment 13

[Kris Taeleman (:ktaeleman)]

No specific reason as far as I can remember. I did this change as an onboarding task not knowing a lot about the code/context, so it might have had overreaching effects.

Comment 14

[Boris Zbarsky [:bzbarsky]]

Changing summary to actually follow the spec, because if the image's current request state is not "broken" it can still have width and/or height be 0 and then the spec says to return null, not throw. The throwing happens only in the 'current request is "broken"' case.

Comment 15

[guest271314]

(In reply to Boris Zbarsky [:bzbarsky, bz on IRC] from comment #14)

Changing summary to actually follow the spec, because if the image's current request state is not "broken" it can still have width and/or height be 0 and then the spec says to return null, not throw. The throwing happens only in the 'current request is "broken"' case.

data:, ("no data could be obtained") is also within the scope of "Broken"

e.g. the image is corrupted, or the format is not supported, or no data could be obtained

are each cases for "Broken".

One issue with relying on width and height at Firefox is the each value is set to 24 even where the image source is "corrupted", e.g., the "invalidImage" reference at the attached HTML. naturalWidth and naturalHeight are set to 0.

Comment 16

[Boris Zbarsky [:bzbarsky]]

Attachment: Bug 1614225. Fix validity checks in createPattern to follow the spec.

Comment 17

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/aac4c06cc1ce
Fix validity checks in createPattern to follow the spec. r=jrmuizel

Comment 18

[Web Platform Test Sync Bot (Matrix: #interop:mozilla.org)]

Created web-platform-tests PR https://github.com/web-platform-tests/wpt/pull/21923 for changes under testing/web-platform/tests

Comment 19

[Web Platform Test Sync Bot (Matrix: #interop:mozilla.org)]

Upstream web-platform-tests status checks passed, PR will merge once commit reaches central.

Comment 20

[Daniel Varga [:dvarga]]

https://hg.mozilla.org/mozilla-central/rev/aac4c06cc1ce

Comment 21

[Web Platform Test Sync Bot (Matrix: #interop:mozilla.org)]

Upstream PR merged by moz-wptsync-bot