Bug 53081 – Need to explicitly check non-finite value of input coordinate in Canvas2D::isPointInPath

• Status: RESOLVED
• Resolution: FIXED
• Component: Canvas

[Johnny(Jianning) Ding]

In test canvas/philip/tests/2d.path.isPointInPath.nonfinite.html, we have test code to check the non-finite coordinate as listed below,
ctx.rect(-100, -50, 200, 100);
_assertSame(ctx.isPointInPath(NaN, 0), false, "ctx.isPointInPath(NaN, 0)", "false");
_assertSame(ctx.isPointInPath(0, NaN), false, "ctx.isPointInPath(0, NaN)", "false");
_assertSame(ctx.isPointInPath(NaN, NaN), false, "ctx.isPointInPath(NaN, NaN)", "false");

In all above tests, the input coordinates contain non-finite value, currently we leave them to let the following platform implementation (Path::contains) handle the non-finite value.
However, most of platform implementation don't explicit check the non-finite value.

Let's see the Skia implementation Path::contains (WebCore/platform/graphics/skia/PathSkia.cpp), it calls SkPathContainsPoint to do the real work.
In SkPathContainsPoint (WebCore/platform/graphics/skia/SkiaUtils.cpp), it doesn't check non-finite value, like NaN, then when executing on line 191-192, it converts float value to int, see the following code.
int x = static_cast<int>(floorf(point.x() / scale));
int y = static_cast<int>(floorf(point.y() / scale));

Unfortunately, doing NaN-to-int casting with static_cast<int>(float) returns different results on different platforms, See the following code

int i = static_cast<float>(d);
printf("INT: %08x\n", i)

On Mac, Linux & Windows, i is set to 0x80000000, on Android, i is set to 0.
So the test 2d.path.isPointInPath.nonfinite.html is passed on Mac, Linux & Windows, but failed on Android because point(0,0) is indeed in the rect

We know NaN(QNaN) means uncertain operation. we can not trust and use the result from operating NaN or other non-finite number, we need to explicitly check non-finite value in input coordinate.

We can add the check in  place.
(1) CanvasRenderingContext2D::isPointInPath, it's a root when calling [e-mail] from JS binding.
(2) Path::contains, add the check logic in each Path::contains implementation.
(3) Platfrom Implementation, like SkPathContainsPoint, CGPathContainsPoint, ..., etc. which can cover other calls which are not from JS binding.

My first patch will use method (1) but may be changed according to the review comments.

Thanks

[Johnny(Jianning) Ding]

typo, int i = static_cast<float>(d); -> int i = static_cast<int>(NAN);

[Johnny(Jianning) Ding]

Created attachment 80060 [details]
patch v1

[Johnny(Jianning) Ding]

Created attachment 80172 [details]
fix patch v1 with a change

[Eric Seidel (no email)]

Comment on attachment 80172 [details]
fix patch v1 with a change

How do we test this?

[Johnny(Jianning) Ding]

(In reply to comment #4)
> (From update of attachment 80172 [details])
> How do we test this?

Test 2d.path.isPointInPath.nonfinite.html under LayoutTests/canvas/philip/tests/ already covered this bug. The reason I filed this bug is because the test is failed on Android. Do we need to write another test?

[Johnny(Jianning) Ding]

Comment on attachment 80172 [details]
fix patch v1 with a change

ask for review again.

[Andreas Kling]

Comment on attachment 80172 [details]
fix patch v1 with a change

View in context: https://bugs.webkit.org/attachment.cgi?id=80172&action=review

r=me, though the ChangeLog should be more specific that this is moving the finiteness check into common code.

> Source/WebCore/html/canvas/CanvasRenderingContext2D.cpp:880
> +    // Test whether the input point contians non-finite value.

Unnecessary comment.

[Johnny(Jianning) Ding]

Created attachment 82307 [details]
patch for landing

[Johnny(Jianning) Ding]

(In reply to comment #7)
> (From update of attachment 80172 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=80172&action=review
> 
> r=me, though the ChangeLog should be more specific that this is moving the finiteness check into common code.
> 
> > Source/WebCore/html/canvas/CanvasRenderingContext2D.cpp:880
> > +    // Test whether the input point contians non-finite value.
> 
> Unnecessary comment.

Thanks Andreas!
I have changed the patch according to your comments.  Will land it

[WebKit Commit Bot]

Comment on attachment 82307 [details]
patch for landing

Clearing flags on attachment: 82307

Committed r78478: <http://trac.webkit.org/changeset/78478>