Bug 38929 - Canvas: Ignore calls to drawImage() with non-finite parameters
Summary: Canvas: Ignore calls to drawImage() with non-finite parameters
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebCore Misc. (show other bugs)
Version: 528+ (Nightly build)
Hardware: Other All
: P3 Critical
Assignee: Nobody
URL:
Keywords: HTML5, Qt, QtTriaged
Depends on:
Blocks:
 
Reported: 2010-05-11 12:55 PDT by Kenneth Rohde Christiansen
Modified: 2010-06-27 10:10 PDT (History)
7 users (show)

See Also:


Attachments
Proposed patch (3.36 KB, patch)
2010-06-27 03:59 PDT, Andreas Kling
no flags Details | Formatted Diff | Diff
Proposed patch (style fixed) (3.36 KB, patch)
2010-06-27 04:15 PDT, Andreas Kling
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Kenneth Rohde Christiansen 2010-05-11 12:55:03 PDT
QTransform::scale with NaN called 
ASSERT: "width > 0.0" in file /home/kenneth/repo/Qt/qt/src/gui/painting/qrasterizer.cpp, line 710
Aborted
Comment 1 Kenneth Rohde Christiansen 2010-05-11 12:56:47 PDT
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.
Comment 2 Csaba Osztrogonác 2010-06-03 04:04:06 PDT
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?
Comment 3 Kent Hansen 2010-06-15 00:57:47 PDT
I can reproduce the crash.
Comment 4 Andreas Kling 2010-06-27 03:59:17 PDT
Created attachment 59851 [details]
Proposed patch
Comment 5 WebKit Review Bot 2010-06-27 04:02:44 PDT
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.
Comment 6 Andreas Kling 2010-06-27 04:15:13 PDT
Created attachment 59852 [details]
Proposed patch (style fixed)

Ugly IMO, but I don't wanna fight with you, stylebot.
Comment 7 Darin Adler 2010-06-27 09:44:11 PDT
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.
Comment 8 Andreas Kling 2010-06-27 09:52:57 PDT
(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.
Comment 9 WebKit Commit Bot 2010-06-27 10:10:21 PDT
Comment on attachment 59852 [details]
Proposed patch (style fixed)

Clearing flags on attachment: 59852

Committed r61970: <http://trac.webkit.org/changeset/61970>
Comment 10 WebKit Commit Bot 2010-06-27 10:10:26 PDT
All reviewed patches have been landed.  Closing bug.