Handle negative arguments to drawImage

RESOLVED DUPLICATE of bug 655328

Status

()

Core
Canvas: 2D
RESOLVED DUPLICATE of bug 655328
6 years ago
2 years ago

People

(Reporter: Ms2ger, Unassigned)

Tracking

(Blocks: 1 bug, {dev-doc-complete, html5})

Trunk
mozilla5
dev-doc-complete, html5
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(firefox5+ fixed, firefox6 fixed)

Details

(URL)

Attachments

(2 attachments, 2 obsolete attachments)

(Reporter)

Description

6 years ago
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
Duplicate of this bug: 635380

Updated

6 years ago
Assignee: nobody → async.processingjs
Created attachment 513824 [details] [diff] [review]
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
Attachment #513824 - Flags: review?(jmuizelaar)
(Reporter)

Comment 3

6 years ago
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.
Attachment #513824 - Flags: feedback+
Created attachment 513887 [details] [diff] [review]
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.
Attachment #513824 - Attachment is obsolete: true
Attachment #513887 - Flags: review?(jmuizelaar)
Attachment #513824 - Flags: review?(jmuizelaar)
(Reporter)

Comment 5

6 years ago
Er, of course that wouldn't be your fault. Not sure where my head was.
What's the status of other browsers on these tests?
(Reporter)

Comment 7

6 years ago
IE9 and Chrome pass.
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?
> > 
> >+    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?
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.
Attachment #513887 - Flags: review?(jmuizelaar) → review+
(Reporter)

Comment 11

6 years ago
Created attachment 522206 [details] [diff] [review]
Patch for checkin
Attachment #513887 - Attachment is obsolete: true
(Reporter)

Updated

6 years ago
Status: NEW → ASSIGNED
Keywords: checkin-needed, html5
http://hg.mozilla.org/projects/cedar/rev/4ede7b9b55bc
Keywords: checkin-needed
Whiteboard: fixed-in-cedar
http://hg.mozilla.org/mozilla-central/rev/4ede7b9b55bc
Status: ASSIGNED → RESOLVED
Last Resolved: 6 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Whiteboard: fixed-in-cedar
Target Milestone: --- → mozilla2.2

Updated

6 years ago
Blocks: 629876
(Reporter)

Updated

6 years ago
No longer blocks: 629876
Duplicate of this bug: 629876
Keywords: dev-doc-needed
Note added to:

https://developer.mozilla.org/en/DOM/CanvasRenderingContext2D#Notes
Keywords: dev-doc-needed → dev-doc-complete

Updated

6 years ago
Depends on: 655328
It looks like we're going to have to back this out well we resolve some semantic issues.
Created attachment 534076 [details] [diff] [review]
Backout while semantic issues are being resolved
Attachment #534076 - Flags: approval-mozilla-beta?
Attachment #534076 - Flags: approval-mozilla-aurora?
Attachment #534076 - Flags: feedback?(bzbarsky)
Attachment #534076 - Flags: feedback?(bzbarsky) → feedback+

Updated

6 years ago
tracking-firefox5: --- → +
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
Attachment #534076 - Flags: approval-mozilla-beta?
Attachment #534076 - Flags: approval-mozilla-beta+
Attachment #534076 - Flags: approval-mozilla-aurora?
Attachment #534076 - Flags: approval-mozilla-aurora+

Comment 19

6 years ago
I don't see this yet in mozilla-beta and time is running out for Firefox 5. Please land this ASAP. Thanks.
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.
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.
Keywords: checkin-needed
http://hg.mozilla.org/releases/mozilla-aurora/rev/72b45a62587a
http://hg.mozilla.org/releases/mozilla-beta/rev/5ae373ff7a19
Attachment #534076 - Flags: checked-in+

Updated

6 years ago
Keywords: checkin-needed

Comment 23

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

Canceling needed due to backout
Keywords: dev-doc-complete → dev-doc-needed
status-firefox5: --- → fixed
status-firefox6: --- → fixed
Attachment #534076 - Flags: checked-in+ → checkin+
Keywords: dev-doc-needed → dev-doc-complete

Updated

6 years ago
Blocks: 669366
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.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
http://hg.mozilla.org/mozilla-central/rev/b9ad0b9119b7

leaving open based on previous comment
Duplicate of this bug: 974367

Updated

3 years ago
Assignee: ydelendik → nobody
Status: REOPENED → RESOLVED
Last Resolved: 6 years ago2 years ago
Resolution: --- → DUPLICATE
Duplicate of bug: 655328
You need to log in before you can comment on or make changes to this bug.