Last Comment Bug 392751 - getImageData is supposed to allow out of bounds rects
: getImageData is supposed to allow out of bounds rects
Status: RESOLVED FIXED
: dev-doc-complete
Product: Core
Classification: Components
Component: Canvas: 2D (show other bugs)
: Trunk
: All All
: P4 normal (vote)
: mozilla5
Assigned To: :Ms2ger (⌚ UTC+1/+2)
:
: Milan Sreckovic [:milan]
Mentors:
Depends on: 629894
Blocks: 622842
  Show dependency treegraph
 
Reported: 2007-08-18 19:33 PDT by pete
Modified: 2011-05-03 12:50 PDT (History)
9 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
getImageData (Out of bounds) (2.14 KB, patch)
2009-05-29 21:37 PDT, Saint Wesonga
no flags Details | Diff | Splinter Review
Testcase (2.21 KB, text/html)
2009-05-29 21:45 PDT, Saint Wesonga
no flags Details
getImageData (Out of bounds) (2.13 KB, patch)
2009-06-03 18:20 PDT, Saint Wesonga
no flags Details | Diff | Splinter Review
Integer over/underflow testcase (4.42 KB, text/html)
2009-06-24 22:06 PDT, Saint Wesonga
no flags Details
Patch allowing out of bounds rects (5.92 KB, patch)
2009-06-24 22:14 PDT, Saint Wesonga
no flags Details | Diff | Splinter Review
Patch allowing out of bounds rects (6.31 KB, patch)
2009-06-25 06:02 PDT, Saint Wesonga
no flags Details | Diff | Splinter Review
Patch v2 (8.29 KB, patch)
2011-02-07 10:32 PST, :Ms2ger (⌚ UTC+1/+2)
bzbarsky: review+
Details | Diff | Splinter Review
Patch for checkin (8.36 KB, patch)
2011-04-07 11:15 PDT, :Ms2ger (⌚ UTC+1/+2)
no flags Details | Diff | Splinter Review

Description pete 2007-08-18 19:33:39 PDT
User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 5.0; en-US; rv:1.9a7pre) Gecko/2007072104 Minefield/3.0a7pre
Build Identifier: 

http://www.whatwg.org/specs/web-apps/current-work/multipage/section-the-canvas.html#getimagedata

3.14.11.1.10. Pixel manipulation

"Pixels outside the canvas must be returned as transparent black"

I'm guessing that putImageData should do something similar but the spec is silent.

Reproducible: Always
Comment 1 Joe Drew (not getting mail) 2008-03-07 08:53:39 PST
This is definitely what the standard says, and we're definitely not returning that in that case (at least from reading the code), but I don't know how big an issue this is.
Comment 2 Saint Wesonga 2009-05-29 21:37:44 PDT
Created attachment 380572 [details] [diff] [review]
getImageData (Out of bounds)

This patch changes the returned error when width=0 or height=0 from SYNTAX_ERR to INDEX_SIZE_ERR (as per spec) and allows for negative dimensions and out of bound rects
Comment 3 Saint Wesonga 2009-05-29 21:45:08 PDT
Created attachment 380573 [details]
Testcase

No exceptions should be thrown (current builds throw SYNTAX_ERR) and the testcase should only display success messages (Note: this testcase is a minor modification of the test_2d.imageData.get.source.outside.html test allowing it to run without the mochitest framework)
Comment 4 Saint Wesonga 2009-05-29 22:44:18 PDT
Comment on attachment 380572 [details] [diff] [review]
getImageData (Out of bounds)

The patch causes the rendering to pass the test at http://philip.html5.org/tests/canvas/suite/tests/2d.imageData.get.source.outside.html but fail the one at http://philip.html5.org/tests/canvas/suite/tests/2d.imageData.get.source.negative.html.

My initial interpretation of the spec requirements was that the 4 corners determined by the four points (sx, sy), (sx+sw, sy), (sx+sw, sy+sh), (sx, sy+sh) form a rectangle for which pixels should be returned from the top left to the bottom right - since the data is being returned in a CanvasPixelArray object (which the spec specifically requires to order the data in left-to-right, top-to-bottom format).

However, the test at http://philip.html5.org/tests/canvas/suite/tests/2d.imageData.get.source.negative.html seems to have a different interpretation, instead expecting the bottom right pixel to appear first in the array (where bottom right in that specific test would be the point 85,25). Did I miss something very obvious about all this in the spec?

I had filed this issue at https://bugzilla.mozilla.org/show_bug.cgi?id=494744, and that bug also pointed out a simple error in the test (should be imgdata1.data.length, not imgdata1.length)
Comment 5 Saint Wesonga 2009-06-03 18:20:22 PDT
Created attachment 381445 [details] [diff] [review]
getImageData (Out of bounds)

I confirmed with Philip Taylor that the above test that fails is actually incorrect, so I'm resubmitting the patch
Comment 6 pete 2009-06-11 15:54:12 PDT
Some things that stand out to me (I haven't ran your code, these are just red flags that are jarring to me):

What if w or h are INT_MIN in the negative dimensions fixups?

What happens if x or y underflows in the negative dimension fixups?

What if x+w or y+h overflows in the fill temp surface calculations?

What if x or y are INT_MIN in the -(int)x, -(int)y calculations?


I don't even know why the spec allows negative sizes anyway. Zero sized lengths make sense to be but are disallowed. Negative lengths make no sense to me but are allowed. Go figure.

I'm hoping there is some way to change the standard to disallow negative sizes (and allow 0 sizes).
Comment 7 Saint Wesonga 2009-06-24 22:06:08 PDT
Created attachment 385049 [details]
Integer over/underflow testcase

Attaching test to verify that overflow/underflow don't go undetected
Comment 8 Saint Wesonga 2009-06-24 22:14:21 PDT
Created attachment 385050 [details] [diff] [review]
Patch allowing out of bounds rects

I'm not sure whether allowing -ve dimensions does any harm, but then again, this may not be the right avenue for that discussion.
Most of the additional code in this patch will simply be skipped in the majority of cases (where +ve dimensions are supplied)
Comment 9 David Baron :dbaron: ⌚️UTC-10 (vacation, returning December 19) 2009-06-24 23:47:40 PDT
You should request review from vlad rather than me.  (But I can say right now that you shouldn't have tabs in the code; you need to indent using spaces.)
Comment 10 Saint Wesonga 2009-06-25 06:02:41 PDT
Created attachment 385090 [details] [diff] [review]
Patch allowing out of bounds rects

Removed tabs from previous patch
Comment 11 :Ms2ger (⌚ UTC+1/+2) 2011-02-07 10:32:16 PST
Created attachment 510314 [details] [diff] [review]
Patch v2

This can do a lot less work. I hope you don't mind me taking over.
Comment 12 Boris Zbarsky [:bz] (still a bit busy) 2011-04-05 19:27:26 PDT
Comment on attachment 510314 [details] [diff] [review]
Patch v2

> +    if (!srcRect.Contains(destRect)) {

Add a comment that this is trying to optimize out the Rectangle() call in the common case, ok?

r=me
Comment 13 :Ms2ger (⌚ UTC+1/+2) 2011-04-07 11:15:49 PDT
Created attachment 524432 [details] [diff] [review]
Patch for checkin
Comment 14 :Ehsan Akhgari 2011-04-07 21:54:54 PDT
This patch does not apply cleanly any more.

Note You need to log in before you can comment on or make changes to this bug.