Closed Bug 1614225 Opened 2 months ago Closed 1 month ago

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

Categories

(Core :: Canvas: 2D, defect, P3)

74 Branch
defect

Tracking

()

RESOLVED FIXED
mozilla75
Tracking Status
firefox-esr68 --- unaffected
firefox73 --- wontfix
firefox74 --- wontfix
firefox75 --- fixed

People

(Reporter: guest271314, Assigned: bzbarsky)

References

(Regression)

Details

(Keywords: regression, testcase)

Attachments

(3 files)

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

Has STR: --- → yes
Component: Untriaged → Canvas: 2D
Keywords: testcase
Product: Firefox → Core

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

Status: UNCONFIRMED → NEW
Ever confirmed: true
Flags: needinfo?(tnikkel)
Priority: -- → P3

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.

Flags: needinfo?(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?

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.

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.

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?

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.

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.

Status: NEW → RESOLVED
Closed: 2 months ago
Resolution: --- → DUPLICATE
Duplicate of bug: 1016482

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..

Status: RESOLVED → REOPENED
Flags: needinfo?(ktaeleman)
Flags: needinfo?(jmuizelaar)
Keywords: regression
Regressed by: 1565991
Resolution: DUPLICATE → ---

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.

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.

Flags: needinfo?(ktaeleman)
Flags: needinfo?(jmuizelaar)

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.

Summary: createPattern should throw "InvalidStateError" when image source width and height are 0 → createPattern should throw "InvalidStateError" when image's current request state is "broken"

(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.

Assignee: nobody → bzbarsky
Pushed by bzbarsky@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/aac4c06cc1ce
Fix validity checks in createPattern to follow the spec. r=jrmuizel
Created web-platform-tests PR https://github.com/web-platform-tests/wpt/pull/21923 for changes under testing/web-platform/tests
Upstream web-platform-tests status checks passed, PR will merge once commit reaches central.
Status: REOPENED → RESOLVED
Closed: 2 months ago1 month ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla75
Upstream PR merged by moz-wptsync-bot
You need to log in before you can comment on or make changes to this bug.