1016482 - 2d.pattern.image.broken test seems to pass intermittently

Status: RESOLVED FIXED

(Core :: Graphics: Canvas2D, defect)

Description

[James Graham [:jgraham]]

To reproduce you may need to check out the web-platform-tests locally, and run the server, as described in the README.md file [1].

When run locally http://web-platform.test:8000/2dcontext/fill-and-stroke-styles/2d.pattern.image.broken.html seems to flip between passing and failing. When run on w3c-test.org this doesn't happen, presumably because of the increased latency. This suggests some kind of race condition in the loading code.

(that also suggests that this might not be a canvas bug. But a good fix might be to make this test always pass, which might be possible without fixing the underlying race since, presumably, the image is always known to be broken when createPattern is called).

[1] https://github.com/w3c/web-platform-tests

Comment 1

[Boris Zbarsky [:bzbarsky]]

At this point, this test is just wrong. Per spec at https://html.spec.whatwg.org/multipage/canvas.html#dom-context-2d-createpattern a unusable image means null is returned, which is what we do.

Comment 2

[guest271314]

This appears to be the portion of the HTML Standard which is being relied on for the expectation for an exception being thrown re "Broken" https://html.spec.whatwg.org/multipage/images.html#img-error (see https://bugzilla.mozilla.org/show_bug.cgi?id=1614225)

However, Firefox throws no such error. null is returned.

    var img = new Image;
    img.src = "data:,";
    img.onerror = e => {
      console.log(e);
      const canvas = document.createElement("canvas");
      const ctx = canvas.getContext("2d");
      try {
        console.dir(ctx.createPattern(img, "repeat"));
      } catch (e) {
        console.error(e);
      }
    }

Comment 3

[guest271314]

The same code at Chromium 81 does throw an exception

DOMException: Failed to execute 'createPattern' on 'CanvasRenderingContext2D': Source image is in the 'broken' state.
    at Image.img.onerror (<anonymous>:8:25)

Comment 4

[guest271314]

The conclusion reached here is that

• the image used in WPT is not "Broken" enough to throw an error at modern browsers, though does log this error (Fatal error reading PNG image file: IDAT: CRC error) at Ristretto Image Viewer at *nix.
• Firefox returns null, then stops.
• Chromium does not return null and throws the exception per "Broken" part of the specification.

meaning the browsers, Firefox and Chromium, respectively, have some parts of implementation correct, yet not all parts, and the implementations are not consistent at all, making testing if not difficult, a trial and error to compose a single test that outputs the expected result for each browser.

If there is any ambiguity in the specification as to which part is controlling, or if the "Broken" part of specification is controlling at all

https://bugzilla.mozilla.org/show_bug.cgi?id=1614225#c7

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 (https://bugzilla.mozilla.org/show_bug.cgi?id=1614225#c3).

perhaps now is a time that can adjust both the specification and the televant test.

Comment 5

[Boris Zbarsky [:bzbarsky]]

Attachment: Bug 1016482. Fix broken createPattern test.

Per spec, createPattern never throws InvalidStateError. It can return null if
the image is not usable (as here) or throw SyntaxError if the second arg is not
a recognized value, but that's it.

Comment 6

[guest271314]

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

At this point, this test is just wrong. Per spec at https://html.spec.whatwg.org/multipage/canvas.html#dom-context-2d-createpattern a unusable image means null is returned, which is what we do.

In fact, following the link from "checking the usability"

To check the usability of the image argument, where image is a CanvasImageSource object, run these steps:

Switch on image:

HTMLOrSVGImageElement
If image's current request's state is broken, then throw an "InvalidStateError" DOMException.

Firefox behaviour is not in conformance with the HTML Standard.

According to the processsing model at step 1. the InvalidStateError exception should be thrown before null is returned at step 2.

Comment 7

[guest271314]

Searched for though did not locate this bug before filing https://bugzilla.mozilla.org/show_bug.cgi?id=1614225 which is probably a duplicate of this bug.

Comment 9

[guest271314]

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

Created attachment 9126974 [details]
Bug 1016482. Fix broken createPattern test.

Per spec, createPattern never throws InvalidStateError. It can return null if
the image is not usable (as here) or throw SyntaxError if the second arg is not
a recognized value, but that's it.

This

_assertSame(ctx.createPattern(img, 'repeat'), null, "ctx.createPattern(img, 'repeat')", "null");

is wrong per the specification section that you are referring to.

If image's current request's state is broken, then throw an "InvalidStateError" DOMException.

Comment 10

[guest271314]

The change at https://phabricator.services.mozilla.com/D63030 for WPT test essentially is hiding the Firefox bug for handling usability of the image at https://html.spec.whatwg.org/multipage/canvas.html#check-the-usability-of-the-image-argument. Not a viable resolution to the bug or the test.

Comment 11

[Boris Zbarsky [:bzbarsky]]

the image used in WPT is not "Broken" enough to throw an error at modern browsers

Ah, indeed. I was specifically talking about the image in question in this test, which does not lead to a "current request's state is broken" state, because the image has a valid header (including size information) and it's just the image data that's not decodable. So it falls into the "image is not fully decodable" case. See definitions of the states at https://html.spec.whatwg.org/multipage/images.html#img-req-state

So for this specific image, my test fix is correct. I will correct the commit message to make this clear.

For the 'data:,' case, I would have expected it to fall into the "broken" case. I'll take a look at that later, but it's not this bug, not this bug's testcase, and not a duplicate of this bug...

Comment 12

[guest271314]

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

the image used in WPT is not "Broken" enough to throw an error at modern browsers

Ah, indeed. I was specifically talking about the image in question in this test, which does not lead to a "current request's state is broken" state, because the image has a valid header (including size information) and it's just the image data that's not decodable. So it falls into the "image is not fully decodable" case. See definitions of the states at https://html.spec.whatwg.org/multipage/images.html#img-req-state

So for this specific image, my test fix is correct. I will correct the commit message to make this clear.

For the 'data:,' case, I would have expected it to fall into the "broken" case. I'll take a look at that later, but it's not this bug, not this bug's testcase, and not a duplicate of this bug...

The WPT test case is based on a "Broken" image being passed to createPattern() (WPT issue https://github.com/web-platform-tests/wpt/issues/21685 and PR https://github.com/web-platform-tests/wpt/pull/21686 based on filed after performing further tests based on the issues raised in https://github.com/web-platform-tests/wpt/issues/21683).

The WPT test change made at https://bugzilla.mozilla.org/show_bug.cgi?id=1016482#c5 is referring to the same test.

However, as pointed out at https://bugzilla.mozilla.org/show_bug.cgi?id=1016482#c6 the change that you made is wrong due to omitting the obvious skipping of Step 1. at https://html.spec.whatwg.org/multipage/canvas.html#dom-context-2d-createpattern, specifically checking image usability, which should result in DOMException being thrown for an image with "no data"; or dimensions cannot be decoded; or, for example, reversing the underlying data of a valid image; all MUST result in InvalidStateError exception being thrown. That is, "data:," is not the only means by which an image can be "Broken" per the specification.

Chromium throws the exception and does not return null which is the correct behaviour per the specification. The current test that you updated conveniently avoids exposing that Firefox does not throw the expected exception at Step 1. Rather the test that you pushed jumps to Step 2. to return null yet that is not expected for the case of a "Broken" image, and ignores checking usability of the image, which effectively hides the Firefox bug.

Closed the related bug as a duplicate with the bare expectation that you would not avoid addressing the obvious bug in Firefox behaviour.

Perhaps the Firefox bug in this case was not immediately obvious to you prior to this post?

Comment 13

[guest271314]

Attachment: firefoxDoesNotThrowForBrokenImageSourcePassedToCanvasPattern.html

Kindly review the relevant disparate specification sections and attached file.

Nightly 75 does not throw exception when Image error event is fired and the same image (with data URI base64 <data> content reversed) is passed to createPattern().

While it is true https://html.spec.whatwg.org/multipage/canvas.html#dom-context-2d-createpattern contains language

2. If result is bad, then return null.

Step 1. is

1. Let usability be the result of checking the usability of image.

which links to https://html.spec.whatwg.org/multipage/canvas.html#check-the-usability-of-the-image-argument

To check the usability of the image argument, where image is a CanvasImageSource object, run these steps:

Switch on image:

HTMLOrSVGImageElement
If image's current request's state is broken, then throw an "InvalidStateError" DOMException.

which also contains language

If image is not fully decodable, then return bad.

If image has an intrinsic width or intrinsic height (or both) equal to zero, then return bad.

In the order of precedence the exception must be thrown, particularly where the Image error event is thrown.

Re "Broken" language specifically in the context of img error (which is the case for the attached file with base64 data set in reverse, meaning there is no conceivable source code at the browser expecting such data to be decoded to a image that is not "Broken") 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).

If the ambiguous language relevant to "bad" "result" is being used as the rationale for not throwing the exception listed prior to that mandate, can you either kindly state that explicitly, or make the appropriate change at the specification to remove one of the two options: DOMException or null, evidently.

Chromium behaviour of throwing DOMException

DOMException: Failed to execute 'createPattern' on 'CanvasRenderingContext2D': Source image is in the 'broken' state.
    at Image.img.onerror

described in the specification cannot be considered to be in non-conformance with the HTML Standard. That leaves only the conclusion that a) Firefox behaviour is non-conformance with the specification; or b) the specification language is ambiguous; capable of more than one interpretation by implementers which in this case does in fact result in very different results for an image that is not usable.

The very different outputs null (Firefox) and DOMException (Chromium) for the same HTML <img> element that is not usable cannot both be in conformace with the specification at the same time.

Comment 14

[guest271314]

Note, from the experimentation that have performed thus far "instead" at https://phabricator.services.mozilla.com/transactions/detail/PHID-XACT-DREV-aesoigtn3rpkp4q/

instead it causes null to be returned.

appears to be a misnomer.

Have not yet been able to create or modify an HTML <img> source that does throw an InvalidStateError under any circumstances at Firefox to meet the requirement of the specification at

HTMLOrSVGImageElement
If image's current request's state is broken, then throw an "InvalidStateError" DOMException.

If you are able to create or modify an image to the degree that Firefox does throw the exception can you share the code which does so here?

Comment 15

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/919c15bc2284
Fix broken createPattern test. r=jgraham

Comment 16

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

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

Comment 17

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

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

Comment 18

[Boris Zbarsky [:bzbarsky]]

The WPT test case is based on a "Broken" image

Broken in the sense that it's not "fully decodable", but it does have a size.

or dimensions cannot be decoded

Exactly. The "broken" image in the test has dimensions that can be decoded.

That is, "data:," is not the only means by which an image can be "Broken" per the specification.

Sure, it's not the only way to get into the "current request's state is broken" scenario. But the point is, the image in this test is NOT in that scenario.

We should absolutely add a test that does exercise the "current request's state is broken" situation, but this test is not it.

Perhaps the Firefox bug in this case was not immediately obvious to you prior to this post?

I don't know what you mean by "this post", but the Firefox bug was obvious after your comment 2, and will be addressed in bug 1614225.

The point of this bug is to fix a bad test. This test does not pass in any browsers, and its test assertions do not match the spec. The test fix makes the test assertions match the spec. Incidentally that also makes it pass in browsers, which is good. Again, we need better test coverage here for the "current request's state is broken" case in https://html.spec.whatwg.org/multipage/canvas.html#check-the-usability-of-the-image-argument and we will add those in bug 1614225.

Now could you please kindly stop pasting all the contents of bug 1614225 in this bug? They don't have much to do with this bug...

Comment 19

[Boris Zbarsky [:bzbarsky]]

I'll take a look at that later, but it's not this bug, not this bug's testcase, and not a duplicate of this bug...

And to be clear, I reopened bug 1614225 to track that.

Comment 20

[Cristina Coroiu [:ccoroiu]]

https://hg.mozilla.org/mozilla-central/rev/919c15bc2284

Comment 21

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

Upstream PR merged by moz-wptsync-bot