Bug 67988 - Large canvas fills should not crash or create unnecessarily large image buffers
Summary: Large canvas fills should not crash or create unnecessarily large image buffers
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Canvas (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Ben Wells
URL:
Keywords:
Depends on: 68070
Blocks:
  Show dependency treegraph
 
Reported: 2011-09-12 23:25 PDT by Ben Wells
Modified: 2011-09-16 18:01 PDT (History)
10 users (show)

See Also:


Attachments
Patch (9.41 KB, patch)
2011-09-12 23:42 PDT, Ben Wells
no flags Details | Formatted Diff | Diff
Patch (9.41 KB, patch)
2011-09-13 16:27 PDT, Ben Wells
no flags Details | Formatted Diff | Diff
Patch (9.44 KB, patch)
2011-09-13 21:43 PDT, Ben Wells
no flags Details | Formatted Diff | Diff
Patch (11.06 KB, patch)
2011-09-14 19:44 PDT, Ben Wells
no flags Details | Formatted Diff | Diff
Patch (11.05 KB, patch)
2011-09-14 20:21 PDT, Ben Wells
no flags Details | Formatted Diff | Diff
Patch (10.71 KB, patch)
2011-09-14 21:09 PDT, Ben Wells
no flags Details | Formatted Diff | Diff
Patch (8.43 KB, patch)
2011-09-15 20:26 PDT, Ben Wells
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Ben Wells 2011-09-12 23:25:55 PDT
Large canvas fills should not crash or create unnecessarily large image buffers
Comment 1 Ben Wells 2011-09-12 23:42:37 PDT
Created attachment 107148 [details]
Patch
Comment 2 Stephen White 2011-09-13 07:22:04 PDT
Comment on attachment 107148 [details]
Patch

Looks good.  r=me
Comment 3 Jeff Timanus 2011-09-13 08:14:50 PDT
Comment on attachment 107148 [details]
Patch

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

> LayoutTests/fast/canvas/canvas-large-fills.html:5
> +<p>Tests that using reasonably large values for canvas.height and canvas.height don't cause a crash</p>

Type-o:  canvas.width
Comment 4 Ben Wells 2011-09-13 16:27:34 PDT
Created attachment 107253 [details]
Patch
Comment 5 Darin Adler 2011-09-13 16:29:24 PDT
Comment on attachment 107253 [details]
Patch

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

I’m concerned about the change from FloatRect to IntRect. Otherwise, the patch looks good.

> Source/WebCore/html/canvas/CanvasRenderingContext2D.cpp:1513
> +    IntRect canvasRect(0, 0, canvas()->width(), canvas()->height());
> +    canvasRect = canvas()->baseTransform().mapRect(canvasRect);

Why is it OK to change this from FloatRect to IntRect? Does the code below always work properly with an integer-rounded rect?
Comment 6 Ben Wells 2011-09-13 16:49:10 PDT
> Why is it OK to change this from FloatRect to IntRect? Does the code below always work properly with an integer-rounded rect?

This rect starts in canvas space and is transformed to device space. The canvas space is integral. The coordinates in device space will also be integral, I think, but if it isn't in some cases it shouldn't be a problem - internally the transform is done with a FloatRect and then EnclosingIntRect is used to expand this if needed.

The canvasRect is used in one call later on, clearRect, which takes a FloatRect. The IntRect will be implicitly cast to a FloatRect.

If the canvasRect ends up being a little bigger than it needs to be through rounding in the transform it won't be a problem. This is used now in two ways - to limit the size of the buffer used and to clear the original canvas. If our rect is a little bigger than needed this will all still work.
Comment 7 Darin Adler 2011-09-13 16:50:39 PDT
(In reply to comment #6)
> The canvas space is integral. The coordinates in device space will also be integral, I think

Could you explain why? I know that doesn’t matter for this patch, but I’m curious why you think that.

> If the canvasRect ends up being a little bigger than it needs to be through rounding in the transform it won't be a problem.

OK.
Comment 8 Ben Wells 2011-09-13 17:05:57 PDT
(In reply to comment #7)
> (In reply to comment #6)
> > The canvas space is integral. The coordinates in device space will also be integral, I think
> 
> Could you explain why? I know that doesn’t matter for this patch, but I’m curious why you think that.
> 

It could be a case of a little knowledge being a dangerous thing. 

I've seen two instances of the base transform - in skia where it is the identity transform, and with core graphics where it seemed to be integral (something like flipping y and an offset if I remember correctly).

I think I have made an assumption that in general the underlying canvas image buffer will always be of integral bounds in device space (i.e. the transformed canvasRect will be integral).

Thinking about it some more for some devices with low resolution, and if the whole space is zoomed (e.g. at a browser level) the bounds not be integral but I don't know whether this is applied at this level or at a higher level (like RenderLayer).

Please, feel free to correct any misunderstandings I have just demonstrated!
Comment 9 WebKit Review Bot 2011-09-13 18:55:54 PDT
Comment on attachment 107253 [details]
Patch

Rejecting attachment 107253 [details] from commit-queue.

Failed to run "['/mnt/git/webkit-commit-queue/Tools/Scripts/webkit-patch', '--status-host=queues.webkit.org', '-..." exit_code: 2

Last 500 characters of output:
ernational/danda-space.html = IMAGE
  fast/text/international/thai-baht-space.html = IMAGE
  fast/text/international/thai-line-breaks.html = IMAGE
  platform/chromium-linux/fast/text/international/complex-joining-using-gpos.html = IMAGE

Regressions: Unexpected DumpRenderTree crashes : (1)
  css3/calc/regression-62276.html = CRASH

Regressions: Unexpected image and text mismatch : (2)
  fast/borders/border-image-scrambled.html = IMAGE+TEXT
  svg/custom/svg-fonts-word-spacing.html = IMAGE+TEXT



Full output: http://queues.webkit.org/results/9657356
Comment 10 WebKit Review Bot 2011-09-13 20:46:33 PDT
Comment on attachment 107253 [details]
Patch

Rejecting attachment 107253 [details] from commit-queue.

benwells@chromium.org does not have committer permissions according to http://trac.webkit.org/browser/trunk/Tools/Scripts/webkitpy/common/config/committers.py.

- If you do not have committer rights please read http://webkit.org/coding/contributing.html for instructions on how to use bugzilla flags.

- If you have committer rights please correct the error in Tools/Scripts/webkitpy/common/config/committers.py by adding yourself to the file (no review needed).  The commit-queue restarts itself every 2 hours.  After restart the commit-queue will correctly respect your committer rights.
Comment 11 Mike Lawther 2011-09-13 21:00:26 PDT
Comment on attachment 107253 [details]
Patch

setting cq+ after Darin's r+
Comment 12 Ben Wells 2011-09-13 21:43:16 PDT
Created attachment 107282 [details]
Patch
Comment 13 Ben Wells 2011-09-13 21:44:09 PDT
Comment on attachment 107282 [details]
Patch

Fixed test, I had not updated the expectations after updating the comment in the html.
Comment 14 noel gordon 2011-09-13 21:51:11 PDT
Ok, lets try it again.
Comment 15 Darin Adler 2011-09-13 22:09:42 PDT
(In reply to comment #8)
> I've seen two instances of the base transform - in skia where it is the identity transform, and with core graphics where it seemed to be integral (something like flipping y and an offset if I remember correctly).

What you’ve described is probably right for the baseTransform of the image buffer. 

But if you look at the implementation of HTMLCanvasElement::baseTransform function you’ll see that the transform can easily be non-integral if there is a non-integral ratio between the size of the canvas (determined by the height and width attributes) and the size of the place where the canvas element is laid out on the page (determined by CSS  height and width).

> I think I have made an assumption that in general the underlying canvas image buffer will always be of integral bounds in device space (i.e. the transformed canvasRect will be integral).

It's an incorrect assumption.
Comment 16 Ben Wells 2011-09-13 22:55:17 PDT
(In reply to comment #15)
> (In reply to comment #8)
> > I've seen two instances of the base transform - in skia where it is the identity transform, and with core graphics where it seemed to be integral (something like flipping y and an offset if I remember correctly).
> 
> What you’ve described is probably right for the baseTransform of the image buffer. 
> 
> But if you look at the implementation of HTMLCanvasElement::baseTransform function you’ll see that the transform can easily be non-integral if there is a non-integral ratio between the size of the canvas (determined by the height and width attributes) and the size of the place where the canvas element is laid out on the page (determined by CSS  height and width).
> 
> > I think I have made an assumption that in general the underlying canvas image buffer will always be of integral bounds in device space (i.e. the transformed canvasRect will be integral).
> 
> It's an incorrect assumption.

Thanks for clarifying. I can see now there are four coordinate spaces in action: transformed canvas, untransformed canvas, page (or css) and device. I wasn't considering the page / css space.

Based on your explanation I can see some areas of the code for me to understand better, and some cases to test to make sure these fill modes are working as expected - in particular when the canvas space and page / css spaces aren't 1:1.
Comment 17 WebKit Review Bot 2011-09-14 01:09:11 PDT
Comment on attachment 107282 [details]
Patch

Clearing flags on attachment: 107282

Committed r95080: <http://trac.webkit.org/changeset/95080>
Comment 18 WebKit Review Bot 2011-09-14 01:09:16 PDT
All reviewed patches have been landed.  Closing bug.
Comment 19 Gabor Rapcsanyi 2011-09-14 02:44:15 PDT
Reopen, because it was rolled out by http://trac.webkit.org/changeset/95084

See https://bugs.webkit.org/show_bug.cgi?id=68070 for details.
Comment 20 Ben Wells 2011-09-14 19:44:12 PDT
Created attachment 107443 [details]
Patch
Comment 21 Ben Wells 2011-09-14 19:52:59 PDT
Comment on attachment 107443 [details]
Patch

The previous patch caused some tests to fail on CG. Specifically when the region being filled did not intersect with the canvas.

I believe this is because clipOut in the CG graphics context clips out the entire context, if called with an empty rect.

This version of the patch optimizes out this case as we just have to clear the canvas.
Comment 22 Ben Wells 2011-09-14 20:03:32 PDT
As I wrote that last comment it sounded implausible. I'm doing more testing to verify this or if its something else figure out what is happening.
Comment 23 Ben Wells 2011-09-14 20:21:25 PDT
Created attachment 107445 [details]
Patch
Comment 24 Ben Wells 2011-09-14 20:23:41 PDT
Comment on attachment 107445 [details]
Patch

The earlier statement was incorrect, it is not clipOut acting differently but ImageBuffer::create. On CG this returns null if the requested buffer size is empty.

Change log has been updated to reflect this.
Comment 25 Ben Wells 2011-09-14 21:09:32 PDT
Created attachment 107448 [details]
Patch
Comment 26 Ben Wells 2011-09-14 21:11:20 PDT
Comment on attachment 107448 [details]
Patch

Change log updated and test formatting changed based on feedback.
Comment 27 Darin Adler 2011-09-15 10:28:43 PDT
Comment on attachment 107448 [details]
Patch

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

I’m going to say review+, but I think it would be much better to post the early return version.

> LayoutTests/fast/canvas/canvas-large-fills-expected.txt:7
> +source-over
> +PASS Actual: 0 Expected: 0
> +PASS Actual: 0 Expected: 0
> +PASS Actual: 255 Expected: 255

This test output is pretty good. I think it would be even better to check these as a single value; less verbose and easier to understand. The test can convert them to hex form or some other easy to understand form. More like:

    PASS: source-over: #0000FF

That would cut the number of lines of output by 4 and make the test clearer.

> Source/WebCore/html/canvas/CanvasRenderingContext2D.cpp:1518
> +    if (!bufferRect.isEmpty()) {

In the WebKit project our usual style is to use “early return”. If we do that here, a side benefit is that the patch will be smaller. Like this:

    if (bufferRect.isEmpty()) {
        clearCanvas();
        return;
    }

This makes it so we don’t have the whole function indented.
Comment 28 Matthew Delaney 2011-09-15 11:39:41 PDT
Comment on attachment 107448 [details]
Patch

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

> LayoutTests/fast/canvas/canvas-large-fills-expected.txt:8
> +source-in

Totally agree with Darin. The output text should optimize the time it takes to find out exactly what the failures are - if there are any. So, I'd say one step further would be to only output failures and if there are none, simply have one "PASS" line and that's it. This minimizes the burden on someone who finds the test failing on the bots or when run locally.

> Source/WebCore/html/canvas/CanvasRenderingContext2D.cpp:1516
> +    bufferRect.intersect(canvasRect);

Do we need to normalize the bufferRect here (since intersect assumes it is)?

> Source/WebCore/html/canvas/CanvasRenderingContext2D.cpp:1521
> +        RenderingMode renderMode = canvas()->buffer()->isAccelerated() ? Accelerated : Unaccelerated;

FYI, this line changed yesterday.
Comment 29 Darin Adler 2011-09-15 11:40:41 PDT
(In reply to comment #28)
> Totally agree with Darin. The output text should optimize the time it takes to find out exactly what the failures are - if there are any.

Glad we agree on this.

> So, I'd say one step further would be to only output failures and if there are none, simply have one "PASS" line and that's it.

I don’t agree with this. I do not like tests that just say PASS.
Comment 30 Ben Wells 2011-09-15 20:26:08 PDT
Created attachment 107593 [details]
Patch
Comment 31 Ben Wells 2011-09-15 20:59:16 PDT
Comment on attachment 107593 [details]
Patch

This patch has added the early exit. This makes the code much nicer and also makes the diff cleaner.

The test output has been trimmed, and now doesn't need scrolling when showing up in test results. I've compromised with the test output: passing lines are all shown, but failure lines have extra text (...EXPECTED x, GOT y versus just ...x) so they stand out. The javascript for the test has also been cleaned up based on offline feedback to be more readable.

About the normalised rectangles: as long as Path.boundingRect returns a normalised rect this code should be fine. Looking at the skia implementation this will be the case; I cannot tell for sure about CG. Is this a safe assumption to make? From my reading of the SVG path code (rendering/svg/RenderSVGPath) it also makes this assumption.
Comment 32 noel gordon 2011-09-15 23:16:52 PDT
cq+ following Darin's r+
Comment 33 Ben Wells 2011-09-16 04:48:29 PDT
Comment on attachment 107593 [details]
Patch

Taking of cq+ as the queue has been stuck for hours. Will try putting through the queue again tomorrow.
Comment 34 WebKit Review Bot 2011-09-16 18:01:50 PDT
Comment on attachment 107593 [details]
Patch

Clearing flags on attachment: 107593

Committed r95346: <http://trac.webkit.org/changeset/95346>
Comment 35 WebKit Review Bot 2011-09-16 18:01:56 PDT
All reviewed patches have been landed.  Closing bug.