1217031 - createPattern throws NS_ERROR_NOT_AVAILABLE

Status: RESOLVED FIXED

(Core :: Graphics: Canvas2D, defect)

Description

[tenner.joshua]

Attachment: Example Of Code run

User Agent: Mozilla/5.0 (Windows NT 10.0; WOW64) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/45.0.2454.101 Safari/537.36

Steps to reproduce:

Use createPattern on an image with a dataurl in the same context.

var img = new Image();
img.src = 'data:image/gif;base64,...';
ctx.createPattern(img, 'no-repeat);


Visit: http://codepen.io/jtenner/pen/PPELad?editors=001

This is a codepen that creates a pattern from a dataurl (an asterisk) and fills a rect on the screen with the pattern.

Normally I would wait for the image to load, but chrome, IE, and opera don't seem to have a problem with this.

If the image is cached this problem doesn't occur.  Repeated refreshes will show the asterisk but a hard refresh (ctrl-f5) reproduces the issue.

I happen to be maintaining a JavaScript library so I was just curious if this is normal proposed behavior for the browser?


Actual results:

Firefox throws: "NS_ERROR_NOT_AVAILABLE:"


Expected results:

It should create a fill pattern, I think.

Comment 1

[Loic]

Attachment: test.html

Comment 2

[Kelsey Gilbert [:jgilbert]]

Sounds like a spec question.

Comment 3

[Jeff Muizelaar [:jrmuizel]]

It sounds like this behaviour is according to spec. See https://bugzilla.mozilla.org/show_bug.cgi?id=1164458#c6

Comment 5

[Boris Zbarsky [:bzbarsky]]

Indeed.  createPattern does exactly the same "Check the usability of the image argument" that drawImage does and the rest of the spec behavior is described in bug 1164458 comment 9.

That said, on "bad" return it should return null, not throw.  So if we're throwing, that's clearly buggy per spec.

Comment 6

[Boris Zbarsky [:bzbarsky]]

Of course the IDL for createPattern says it never returns null, so speccing it to return null is quite odd.  ;)

Comment 7

[Boris Zbarsky [:bzbarsky]]

https://github.com/whatwg/html/issues/655 filed on the spec self-inconsistency wrt null.

Comment 8

[Anne (:annevk)]

Seems like the resolution here is that we'll change the specification to make that nullable. (ETA for that change is unknown, the more experienced editors try to leave "good first bug" issues alone.)

Comment 9

[tenner.joshua]

I just wanted to be clear:

(In reply to Boris Zbarsky [:bz] from comment #6)
> Of course the IDL for createPattern says it never returns null, so speccing
> it to return null is quite odd.  ;)

This is exactly the problem :).

Also, I didn't mean to file a duplicate issue. I searched for createPattern and fillStyle to check and make sure I wasn't wasting people's time.  Looks like this is being taken care of.

Thanks everyone!

Comment 10

[Michael Kohler [:mkohler]]

Spec has been changed to allow null return values: https://github.com/whatwg/html/commit/392c10a84c9fffc20d05157343d6f92fa55dc71d

Do I see this correctly that the thing to do here apart from changing the IDL (http://mxr.mozilla.org/mozilla-central/source/dom/webidl/CanvasRenderingContext2D.webidl#72) is to remove the following line? http://mxr.mozilla.org/mozilla-central/source/dom/canvas/CanvasRenderingContext2D.cpp#2124

Comment 11

[Michael Kohler [:mkohler]]

Attachment: canvas_createPattern.patch

This also includes two removals of trailing whitespaces, I can take them out if you want me too.

Comment 12

[Michael Kohler [:mkohler]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=41bf83526fbc

Comment 13

[Ryan VanderMeulen (PTO, back July 27)]

Please attach a patch with proper commit information before requesting checkin.
https://developer.mozilla.org/en-US/docs/Mercurial/Using_Mercurial#How_can_I_generate_a_patch_for_somebody_else_to_check-in_for_me.3F

Comment 14

[Michael Kohler [:mkohler]]

Attachment: bug_1217031.patch_commitmessage

Comment 15

[Ryan VanderMeulen (PTO, back July 27)]

For future reference, your commit messages should be explaining what the patch is actually doing, not just restating the problem they're solving.

Comment 16

[Ryan VanderMeulen (PTO, back July 27)]

Also, webidl changes need review from a DOM peer.

Comment 17

[Michael Kohler [:mkohler]]

Attachment: bug_1217031_goodcommitmessage.patch

Comment 18

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8744734 [details] [diff] [review]
bug_1217031_goodcommitmessage.patch

r=me if security check failure is the only way res.GetSourceSurface() can return null.  If there are other reasons (e.g. OOM), we should probably pass an Errorresult to either SurfaceFromElement or GetSourceSurface to throw an exception on it as needed.

Comment 19

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/d6cca05b4e9f

Comment 20

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

Seems like this needs a test

Comment 21

[Carsten Book [:Tomcat]]

seems this caused https://treeherder.mozilla.org/logviewer.html#?job_id=27100597&repo=mozilla-inbound and so had to backed out

Comment 22

[Pulsebot]

Backout:
https://hg.mozilla.org/integration/mozilla-inbound/rev/9e9f61f2b980

Comment 23

[Boris Zbarsky [:bzbarsky]]

Looks like the test expectations needs to be updated; we're now passing the test when we used to fail it.

Note that this means we don't actually need to add another test.  ;)

Comment 24

[Michael Kohler [:mkohler]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=401f40e53618

Comment 25

[Michael Kohler [:mkohler]]

Attachment: 0001-Bug-1217031-createPattern-should-not-throw-NS_ERROR_.patch

Comment 26

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8748292 [details] [diff] [review]
0001-Bug-1217031-createPattern-should-not-throw-NS_ERROR_.patch

r=me

Comment 27

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/4c899c66ce31

Comment 28

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/4c899c66ce31