Check for invalid element id when deserializing CompositorProxyTag

Check for invalid element id when deserializing CompositorProxyTag

The element id is created from WeakIdentifierMap::next() and should
never be '0'.

BUG=699484
Review-Url: https://codereview.chromium.org/2742713004
Cr-Commit-Position: refs/heads/master@{#455901}
Committed: https://chromium.googlesource.com/chromium/src/+/e5e975b33027b19dc96bfbab42f09c9c7839c63e

[smcgruer, 2017-03-09 16:37:58]

smcgruer@chromium.org changed reviewers:
+ jbroman@chromium.org

[haraken, 2017-03-09 17:35:41]

LGTM

[jbroman, 2017-03-09 18:13:28]

jbroman@chromium.org changed reviewers:
+ haraken@chromium.org

[jbroman, 2017-03-09 18:13:28]

Sure, LGTM.

Ultimately (cf. before shipping) we should probably actually make whether
compositor proxies are accepted configurable, as they are inappropriate for many
uses of serialization. (e.g. writing a compositor proxy to IndexedDB and reading
it out after Chrome restarts will surely be garbage, as will sending it to
another process).

In that world these IDs won't be able to come from untrusted code. (But most of
the serialization stuff does have to be able to handle the data being
untrusted.)

[smcgruer, 2017-03-09 18:49:42]

Added a unittest, PTAL.

[jbroman, 2017-03-09 19:06:15]

lgtm

[smcgruer, 2017-03-09 19:14:08]

The CQ bit was checked by smcgruer@chromium.org

[smcgruer, 2017-03-09 19:14:09]

The patchset sent to the CQ was uploaded after l-g-t-m from haraken@chromium.org
Link to the patchset: https://codereview.chromium.org/2742713004/#ps20001
(title: "Add unittest")

[commit-bot: I haz the power, 2017-03-09 19:14:33]

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, 2017-03-09 20:29:57]

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

[commit-bot: I haz the power, 2017-03-09 20:29:58]

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

[smcgruer, 2017-03-09 20:32:59]

On 2017/03/09 20:29:58, commit-bot: I haz the power wrote:
> Try jobs failed on following builders:
>   linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
>
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

I am not smart. I just setup code in the unittest that directly triggers the
original failure from issue 699484 - creating a CompositorProxy with element_id
= 0. Whoopsie. Will fix.

[jbroman, 2017-03-09 21:36:16]

still lgtm (bots-willing)

https://codereview.chromium.org/2742713004/diff/40001/third_party/WebKit/Sour...
File
third_party/WebKit/Source/bindings/core/v8/serialization/V8ScriptValueSerializerTest.cpp
(right):

https://codereview.chromium.org/2742713004/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/bindings/core/v8/serialization/V8ScriptValueSerializerTest.cpp:1046:
{0xff, 0x04, 0x3f, 0x00, 0x43, elementId, mutableProperties, 0xff});
Deserializer accepts this, but nit:
- technically CompositorProxy didn't exist in version 0x04. You could use 0x09,
though it happens not to be checked for CompositorProxy.
- trailing padding is usually 0x00, not 0xff

[smcgruer, 2017-03-09 21:43:16]

https://codereview.chromium.org/2742713004/diff/40001/third_party/WebKit/Sour...
File
third_party/WebKit/Source/bindings/core/v8/serialization/V8ScriptValueSerializerTest.cpp
(right):

https://codereview.chromium.org/2742713004/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/bindings/core/v8/serialization/V8ScriptValueSerializerTest.cpp:1046:
{0xff, 0x04, 0x3f, 0x00, 0x43, elementId, mutableProperties, 0xff});
On 2017/03/09 21:36:16, jbroman wrote:
> Deserializer accepts this, but nit:
> - technically CompositorProxy didn't exist in version 0x04. You could use
0x09,
> though it happens not to be checked for CompositorProxy.
> - trailing padding is usually 0x00, not 0xff

Done.

[smcgruer, 2017-03-09 21:43:44]

The CQ bit was checked by smcgruer@chromium.org

[smcgruer, 2017-03-09 21:43:44]

The patchset sent to the CQ was uploaded after l-g-t-m from
haraken@chromium.org, jbroman@chromium.org
Link to the patchset: https://codereview.chromium.org/2742713004/#ps60001
(title: "Address nits")

[commit-bot: I haz the power, 2017-03-09 21:44:12]

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, 2017-03-09 23:44:18]

CQ is committing da patch.

Bot data: {"patchset_id": 60001, "attempt_start_ts": 1489095824107920,
"parent_rev": "d41491179d6dd91eeca40fc743889eafe98a7050", "commit_rev":
"e5e975b33027b19dc96bfbab42f09c9c7839c63e"}

[commit-bot: I haz the power, 2017-03-09 23:45:04]

Description was changed from

==========
Check for invalid element id when deserializing CompositorProxyTag

The element id is created from WeakIdentifierMap::next() and should
never be '0'.

BUG=699484
==========

to

==========
Check for invalid element id when deserializing CompositorProxyTag

The element id is created from WeakIdentifierMap::next() and should
never be '0'.

BUG=699484

Review-Url: https://codereview.chromium.org/2742713004
Cr-Commit-Position: refs/heads/master@{#455901}
Committed:
https://chromium.googlesource.com/chromium/src/+/e5e975b33027b19dc96bfbab42f0...
==========

[commit-bot: I haz the power, 2017-03-09 23:45:05]

Committed patchset #4 (id:60001) as
https://chromium.googlesource.com/chromium/src/+/e5e975b33027b19dc96bfbab42f0...