629875 - Handle negative arguments to drawImage

Status: RESOLVED DUPLICATE of bug 655328

(Core :: Graphics: Canvas2D, defect)

Description

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

http://www.w3c-test.org/html/tests/submission/PhilipTaylor/canvas/2d.drawImage.negativedest.html
http://www.w3c-test.org/html/tests/submission/PhilipTaylor/canvas/2d.drawImage.negativedir.html
http://www.w3c-test.org/html/tests/submission/PhilipTaylor/canvas/2d.drawImage.negativesource.html

Comment 2

[Yury Delendik (:yury)]

Attachment: Fix to pass 2d.drawImage.negativeXXX w3c tests (plus test_canvas fixes)

Fixes DrawImage arugments checks. Also, changes TODO's in test_canvas.html to regular checks, adds 2d.drawImage.negativedir

Comment 3

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

Comment on attachment 513824 [details] [diff] [review]
Fix to pass 2d.drawImage.negativeXXX w3c tests (plus test_canvas fixes)

>+    // Making sw, sh, dw and dh positive for 2d.drawImage.negativeXXX

How about

    // Handle negative sw, sh, dw and dh by flipping the rectangle over in the
    // relevant direction.

This patch fails 2d.drawImage.wrongtype, because that has a bogus assertion.
I filed <http://www.w3.org/Bugs/Public/show_bug.cgi?id=12142>. Please fix the
test in our tree.

Also, our version of 2d.voidreturn is out of date. Please replace

ok(ctx.drawImage(canvas, 0, 0, 0, 0, 0, 0, 0, 0) === undefined, "ctx.drawImage(canvas, 0, 0, 0, 0, 0, 0, 0, 0) === undefined");

with

ok(ctx.drawImage(canvas, 0, 0, 1, 1, 0, 0, 0, 0) === undefined, "ctx.drawImage(canvas, 0, 0, 1, 1, 0, 0, 0, 0) === undefined");

Beside that, looks good to me.

Comment 4

[Yury Delendik (:yury)]

Attachment: Fix to pass 2d.drawImage.negativeXXX w3c tests (plus test_canvas fixes) v2

Addressed the "Handle negative sw, sh, dw and dh..."-comment and 2d.voidreturn items.

Ms2ger, I was not able to replicate the failure of the 2d.drawImage.wrongtype test. Fixing the test_canvas.html to the following causes the failure on my configuration.

var _thrown = undefined; try {
  ctx.drawImage(undefined, 0, 0);
} catch (e) { _thrown = e }; ok(_thrown && _thrown.code == DOMException.TYPE_MISMATCH_ERR, "should throw TYPE_MISMATCH_ERR");

That item was not addressed by the new patch.

Comment 5

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

Er, of course that wouldn't be your fault. Not sure where my head was.

Comment 6

[Jeff Muizelaar [:jrmuizel]]

What's the status of other browsers on these tests?

Comment 7

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

IE9 and Chrome pass.

Comment 8

[Jeff Muizelaar [:jrmuizel]]

Comment on attachment 513887 [details] [diff] [review]
Fix to pass 2d.drawImage.negativeXXX w3c tests (plus test_canvas fixes) v2

Thanks for looking into all of these cases and sorry for not getting to this review sooner.


> 
>+    if (sw == 0.0 || sh == 0.0) {
>+        // zero-sized source -- failure !?
>+        rv = NS_ERROR_DOM_INDEX_SIZE_ERR;
>+        goto FINISH;
>+    }

Does this change the behaviour for sw/sh == 0?
If so do we have tests for the new behaviour?

>     if (dw == 0.0 || dh == 0.0) {
>         rv = NS_OK;
>         // not really failure, but nothing to do --
>         // and noone likes a divide-by-zero
>         goto FINISH;
>     }
> 
>-    if (!FloatValidate(sx, sy, sw, sh) || !FloatValidate(dx, dy, dw, dh)) {
>+    // The following check might do the validation of the float arguments:
>+    //   (!FloatValidate(sx, sy, sw, sh) || !FloatValidate(dx, dy, dw, dh))
>+    // but we would also need to validate some sums for overflow (e.g. sx + sw).
>+    if (!FloatValidate(sx + sw, sy + sh, dx + dw, dy + dh)) {
>         rv = NS_ERROR_DOM_SYNTAX_ERR;
>         goto FINISH;
>     }
> 
>-    // check args
>-    if (sx < 0.0 || sy < 0.0 ||
>-        sw < 0.0 || sw > (double) imgSize.width ||
>-        sh < 0.0 || sh > (double) imgSize.height ||
>-        dw < 0.0 || dh < 0.0)
>-    {
>-        // XXX ERRMSG we need to report an error to developers here! (bug 329026)
>+    // Handle negative sw, sh, dw and dh by flipping the rectangle over in the
>+    // relevant direction.
>+    if (sw < 0.0) {
>+      sx += sw;
>+      sw = -sw;
>+    }
>+    if (sh < 0.0) {
>+      sy += sh;
>+      sh = -sh;
>+    }
>+    if (dw < 0.0) {
>+      dx += dw;
>+      dw = -dw;
>+    }
>+    if (dh < 0.0) {
>+      dy += dh;
>+      dh = -dh;
>+    }
>+
>+    // Checking source image boundaries.
>+    if (sx < 0 || sx + sw > (double) imgSize.width || 
>+        sy < 0 || sy + sh > (double) imgSize.height) {
>         rv = NS_ERROR_DOM_INDEX_SIZE_ERR;
>         goto FINISH;
>     }

This also looks like a behaviour change too?

> 
>+<!-- [[[ test_2d.drawImage.negativedir.html ]]] -->
>+
>+<p>Canvas test: 2d.drawImage.negativedir</p>
>+<canvas id="c117a" width="100" height="50"><p class="fallback">FAIL (fallback content)</p></canvas>
>+<script>


It looks like a bunch of different test changes are happening here. It might be easier to review if they were split into separate patches. Can you give a summary of the end result?

Comment 9

[Yury Delendik (:yury)]

> > 
> >+    if (sw == 0.0 || sh == 0.0) {
> >+        // zero-sized source -- failure !?
> >+        rv = NS_ERROR_DOM_INDEX_SIZE_ERR;
> >+        goto FINISH;
> >+    }
> 
> Does this change the behaviour for sw/sh == 0?
> If so do we have tests for the new behaviour?

This was a TODO in the test_canvas.html the test_2d_drawImage_outsidesource function and this particular function has modification that were made in scope of this bug. The modification was made to match the exiting w3c test files. See details below. It looks like this particular patch belong to the bug 629876 (I found out that later).

> 
> This also looks like a behaviour change too?
> 

As I understand content/canvas/test/test_canvas.html is combination of the w3c tests. Some of the checks marked as TODOs, some of them are outdated. I just changed the tests to match the w3c test referred in the bug description, also I reviewed all .drawImage() check/tests within test_canvas.html that a marked as TODO and converted those that are applicable to this bug into the normal checks.


> > 
> >+<!-- [[[ test_2d.drawImage.negativedir.html ]]] -->
> >+
> >+<p>Canvas test: 2d.drawImage.negativedir</p>
> >+<canvas id="c117a" width="100" height="50"><p class="fallback">FAIL (fallback content)</p></canvas>
> >+<script>
> 

This file/tested was not included in the test_canvas.html, but present in W3C tests and referred by this bug.

> 
> It looks like a bunch of different test changes are happening here. It might be
> easier to review if they were split into separate patches. Can you give a
> summary of the end result?

All test_canvas.html's TODOs, that were converted to normal checks, are passed with this patch. Shall I split the existing patch into two patches: for code and for tests?

Comment 10

[Jeff Muizelaar [:jrmuizel]]

No just splitting the tests changes out doesn't help. Splitting out the things like the behaviour change for bug 629876 would though. i.e. Ideally each behaviour change would be a separate patch. However, I'll try to take a look at your patch again tomorrow.

Comment 11

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

Attachment: Patch for checkin

Comment 12

[Dão Gottwald [:dao]]

http://hg.mozilla.org/projects/cedar/rev/4ede7b9b55bc

Comment 13

[(no longer active)]

http://hg.mozilla.org/mozilla-central/rev/4ede7b9b55bc

Comment 15

[Eric Shepherd [:sheppy]]

Note added to:

https://developer.mozilla.org/en/DOM/CanvasRenderingContext2D#Notes

Comment 16

[Jeff Muizelaar [:jrmuizel]]

It looks like we're going to have to back this out well we resolve some semantic issues.

Comment 17

[Jeff Muizelaar [:jrmuizel]]

Attachment: Backout while semantic issues are being resolved

Comment 18

[Daniel Veditz [:dveditz]]

Comment on attachment 534076 [details] [diff] [review]
Backout while semantic issues are being resolved

Approved for the mozilla-beta and mozilla-aurora repositories, a=dveditz for release-drivers

Comment 19

[Asa Dotzler [:asa]]

I don't see this yet in mozilla-beta and time is running out for Firefox 5. Please land this ASAP. Thanks.

Comment 20

[(no longer active)]

Comment on attachment 534076 [details] [diff] [review]
Backout while semantic issues are being resolved

This patch does not apply cleanly to aurora, but it does apply cleanly on beta.

Comment 21

[Jeff Muizelaar [:jrmuizel]]

This has been run on try and should be checked in with the following message:

Bug 629875. Backout 4ede7b9b55bc until semantics issues are resolved. f=bzbarsky, a=dveditz

We haven't decided what the behaviour for source rects larger than
the source image should be, and 4ede7b9b55bc breaks content like
20thingsilearned.com.

Comment 22

[Jeff Muizelaar [:jrmuizel]]

http://hg.mozilla.org/releases/mozilla-aurora/rev/72b45a62587a
http://hg.mozilla.org/releases/mozilla-beta/rev/5ae373ff7a19

Comment 23

[j.j.]

(In reply to comment #15)
> https://developer.mozilla.org/en/DOM/CanvasRenderingContext2D#Notes

Canceling needed due to backout

Comment 24

[Joe Drew (not getting mail)]

I pushed this reversion and bug 669366 to aurora:

http://hg.mozilla.org/releases/mozilla-aurora/rev/20e76c984d82

and to inbound:

http://hg.mozilla.org/integration/mozilla-inbound/rev/b9ad0b9119b7

Reopening because we're not sure what we want to do here.

Comment 25

[Marco Bonardo [:mak] (Away Apr 25 - May 5)]

http://hg.mozilla.org/mozilla-central/rev/b9ad0b9119b7

leaving open based on previous comment