Bug 132407 – Calling createPattern with a broken image must throw an invalidstate error

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

[Rik Cabanier]

See: 
 http://www.whatwg.org/specs/web-apps/current-work/multipage/the-canvas-element.html#dom-context-2d-createpattern
and
  http://www.w3.org/html/wg/drafts/2dcontext/html5_canvas_CR/#dom-context-2d-createpattern

Chrome and Firefox also have this behavior.

[Rik Cabanier]

Created attachment 230534 [details]
Patch

[Build Bot]

Comment on attachment 230534 [details]
Patch

Attachment 230534 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.appspot.com/results/6656302818263040

New failing tests:
canvas/philip/tests/2d.pattern.image.incomplete.empty.html
fast/canvas/canvas-empty-image-pattern.html
fast/dom/gc-9.html

[Build Bot]

Created attachment 230554 [details]
Archive of layout-test-results from webkit-ews-13 for mac-mountainlion-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: webkit-ews-13  Port: mac-mountainlion-wk2  Platform: Mac OS X 10.8.5

[Rik Cabanier]

Created attachment 230557 [details]
Patch

[Build Bot]

Comment on attachment 230557 [details]
Patch

Attachment 230557 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.appspot.com/results/4690787807789056

New failing tests:
fast/dom/gc-9.html

[Build Bot]

Created attachment 230559 [details]
Archive of layout-test-results from webkit-ews-10 for mac-mountainlion-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: webkit-ews-10  Port: mac-mountainlion-wk2  Platform: Mac OS X 10.8.5

[Build Bot]

Comment on attachment 230557 [details]
Patch

Attachment 230557 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.appspot.com/results/5324877452017664

New failing tests:
fast/dom/gc-9.html

[Build Bot]

Created attachment 230562 [details]
Archive of layout-test-results from webkit-ews-09 for mac-mountainlion-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: webkit-ews-09  Port: mac-mountainlion-wk2  Platform: Mac OS X 10.8.5

[Rik Cabanier]

Created attachment 230564 [details]
Patch

[Andreas Kling]

Comment on attachment 230564 [details]
Patch

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

> Source/WebCore/ChangeLog:13
> +        * canvas/philip/tests/2d.pattern.image.broken.html:
> +        * canvas/philip/tests/2d.pattern.image.incomplete.empty.html:

This is an imported test suite; do your changes match upstream?

[Rik Cabanier]

(In reply to comment #10)
> (From update of attachment 230564 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=230564&action=review
> 
> > Source/WebCore/ChangeLog:13
> > +        * canvas/philip/tests/2d.pattern.image.broken.html:
> > +        * canvas/philip/tests/2d.pattern.image.incomplete.empty.html:
> 
> This is an imported test suite; do your changes match upstream?

yes,this test suite is now maintained by the w3c. I updated this test yesterday but it needs to be regenerated.
see http://www.w3c-test.org/2dcontext/fill-and-stroke-styles/2d.pattern.image.broken.html 

I'm looking over the cases where WK is not spec compliant

[Dirk Schulze]

Comment on attachment 230564 [details]
Patch

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

> LayoutTests/fast/canvas/canvas-empty-image-pattern.html:2
> +<title>Canvas test: filling a pattern with an empty image should throw an exception.</title>

This is a crash test, why do you change it?

> LayoutTests/fast/dom/gc-9-expected.txt:-15
> -PASS: document.getElementsByTagName('canvas')[0].getContext('2d').createPattern(new Image(), 'no-repeat').myCustomProperty should be undefined and is.

Why do the pass messages disappear?

> LayoutTests/fast/dom/gc-9.html:-124
> -    [ "document.getElementsByTagName('canvas')[0].getContext('2d').createPattern(new Image(), 'no-repeat')" ], // CanvasPattern

Why did you remove this test?

[Rik Cabanier]

Comment on attachment 230564 [details]
Patch

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

>> LayoutTests/fast/canvas/canvas-empty-image-pattern.html:2
>> +<title>Canvas test: filling a pattern with an empty image should throw an exception.</title>
> 
> This is a crash test, why do you change it?

Not sure if I follow. The test will now throw an exception. Should I remove it completely?

>> LayoutTests/fast/dom/gc-9-expected.txt:-15
>> -PASS: document.getElementsByTagName('canvas')[0].getContext('2d').createPattern(new Image(), 'no-repeat').myCustomProperty should be undefined and is.
> 
> Why do the pass messages disappear?

because I removed the test that creates an empty image.

>> LayoutTests/fast/dom/gc-9.html:-124
>> -    [ "document.getElementsByTagName('canvas')[0].getContext('2d').createPattern(new Image(), 'no-repeat')" ], // CanvasPattern
> 
> Why did you remove this test?

createPattern(new Image()) will throw an exception as per the spec so this test no longer works.

[Dirk Schulze]

Comment on attachment 230564 [details]
Patch

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

recover canvas-empty-image-pattern.html and I am r+ the patch.

> LayoutTests/canvas/philip/tests/2d.pattern.image.incomplete.empty.html:23
> +var _thrown = undefined;
> +try{
> +  ctx.createPattern(img, 'repeat');
> +}catch(e) {
> +  _thrown = e;
> +}
> +_assert(_thrown && _thrown.name == "InvalidStateError" && _thrown.code == DOMException.INVALID_STATE_ERR, "should throw InvalidStateError");

You assured before that these changes were made in the official test suite as well, right? Then I am fine with the changes.

>>> LayoutTests/fast/canvas/canvas-empty-image-pattern.html:2
>>> +<title>Canvas test: filling a pattern with an empty image should throw an exception.</title>
>> 
>> This is a crash test, why do you change it?
> 
> Not sure if I follow. The test will now throw an exception. Should I remove it completely?

Not, just leave the comment unchanged.

> LayoutTests/fast/canvas/canvas-empty-image-pattern.html:4
> +<script src="../../resources/js-test-pre.js"></script>

don't include more things. crash tests are done to demonstrate that we do not fail again under the same circumstances.

> LayoutTests/fast/canvas/canvas-empty-image-pattern.html:16
> +var _thrown = undefined;
> +try {
> +  canvas.fillStyle = canvas.createPattern(new Image, "repeat")
> +} catch(e) {
> +  _thrown = e;
> +}
> +shouldBe('_thrown && _thrown.name == "InvalidStateError" && _thrown.code == DOMException.INVALID_STATE_ERR', 'true');

Just let it throw and update the expected file.

> LayoutTests/fast/canvas/canvas-empty-image-pattern.html:19
> +<script src="../../resources/js-test-post.js"></script>

Ditto.

>>> LayoutTests/fast/dom/gc-9.html:-124
>>> -    [ "document.getElementsByTagName('canvas')[0].getContext('2d').createPattern(new Image(), 'no-repeat')" ], // CanvasPattern
>> 
>> Why did you remove this test?
> 
> createPattern(new Image()) will throw an exception as per the spec so this test no longer works.

Ok.

[Rik Cabanier]

Created attachment 230949 [details]
Patch

[Darin Adler]

Comment on attachment 230949 [details]
Patch

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

> Source/WebCore/html/canvas/CanvasRenderingContext2D.cpp:1756
> +    if (!cachedImage || cachedImage->status() == CachedResource::LoadError) {

Does the test case cover the "!cachedImage" case or only the LoadError case?

[Rik Cabanier]

Comment on attachment 230949 [details]
Patch

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

>> Source/WebCore/html/canvas/CanvasRenderingContext2D.cpp:1756
>> +    if (!cachedImage || cachedImage->status() == CachedResource::LoadError) {
> 
> Does the test case cover the "!cachedImage" case or only the LoadError case?

Just the LoadError.
When is there no cachedImage?

[WebKit Commit Bot]

Comment on attachment 230949 [details]
Patch

Clearing flags on attachment: 230949

Committed r168400: <http://trac.webkit.org/changeset/168400>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.