Replaced cc::SurfaceId::nonce_ with base::UnguessableToken

Replaced cc::SurfaceId::nonce_ with base::UnguessableToken

base::SurfaceId::nonce_ is an uint64 generated at random to make the entire
surface id unguessable.
base::UnguessableToken is a wrapper for 2 uint64 (high_ and low_).
high_ and low_ are generated at random using base::UnguessableToken::Create().
base::UnguessableToken achieves the same goal as the original
cc::SurfaceId::nonce_ with better security and uses more common code.

BUG=649800
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_precise_blink_rel

Committed: https://crrev.com/6b01e58ff708c17f74b560fe1b57f7733b78da53
Committed: https://crrev.com/7d7f6ebb02ae3b9e354406ce2670399a241baeca
Cr-Original-Commit-Position: refs/heads/master@{#431294}
Cr-Commit-Position: refs/heads/master@{#431398}

[Alex Z., 2016-09-30 19:55:08]

Description was changed from

==========
Replaced cc::SurfaceId::nonce_ with base::UnguessableToken

base::SurfaceId::nonce_ is an uint64 generated at random to make the entire
surface id unguessable.
base::NonguessableToken is a wrapper for 2 uint64 (high_ and low_).
high_ and low_ are generated at random using base::NonguessableToken::Create().
base::NonguessableToken achieves the same goal as the original
cc::SurfaceId::nonce_ with better security and uses more common code.

BUG=649800
==========

to

==========
Replaced cc::SurfaceId::nonce_ with base::UnguessableToken

base::SurfaceId::nonce_ is an uint64 generated at random to make the entire
surface id unguessable.
base::NonguessableToken is a wrapper for 2 uint64 (high_ and low_).
high_ and low_ are generated at random using base::NonguessableToken::Create().
base::NonguessableToken achieves the same goal as the original
cc::SurfaceId::nonce_ with better security and uses more common code.

BUG=649800
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_precise_blink_rel
==========

[Alex Z., 2016-09-30 20:04:01]

staraz@chromium.org changed reviewers:
+ danakj@chromium.org, sky@chromium.org, tsepez@chromium.org

[Alex Z., 2016-09-30 20:04:02]

danakj@chromium.org: Please review changes in cc code.

sky@chromium.org: Please review changes in WebKit code.

tsepez@: Please review changes in ipc code.

[Tom Sepez, 2016-09-30 20:23:24]

LGTM.

[Fady Samuel, 2016-09-30 20:30:40]

fsamuel@chromium.org changed reviewers:
+ fsamuel@chromium.org

[Fady Samuel, 2016-09-30 20:30:41]

Drive by comments.

https://codereview.chromium.org/2379653006/diff/1/cc/ipc/surface_id_struct_tr...
File cc/ipc/surface_id_struct_traits.h (right):

https://codereview.chromium.org/2379653006/diff/1/cc/ipc/surface_id_struct_tr...
cc/ipc/surface_id_struct_traits.h:20: static base::UnguessableToken nonce(const
cc::SurfaceId& id) {
const base::UnguessableToken&

https://codereview.chromium.org/2379653006/diff/1/cc/surfaces/surface_id.h
File cc/surfaces/surface_id.h (right):

https://codereview.chromium.org/2379653006/diff/1/cc/surfaces/surface_id.h#ne...
cc/surfaces/surface_id.h:22: SurfaceId() : client_id_(0), local_id_(0), nonce_()
{}
no need for calling the default constructor for nonce.

https://codereview.chromium.org/2379653006/diff/1/cc/surfaces/surface_id.h#ne...
cc/surfaces/surface_id.h:47: return client_id_ == 0 && nonce_ ==
base::UnguessableToken() &&
nonce_.is_empty()

https://codereview.chromium.org/2379653006/diff/1/cc/surfaces/surface_id.h#ne...
cc/surfaces/surface_id.h:61: base::UnguessableToken nonce() const { return
nonce_; }
const base::UnguessableToken&

[danakj, 2016-09-30 20:45:12]

https://codereview.chromium.org/2379653006/diff/1/cc/surfaces/surface_id.h
File cc/surfaces/surface_id.h (right):

https://codereview.chromium.org/2379653006/diff/1/cc/surfaces/surface_id.h#ne...
cc/surfaces/surface_id.h:43: nonce_ = base::UnguessableToken::Create();
do this as a constructor initializer

https://codereview.chromium.org/2379653006/diff/1/cc/surfaces/surface_id.h#ne...
cc/surfaces/surface_id.h:53: return
base::HashInts(static_cast<uint64_t>(interim),
drop the static cast

https://codereview.chromium.org/2379653006/diff/1/cc/surfaces/surface_id.h#ne...
cc/surfaces/surface_id.h:65: .append(nonce_.ToString());
you need a space between fields tho. i'd rather you %s in the format and pass
ToString().c_str() to StringPrintf TBH

[sky, 2016-10-03 15:50:59]

I'm not a good reviewer for webkit changes. Please find a more local reviewer.

[sky, 2016-10-03 15:51:09]

Description was changed from

==========
Replaced cc::SurfaceId::nonce_ with base::UnguessableToken

base::SurfaceId::nonce_ is an uint64 generated at random to make the entire
surface id unguessable.
base::NonguessableToken is a wrapper for 2 uint64 (high_ and low_).
high_ and low_ are generated at random using base::NonguessableToken::Create().
base::NonguessableToken achieves the same goal as the original
cc::SurfaceId::nonce_ with better security and uses more common code.

BUG=649800
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_precise_blink_rel
==========

to

==========
Replaced cc::SurfaceId::nonce_ with base::UnguessableToken

base::SurfaceId::nonce_ is an uint64 generated at random to make the entire
surface id unguessable.
base::NonguessableToken is a wrapper for 2 uint64 (high_ and low_).
high_ and low_ are generated at random using base::NonguessableToken::Create().
base::NonguessableToken achieves the same goal as the original
cc::SurfaceId::nonce_ with better security and uses more common code.

BUG=649800
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_precise_blink_rel
==========

[sky, 2016-10-03 15:51:10]

sky@chromium.org changed reviewers:
- sky@chromium.org

[Alex Z., 2016-10-05 18:10:46]

https://codereview.chromium.org/2379653006/diff/1/cc/ipc/surface_id_struct_tr...
File cc/ipc/surface_id_struct_traits.h (right):

https://codereview.chromium.org/2379653006/diff/1/cc/ipc/surface_id_struct_tr...
cc/ipc/surface_id_struct_traits.h:20: static base::UnguessableToken nonce(const
cc::SurfaceId& id) {
On 2016/09/30 20:30:41, Fady Samuel wrote:
> const base::UnguessableToken&

Done.

https://codereview.chromium.org/2379653006/diff/1/cc/surfaces/surface_id.h
File cc/surfaces/surface_id.h (right):

https://codereview.chromium.org/2379653006/diff/1/cc/surfaces/surface_id.h#ne...
cc/surfaces/surface_id.h:22: SurfaceId() : client_id_(0), local_id_(0), nonce_()
{}
On 2016/09/30 20:30:41, Fady Samuel wrote:
> no need for calling the default constructor for nonce.

Done.

https://codereview.chromium.org/2379653006/diff/1/cc/surfaces/surface_id.h#ne...
cc/surfaces/surface_id.h:43: nonce_ = base::UnguessableToken::Create();
On 2016/09/30 20:45:12, danakj wrote:
> do this as a constructor initializer

Done.

https://codereview.chromium.org/2379653006/diff/1/cc/surfaces/surface_id.h#ne...
cc/surfaces/surface_id.h:47: return client_id_ == 0 && nonce_ ==
base::UnguessableToken() &&
On 2016/09/30 20:30:41, Fady Samuel wrote:
> nonce_.is_empty()

Done.

https://codereview.chromium.org/2379653006/diff/1/cc/surfaces/surface_id.h#ne...
cc/surfaces/surface_id.h:53: return
base::HashInts(static_cast<uint64_t>(interim),
On 2016/09/30 20:45:12, danakj wrote:
> drop the static cast

Done.

https://codereview.chromium.org/2379653006/diff/1/cc/surfaces/surface_id.h#ne...
cc/surfaces/surface_id.h:61: base::UnguessableToken nonce() const { return
nonce_; }
On 2016/09/30 20:30:41, Fady Samuel wrote:
> const base::UnguessableToken&

Done.

https://codereview.chromium.org/2379653006/diff/1/cc/surfaces/surface_id.h#ne...
cc/surfaces/surface_id.h:65: .append(nonce_.ToString());
On 2016/09/30 20:45:11, danakj wrote:
> you need a space between fields tho. i'd rather you %s in the format and pass
> ToString().c_str() to StringPrintf TBH

Done.

[Alex Z., 2016-10-05 18:17:47]

staraz@chromium.org changed reviewers:
+ jam@chromium.org

[Alex Z., 2016-10-05 18:17:48]

jam@chromium.org: Please review changes in WebKit

[Fady Samuel, 2016-10-05 20:30:48]

please hold off on this patch for another day. I'm wrapping up a change that
touches a lot of files and I'd like to land it sooner rather than later to
minimize time spent rebasing: https://codereview.chromium.org/2388753003/

[jam, 2016-10-05 22:55:55]

On 2016/10/05 18:17:48, StarAZ1 wrote:
> mailto:jam@chromium.org: Please review changes in WebKit

please pick an owner from webkit/

[jam, 2016-10-05 22:56:04]

jam@chromium.org changed reviewers:
- jam@chromium.org

[haraken, 2016-10-06 00:31:46]

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

[haraken, 2016-10-06 00:31:47]

bindings/ LGTM

jbroman@: We'll need to make the same change to the new serializer.

[Fady Samuel, 2016-10-06 02:42:40]

Please rebase now. I'll L-G-T-M it once the rebase is complete. Thanks!

[jbroman, 2016-10-06 02:45:17]

On 2016/10/06 at 00:31:47, haraken wrote:
> bindings/ LGTM
> 
> jbroman@: We'll need to make the same change to the new serializer.

This CL does make the corresponding updates to the new serializer, so it looks
good to me.

[Alex Z., 2016-10-07 14:12:58]

On 2016/10/06 02:42:40, Fady Samuel wrote:
> Please rebase now. I'll L-G-T-M it once the rebase is complete. Thanks!

Rebase finished. PTAL.

[Fady Samuel, 2016-10-07 14:18:11]

https://codereview.chromium.org/2379653006/diff/100001/cc/ipc/local_frame_id....
File cc/ipc/local_frame_id.mojom (left):

https://codereview.chromium.org/2379653006/diff/100001/cc/ipc/local_frame_id....
cc/ipc/local_frame_id.mojom:12: // A cryptographically secure random int chosen
to make the LocalFrameId
nit: This comment seems redundant now.

https://codereview.chromium.org/2379653006/diff/100001/cc/ipc/local_frame_id_...
File cc/ipc/local_frame_id_struct_traits.h (right):

https://codereview.chromium.org/2379653006/diff/100001/cc/ipc/local_frame_id_...
cc/ipc/local_frame_id_struct_traits.h:28: if (!data.ReadNonce(&nonce)) {
nit: drop the braces.

https://codereview.chromium.org/2379653006/diff/100001/cc/surfaces/local_fram...
File cc/surfaces/local_frame_id.h (right):

https://codereview.chromium.org/2379653006/diff/100001/cc/surfaces/local_fram...
cc/surfaces/local_frame_id.h:24: explicit LocalFrameId(uint32_t local_id)
Do we need this constructor? Can we simply create the unguessabletoken in
SurfaceIdAllocator and pass it in like we did before.

[Alex Z., 2016-10-07 14:27:50]

https://codereview.chromium.org/2379653006/diff/100001/cc/ipc/local_frame_id....
File cc/ipc/local_frame_id.mojom (left):

https://codereview.chromium.org/2379653006/diff/100001/cc/ipc/local_frame_id....
cc/ipc/local_frame_id.mojom:12: // A cryptographically secure random int chosen
to make the LocalFrameId
On 2016/10/07 14:18:11, Fady Samuel wrote:
> nit: This comment seems redundant now.

Done.

https://codereview.chromium.org/2379653006/diff/100001/cc/ipc/local_frame_id_...
File cc/ipc/local_frame_id_struct_traits.h (right):

https://codereview.chromium.org/2379653006/diff/100001/cc/ipc/local_frame_id_...
cc/ipc/local_frame_id_struct_traits.h:28: if (!data.ReadNonce(&nonce)) {
On 2016/10/07 14:18:11, Fady Samuel wrote:
> nit: drop the braces.

Done.

https://codereview.chromium.org/2379653006/diff/100001/cc/surfaces/local_fram...
File cc/surfaces/local_frame_id.h (right):

https://codereview.chromium.org/2379653006/diff/100001/cc/surfaces/local_fram...
cc/surfaces/local_frame_id.h:24: explicit LocalFrameId(uint32_t local_id)
On 2016/10/07 14:18:11, Fady Samuel wrote:
> Do we need this constructor? Can we simply create the unguessabletoken in
> SurfaceIdAllocator and pass it in like we did before.

Done.

[Fady Samuel, 2016-10-07 14:29:36]

lgtm

[vmpstr, 2016-10-11 23:39:35]

vmpstr@chromium.org changed reviewers:
+ vmpstr@chromium.org

[vmpstr, 2016-10-11 23:39:36]

cc lgtm

[Fady Samuel, 2016-10-11 23:42:34]

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

[commit-bot: I haz the power, 2016-10-11 23:43:17]

Dry run: 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-10-12 00:12:22]

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

[commit-bot: I haz the power, 2016-10-12 00:12:23]

Dry run: Try jobs failed on following builders:
  android_compile_dbg on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_comp...)
  cast_shell_linux on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/cast_shell_linu...)

[Alex Z., 2016-10-12 18:32:42]

The CQ bit was checked by staraz@chromium.org

[Alex Z., 2016-10-12 18:32:42]

The patchset sent to the CQ was uploaded after l-g-t-m from tsepez@chromium.org,
haraken@chromium.org, vmpstr@chromium.org, fsamuel@chromium.org
Link to the patchset: https://codereview.chromium.org/2379653006/#ps140001
(title: "Changed unit tests to use base::UnguessableToken as
LocalFrameId::nonce_")

[commit-bot: I haz the power, 2016-10-12 18:33:19]

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-10-12 18:47:24]

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

[commit-bot: I haz the power, 2016-10-12 18:47:26]

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

[Alex Z., 2016-10-12 18:55:01]

staraz@chromium.org changed reviewers:
+ sky@chromium.org

[Alex Z., 2016-10-12 18:55:02]

sky@: Please review my changes in cc/ipc/DEPS (added dependency to mojo/common)

[sky, 2016-10-12 19:31:52]

DEPS LGTM

[Alex Z., 2016-10-12 19:33:37]

The CQ bit was checked by staraz@chromium.org

[commit-bot: I haz the power, 2016-10-12 19:34:01]

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

[Fady Samuel, 2016-10-12 22:39:48]

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

[commit-bot: I haz the power, 2016-10-12 22:40:19]

Dry run: 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-10-12 23:07:49]

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

[commit-bot: I haz the power, 2016-10-12 23:07:50]

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

[Alex Z., 2016-10-14 15:09:10]

The CQ bit was checked by staraz@chromium.org

[Alex Z., 2016-10-14 15:09:11]

The patchset sent to the CQ was uploaded after l-g-t-m from vmpstr@chromium.org,
tsepez@chromium.org, haraken@chromium.org, fsamuel@chromium.org,
sky@chromium.org
Link to the patchset: https://codereview.chromium.org/2379653006/#ps180001
(title: "Chagned SurfaceAggregator pert tests to use base::UnguessableToken")

[commit-bot: I haz the power, 2016-10-14 15:09:29]

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-10-14 15:38:55]

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

[commit-bot: I haz the power, 2016-10-14 15:38:57]

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

[Fady Samuel, 2016-10-18 03:56:09]

https://codereview.chromium.org/2379653006/diff/200001/cc/surfaces/surface_ag...
File cc/surfaces/surface_aggregator_perftest.cc (right):

https://codereview.chromium.org/2379653006/diff/200001/cc/surfaces/surface_ag...
cc/surfaces/surface_aggregator_perftest.cc:54: LocalFrameId local_frame_id(i,
base::UnguessableToken());
Let's avoid using the default constructor.

[Alex Z., 2016-10-27 17:56:21]

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

[commit-bot: I haz the power, 2016-10-27 17:56:50]

Dry run: 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-10-27 18:23:16]

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

[commit-bot: I haz the power, 2016-10-27 18:23:17]

Dry run: Try jobs failed on following builders:
  win_chromium_rel_ng on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)
  win_chromium_x64_rel_ng on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)

[Alex Z., 2016-10-28 13:47:46]

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

[commit-bot: I haz the power, 2016-10-28 13:48:02]

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

[Alex Z., 2016-10-28 17:38:59]

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

[commit-bot: I haz the power, 2016-10-28 17:39:23]

Dry run: 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-10-28 18:28:07]

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

[commit-bot: I haz the power, 2016-10-28 18:28:09]

Dry run: Try jobs failed on following builders:
  win_chromium_compile_dbg_ng on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_comp...)

[Alex Z., 2016-10-28 19:06:51]

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

[commit-bot: I haz the power, 2016-10-28 19:07:25]

Dry run: 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-10-28 20:22:51]

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

[commit-bot: I haz the power, 2016-10-28 20:22:53]

Dry run: Try jobs failed on following builders:
  win_chromium_x64_rel_ng on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)

[Alex Z., 2016-10-31 14:20:33]

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

[commit-bot: I haz the power, 2016-10-31 14:20:46]

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

[Alex Z., 2016-10-31 15:29:12]

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

[commit-bot: I haz the power, 2016-10-31 15:29:29]

Dry run: 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-10-31 17:45:42]

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

[commit-bot: I haz the power, 2016-10-31 17:45:44]

Dry run: This issue passed the CQ dry run.

[Alex Z., 2016-11-02 17:10:20]

https://codereview.chromium.org/2379653006/diff/200001/cc/surfaces/surface_ag...
File cc/surfaces/surface_aggregator_perftest.cc (right):

https://codereview.chromium.org/2379653006/diff/200001/cc/surfaces/surface_ag...
cc/surfaces/surface_aggregator_perftest.cc:54: LocalFrameId local_frame_id(i,
base::UnguessableToken());
On 2016/10/18 03:56:09, Fady Samuel wrote:
> Let's avoid using the default constructor.

Done.

[Fady Samuel, 2016-11-09 21:25:37]

Now that your is_valid() patch has landed, you'll likely need to rebase this
patch. Thanks! :-)

[Fady Samuel, 2016-11-09 21:41:57]

https://codereview.chromium.org/2379653006/diff/420001/cc/surfaces/surface_id.h
File cc/surfaces/surface_id.h (right):

https://codereview.chromium.org/2379653006/diff/420001/cc/surfaces/surface_id...
cc/surfaces/surface_id.h:29: // A SurfaceId consists of two components:
FrameSinkId and local_frame_id.
nit: FrameSinkId and LocalFrameId.

[Alex Z., 2016-11-09 21:59:16]

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

[commit-bot: I haz the power, 2016-11-09 21:59:51]

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

[Alex Z., 2016-11-09 22:04:36]

staraz@chromium.org changed reviewers:
+ dcheng@chromium.org

[Alex Z., 2016-11-09 22:04:38]

dcheng@: Please review my changes in base/unguessable_token.h

[Alex Z., 2016-11-09 22:05:21]

https://codereview.chromium.org/2379653006/diff/420001/cc/surfaces/surface_id.h
File cc/surfaces/surface_id.h (right):

https://codereview.chromium.org/2379653006/diff/420001/cc/surfaces/surface_id...
cc/surfaces/surface_id.h:29: // A SurfaceId consists of two components:
FrameSinkId and local_frame_id.
On 2016/11/09 21:41:57, Fady Samuel wrote:
> nit: FrameSinkId and LocalFrameId.

Done.

[Fady Samuel, 2016-11-09 22:07:03]

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

[commit-bot: I haz the power, 2016-11-09 22:07:46]

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

[dcheng, 2016-11-09 22:10:49]

base LGTM

There are some typos in the CL description (NonguessableToken ->
UnguessableToken), and I'm surprised that the bots don't complain about static
initializers (unless they're ignoring tests? But even if they are, the style
guide prohibits this AFAIK)

https://codereview.chromium.org/2379653006/diff/460001/cc/layers/surface_laye...
File cc/layers/surface_layer_impl_unittest.cc (right):

https://codereview.chromium.org/2379653006/diff/460001/cc/layers/surface_laye...
cc/layers/surface_layer_impl_unittest.cc:18: base::UnguessableToken::Create());
I'm surprised that clang doesn't complain here.

https://codereview.chromium.org/2379653006/diff/460001/cc/layers/surface_laye...
File cc/layers/surface_layer_unittest.cc (right):

https://codereview.chromium.org/2379653006/diff/460001/cc/layers/surface_laye...
cc/layers/surface_layer_unittest.cc:33: base::UnguessableToken::Create();
And here, etc.

[danakj, 2016-11-10 00:01:37]

On 2016/11/09 22:10:49, dcheng wrote:
> base LGTM

base LGTM also.

> There are some typos in the CL description (NonguessableToken ->
> UnguessableToken), and I'm surprised that the bots don't complain about static
> initializers (unless they're ignoring tests? But even if they are, the style
> guide prohibits this AFAIK)

If they are constexpr it will be okay, but tests are explicitly ignored for
static init, I think. I recall that I've seen them before.

>
https://codereview.chromium.org/2379653006/diff/460001/cc/layers/surface_laye...
> File cc/layers/surface_layer_impl_unittest.cc (right):
> 
>
https://codereview.chromium.org/2379653006/diff/460001/cc/layers/surface_laye...
> cc/layers/surface_layer_impl_unittest.cc:18:
base::UnguessableToken::Create());
> I'm surprised that clang doesn't complain here.
> 
>
https://codereview.chromium.org/2379653006/diff/460001/cc/layers/surface_laye...
> File cc/layers/surface_layer_unittest.cc (right):
> 
>
https://codereview.chromium.org/2379653006/diff/460001/cc/layers/surface_laye...
> cc/layers/surface_layer_unittest.cc:33: base::UnguessableToken::Create();
> And here, etc.

[commit-bot: I haz the power, 2016-11-10 00:15:51]

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

[commit-bot: I haz the power, 2016-11-10 00:15:52]

Dry run: This issue passed the CQ dry run.

[Alex Z., 2016-11-10 15:30:25]

Description was changed from

==========
Replaced cc::SurfaceId::nonce_ with base::UnguessableToken

base::SurfaceId::nonce_ is an uint64 generated at random to make the entire
surface id unguessable.
base::NonguessableToken is a wrapper for 2 uint64 (high_ and low_).
high_ and low_ are generated at random using base::NonguessableToken::Create().
base::NonguessableToken achieves the same goal as the original
cc::SurfaceId::nonce_ with better security and uses more common code.

BUG=649800
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_precise_blink_rel
==========

to

==========
Replaced cc::SurfaceId::nonce_ with base::UnguessableToken

base::SurfaceId::nonce_ is an uint64 generated at random to make the entire
surface id unguessable.
base::UnguessableToken is a wrapper for 2 uint64 (high_ and low_).
high_ and low_ are generated at random using base::UnguessableToken::Create().
base::UnguessableToken achieves the same goal as the original
cc::SurfaceId::nonce_ with better security and uses more common code.

BUG=649800
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_precise_blink_rel
==========

[Alex Z., 2016-11-10 15:54:20]

The CQ bit was checked by staraz@chromium.org

[Alex Z., 2016-11-10 15:54:21]

The patchset sent to the CQ was uploaded after l-g-t-m from vmpstr@chromium.org,
tsepez@chromium.org, haraken@chromium.org, fsamuel@chromium.org,
sky@chromium.org, dcheng@chromium.org, danakj@chromium.org
Link to the patchset: https://codereview.chromium.org/2379653006/#ps480001
(title: "Localized kArbitraryLocalFrameId")

[commit-bot: I haz the power, 2016-11-10 15:54:41]

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

[Alex Z., 2016-11-10 15:57:29]

On 2016/11/09 22:10:49, dcheng wrote:
> base LGTM
> 
> There are some typos in the CL description (NonguessableToken ->
> UnguessableToken), and I'm surprised that the bots don't complain about static
> initializers (unless they're ignoring tests? But even if they are, the style
> guide prohibits this AFAIK)
> 
>
https://codereview.chromium.org/2379653006/diff/460001/cc/layers/surface_laye...
> File cc/layers/surface_layer_impl_unittest.cc (right):
> 
>
https://codereview.chromium.org/2379653006/diff/460001/cc/layers/surface_laye...
> cc/layers/surface_layer_impl_unittest.cc:18:
base::UnguessableToken::Create());
> I'm surprised that clang doesn't complain here.
> 
>
https://codereview.chromium.org/2379653006/diff/460001/cc/layers/surface_laye...
> File cc/layers/surface_layer_unittest.cc (right):
> 
>
https://codereview.chromium.org/2379653006/diff/460001/cc/layers/surface_laye...
> cc/layers/surface_layer_unittest.cc:33: base::UnguessableToken::Create();
> And here, etc.

I moved the kArbitraryLocalFrameIds into tests so they are now local consts.

[Alex Z., 2016-11-10 15:57:43]

https://codereview.chromium.org/2379653006/diff/460001/cc/layers/surface_laye...
File cc/layers/surface_layer_impl_unittest.cc (right):

https://codereview.chromium.org/2379653006/diff/460001/cc/layers/surface_laye...
cc/layers/surface_layer_impl_unittest.cc:18: base::UnguessableToken::Create());
On 2016/11/09 22:10:48, dcheng wrote:
> I'm surprised that clang doesn't complain here.

Done.

https://codereview.chromium.org/2379653006/diff/460001/cc/layers/surface_laye...
File cc/layers/surface_layer_unittest.cc (right):

https://codereview.chromium.org/2379653006/diff/460001/cc/layers/surface_laye...
cc/layers/surface_layer_unittest.cc:33: base::UnguessableToken::Create();
On 2016/11/09 22:10:48, dcheng wrote:
> And here, etc.

Done.

[commit-bot: I haz the power, 2016-11-10 17:23:45]

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

[commit-bot: I haz the power, 2016-11-10 17:23:47]

Try jobs failed on following builders:
  android_n5x_swarming_rel on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_n5x_...)

[Alex Z., 2016-11-10 17:41:21]

The CQ bit was checked by staraz@chromium.org

[commit-bot: I haz the power, 2016-11-10 17:41:49]

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-11-10 18:19:38]

Description was changed from

==========
Replaced cc::SurfaceId::nonce_ with base::UnguessableToken

base::SurfaceId::nonce_ is an uint64 generated at random to make the entire
surface id unguessable.
base::UnguessableToken is a wrapper for 2 uint64 (high_ and low_).
high_ and low_ are generated at random using base::UnguessableToken::Create().
base::UnguessableToken achieves the same goal as the original
cc::SurfaceId::nonce_ with better security and uses more common code.

BUG=649800
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_precise_blink_rel
==========

to

==========
Replaced cc::SurfaceId::nonce_ with base::UnguessableToken

base::SurfaceId::nonce_ is an uint64 generated at random to make the entire
surface id unguessable.
base::UnguessableToken is a wrapper for 2 uint64 (high_ and low_).
high_ and low_ are generated at random using base::UnguessableToken::Create().
base::UnguessableToken achieves the same goal as the original
cc::SurfaceId::nonce_ with better security and uses more common code.

BUG=649800
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_precise_blink_rel
==========

[commit-bot: I haz the power, 2016-11-10 18:19:41]

Committed patchset #25 (id:480001)

[commit-bot: I haz the power, 2016-11-10 18:38:46]

Description was changed from

==========
Replaced cc::SurfaceId::nonce_ with base::UnguessableToken

base::SurfaceId::nonce_ is an uint64 generated at random to make the entire
surface id unguessable.
base::UnguessableToken is a wrapper for 2 uint64 (high_ and low_).
high_ and low_ are generated at random using base::UnguessableToken::Create().
base::UnguessableToken achieves the same goal as the original
cc::SurfaceId::nonce_ with better security and uses more common code.

BUG=649800
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_precise_blink_rel
==========

to

==========
Replaced cc::SurfaceId::nonce_ with base::UnguessableToken

base::SurfaceId::nonce_ is an uint64 generated at random to make the entire
surface id unguessable.
base::UnguessableToken is a wrapper for 2 uint64 (high_ and low_).
high_ and low_ are generated at random using base::UnguessableToken::Create().
base::UnguessableToken achieves the same goal as the original
cc::SurfaceId::nonce_ with better security and uses more common code.

BUG=649800
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_precise_blink_rel

Committed: https://crrev.com/6b01e58ff708c17f74b560fe1b57f7733b78da53
Cr-Commit-Position: refs/heads/master@{#431294}
==========

[commit-bot: I haz the power, 2016-11-10 18:38:48]

Patchset 25 (id:??) landed as
https://crrev.com/6b01e58ff708c17f74b560fe1b57f7733b78da53
Cr-Commit-Position: refs/heads/master@{#431294}

[Dirk Pranke, 2016-11-10 20:03:02]

A revert of this CL (patchset #25 id:480001) has been created in
https://codereview.chromium.org/2494833002/ by dpranke@chromium.org.

The reason for reverting is: Looks like this added a static initializer to
surface_manager.cc via the change to unguessable_token

https://build.chromium.org/p/chromium/builders/Linux%20x64/builds/28400/steps....

[Fady Samuel, 2016-11-10 20:40:25]

On 2016/11/10 20:03:02, Dirk Pranke wrote:
> A revert of this CL (patchset #25 id:480001) has been created in
> https://codereview.chromium.org/2494833002/ by mailto:dpranke@chromium.org.
> 
> The reason for reverting is: Looks like this added a static initializer to
> surface_manager.cc via the change to unguessable_token
> 
>
https://build.chromium.org/p/chromium/builders/Linux%20x64/builds/28400/steps....

Chatted with staraz@ and kylechar@ offline. We definitely want root surface IDs
to be unguessable so that clients cannot pretend to be the root and add and
remove references from the root.

We can hang the root surface ID off SurfaceManager instead of making it a
static.

[Alex Z., 2016-11-10 20:49:29]

Description was changed from

==========
Replaced cc::SurfaceId::nonce_ with base::UnguessableToken

base::SurfaceId::nonce_ is an uint64 generated at random to make the entire
surface id unguessable.
base::UnguessableToken is a wrapper for 2 uint64 (high_ and low_).
high_ and low_ are generated at random using base::UnguessableToken::Create().
base::UnguessableToken achieves the same goal as the original
cc::SurfaceId::nonce_ with better security and uses more common code.

BUG=649800
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_precise_blink_rel

Committed: https://crrev.com/6b01e58ff708c17f74b560fe1b57f7733b78da53
Cr-Commit-Position: refs/heads/master@{#431294}
==========

to

==========
Replaced cc::SurfaceId::nonce_ with base::UnguessableToken

base::SurfaceId::nonce_ is an uint64 generated at random to make the entire
surface id unguessable.
base::UnguessableToken is a wrapper for 2 uint64 (high_ and low_).
high_ and low_ are generated at random using base::UnguessableToken::Create().
base::UnguessableToken achieves the same goal as the original
cc::SurfaceId::nonce_ with better security and uses more common code.

BUG=649800
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_precise_blink_rel

Committed: https://crrev.com/6b01e58ff708c17f74b560fe1b57f7733b78da53
Cr-Commit-Position: refs/heads/master@{#431294}
==========

[Alex Z., 2016-11-10 20:59:06]

The CQ bit was checked by staraz@chromium.org

[Alex Z., 2016-11-10 20:59:07]

The patchset sent to the CQ was uploaded after l-g-t-m from vmpstr@chromium.org,
dcheng@chromium.org, danakj@chromium.org, tsepez@chromium.org,
haraken@chromium.org, fsamuel@chromium.org, sky@chromium.org
Link to the patchset: https://codereview.chromium.org/2379653006/#ps520001
(title: "Changed SurfaceManager::kRootSurfaceId to a private field to avoid
static initialization")

[commit-bot: I haz the power, 2016-11-10 20:59:39]

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

[Alex Z., 2016-11-10 22:07:18]

The CQ bit was unchecked by staraz@chromium.org

[Alex Z., 2016-11-10 22:07:29]

The CQ bit was checked by staraz@chromium.org

[commit-bot: I haz the power, 2016-11-10 22:08:16]

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-11-11 00:19:47]

Description was changed from

==========
Replaced cc::SurfaceId::nonce_ with base::UnguessableToken

base::SurfaceId::nonce_ is an uint64 generated at random to make the entire
surface id unguessable.
base::UnguessableToken is a wrapper for 2 uint64 (high_ and low_).
high_ and low_ are generated at random using base::UnguessableToken::Create().
base::UnguessableToken achieves the same goal as the original
cc::SurfaceId::nonce_ with better security and uses more common code.

BUG=649800
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_precise_blink_rel

Committed: https://crrev.com/6b01e58ff708c17f74b560fe1b57f7733b78da53
Cr-Commit-Position: refs/heads/master@{#431294}
==========

to

==========
Replaced cc::SurfaceId::nonce_ with base::UnguessableToken

base::SurfaceId::nonce_ is an uint64 generated at random to make the entire
surface id unguessable.
base::UnguessableToken is a wrapper for 2 uint64 (high_ and low_).
high_ and low_ are generated at random using base::UnguessableToken::Create().
base::UnguessableToken achieves the same goal as the original
cc::SurfaceId::nonce_ with better security and uses more common code.

BUG=649800
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_precise_blink_rel

Committed: https://crrev.com/6b01e58ff708c17f74b560fe1b57f7733b78da53
Cr-Commit-Position: refs/heads/master@{#431294}
==========

[commit-bot: I haz the power, 2016-11-11 00:19:50]

Committed patchset #27 (id:520001)

[commit-bot: I haz the power, 2016-11-11 00:21:34]

Description was changed from

==========
Replaced cc::SurfaceId::nonce_ with base::UnguessableToken

base::SurfaceId::nonce_ is an uint64 generated at random to make the entire
surface id unguessable.
base::UnguessableToken is a wrapper for 2 uint64 (high_ and low_).
high_ and low_ are generated at random using base::UnguessableToken::Create().
base::UnguessableToken achieves the same goal as the original
cc::SurfaceId::nonce_ with better security and uses more common code.

BUG=649800
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_precise_blink_rel

Committed: https://crrev.com/6b01e58ff708c17f74b560fe1b57f7733b78da53
Cr-Commit-Position: refs/heads/master@{#431294}
==========

to

==========
Replaced cc::SurfaceId::nonce_ with base::UnguessableToken

base::SurfaceId::nonce_ is an uint64 generated at random to make the entire
surface id unguessable.
base::UnguessableToken is a wrapper for 2 uint64 (high_ and low_).
high_ and low_ are generated at random using base::UnguessableToken::Create().
base::UnguessableToken achieves the same goal as the original
cc::SurfaceId::nonce_ with better security and uses more common code.

BUG=649800
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_precise_blink_rel

Committed: https://crrev.com/6b01e58ff708c17f74b560fe1b57f7733b78da53
Committed: https://crrev.com/7d7f6ebb02ae3b9e354406ce2670399a241baeca
Cr-Original-Commit-Position: refs/heads/master@{#431294}
Cr-Commit-Position: refs/heads/master@{#431398}
==========

[commit-bot: I haz the power, 2016-11-11 00:21:37]

Patchset 27 (id:??) landed as
https://crrev.com/7d7f6ebb02ae3b9e354406ce2670399a241baeca
Cr-Commit-Position: refs/heads/master@{#431398}