630052 - Handle non-finite and negative arguments to createImageData according to spec

Status: RESOLVED FIXED

(Core :: Graphics: Canvas2D, defect)

Description

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

Attachment: Patch v1

This patch makes us pass

http://www.w3c-test.org/html/tests/approved/canvas/2d.imageData.create2.negative.html
http://www.w3c-test.org/html/tests/approved/canvas/2d.imageData.create2.nonfinite.html

I'm not entirely happy about NonFinite(), but I couldn't find something better.

Comment 1

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 508263 [details] [diff] [review]
Patch v1

Yeah, the nonfinite part is bogus.  In particular, if v is an object whose valueOf returns Infinity, this code will do the wrong thing.  Worth adding a test for this (to our tests and the official test suite).

What you probably want to do instead if use JS_ValueToNumber and then use the DoubleIsFinite helper in nsCanvasRenderingContext.cpp (which therefore needs to move elsewhere).

Then of course you have to worry about how to convert the doubles into integers.  It looks like the spec doesn't actually define the behavior here, right?

Comment 2

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

Attachment: Preparatory patch

This code duplication was poking my eye out.

(Also, now I can fit nsIDOMCanvasRenderingContext2D_CreateImageData on my screen.)

Comment 3

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 508843 [details] [diff] [review]
Preparatory patch

>+++ b/content/canvas/src/CustomQS_Canvas2D.h
>+    // create the fast typed array

Keep the comment about it being 0-filled by default?

>+        nsresult rv = self->GetImageData_explicit(x, y, w, h,
>+                                         static_cast<PRUint8*>(tdest->data),

I'd prefer line-breaking after the '=' and lining up the arguments properly.

r=me with those nits.

Thanks for reducing the code duplication!

Comment 4

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

Attachment: Patch v2

This should look a bit better. The test will need some more updates.

Comment 5

[Boris Zbarsky [:bzbarsky]]

That's missing the changes to nsMathUtils.h.

The new code is not equivalent to the old for various finite doubles.  For example, consider width == 2^32 - 1.  Compare these two programs:

JS:

  var x = (1 << 16);
  x = x * x - 1;
  print(x | 0);

C++:

  double x = (1 << 16);
  x = x * x - 1;
  printf("%d\n", (int)x);

The former prints -1.  The latter prints -2147483648.

You can get the behavior of the former, maybe, if you cast through PRUint32 first... but you'd need to check, the spec.

Though again, the behavior here is not exactly defined; what's the desired behavior?

Comment 6

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

Attachment: MathUtils code v1

This patch moves the DoubleIsFinite code. Who should I ask to review the code in nsMathUtils?

Comment 7

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 509746 [details] [diff] [review]
MathUtils code v1

I can review this.

Still need to figure out how the double-to-int conversion should work...

Comment 8

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

Attachment: Patch v3

I think this is going to be better for my (limited) sanity.

Comment 9

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 510319 [details] [diff] [review]
Patch v3

>+    if (width < 0) {
>+        w = -wi;
>+        x -= w;
>+    } else {
>+        w = wi;
>+    }

Maybe add a comment like "Handle negative width and height by flipping the rectangle over in the relevant direction" before this code or some such?

r=me

Comment 10

[Yury Delendik (:yury)]

Comment on attachment 508843 [details] [diff] [review]
Preparatory patch

>+        nsresult rv = self->GetImageData_explicit(x, y, w, h,
>+                                         static_cast<PRUint8*>(tdest->data),

The static CreateImageData has no arguments or variables with x and y names. Also, there is no "CheckedInt.h" include.

Comment on attachment 510319 [details] [diff] [review]
Patch v3

It looks like JS_ValueToECMAInt32 better way to validate the arguments than JS_ValueToNumber/JS_DoubleToInt32 combo (and the compiler fails on JS_DoubleToInt32).

Comment 11

[Boris Zbarsky [:bzbarsky]]

> It looks like JS_ValueToECMAInt32 better way to validate the arguments

No, it's not.  It'll return 0 silently if passed a NaN, whereas the API says it needs to throw.

> and the compiler fails on JS_DoubleToInt32

That's because you didn't apply the patch for bug 631132, which this one is clearly marked as depending on....

Comment 12

[Yury Delendik (:yury)]

Yeah, I noticed that later :) Thanks

Comment 13

[Yury Delendik (:yury)]

Attachment: Preparatory patch fix (adds missing x and y args)

Comment 14

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

I've got that fixed locally. I'll try to upload tonight.

Comment 15

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

Attachment: Part a: Add NS_finite function to nsMathUtils

Comment 16

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

Attachment: Part b: Factor out duplicated code from GetImageData and CreateImageData

Comment 17

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

Attachment: Part c: Handle non-finite and negative arguments to createImageData according to spec

Comment 18

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

And adding a dependency for the test I missed.

Comment 19

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

Attachment: Part a for checkin

Comment 20

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

Attachment: Part b for checkin

Comment 21

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

Attachment: Part c for checkin

Comment 22

[(no longer active)]

http://hg.mozilla.org/projects/cedar/rev/8e8e6f2f737d
http://hg.mozilla.org/projects/cedar/rev/2676052e13a8
http://hg.mozilla.org/projects/cedar/rev/c51360e5cc59

Comment 23

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

http://hg.mozilla.org/mozilla-central/rev/8e8e6f2f737d
http://hg.mozilla.org/mozilla-central/rev/2676052e13a8
http://hg.mozilla.org/mozilla-central/rev/c51360e5cc59

Comment 24

[Eric Shepherd [:sheppy]]

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