Bug 250382 - Check for non-finite points before transforming them instead of after
Summary: Check for non-finite points before transforming them instead of after
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Canvas (show other bugs)
Version: Safari Technology Preview
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Nobody
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2023-01-10 06:54 PST by Ahmad Saleem
Modified: 2023-02-28 09:32 PST (History)
4 users (show)

See Also:


Attachments

Note You need to log in before you can comment on or make changes to this bug.
Description Ahmad Saleem 2023-01-10 06:54:11 PST
Hi Team,

While going through Blink's commit, I came across potential assertion fix and also to improve handling NaN cases:

Blink Commit - https://src.chromium.org/viewvc/blink?revision=183090&view=revision

WebKit Source - https://github.com/WebKit/WebKit/blob/28de25b653f64ca3fc8e87fbf1d1a963329a5342/Source/WebCore/html/canvas/CanvasRenderingContext2DBase.cpp#L520

Unfortunately it does not have any test case to identify whether it is causing assert or failing or something but I just wanted to get input.

Thanks!
Comment 1 Radar WebKit Bug Importer 2023-01-17 06:55:15 PST
<rdar://problem/104331898>
Comment 2 Said Abou-Hallawa 2023-02-23 12:20:23 PST
The layout tests:

1) https://github.com/WebKit/WebKit/blob/28de25b653f64ca3fc8e87fbf1d1a963329a5342/LayoutTests/fast/canvas/canvas-path-isPointInPath.html
2) https://github.com/WebKit/WebKit/blob/28de25b653f64ca3fc8e87fbf1d1a963329a5342/LayoutTests/fast/canvas/canvas-path-isPointInStroke.html

test passing NaN values to isPointInPath() and isPointInStroke().

But we are fine because the complier ensures the math with NaN values are handled correctly all the way. However I think we should check 

    if (!std::isfinite(x) || !std::isfinite(y))
        return false;

In CanvasRenderingContext2DBase::isPointInPathInternal() and CanvasRenderingContext2DBase::isPointInStrokeInternal() instead of checking the values of the transformedPoint.

We do not need to check whether the values transformedPoint.x() and transformedPoint.y() are finite because AffineTransform::mapPoint() does it math in double and then it clamp it to float.
Comment 3 Ahmad Saleem 2023-02-25 05:10:39 PST
PR - https://github.com/WebKit/WebKit/pull/10673
Comment 4 EWS 2023-02-28 09:32:12 PST
Committed 260946@main (90fc0225ff34): <https://commits.webkit.org/260946@main>

Reviewed commits have been landed. Closing PR #10673 and removing active labels.