CanvasRenderingContext2D.setTransfrom() reads only the aliases attributes of DOMMatrix2DInit

• Component: Canvas
• Status: RESOLVED FIXED

[Kagami Sascha Rosylight]

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.

[Radar WebKit Bug Importer]

<rdar://problem/53339548>

[Said Abou-Hallawa]

Created attachment 374689 [details]
Patch

[Simon Fraser (smfr)]

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?

[Said Abou-Hallawa]

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.

[Said Abou-Hallawa]

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).

[Said Abou-Hallawa]

Created attachment 374691 [details]
Patch

[WebKit Commit Bot]

Comment on attachment 374691 [details]
Patch

Clearing flags on attachment: 374691

Committed r247735: <https://trac.webkit.org/changeset/247735>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.