cc: Use CheckedNumeric for resource size calculations.

cc: Use CheckedNumeric for resource size calculations.

This patch uses CheckedNumeric for size calculations that can overflow
in resources.

R=danakj, jschuh
BUG=495867
CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel

Committed: https://crrev.com/93a24d4dffd492f6ea885fb1280b3386567ae83b
Cr-Commit-Position: refs/heads/master@{#333160}

[danakj, 2015-06-04 19:02:38]

https://codereview.chromium.org/1154393003/diff/1/cc/resources/resource.h
File cc/resources/resource.h (right):

https://codereview.chromium.org/1154393003/diff/1/cc/resources/resource.h#new...
cc/resources/resource.h:35: return result.ValueOrDie();
This would work, but looking at the callsites of MemorySizeBytes, these are all
for tiles. So they are limited sizes determined by us, and not from arbitrary
inputs.

If I was to duplicate the pattern from SharedBitmap here, I'd make 2 methods:

size_t UncheckedSizeInBytes(..)
bool VerifySizeInBytes(..)

And I'd DCHECK(VerifySizeInBytes(..)) and comment explaining why the size is
controlled at the callsites of UncheckedSizeInBytes.

https://codereview.chromium.org/1154393003/diff/1/cc/resources/resource_provi...
File cc/resources/resource_provider.cc (right):

https://codereview.chromium.org/1154393003/diff/1/cc/resources/resource_provi...
cc/resources/resource_provider.cc:776: base::CheckedNumeric<int> num_bytes =
BitsPerPixel(ETC1);
This function is called with less trustworthy info, eg called with the size of a
video frame resource. I don't think we can guarantee anything there. So always
Checking here sounds right to me.

[vmpstr, 2015-06-04 20:53:00]

PTAL

https://codereview.chromium.org/1154393003/diff/20001/cc/trees/layer_tree_hos...
File cc/trees/layer_tree_host.cc (right):

https://codereview.chromium.org/1154393003/diff/20001/cc/trees/layer_tree_hos...
cc/trees/layer_tree_host.cc:936:
CHECK(Resource::VerifySizeInBytes(render_surface->content_rect().size(),
I wasn't sure about this function, so I think it's better to CHECK than not? Let
me know.

[danakj, 2015-06-04 20:58:44]

https://codereview.chromium.org/1154393003/diff/20001/cc/resources/resource.h
File cc/resources/resource.h (right):

https://codereview.chromium.org/1154393003/diff/20001/cc/resources/resource.h...
cc/resources/resource.h:26: size_t bytes() const;
we should remove this function.

https://codereview.chromium.org/1154393003/diff/20001/cc/resources/resource.h...
cc/resources/resource.h:31: if (((BitsPerPixel(format) * size.width() *
size.height()) % 8) != 0)
can you do this only if result.IsValid() and convert these to size_t's instead
of multiplying ints?

https://codereview.chromium.org/1154393003/diff/20001/cc/trees/layer_tree_hos...
File cc/trees/layer_tree_host.cc (right):

https://codereview.chromium.org/1154393003/diff/20001/cc/trees/layer_tree_hos...
cc/trees/layer_tree_host.cc:936:
CHECK(Resource::VerifySizeInBytes(render_surface->content_rect().size(),
On 2015/06/04 20:52:59, vmpstr wrote:
> I wasn't sure about this function, so I think it's better to CHECK than not?
Let
> me know.

RenderSurface content rects are bounded by the max texture size which is small.

https://codereview.chromium.org/1154393003/diff/20001/cc/trees/layer_tree_hos...
cc/trees/layer_tree_host.cc:947:
CHECK(Resource::VerifySizeInBytes(device_viewport_size_, RGBA_8888));
This is correct to check about I think it comes from outside cc and is somewhat
opaque to us. How about adding a Resource::CheckedMemorySizeBytes that does
ValueOrDie()?

[danakj, 2015-06-04 22:43:49]

https://codereview.chromium.org/1154393003/diff/60001/cc/resources/resource.h
File cc/resources/resource.h (right):

https://codereview.chromium.org/1154393003/diff/60001/cc/resources/resource.h...
cc/resources/resource.h:47: *bytes = value / 8;
Eh, you're adding this optional pointer thing just to avoid duplicating like 3
lines of code over to CheckedMemorySizeBytes?

If it's the %8 stuff you're worried about, DCHECK(VerifySizeInBytes) there too?
CheckedMemorySizeBytes should just be DCHECK(..whatever you want..) and then
computing CheckedNumeric and return ValueOrDie. Else this is so many branches
just to compute a number.

[vmpstr, 2015-06-04 23:17:30]

PTAL

https://codereview.chromium.org/1154393003/diff/60001/cc/resources/resource.h
File cc/resources/resource.h (right):

https://codereview.chromium.org/1154393003/diff/60001/cc/resources/resource.h...
cc/resources/resource.h:47: *bytes = value / 8;
On 2015/06/04 22:43:49, danakj wrote:
> Eh, you're adding this optional pointer thing just to avoid duplicating like 3
> lines of code over to CheckedMemorySizeBytes?
> 
> If it's the %8 stuff you're worried about, DCHECK(VerifySizeInBytes) there
too?
> CheckedMemorySizeBytes should just be DCHECK(..whatever you want..) and then
> computing CheckedNumeric and return ValueOrDie. Else this is so many branches
> just to compute a number.

Done.

[danakj, 2015-06-05 00:32:37]

LGTM

https://codereview.chromium.org/1154393003/diff/100001/cc/resources/resource.h
File cc/resources/resource.h (right):

https://codereview.chromium.org/1154393003/diff/100001/cc/resources/resource....
cc/resources/resource.h:55: if (size.IsEmpty())
is this doing anything useful?

[jschuh, 2015-06-05 16:24:31]

lgtm. thanks for taking care of this.

[vmpstr, 2015-06-05 19:08:00]

https://codereview.chromium.org/1154393003/diff/100001/cc/resources/resource.h
File cc/resources/resource.h (right):

https://codereview.chromium.org/1154393003/diff/100001/cc/resources/resource....
cc/resources/resource.h:55: if (size.IsEmpty())
On 2015/06/05 00:32:37, danakj wrote:
> is this doing anything useful?

Not really. Removed.

[vmpstr, 2015-06-05 19:08:05]

The CQ bit was checked by vmpstr@chromium.org

[vmpstr, 2015-06-05 19:08:06]

The patchset sent to the CQ was uploaded after l-g-t-m from danakj@chromium.org,
jschuh@chromium.org
Link to the patchset: https://codereview.chromium.org/1154393003/#ps120001
(title: " ")

[commit-bot: I haz the power, 2015-06-05 19:09:29]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1154393003/120001

[commit-bot: I haz the power, 2015-06-05 19:22:23]

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

[commit-bot: I haz the power, 2015-06-05 19:22:24]

Try jobs failed on following builders:
  android_chromium_gn_compile_dbg on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/android_chromiu...)
  linux_chromium_gn_chromeos_rel on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  linux_chromium_gn_dbg on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[vmpstr, 2015-06-05 19:32:41]

The CQ bit was checked by vmpstr@chromium.org

[vmpstr, 2015-06-05 19:32:41]

The patchset sent to the CQ was uploaded after l-g-t-m from danakj@chromium.org,
jschuh@chromium.org
Link to the patchset: https://codereview.chromium.org/1154393003/#ps140001
(title: "BUILD.gn")

[commit-bot: I haz the power, 2015-06-05 19:34:28]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1154393003/140001

[commit-bot: I haz the power, 2015-06-05 20:43:06]

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

[commit-bot: I haz the power, 2015-06-05 20:43:08]

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

[vmpstr, 2015-06-05 21:10:06]

The CQ bit was checked by vmpstr@chromium.org

[vmpstr, 2015-06-05 21:10:06]

The patchset sent to the CQ was uploaded after l-g-t-m from danakj@chromium.org,
jschuh@chromium.org
Link to the patchset: https://codereview.chromium.org/1154393003/#ps160001
(title: "initialize var")

[commit-bot: I haz the power, 2015-06-05 21:10:45]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1154393003/160001

[commit-bot: I haz the power, 2015-06-05 22:39:54]

Committed patchset #9 (id:160001)

[commit-bot: I haz the power, 2015-06-05 22:40:59]

Patchset 9 (id:??) landed as
https://crrev.com/93a24d4dffd492f6ea885fb1280b3386567ae83b
Cr-Commit-Position: refs/heads/master@{#333160}