Bug 199850

Summary: CanvasRenderingContext2D.setTransfrom() reads only the aliases attributes of DOMMatrix2DInit
Product: WebKit Reporter: Kagami Sascha Rosylight <saschanaz>
Component: CanvasAssignee: Said Abou-Hallawa <sabouhallawa>
Status: RESOLVED FIXED    
Severity: Normal CC: cdumez, commit-queue, dino, esprehn+autocc, ews, gyuyoung.kim, sabouhallawa, simon.fraser, webkit-bug-importer
Priority: P2 Keywords: InRadar
Version: WebKit Nightly Build   
Hardware: Unspecified   
OS: Linux   
Attachments:
Description Flags
Patch
none
Patch none

Description Kagami Sascha Rosylight 2019-07-16 20:55:46 PDT
Relevant WPT test: https://w3c-test.org/css/geometry/DOMMatrix2DInit-validate-fixup.html
Smaller visual test: https://codepen.io/SaschaNaz/pen/voBBee (Both rectangle must look same with their width being twice of their height.)

Relevant source: https://github.com/WebKit/webkit/blob/89c28d471fae35f1788a0f857067896a10af8974/Source/WebCore/html/canvas/CanvasRenderingContext2DBase.cpp#L910

It seems the method internally reads a/b/c/d/c/f instead of m11/m12/m21/m22/m41/m42, which is a problem because validateAndFixup only ensures correct values for m** fields.
Comment 1 Radar WebKit Bug Importer 2019-07-19 23:50:45 PDT
<rdar://problem/53339548>
Comment 2 Said Abou-Hallawa 2019-07-23 11:43:52 PDT
Created attachment 374689 [details]
Patch
Comment 3 Simon Fraser (smfr) 2019-07-23 11:51:20 PDT
Comment on attachment 374689 [details]
Patch

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

> Source/WebCore/html/canvas/CanvasRenderingContext2DBase.cpp:916
> +    setTransform(matrixInit.m11.valueOr(1), matrixInit.m12.valueOr(0), matrixInit.m21.valueOr(0), matrixInit.m22.valueOr(1), matrixInit.m41.valueOr(0), matrixInit.m42.valueOr(0));

Should this be matrixInit.m11.valueOr(matrixInit.a.valueOr(1)) etc?
Comment 4 Said Abou-Hallawa 2019-07-23 12:06:51 PDT
Comment on attachment 374689 [details]
Patch

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

>> Source/WebCore/html/canvas/CanvasRenderingContext2DBase.cpp:916
>> +    setTransform(matrixInit.m11.valueOr(1), matrixInit.m12.valueOr(0), matrixInit.m21.valueOr(0), matrixInit.m22.valueOr(1), matrixInit.m41.valueOr(0), matrixInit.m42.valueOr(0));
> 
> Should this be matrixInit.m11.valueOr(matrixInit.a.valueOr(1)) etc?

Just before this line we call DOMMatrixReadOnly::validateAndFixup(matrixInit) which ensures the m** fields are set if they are not set and the aliases fields are set for example.

    if (!init.m11)
        init.m11 = init.a.valueOr(1);

So I think I do not need to repeat this code again here.
Comment 5 Said Abou-Hallawa 2019-07-23 12:11:04 PDT
Comment on attachment 374689 [details]
Patch

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

>>> Source/WebCore/html/canvas/CanvasRenderingContext2DBase.cpp:916
>>> +    setTransform(matrixInit.m11.valueOr(1), matrixInit.m12.valueOr(0), matrixInit.m21.valueOr(0), matrixInit.m22.valueOr(1), matrixInit.m41.valueOr(0), matrixInit.m42.valueOr(0));
>> 
>> Should this be matrixInit.m11.valueOr(matrixInit.a.valueOr(1)) etc?
> 
> Just before this line we call DOMMatrixReadOnly::validateAndFixup(matrixInit) which ensures the m** fields are set if they are not set and the aliases fields are set for example.
> 
>     if (!init.m11)
>         init.m11 = init.a.valueOr(1);
> 
> So I think I do not need to repeat this code again here.

I think I should even use matrixInit.m11.value() since it has been set to a.valueOr(1).
Comment 6 Said Abou-Hallawa 2019-07-23 12:15:15 PDT
Created attachment 374691 [details]
Patch
Comment 7 WebKit Commit Bot 2019-07-23 13:39:31 PDT
Comment on attachment 374691 [details]
Patch

Clearing flags on attachment: 374691

Committed r247735: <https://trac.webkit.org/changeset/247735>
Comment 8 WebKit Commit Bot 2019-07-23 13:39:33 PDT
All reviewed patches have been landed.  Closing bug.