[chromium] Clip TransparencyWin to prevent OOM from large Skia canvas
Created attachment 137784 [details] Patch
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 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.
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 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 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/
(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. :(
Created attachment 137803 [details] Use enclosingIntRect
Comment on attachment 137803 [details] Use enclosingIntRect Thanks LGTM. Opened http://code.google.com/p/chromium/issues/detail?id=124127 for enclosingIntRect issues.
(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.
(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?
(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 on attachment 137803 [details] Use enclosingIntRect R=me
Comment on attachment 137803 [details] Use enclosingIntRect Clearing flags on attachment: 137803 Committed r114791: <http://trac.webkit.org/changeset/114791>
All reviewed patches have been landed. Closing bug.