cc: Do a safe intersect when gathering images.

cc: Do a safe intersect when gathering images.

This patch ensures that we don't overflow int bounds when adding a
discardable image into the rtree. We know that all of it is bound by
the canvas size (ie, max layer size). So, we can do an intersect to
restrict the bounds. Furthermore, use custom intersect code because
gfx::Rect::Intersect uses right() and bottom() which will also trigger
an overflow.

R=enne
BUG=630572
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_precise_blink_rel

Committed: https://crrev.com/3127243212fe6c1e075b7d1888d900ba7860e7ce
Cr-Commit-Position: refs/heads/master@{#411136}

[vmpstr, 2016-08-08 21:25:04]

Description was changed from

==========
cc: Do a safe intersect when gathering images.

This patch ensures that we don't overflow int bounds when adding a
discardable image into the rtree. We know that all of it is bound by
the canvas size (ie, max layer size). So, we can do an intersect to
restrict the bounds. Furthermore, use custom intersect code because
gfx::Rect::Intersect uses right() and bottom() which will also trigger
an overflow.

R=enne
BUG=630572
==========

to

==========
cc: Do a safe intersect when gathering images.

This patch ensures that we don't overflow int bounds when adding a
discardable image into the rtree. We know that all of it is bound by
the canvas size (ie, max layer size). So, we can do an intersect to
restrict the bounds. Furthermore, use custom intersect code because
gfx::Rect::Intersect uses right() and bottom() which will also trigger
an overflow.

R=enne
BUG=630572
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_precise_blink_rel
==========

[vmpstr, 2016-08-08 21:25:12]

The CQ bit was checked by vmpstr@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-08-08 21:25:35]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[vmpstr, 2016-08-08 21:26:05]

Please take a look.

[enne (OOO), 2016-08-08 21:54:01]

https://codereview.chromium.org/2229603002/diff/1/cc/playback/discardable_ima...
File cc/playback/discardable_image_map.cc (right):

https://codereview.chromium.org/2229603002/diff/1/cc/playback/discardable_ima...
cc/playback/discardable_image_map.cc:29: DCHECK_EQ(0, max_bounds.x());
How about just passing a gfx::Size? If that makes SafeIntersectRects confusing
what about just "SafeClampPaintRectToSize"

https://codereview.chromium.org/2229603002/diff/1/cc/playback/discardable_ima...
cc/playback/discardable_image_map.cc:39: std::min(bounds_rect.width(),
max_bounds.width() - bounds_rect.x()));
Can this be negative if the paint offset is outside the max_bounds / canvas
rect?

[commit-bot: I haz the power, 2016-08-08 22:02:39]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-08-08 22:02:40]

Dry run: Try jobs failed on following builders:
  linux_chromium_chromeos_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[vmpstr, 2016-08-08 22:25:15]

PTAL

https://codereview.chromium.org/2229603002/diff/1/cc/playback/discardable_ima...
File cc/playback/discardable_image_map.cc (right):

https://codereview.chromium.org/2229603002/diff/1/cc/playback/discardable_ima...
cc/playback/discardable_image_map.cc:29: DCHECK_EQ(0, max_bounds.x());
On 2016/08/08 21:54:01, enne wrote:
> How about just passing a gfx::Size? If that makes SafeIntersectRects confusing
> what about just "SafeClampPaintRectToSize"

Done.

https://codereview.chromium.org/2229603002/diff/1/cc/playback/discardable_ima...
cc/playback/discardable_image_map.cc:39: std::min(bounds_rect.width(),
max_bounds.width() - bounds_rect.x()));
On 2016/08/08 21:54:01, enne wrote:
> Can this be negative if the paint offset is outside the max_bounds / canvas
> rect?

That shouldn't be possible since there's an early out where we reject any rects
that fall outside of canvas bounds already. I've added a comment to this
function to make this clear.

[enne (OOO), 2016-08-09 18:23:42]

lgtm

https://codereview.chromium.org/2229603002/diff/20001/cc/playback/discardable...
File cc/playback/discardable_image_map.cc (right):

https://codereview.chromium.org/2229603002/diff/20001/cc/playback/discardable...
cc/playback/discardable_image_map.cc:27: // Returns a rect clamped to
|max_size|. Note that |paint_rect| should intersect
Can you DCHECK this too just for sanity's sake?

Also, I think you mean "|paint_rect| should be contained by..." and not "should
intersect"

[vmpstr, 2016-08-09 20:52:10]

The CQ bit was checked by vmpstr@chromium.org

[vmpstr, 2016-08-09 20:52:11]

The CQ bit was unchecked by vmpstr@chromium.org

[vmpstr, 2016-08-09 22:07:12]

So I realized I wasn't clamping correctly if (x, y) of the rect was negative,
which can happen. Can you please take another look at the function? I've also
added tests to cover interesting cases. Let me know if there are other cases you
want me to add.

[enne (OOO), 2016-08-09 22:31:11]

I feel vindicated about the negatives.

I assume the max_size is always going to be something reasonable, because it's
always going to be < max texture size? I'm dubious that you won't have overflows
or dchecks when max_size is max int and offset is max int, but maybe this case
will never happen?

[vmpstr, 2016-08-09 22:36:10]

On 2016/08/09 22:31:11, enne wrote:
> I feel vindicated about the negatives.
> 
> I assume the max_size is always going to be something reasonable, because it's
> always going to be < max texture size? I'm dubious that you won't have
overflows
> or dchecks when max_size is max int and offset is max int, but maybe this case
> will never happen?

The canvas size here refers to the recording size (aka raster source size). So
yeah, I think more things will go wrong if it's too large.

[enne (OOO), 2016-08-09 22:50:44]

On 2016/08/09 at 22:36:10, vmpstr wrote:
> On 2016/08/09 22:31:11, enne wrote:
> > I feel vindicated about the negatives.
> > 
> > I assume the max_size is always going to be something reasonable, because
it's
> > always going to be < max texture size? I'm dubious that you won't have
overflows
> > or dchecks when max_size is max int and offset is max int, but maybe this
case
> > will never happen?
> 
> The canvas size here refers to the recording size (aka raster source size). So
yeah, I think more things will go wrong if it's too large.

Oh, if it's recording size, then maybe you need to care.  It seems possible to
have a layer that's max int size that's scaled down and entirely in the
viewport? We've definitely had to deal with overflows in tiling data before.

Could you maybe make a test with a max_size that's max int, and add cases for a
paint rect with a max int size and (1) negative offset (2) zero offset (3)
positive offset?

[vmpstr, 2016-08-10 00:00:06]

On 2016/08/09 22:50:44, enne wrote:
> On 2016/08/09 at 22:36:10, vmpstr wrote:
> > On 2016/08/09 22:31:11, enne wrote:
> > > I feel vindicated about the negatives.
> > > 
> > > I assume the max_size is always going to be something reasonable, because
> it's
> > > always going to be < max texture size? I'm dubious that you won't have
> overflows
> > > or dchecks when max_size is max int and offset is max int, but maybe this
> case
> > > will never happen?
> > 
> > The canvas size here refers to the recording size (aka raster source size).
So
> yeah, I think more things will go wrong if it's too large.
> 
> Oh, if it's recording size, then maybe you need to care.  It seems possible to
> have a layer that's max int size that's scaled down and entirely in the
> viewport? We've definitely had to deal with overflows in tiling data before.
> 
> Could you maybe make a test with a max_size that's max int, and add cases for
a
> paint rect with a max int size and (1) negative offset (2) zero offset (3)
> positive offset?

PTAL. So there's a pretty bad interaction at near max int size where x !=
static_cast<int>(static_cast<float>(x)). So the test is a bit weird. But the
math works to up to int_max - 64. After that, Skia seems to not issue draw
commands at all, so there are no images.

[enne (OOO), 2016-08-10 00:07:12]

lgtm

https://codereview.chromium.org/2229603002/diff/60001/cc/playback/discardable...
File cc/playback/discardable_image_map_unittest.cc (right):

https://codereview.chromium.org/2229603002/diff/60001/cc/playback/discardable...
cc/playback/discardable_image_map_unittest.cc:371:
static_cast<float>(std::numeric_limits<int>::max() - 64));
This really needs a comment.

[vmpstr, 2016-08-10 19:19:40]

https://codereview.chromium.org/2229603002/diff/60001/cc/playback/discardable...
File cc/playback/discardable_image_map_unittest.cc (right):

https://codereview.chromium.org/2229603002/diff/60001/cc/playback/discardable...
cc/playback/discardable_image_map_unittest.cc:371:
static_cast<float>(std::numeric_limits<int>::max() - 64));
On 2016/08/10 00:07:12, enne wrote:
> This really needs a comment.

Done.

[vmpstr, 2016-08-10 19:19:45]

The CQ bit was checked by vmpstr@chromium.org

[vmpstr, 2016-08-10 19:19:46]

The patchset sent to the CQ was uploaded after l-g-t-m from enne@chromium.org
Link to the patchset: https://codereview.chromium.org/2229603002/#ps80001
(title: "update")

[commit-bot: I haz the power, 2016-08-10 19:20:34]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-08-10 20:32:55]

Description was changed from

==========
cc: Do a safe intersect when gathering images.

This patch ensures that we don't overflow int bounds when adding a
discardable image into the rtree. We know that all of it is bound by
the canvas size (ie, max layer size). So, we can do an intersect to
restrict the bounds. Furthermore, use custom intersect code because
gfx::Rect::Intersect uses right() and bottom() which will also trigger
an overflow.

R=enne
BUG=630572
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_precise_blink_rel
==========

to

==========
cc: Do a safe intersect when gathering images.

This patch ensures that we don't overflow int bounds when adding a
discardable image into the rtree. We know that all of it is bound by
the canvas size (ie, max layer size). So, we can do an intersect to
restrict the bounds. Furthermore, use custom intersect code because
gfx::Rect::Intersect uses right() and bottom() which will also trigger
an overflow.

R=enne
BUG=630572
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_precise_blink_rel

Committed: https://crrev.com/3127243212fe6c1e075b7d1888d900ba7860e7ce
Cr-Commit-Position: refs/heads/master@{#411136}
==========

[commit-bot: I haz the power, 2016-08-10 20:32:56]

Patchset 5 (id:??) landed as
https://crrev.com/3127243212fe6c1e075b7d1888d900ba7860e7ce
Cr-Commit-Position: refs/heads/master@{#411136}