Bug 38929 – Canvas: Ignore calls to drawImage() with non-finite parameters

• Status: RESOLVED
• Resolution: FIXED
• Component: WebCore Misc.

[Kenneth Rohde Christiansen]

QTransform::scale with NaN called 
ASSERT: "width > 0.0" in file /home/kenneth/repo/Qt/qt/src/gui/painting/qrasterizer.cpp, line 710
Aborted

[Kenneth Rohde Christiansen]

This only crashes when using the raster graphics system, so it might be a bug in Qt, but the warning 'QTransform::scale with NaN called' is shown for all graphicssystems so it might be in WebKit as well.

[Csaba Osztrogonác]

I tried to reproduce this crash with the following command:
$ WebKitBuild/Release/bin/QtTestBrowser -graphicssystem raster -style windows LayoutTests/canvas/philip/tests/2d.drawImage.nonfinite.html

I got the following output, but no crash:

2d.drawImage.nonfinite
drawImage() with Infinity/NaN is ignored
References: 2d.nonfinite
Actual output: <--- here is a red rectangle
Expected output: <--- here is a green rectangle

Aborted with exception: INDEX_SIZE_ERR: DOM Exception 1

--

Is there crash still or I did something wrong?
How should I reproduce the crash?

[Kent Hansen]

I can reproduce the crash.

[Andreas Kling]

Created attachment 59851 [details]
Proposed patch

[WebKit Review Bot]

Attachment 59851 [details] did not pass style-queue:

Failed to run "['WebKitTools/Scripts/check-webkit-style', '--no-squash']" exit_code: 1
WebCore/html/canvas/CanvasRenderingContext2D.cpp:1020:  Boolean expressions that span multiple lines should have their operators on the left side of the line instead of the right side.  [whitespace/operators] [4]
Total errors found: 1 in 4 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Andreas Kling]

Created attachment 59852 [details]
Proposed patch (style fixed)

Ugly IMO, but I don't wanna fight with you, stylebot.

[Darin Adler]

Comment on attachment 59852 [details]
Proposed patch (style fixed)

I read the section on drawImage in the WHATWG specification and I see nothing there that says a non-finite coordinate in an argument results in silently doing nothing. In some cases that may be correct, but in other cases it seems clear it is wrong.

For example it seems to me that if sx is infinite and all other values are finite then we should raise an INDEX_SIZE_ERR exception.

Am I missing something?

> +    if (!isfinite(dstRect.x()) || !isfinite(dstRect.y()) || !isfinite(dstRect.width()) || !isfinite(dstRect.height())
> +        || !isfinite(srcRect.x()) || !isfinite(srcRect.y()) || !isfinite(srcRect.width()) || !isfinite(srcRect.height()))
> +        return;

If you don't like all this on one line there are two obvious things you can do:

    1) Break it up into two separate if statements. There's no reason it has to be all in one line.

    2) Add a function named something like isFinite or isAllFinite or some better name that returns true only if all coordinates of a FloatRect finite. This could be a free function or a member function of FloatRect. If that was added, the code would be much easier to read.

[Andreas Kling]

(In reply to comment #7)
> (From update of attachment 59852 [details])
> I read the section on drawImage in the WHATWG specification and I see nothing there that says a non-finite coordinate in an argument results in silently doing nothing. In some cases that may be correct, but in other cases it seems clear it is wrong.
> 
> For example it seems to me that if sx is infinite and all other values are finite then we should raise an INDEX_SIZE_ERR exception.
> 
> Am I missing something?

From the 2D context section:

"Except where otherwise specified, for the 2D context interface, any method call with a numeric argument whose value is infinite or a NaN value must be ignored."

> If you don't like all this on one line there are two obvious things you can do...

I like the FloatRect::isFinite() idea, will add that in the next version of this patch.

[WebKit Commit Bot]

Comment on attachment 59852 [details]
Proposed patch (style fixed)

Clearing flags on attachment: 59852

Committed r61970: <http://trac.webkit.org/changeset/61970>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.