Bug 84289 – [chromium] Clip TransparencyWin to prevent OOM from large Skia canvas

• Component: New Bugs
• Status: RESOLVED FIXED

[Adrienne Walker]

[chromium] Clip TransparencyWin to prevent OOM from large Skia canvas

[Adrienne Walker]

Created attachment 137784 [details]
Patch

[Adrienne Walker]

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.

[Dana Jansens]

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.

[Adrienne Walker]

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?

[Dana Jansens]

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.

[Dana Jansens]

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/

[Dana Jansens]

(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. :(

[Adrienne Walker]

Created attachment 137803 [details]
Use enclosingIntRect

[Dana Jansens]

Comment on attachment 137803 [details]
Use enclosingIntRect

Thanks LGTM. Opened http://code.google.com/p/chromium/issues/detail?id=124127 for enclosingIntRect issues.

[Adrienne Walker]

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

[Dana Jansens]

(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?

[Adrienne Walker]

(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.  :(

[James Robinson]

Comment on attachment 137803 [details]
Use enclosingIntRect

R=me

[WebKit Review Bot]

Comment on attachment 137803 [details]
Use enclosingIntRect

Clearing flags on attachment: 137803

Committed r114791: <http://trac.webkit.org/changeset/114791>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.