Make cc::SurfaceId unguessable

Make cc::SurfaceId unguessable

This CL makes surface IDs unguessable.

With this patch, surface ID consists of three components.

1. Namespace ID is a display compositor allocated 32-bit
   ID.

2. Nonce is a cryptographically secure random, unguessable
   64-bit integer generated by the client.

3. Local ID is a 32-bit sequentially increasing integer
   generated by the client.

With this CL, once a surface ID is allocated, its
components cannot be modified.

BUG=613300
CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel

Committed: https://crrev.com/845b871f4cfe95ebe15330a9dac32d3c69b598b9
Cr-Commit-Position: refs/heads/master@{#395926}

[Fady Samuel, 2016-05-19 22:54:18]

Description was changed from

==========
Make cc::SurfaceId process-local ID 64-bit

This CL is a first step towards making SurfaceIds unguessable by making the
process-local component of a surface ID a 64-bit quantity up from 32-bits.

A followup CL will allocate unguessable IDs.

BUG=
==========

to

==========
Make cc::SurfaceId process-local ID 64-bit

This CL is a first step towards making SurfaceIds unguessable by making the
process-local component of a surface ID a 64-bit quantity up from 32-bits.

A followup CL will allocate unguessable IDs.

BUG=
CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel
==========

[Fady Samuel, 2016-05-19 22:54:45]

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

[Fady Samuel, 2016-05-19 22:54:51]

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

[commit-bot: I haz the power, 2016-05-19 22:55:51]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1996783002/1
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1996783002/1

[Fady Samuel, 2016-05-19 22:56:46]

Description was changed from

==========
Make cc::SurfaceId process-local ID 64-bit

This CL is a first step towards making SurfaceIds unguessable by making the
process-local component of a surface ID a 64-bit quantity up from 32-bits.

A followup CL will allocate unguessable IDs.

BUG=
CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel
==========

to

==========
Make cc::SurfaceId process-local ID 64-bit

This CL is a first step towards making SurfaceIds unguessable by making the
process-local component of a surface ID a 64-bit quantity up from 32-bits.

A followup CL will allocate unguessable IDs.

BUG=613300
CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel
==========

[commit-bot: I haz the power, 2016-05-19 23:10:16]

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

[commit-bot: I haz the power, 2016-05-19 23:10:17]

Dry run: Try jobs failed on following builders:
  android_arm64_dbg_recipe on tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_arm6...)

[Fady Samuel, 2016-05-20 00:22:52]

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

[commit-bot: I haz the power, 2016-05-20 00:23:30]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1996783002/20001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1996783002/20001

[jbauman, 2016-05-20 00:29:00]

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

https://codereview.chromium.org/1996783002/diff/1/cc/surfaces/surface_id.h#ne...
cc/surfaces/surface_id.h:42: return std::hash<uint64_t>()((key.id_namespace) |
(key.id << 32));
Probably better to use base::HashInts64.

https://codereview.chromium.org/1996783002/diff/20001/mojo/converters/surface...
File mojo/converters/surfaces/surfaces_type_converters.cc (right):

https://codereview.chromium.org/1996783002/diff/20001/mojo/converters/surface...
mojo/converters/surfaces/surfaces_type_converters.cc:259: id->local =
static_cast<uint32_t>(input.id);
src/components/mus/public/interfaces/surface_id.mojom needs to be modified, and
this cast should be changed.

[commit-bot: I haz the power, 2016-05-20 00:37:37]

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

[commit-bot: I haz the power, 2016-05-20 00:37:38]

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

[Fady Samuel, 2016-05-20 00:40:14]

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

[Fady Samuel, 2016-05-20 00:40:32]

PTAL

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

https://codereview.chromium.org/1996783002/diff/1/cc/surfaces/surface_id.h#ne...
cc/surfaces/surface_id.h:42: return std::hash<uint64_t>()((key.id_namespace) |
(key.id << 32));
On 2016/05/20 00:29:00, jbauman wrote:
> Probably better to use base::HashInts64.

Done.

https://codereview.chromium.org/1996783002/diff/20001/mojo/converters/surface...
File mojo/converters/surfaces/surfaces_type_converters.cc (right):

https://codereview.chromium.org/1996783002/diff/20001/mojo/converters/surface...
mojo/converters/surfaces/surfaces_type_converters.cc:259: id->local =
static_cast<uint32_t>(input.id);
On 2016/05/20 00:29:00, jbauman wrote:
> src/components/mus/public/interfaces/surface_id.mojom needs to be modified,
and
> this cast should be changed.

Done.

[commit-bot: I haz the power, 2016-05-20 00:41:24]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1996783002/40001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1996783002/40001

[jbauman, 2016-05-20 00:44:02]

lgtm

[commit-bot: I haz the power, 2016-05-20 00:53:59]

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

[commit-bot: I haz the power, 2016-05-20 00:54:00]

Dry run: Try jobs failed on following builders:
  linux_blink_rel on tryserver.blink (JOB_FAILED,
http://build.chromium.org/p/tryserver.blink/builders/linux_blink_rel/builds/8...)

[Fady Samuel, 2016-05-20 01:22:59]

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

[Fady Samuel, 2016-05-20 01:23:00]

+tsepez@ for ipc: This introduces 64-bit local IDs, but SurfaceIdAllocator and
various call sites need to be updated to allocate random, non-colliding IDs. I'm
doing that in the next CL. This one is fairly mechanical first.

[Fady Samuel, 2016-05-20 01:28:35]

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

[Fady Samuel, 2016-05-20 01:29:31]

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

[Fady Samuel, 2016-05-20 01:29:32]

+danakj@ for cc layers and cc quads

[commit-bot: I haz the power, 2016-05-20 01:29:54]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1996783002/60001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1996783002/60001

[commit-bot: I haz the power, 2016-05-20 01:52:16]

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

[commit-bot: I haz the power, 2016-05-20 01:52:17]

Dry run: Try jobs failed on following builders:
  android_arm64_dbg_recipe on tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_arm6...)

[Fady Samuel, 2016-05-20 01:54:06]

I added a question for Tom inline regarding whether it makes sense to give
SurfaceId a nonce component instead of randomly picking the local_id.

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

https://codereview.chromium.org/1996783002/diff/60001/cc/surfaces/surface_id....
cc/surfaces/surface_id.h:28: uint64_t local_id;
Tom, I was looking at other code that uses RandBytes and it seems to be used to
generate a nonce. I wonder if instead, SurfaceId could have three components:
id_namespace, nonce, and then local_id?

SurfaceIdAllocator could take in the id_namespace, and everytime "GenerateId" is
called, it generates a nonce, and a local_id. The local_id can be incremented
each time, and the nonce is random that way we don't have to worry about
collisions. WDYT Tom? Thanks!

[Fady Samuel, 2016-05-20 02:25:48]

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

[commit-bot: I haz the power, 2016-05-20 02:26:26]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1996783002/80001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1996783002/80001

[commit-bot: I haz the power, 2016-05-20 03:36:09]

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

[commit-bot: I haz the power, 2016-05-20 03:36:10]

Dry run: This issue passed the CQ dry run.

[Tom Sepez, 2016-05-20 16:09:57]

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

https://codereview.chromium.org/1996783002/diff/60001/cc/surfaces/surface_id....
cc/surfaces/surface_id.h:28: uint64_t local_id;
On 2016/05/20 01:54:05, Fady Samuel wrote:
> Tom, I was looking at other code that uses RandBytes and it seems to be used
to
> generate a nonce. I wonder if instead, SurfaceId could have three components:
> id_namespace, nonce, and then local_id?
> 
> SurfaceIdAllocator could take in the id_namespace, and everytime "GenerateId"
is
> called, it generates a nonce, and a local_id. The local_id can be incremented
> each time, and the nonce is random that way we don't have to worry about
> collisions. WDYT Tom? Thanks!

They're equivalent in theory.  With regards to the index/nonce pair:

Pros: simpler in practice because you can use the index to lookup the item more
efficiently.

Cons: having looked up via the index, you can forget to check the nonce in your
code in every spot that it is required to do so, and the code will still
function but be vulnerable.

[Fady Samuel, 2016-05-20 17:17:36]

Sounds like using a 64-bit randomly generated ID is the way to go. I'll stick to
this CL then. PTAL Tom and Dana

[Tom Sepez, 2016-05-20 20:22:02]

lgtm

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

https://codereview.chromium.org/1996783002/diff/80001/cc/surfaces/surface_id....
cc/surfaces/surface_id.h:32: return (a.id_namespace == b.id_namespace) &&
(a.local_id == b.local_id);
nit: overparenthesized, also line 40

[danakj, 2016-05-20 21:01:36]

LGTM

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

https://codereview.chromium.org/1996783002/diff/80001/cc/surfaces/surface_id....
cc/surfaces/surface_id.h:46: return base::HashInts64(key.id_namespace,
key.local_id);
Just use HashInts() it picks the right function.

[Fady Samuel, 2016-05-24 20:25:18]

Description was changed from

==========
Make cc::SurfaceId process-local ID 64-bit

This CL is a first step towards making SurfaceIds unguessable by making the
process-local component of a surface ID a 64-bit quantity up from 32-bits.

A followup CL will allocate unguessable IDs.

BUG=613300
CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel
==========

to

==========
Make cc::SurfaceId unguessable

This CL makes surface IDs unguessable.

With this patch, surface ID consists of three components.

1. Namespace ID is a display compositor allocated 32-bit  
   ID.

2. Nonce is a cryptographically secure random, unguessable 
   64-bit integer generated by the client.

3. Local ID is a 32-bit sequentially increasing integer 
   generated by the client.

With this CL, once a surface ID is allocated, its 
components cannot be modified.

BUG=613300
CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel
==========

[Fady Samuel, 2016-05-24 20:28:59]

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

[commit-bot: I haz the power, 2016-05-24 20:29:45]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1996783002/120001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1996783002/120001

[Fady Samuel, 2016-05-24 20:33:07]

PTAL everyone, I've made some major changes here per offline conversations.

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

https://codereview.chromium.org/1996783002/diff/80001/cc/surfaces/surface_id....
cc/surfaces/surface_id.h:32: return (a.id_namespace == b.id_namespace) &&
(a.local_id == b.local_id);
On 2016/05/20 20:22:02, Tom Sepez wrote:
> nit: overparenthesized, also line 40

Done. I've switched to if statements for readability. I can never remember the
order of operations and I always have to think about it if I don't see parens.

https://codereview.chromium.org/1996783002/diff/80001/cc/surfaces/surface_id....
cc/surfaces/surface_id.h:46: return base::HashInts64(key.id_namespace,
key.local_id);
On 2016/05/20 21:01:36, danakj wrote:
> Just use HashInts() it picks the right function.

Done.

[commit-bot: I haz the power, 2016-05-24 20:50:14]

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

[commit-bot: I haz the power, 2016-05-24 20:50:17]

Dry run: Try jobs failed on following builders:
  cast_shell_android on tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/cast_shell_a...)

[Tom Sepez, 2016-05-24 22:35:45]

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

https://codereview.chromium.org/1996783002/diff/120001/cc/surfaces/surface_id...
cc/surfaces/surface_id.h:50: return base::StringPrintf("(%d, %lu, %d)",
id_namespace_, nonce_,
might we ever have a FromString()?  If so, we might want to pick a
representation like x.y.z or x:y:z so that we don't get hung up on spaces as it
gets passed around as a string.

https://codereview.chromium.org/1996783002/diff/120001/cc/surfaces/surface_id...
cc/surfaces/surface_id.h:55: if (id_namespace_ != b.id_namespace_)
nit or just:
  return id_namespace == b.id_namespace &&
      nonce == b.nonce && local_id == b.local_id;

https://codereview.chromium.org/1996783002/diff/120001/cc/surfaces/surface_id...
cc/surfaces/surface_id.h:67: if (id_namespace_ < b.id_namespace_)
I'd sort these by namespace, then local, then nonce, if I'm looking at an
ordered dump of some sort, it might be easier to pick out a duplicated local id.

https://codereview.chromium.org/1996783002/diff/120001/cc/surfaces/surface_id...
cc/surfaces/surface_id.h:82: uint32_t id_namespace_;
I'd order these as namespace_id_, local_id, nonce_, in case the compiler wants
to align int64's to 8-byte boundary, you'll save 8 bytes.

https://codereview.chromium.org/1996783002/diff/120001/cc/surfaces/surface_id...
File cc/surfaces/surface_id_allocator.cc (right):

https://codereview.chromium.org/1996783002/diff/120001/cc/surfaces/surface_id...
cc/surfaces/surface_id_allocator.cc:30: uint64_t nonce = base::RandUint64();
Can we use the random number generator in crypto?  This one isn't advertised as
being secure (though it is in practice).

https://codereview.chromium.org/1996783002/diff/120001/components/mus/public/...
File components/mus/public/interfaces/surface_id.mojom (right):

https://codereview.chromium.org/1996783002/diff/120001/components/mus/public/...
components/mus/public/interfaces/surface_id.mojom:19: uint64 nonce;
again, I'd flip these.  Just because.

[Fady Samuel, 2016-05-25 00:04:19]

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

[Fady Samuel, 2016-05-25 00:04:29]

PTAL

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

https://codereview.chromium.org/1996783002/diff/120001/cc/surfaces/surface_id...
cc/surfaces/surface_id.h:50: return base::StringPrintf("(%d, %lu, %d)",
id_namespace_, nonce_,
On 2016/05/24 22:35:45, Tom Sepez wrote:
> might we ever have a FromString()?  If so, we might want to pick a
> representation like x.y.z or x:y:z so that we don't get hung up on spaces as
it
> gets passed around as a string.

Done. Went with namespace:local:nonce

https://codereview.chromium.org/1996783002/diff/120001/cc/surfaces/surface_id...
cc/surfaces/surface_id.h:55: if (id_namespace_ != b.id_namespace_)
On 2016/05/24 22:35:45, Tom Sepez wrote:
> nit or just:
>   return id_namespace == b.id_namespace &&
>       nonce == b.nonce && local_id == b.local_id;

Done.

https://codereview.chromium.org/1996783002/diff/120001/cc/surfaces/surface_id...
cc/surfaces/surface_id.h:67: if (id_namespace_ < b.id_namespace_)
On 2016/05/24 22:35:45, Tom Sepez wrote:
> I'd sort these by namespace, then local, then nonce, if I'm looking at an
> ordered dump of some sort, it might be easier to pick out a duplicated local
id.

Done.

https://codereview.chromium.org/1996783002/diff/120001/cc/surfaces/surface_id...
cc/surfaces/surface_id.h:82: uint32_t id_namespace_;
On 2016/05/24 22:35:45, Tom Sepez wrote:
> I'd order these as namespace_id_, local_id, nonce_, in case the compiler wants
> to align int64's to 8-byte boundary, you'll save 8 bytes.
>  

Done.

https://codereview.chromium.org/1996783002/diff/120001/cc/surfaces/surface_id...
File cc/surfaces/surface_id_allocator.cc (right):

https://codereview.chromium.org/1996783002/diff/120001/cc/surfaces/surface_id...
cc/surfaces/surface_id_allocator.cc:30: uint64_t nonce = base::RandUint64();
On 2016/05/24 22:35:45, Tom Sepez wrote:
> Can we use the random number generator in crypto?  This one isn't advertised
as
> being secure (though it is in practice).

I'd prefer not to have cc/surfaces depend on all of crypto just for random.cc.
Perhaps we can split that off to a separate target in a future CL.

https://codereview.chromium.org/1996783002/diff/120001/components/mus/public/...
File components/mus/public/interfaces/surface_id.mojom (right):

https://codereview.chromium.org/1996783002/diff/120001/components/mus/public/...
components/mus/public/interfaces/surface_id.mojom:19: uint64 nonce;
On 2016/05/24 22:35:45, Tom Sepez wrote:
> again, I'd flip these.  Just because.

Done.

[commit-bot: I haz the power, 2016-05-25 00:04:51]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1996783002/140001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1996783002/140001

[Tom Sepez, 2016-05-25 00:09:53]

++LGTM, a few nits which you can ignore if you like.

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

https://codereview.chromium.org/1996783002/diff/140001/cc/surfaces/surface_id...
cc/surfaces/surface_id.h:56: return (id_namespace_ == other.id_namespace_) &&
nit: overparenthesized.

https://codereview.chromium.org/1996783002/diff/140001/cc/surfaces/surface_id...
cc/surfaces/surface_id.h:63: if (id_namespace_ < other.id_namespace_)
nit: this is fine, but still reads a little weird.  Maybe

if (id_namespace != other.id_namespace)
  return id_namespace < other.id_namespace;

if (local_id != other.local_id)
  return local_id < other.local_id;

return nonce < other.nonce;

[jbauman, 2016-05-25 00:10:01]

lgtm

[commit-bot: I haz the power, 2016-05-25 00:18:24]

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

[commit-bot: I haz the power, 2016-05-25 00:18:26]

Dry run: Try jobs failed on following builders:
  chromeos_x86-generic_chromium_compile_only_ng on tryserver.chromium.linux
(JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_x86-ge...)

[danakj, 2016-05-25 00:19:08]

layers and quads LGTM. More comments plz.

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

https://codereview.chromium.org/1996783002/diff/120001/cc/surfaces/surface_id...
cc/surfaces/surface_id.h:27: SurfaceId(uint32_t id_namespace, uint64_t nonce,
uint32_t local_id)
Can you please document what is the namespace, nonce and local_id used for, so
callers can figure out what they should give here? Especially in unit tests or
other situations where they are making them up and need to make sense of them.

[Fady Samuel, 2016-05-25 00:56:08]

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

[commit-bot: I haz the power, 2016-05-25 00:56:38]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1996783002/160001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1996783002/160001

[Fady Samuel, 2016-05-25 00:57:43]

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

[Fady Samuel, 2016-05-25 00:57:45]

Thanks everyone
+ben@ for mus, content and mojo.

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

https://codereview.chromium.org/1996783002/diff/120001/cc/surfaces/surface_id...
cc/surfaces/surface_id.h:27: SurfaceId(uint32_t id_namespace, uint64_t nonce,
uint32_t local_id)
On 2016/05/25 00:19:08, danakj wrote:
> Can you please document what is the namespace, nonce and local_id used for, so
> callers can figure out what they should give here? Especially in unit tests or
> other situations where they are making them up and need to make sense of them.

Done.

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

https://codereview.chromium.org/1996783002/diff/140001/cc/surfaces/surface_id...
cc/surfaces/surface_id.h:56: return (id_namespace_ == other.id_namespace_) &&
On 2016/05/25 00:09:53, Tom Sepez wrote:
> nit: overparenthesized.

Done.

https://codereview.chromium.org/1996783002/diff/140001/cc/surfaces/surface_id...
cc/surfaces/surface_id.h:63: if (id_namespace_ < other.id_namespace_)
On 2016/05/25 00:09:53, Tom Sepez wrote:
> nit: this is fine, but still reads a little weird.  Maybe
> 
> if (id_namespace != other.id_namespace)
>   return id_namespace < other.id_namespace;
> 
> if (local_id != other.local_id)
>   return local_id < other.local_id;
> 
> return nonce < other.nonce;

Nice! Much nicer! Done.

[danakj, 2016-05-25 00:58:55]

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

https://codereview.chromium.org/1996783002/diff/140001/cc/surfaces/surface_id...
cc/surfaces/surface_id.h:63: if (id_namespace_ < other.id_namespace_)
On 2016/05/25 00:57:45, Fady Samuel wrote:
> On 2016/05/25 00:09:53, Tom Sepez wrote:
> > nit: this is fine, but still reads a little weird.  Maybe
> > 
> > if (id_namespace != other.id_namespace)
> >   return id_namespace < other.id_namespace;
> > 
> > if (local_id != other.local_id)
> >   return local_id < other.local_id;
> > 
> > return nonce < other.nonce;
> 
> Nice! Much nicer! Done.

The "modern" way to do that is with std::tie(), and compare 2 tuples.

[commit-bot: I haz the power, 2016-05-25 01:07:27]

Dry run: None

[Fady Samuel, 2016-05-25 01:07:42]

Gone modern! PTAL

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

https://codereview.chromium.org/1996783002/diff/140001/cc/surfaces/surface_id...
cc/surfaces/surface_id.h:63: if (id_namespace_ < other.id_namespace_)
On 2016/05/25 00:58:55, danakj wrote:
> On 2016/05/25 00:57:45, Fady Samuel wrote:
> > On 2016/05/25 00:09:53, Tom Sepez wrote:
> > > nit: this is fine, but still reads a little weird.  Maybe
> > > 
> > > if (id_namespace != other.id_namespace)
> > >   return id_namespace < other.id_namespace;
> > > 
> > > if (local_id != other.local_id)
> > >   return local_id < other.local_id;
> > > 
> > > return nonce < other.nonce;
> > 
> > Nice! Much nicer! Done.
> 
> The "modern" way to do that is with std::tie(), and compare 2 tuples.

I totally forgot about std::tie. Thanks! Let's go modern! Done. Thanks!

[Fady Samuel, 2016-05-25 01:07:52]

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

[commit-bot: I haz the power, 2016-05-25 01:08:32]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1996783002/180001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1996783002/180001

[commit-bot: I haz the power, 2016-05-25 01:21:15]

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

[commit-bot: I haz the power, 2016-05-25 01:21:16]

Dry run: Try jobs failed on following builders:
  chromeos_daisy_chromium_compile_only_ng on tryserver.chromium.linux
(JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_daisy_...)
  chromeos_x86-generic_chromium_compile_only_ng on tryserver.chromium.linux
(JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_x86-ge...)

[Fady Samuel, 2016-05-25 02:10:45]

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

[commit-bot: I haz the power, 2016-05-25 02:11:05]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1996783002/200001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1996783002/200001

[commit-bot: I haz the power, 2016-05-25 02:28:33]

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

[commit-bot: I haz the power, 2016-05-25 02:28:35]

Dry run: Try jobs failed on following builders:
  mac_chromium_compile_dbg_ng on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)
  mac_chromium_rel_ng on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[Fady Samuel, 2016-05-25 02:29:01]

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

[commit-bot: I haz the power, 2016-05-25 02:29:26]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1996783002/200001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1996783002/200001

[commit-bot: I haz the power, 2016-05-25 02:35:49]

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

[commit-bot: I haz the power, 2016-05-25 02:35:50]

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

[Fady Samuel, 2016-05-25 03:04:04]

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

[commit-bot: I haz the power, 2016-05-25 03:04:28]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1996783002/220001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1996783002/220001

[commit-bot: I haz the power, 2016-05-25 03:19:10]

Dry run: None

[Fady Samuel, 2016-05-25 03:19:18]

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

[commit-bot: I haz the power, 2016-05-25 03:19:33]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1996783002/240001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1996783002/240001

[commit-bot: I haz the power, 2016-05-25 04:04:16]

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

[commit-bot: I haz the power, 2016-05-25 04:04:19]

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

[Fady Samuel, 2016-05-25 04:16:26]

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

[commit-bot: I haz the power, 2016-05-25 04:16:47]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1996783002/260001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1996783002/260001

[commit-bot: I haz the power, 2016-05-25 05:22:52]

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

[commit-bot: I haz the power, 2016-05-25 05:22:54]

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

[Fady Samuel, 2016-05-25 11:47:00]

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

[commit-bot: I haz the power, 2016-05-25 11:47:10]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1996783002/280001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1996783002/280001

[commit-bot: I haz the power, 2016-05-25 13:27:43]

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

[commit-bot: I haz the power, 2016-05-25 13:27:44]

Dry run: This issue passed the CQ dry run.

[commit-bot: I haz the power, 2016-05-25 13:27:56]

Dry run: None

[Ben Goodger (Google), 2016-05-25 15:30:43]

mus, content, mojo lgtm

https://codereview.chromium.org/1996783002/diff/280001/components/mus/public/...
File components/mus/public/interfaces/surface_id.mojom (right):

https://codereview.chromium.org/1996783002/diff/280001/components/mus/public/...
components/mus/public/interfaces/surface_id.mojom:19: uint32 local_id;
you should document this field:

[Fady Samuel, 2016-05-25 16:13:52]

Thanks! CQ'ing!

https://codereview.chromium.org/1996783002/diff/280001/components/mus/public/...
File components/mus/public/interfaces/surface_id.mojom (right):

https://codereview.chromium.org/1996783002/diff/280001/components/mus/public/...
components/mus/public/interfaces/surface_id.mojom:19: uint32 local_id;
On 2016/05/25 15:30:42, Ben Goodger (Google) wrote:
> you should document this field:

Done.

[Fady Samuel, 2016-05-25 16:14:00]

The CQ bit was checked by fsamuel@chromium.org

[Fady Samuel, 2016-05-25 16:14:00]

The patchset sent to the CQ was uploaded after l-g-t-m from tsepez@chromium.org,
danakj@chromium.org, jbauman@chromium.org, ben@chromium.org
Link to the patchset: https://codereview.chromium.org/1996783002/#ps300001
(title: "Addressed Ben's comment")

[commit-bot: I haz the power, 2016-05-25 16:14:28]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1996783002/300001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1996783002/300001

[commit-bot: I haz the power, 2016-05-25 17:36:22]

Description was changed from

==========
Make cc::SurfaceId unguessable

This CL makes surface IDs unguessable.

With this patch, surface ID consists of three components.

1. Namespace ID is a display compositor allocated 32-bit  
   ID.

2. Nonce is a cryptographically secure random, unguessable 
   64-bit integer generated by the client.

3. Local ID is a 32-bit sequentially increasing integer 
   generated by the client.

With this CL, once a surface ID is allocated, its 
components cannot be modified.

BUG=613300
CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel
==========

to

==========
Make cc::SurfaceId unguessable

This CL makes surface IDs unguessable.

With this patch, surface ID consists of three components.

1. Namespace ID is a display compositor allocated 32-bit  
   ID.

2. Nonce is a cryptographically secure random, unguessable 
   64-bit integer generated by the client.

3. Local ID is a 32-bit sequentially increasing integer 
   generated by the client.

With this CL, once a surface ID is allocated, its 
components cannot be modified.

BUG=613300
CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel
==========

[commit-bot: I haz the power, 2016-05-25 17:36:24]

Committed patchset #16 (id:300001)

[commit-bot: I haz the power, 2016-05-25 17:37:45]

Description was changed from

==========
Make cc::SurfaceId unguessable

This CL makes surface IDs unguessable.

With this patch, surface ID consists of three components.

1. Namespace ID is a display compositor allocated 32-bit  
   ID.

2. Nonce is a cryptographically secure random, unguessable 
   64-bit integer generated by the client.

3. Local ID is a 32-bit sequentially increasing integer 
   generated by the client.

With this CL, once a surface ID is allocated, its 
components cannot be modified.

BUG=613300
CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel
==========

to

==========
Make cc::SurfaceId unguessable

This CL makes surface IDs unguessable.

With this patch, surface ID consists of three components.

1. Namespace ID is a display compositor allocated 32-bit
   ID.

2. Nonce is a cryptographically secure random, unguessable
   64-bit integer generated by the client.

3. Local ID is a 32-bit sequentially increasing integer
   generated by the client.

With this CL, once a surface ID is allocated, its
components cannot be modified.

BUG=613300
CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel

Committed: https://crrev.com/845b871f4cfe95ebe15330a9dac32d3c69b598b9
Cr-Commit-Position: refs/heads/master@{#395926}
==========

[commit-bot: I haz the power, 2016-05-25 17:37:47]

Patchset 16 (id:??) landed as
https://crrev.com/845b871f4cfe95ebe15330a9dac32d3c69b598b9
Cr-Commit-Position: refs/heads/master@{#395926}