Move SoftwareFrameData overflow checks to the IPC code.

Move SoftwareFrameData overflow checks to the IPC code.

Instead of doing this check in SoftwareFrameManager and silently
dropping the frame, if we have an overflow, drop the IPC from the
renderer (and cause a renderer crash which we can see).

Also move computation code for the frame size in bytes to
SoftwareFrameData so the computation and the check can be beside
each other.

Also add unit tests for SoftwareFrameData IPC.

R=ccameron@chromium.org, jschuh@chromium.org, piman@chromium.org, ccameron, piman
BUG=348332
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=258418

[danakj, 2014-03-19 18:51:32]

Hello, this is a followup for https://codereview.chromium.org/196283018. What do
you think?

Thanks.

[piman, 2014-03-19 18:57:17]

lgtm

https://codereview.chromium.org/196423027/diff/1/content/common/cc_messages.cc
File content/common/cc_messages.cc (right):

https://codereview.chromium.org/196423027/diff/1/content/common/cc_messages.c...
content/common/cc_messages.cc:760: WriteParam(m, p.size);
Do you want to add a DCHECK here?

[danakj, 2014-03-19 18:58:32]

https://codereview.chromium.org/196423027/diff/1/content/common/cc_messages.cc
File content/common/cc_messages.cc (right):

https://codereview.chromium.org/196423027/diff/1/content/common/cc_messages.c...
content/common/cc_messages.cc:760: WriteParam(m, p.size);
On 2014/03/19 18:57:17, piman wrote:
> Do you want to add a DCHECK here?

I did at first, but then the unit test is a death test, and I wasn't sure how to
generate a message to unit-test the read method. And the read method seems more
important, so I left out the DCHECK.

I guess in the test I could WriteParam the fields by hand instead of calling
Write() WDYT is that a sane idea or sketchy?

[danakj, 2014-03-19 19:11:16]

Here's a patchset with a DCHECK and WriteParam in the unit test WDYT? Like it
better/worse than before?

[piman, 2014-03-19 20:33:31]

LGTM. I think if the serialization changes, the unit tests will catch it and
we'll remember to change both the positive and negative cases.

[danakj, 2014-03-19 20:37:15]

ccameron, jschuh: can you review please?

[ccameron, 2014-03-19 23:34:17]

On 2014/03/19 20:37:15, danakj wrote:
> ccameron, jschuh: can you review please?

I'd vaguely prefer to have the explicit check rather than relying on the IPC
code. It's easier to verify by inspection that way.

[danakj, 2014-03-19 23:36:02]

On 2014/03/19 23:34:17, ccameron1 wrote:
> On 2014/03/19 20:37:15, danakj wrote:
> > ccameron, jschuh: can you review please?
> 
> I'd vaguely prefer to have the explicit check rather than relying on the IPC
> code. It's easier to verify by inspection that way.

It's also very late, if we crash there it's a much bigger pain to figure out
what renderer it was from, what frame, what state it was in. If we DCHECK in the
renderer process, we can see it immediately there. And for official builds we're
safe because we drop the frame in the browser.

It also means a compromised renderer does not have the ability to crash the
browser process (even in a safe way). Instead it would just crash itself.

[jschuh, 2014-03-20 16:33:04]

On 2014/03/19 23:36:02, danakj wrote:
> On 2014/03/19 23:34:17, ccameron1 wrote:
> > On 2014/03/19 20:37:15, danakj wrote:
> > > ccameron, jschuh: can you review please?
> > 
> > I'd vaguely prefer to have the explicit check rather than relying on the IPC
> > code. It's easier to verify by inspection that way.
> 
> It's also very late, if we crash there it's a much bigger pain to figure out
> what renderer it was from, what frame, what state it was in. If we DCHECK in
the
> renderer process, we can see it immediately there. And for official builds
we're
> safe because we drop the frame in the browser.
> 
> It also means a compromised renderer does not have the ability to crash the
> browser process (even in a safe way). Instead it would just crash itself.

Seems reasonable for now, and we should merge this to m34. I still want to
investigate changes to the gfx templates, because I think we can address the
whole class of issues there in a performant way (), but lgtm on this.

[ccameron, 2014-03-20 17:36:01]

On 2014/03/19 23:36:02, danakj wrote:
> On 2014/03/19 23:34:17, ccameron1 wrote:
> > On 2014/03/19 20:37:15, danakj wrote:
> > > ccameron, jschuh: can you review please?
> > 
> > I'd vaguely prefer to have the explicit check rather than relying on the IPC
> > code. It's easier to verify by inspection that way.
> 
> It's also very late, if we crash there it's a much bigger pain to figure out
> what renderer it was from, what frame, what state it was in. If we DCHECK in
the
> renderer process, we can see it immediately there. And for official builds
we're
> safe because we drop the frame in the browser.
> 
> It also means a compromised renderer does not have the ability to crash the
> browser process (even in a safe way). Instead it would just crash itself.

That makes sense. lgtm as well.

[danakj, 2014-03-20 17:36:52]

Cool, thanks!

[danakj, 2014-03-20 17:36:55]

The CQ bit was checked by danakj@chromium.org

[commit-bot: I haz the power, 2014-03-20 17:39:49]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/danakj@chromium.org/196423027/20001

[commit-bot: I haz the power, 2014-03-20 19:54:54]

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

[commit-bot: I haz the power, 2014-03-20 19:54:54]

Retried try job too often on win_rel for step(s) content_unittests
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win_rel&nu...

[danakj, 2014-03-20 19:58:15]

The CQ bit was checked by danakj@chromium.org

[commit-bot: I haz the power, 2014-03-20 20:00:28]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/danakj@chromium.org/196423027/20001

[danakj, 2014-03-20 21:43:44]

Committed patchset #2 manually as r258418 (presubmit successful).