Closed Bug 1011574 Opened 7 years ago Closed 7 years ago

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


(Core :: Canvas: 2D, defect)

Not set





(Reporter: bzbarsky, Assigned: wlitwinczyk)



(1 file, 2 obsolete files)

Right now we throw SyntaxError, afaict.  Per 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):

I also did a try build to see if anything non-obvious broke:

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]

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:

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]

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]

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

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

and the current canvas spec does agree that this should be an InvalidStateError
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
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla32
You need to log in before you can comment on or make changes to this bug.