Closed Bug 1011574 Opened 10 years ago Closed 10 years ago

Throw the right exception when putImageData is passed an ImageData with a neutered buffer

Categories

(Core :: Graphics: Canvas2D, defect)

x86
macOS
defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla32

People

(Reporter: bzbarsky, Assigned: wlitwinczyk)

Details

Attachments

(1 file, 2 obsolete files)

Right now we throw SyntaxError, afaict.  Per https://www.w3.org/Bugs/Public/show_bug.cgi?id=25672 this should throw InvalidStateError.
Walter, this is in CanvasRenderingContext2D::PutImageData_explicit method, but let's also see if some of the callers are explicitly expecting the "syntax" error...
Assignee: nobody → wlitwinczyk
Attached patch canvas_2d_patch (obsolete) — Splinter Review
I believe this is all that needed to be changed. I did a grep and a dxr search and it appears that nothing calls these putimage functions (that I can tell): http://dxr.mozilla.org/mozilla-central/search?q=PutImageData&case=true

I also did a try build to see if anything non-obvious broke: https://tbpl.mozilla.org/?tree=Try&rev=bc761e7ee351

I don't think the two orange tests are caused by this (but I may be wrong).
Attachment #8433458 - Flags: review?(milan)
Comment on attachment 8433458 [details] [diff] [review]
canvas_2d_patch

Per spec it actually looks to me like w==0/h==0 should be a no-op instead of throwing.  At least at first glance.
I think it's still an error, although the wording is a little tough:

http://www.whatwg.org/specs/web-apps/current-work/multipage/the-canvas-element.html#dom-imagedata-width

says that "At least one pixel's worth of image data must be returned." 

Combined with: "[the] width attribute is set to the number of pixels per row in the image data, their height attribute is set to the number of rows in the image data".

So I think it's still an error because PutImageData() shouldn't receive an ImageData object with w==0 or h==0 because then it would have less than 1 pixel of data.
That's for getImageData.  But I guess createImageData also doesn't allow 0x0 size.  So it doesn't matter too much what the w==0/h==0 case does, since that code is never hit anyway.
Comment on attachment 8433458 [details] [diff] [review]
canvas_2d_patch

Review of attachment 8433458 [details] [diff] [review]:
-----------------------------------------------------------------

I like it!  Let's get one of the graphics peers to give it the official review.
Attachment #8433458 - Flags: review?(milan)
Attachment #8433458 - Flags: review?(bjacob)
Attachment #8433458 - Flags: feedback+
Comment on attachment 8433458 [details] [diff] [review]
canvas_2d_patch

Review of attachment 8433458 [details] [diff] [review]:
-----------------------------------------------------------------

Ah, I see, neutering an ArrayBuffer does set its length to 0 here:

http://dxr.mozilla.org/mozilla-central/source/js/src/vm/ArrayBufferObject.cpp#349

and the current canvas spec does agree that this should be an InvalidStateError

http://www.whatwg.org/specs/web-apps/current-work/multipage/the-canvas-element.html#dom-context-2d-putimagedata
Attachment #8433458 - Flags: review?(bjacob) → review+
Attached patch canvas_2d_syn_error_patch (obsolete) — Splinter Review
Only updated patch information. Was missing some author information.
Attachment #8433458 - Attachment is obsolete: true
Attachment #8435120 - Flags: review+
Attachment #8435120 - Flags: feedback+
Now it should be correct...
Attachment #8435120 - Attachment is obsolete: true
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/ae35b0953851
Status: NEW → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla32
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: