Bug 84289 - [chromium] Clip TransparencyWin to prevent OOM from large Skia canvas
Summary: [chromium] Clip TransparencyWin to prevent OOM from large Skia canvas
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: New Bugs (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Adrienne Walker
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2012-04-18 15:17 PDT by Adrienne Walker
Modified: 2012-04-20 15:36 PDT (History)
6 users (show)

See Also:


Attachments
Patch (6.59 KB, patch)
2012-04-18 15:30 PDT, Adrienne Walker
no flags Details | Formatted Diff | Diff
Use enclosingIntRect (6.57 KB, patch)
2012-04-18 16:44 PDT, Adrienne Walker
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Adrienne Walker 2012-04-18 15:17:06 PDT
[chromium] Clip TransparencyWin to prevent OOM from large Skia canvas
Comment 1 Adrienne Walker 2012-04-18 15:30:09 PDT
Created attachment 137784 [details]
Patch
Comment 2 Adrienne Walker 2012-04-18 15:36:41 PDT
Chromium bug: http://crbug.com/121318

brettw: I CC'd you because your name was all over the TransparencyWin blame list.  If there's somebody else who would be interested in this, please let me know.

I only implemented this for KeepTransform.

I wanted to implement this for ScaleTransform too, but drawing into a transparency layer with a ScaleTransform appears to place the results of drawing at (x, y) at different translations in the final canvas depending on what layer mode is being used.  (Clipping tends to add a translation, but a translation on the context without clipping is also sufficient to demonstrate this.) I don't know if that's a bug, but it's really hairy to try to write unit tests for ScaleTransform.  So, I punted.
Comment 3 Dana Jansens 2012-04-18 15:55:58 PDT
Comment on attachment 137784 [details]
Patch

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

> Source/WebCore/platform/graphics/chromium/TransparencyWin.cpp:219
> +            IntRect clipRect = IntRect(clipBounds.left(), clipBounds.top(), clipBounds.width(), clipBounds.height());

passing unguarded floats to the IntRect() constructor seems like a recipe for bad.

> Source/WebKit/chromium/tests/TransparencyWinTest.cpp:398
> +static void testClippedLayerKeepTransform(TransparencyWin::LayerMode layerMode)

I love this test, very nice.
Comment 4 Adrienne Walker 2012-04-18 16:02:39 PDT
I'm not sure if the scale issues are an actual bug, but I filed a bug with a unit test here: https://bugs.webkit.org/show_bug.cgi?id=84294

(In reply to comment #3)
> (From update of attachment 137784 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=137784&action=review
> 
> > Source/WebCore/platform/graphics/chromium/TransparencyWin.cpp:219
> > +            IntRect clipRect = IntRect(clipBounds.left(), clipBounds.top(), clipBounds.width(), clipBounds.height());
> 
> passing unguarded floats to the IntRect() constructor seems like a recipe for bad.

Unguarded in what sense? I called roundOut() which does floor/ceil calls.  I could put it in a FloatRect and call enclosedIntRect, which seems like the same thing?
Comment 5 Dana Jansens 2012-04-18 16:07:13 PDT
Comment on attachment 137784 [details]
Patch

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

>>> Source/WebCore/platform/graphics/chromium/TransparencyWin.cpp:219
>>> +            IntRect clipRect = IntRect(clipBounds.left(), clipBounds.top(), clipBounds.width(), clipBounds.height());
>> 
>> passing unguarded floats to the IntRect() constructor seems like a recipe for bad.
> 
> Unguarded in what sense? I called roundOut() which does floor/ceil calls.  I could put it in a FloatRect and call enclosedIntRect, which seems like the same thing?

In the sense that roundOut() rounds in float space. Then you pass to IntRect() and you can have an invalid IntRect().

Consider left() == INT_MAX and width() > 0. Now maxX() is inexpressible and will overflow.

I think enclosedIntRect is different and better because it considers these things and always gives a valid IntRect() back.
Comment 6 Dana Jansens 2012-04-18 16:07:53 PDT
Comment on attachment 137784 [details]
Patch

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

>>>> Source/WebCore/platform/graphics/chromium/TransparencyWin.cpp:219
>>>> +            IntRect clipRect = IntRect(clipBounds.left(), clipBounds.top(), clipBounds.width(), clipBounds.height());
>>> 
>>> passing unguarded floats to the IntRect() constructor seems like a recipe for bad.
>> 
>> Unguarded in what sense? I called roundOut() which does floor/ceil calls.  I could put it in a FloatRect and call enclosedIntRect, which seems like the same thing?
> 
> In the sense that roundOut() rounds in float space. Then you pass to IntRect() and you can have an invalid IntRect().
> 
> Consider left() == INT_MAX and width() > 0. Now maxX() is inexpressible and will overflow.
> 
> I think enclosedIntRect is different and better because it considers these things and always gives a valid IntRect() back.

s/enclosedIntRect/enclosingIntRect/
Comment 7 Dana Jansens 2012-04-18 16:09:38 PDT
(In reply to comment #5)
> I think enclosedIntRect is different and better because it considers these things and always gives a valid IntRect() back.

Actually, I was reading enclosedIntRect(). It looks to me like enclosingIntRect() can give you an IntRect() that overflows too. :(
Comment 8 Adrienne Walker 2012-04-18 16:44:44 PDT
Created attachment 137803 [details]
Use enclosingIntRect
Comment 9 Dana Jansens 2012-04-18 16:50:15 PDT
Comment on attachment 137803 [details]
Use enclosingIntRect

Thanks LGTM. Opened http://code.google.com/p/chromium/issues/detail?id=124127 for enclosingIntRect issues.
Comment 10 Adrienne Walker 2012-04-18 16:55:12 PDT
(In reply to comment #7)
> (In reply to comment #5)
> > I think enclosedIntRect is different and better because it considers these things and always gives a valid IntRect() back.
> 
> Actually, I was reading enclosedIntRect(). It looks to me like enclosingIntRect() can give you an IntRect() that overflows too. :(

enclosingIntRect also uses clampToInteger.  I think it's fine.
Comment 11 Dana Jansens 2012-04-18 16:56:59 PDT
(In reply to comment #10)
> enclosingIntRect also uses clampToInteger.  I think it's fine.

However it only clamps the x, y, width and height. maxX in this case will be representable through two ints (x + width) but not necessarily though one. No?
Comment 12 Adrienne Walker 2012-04-18 16:59:27 PDT
(In reply to comment #11)
> (In reply to comment #10)
> > enclosingIntRect also uses clampToInteger.  I think it's fine.
> 
> However it only clamps the x, y, width and height. maxX in this case will be representable through two ints (x + width) but not necessarily though one. No?

Ack, quite right.  :(
Comment 13 James Robinson 2012-04-20 11:53:17 PDT
Comment on attachment 137803 [details]
Use enclosingIntRect

R=me
Comment 14 WebKit Review Bot 2012-04-20 15:35:59 PDT
Comment on attachment 137803 [details]
Use enclosingIntRect

Clearing flags on attachment: 137803

Committed r114791: <http://trac.webkit.org/changeset/114791>
Comment 15 WebKit Review Bot 2012-04-20 15:36:04 PDT
All reviewed patches have been landed.  Closing bug.