Introduce cc::LocalFrameId and use in SurfaceFactory

Introduce cc::LocalFrameId and use in SurfaceFactory

This CL introduces cc::LocalFrameId which is the (local_id, nonce)
part of the SurfaceId. This is the part that can be allocated by
the renderer asynchronously on resize or initial creation. The
FrameSinkId component of a SurfaceId remains constant across resizes
as it identifies a FrameSink/FrameSource as opposed to individual
surfaces.

Since SurfaceFactory is tied to a single FrameSink now, passing in
SurfaceIds including FrameSinkIds is redundant and invites API
abuse by passing in SurfaceIds that don't belong to the given
FrameSink. This will result in a DCHECK, but in production, once
SurfaceIds are allocated in the renderer, then one client could
clobber a surface of another client (albeit this is unlikely due to the
nonce), or it could confuse surface ID routing and fool the parent
to embed a surface ID belonging to a different client (since
FrameSinkIds are guessable).

This change eliminates this concern by making it impossible to pass a
different FrameSinkId into SurfaceFactory; the API simply does not allow
for it.

BUG=647852
TBR=dtrainor@chromium.org, reveman@chromium.org, sky@chromium.org, boliu@chromium.org
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_precise_blink_rel

Committed: https://crrev.com/01f3620439dc6473c838e8e4700e4be84d1b4423
Cr-Commit-Position: refs/heads/master@{#423371}

[Fady Samuel, 2016-10-04 00:01:46]

Description was changed from

==========
WIP: Introduce LocalFrameId and use in SurfaceFactory

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

to

==========
WIP: Introduce LocalFrameId and use in SurfaceFactory

BUG=
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation
==========

[Fady Samuel, 2016-10-04 00:01:47]

Description was changed from

==========
WIP: Introduce LocalFrameId and use in SurfaceFactory

BUG=
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation
==========

to

==========
WIP: Introduce LocalFrameId and use in SurfaceFactory

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

[Fady Samuel, 2016-10-05 15:41:45]

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

[commit-bot: I haz the power, 2016-10-05 15:42: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-05 15:58:41]

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

[commit-bot: I haz the power, 2016-10-05 15:58:41]

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...)

[Fady Samuel, 2016-10-05 16:09:07]

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

[commit-bot: I haz the power, 2016-10-05 16:09:27]

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-05 16:20:52]

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

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

Dry run: 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/...)

[Fady Samuel, 2016-10-05 16:31:26]

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

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

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-05 16:42:47]

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

[commit-bot: I haz the power, 2016-10-05 16:42:50]

Dry run: 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/...)
  cast_shell_android on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/cast_shell_a...)

[Fady Samuel, 2016-10-05 16:53:03]

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

[commit-bot: I haz the power, 2016-10-05 16:53:35]

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-05 17:09:09]

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

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

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

[Fady Samuel, 2016-10-05 17:47:18]

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

[commit-bot: I haz the power, 2016-10-05 17:47:45]

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-05 18:04:43]

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

[commit-bot: I haz the power, 2016-10-05 18:04:44]

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

[Fady Samuel, 2016-10-05 19:23:11]

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

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

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

[Fady Samuel, 2016-10-05 19:56:01]

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

[commit-bot: I haz the power, 2016-10-05 19:56:28]

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

[Fady Samuel, 2016-10-05 20:08:06]

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

[commit-bot: I haz the power, 2016-10-05 20:08:40]

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-05 20:48:24]

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

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

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

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

Description was changed from

==========
WIP: Introduce LocalFrameId and use in SurfaceFactory

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

to

==========
Introduce cc::LocalFrameId and use in SurfaceFactory

This CL introduces cc::LocalFrameId which is the (local_id, nonce)
part of the SurfaceId. This is the part that can be allocated by
the renderer asynchronously on resize or initial creation. The 
FrameSinkId component of a SurfaceId remains constant across resizes
as it identifies a FrameSink/FrameSource as opposed to individual
surfaces.

Since SurfaceFactory is tied to a single FrameSink now, passing in
SurfaceIds including FrameSinkIds is redundant and invites API
abuse by passing in SurfaceIds that don't belong to the given 
FrameSink. This will result in a DCHECK, but in production, once
SurfaceIds are allocated in the renderer, then one client could
clobber a surface of another client (albeit this is unlikely due to the nonce),
or it could confuse surface ID routing and fool the parent
to embed a surface ID belonging to a different client (since 
FrameSinkIds are guessable).

This change eliminates this concern by making it impossible to pass a
different FrameSinkId into SurfaceFactory; the API simply does not allow 
for it.

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

[Fady Samuel, 2016-10-05 20:49:11]

Description was changed from

==========
Introduce cc::LocalFrameId and use in SurfaceFactory

This CL introduces cc::LocalFrameId which is the (local_id, nonce)
part of the SurfaceId. This is the part that can be allocated by
the renderer asynchronously on resize or initial creation. The 
FrameSinkId component of a SurfaceId remains constant across resizes
as it identifies a FrameSink/FrameSource as opposed to individual
surfaces.

Since SurfaceFactory is tied to a single FrameSink now, passing in
SurfaceIds including FrameSinkIds is redundant and invites API
abuse by passing in SurfaceIds that don't belong to the given 
FrameSink. This will result in a DCHECK, but in production, once
SurfaceIds are allocated in the renderer, then one client could
clobber a surface of another client (albeit this is unlikely due to the nonce),
or it could confuse surface ID routing and fool the parent
to embed a surface ID belonging to a different client (since 
FrameSinkIds are guessable).

This change eliminates this concern by making it impossible to pass a
different FrameSinkId into SurfaceFactory; the API simply does not allow 
for it.

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

to

==========
Introduce cc::LocalFrameId and use in SurfaceFactory

This CL introduces cc::LocalFrameId which is the (local_id, nonce)
part of the SurfaceId. This is the part that can be allocated by
the renderer asynchronously on resize or initial creation. The 
FrameSinkId component of a SurfaceId remains constant across resizes
as it identifies a FrameSink/FrameSource as opposed to individual
surfaces.

Since SurfaceFactory is tied to a single FrameSink now, passing in
SurfaceIds including FrameSinkIds is redundant and invites API
abuse by passing in SurfaceIds that don't belong to the given 
FrameSink. This will result in a DCHECK, but in production, once
SurfaceIds are allocated in the renderer, then one client could
clobber a surface of another client (albeit this is unlikely due to the 
nonce), or it could confuse surface ID routing and fool the parent
to embed a surface ID belonging to a different client (since 
FrameSinkIds are guessable).

This change eliminates this concern by making it impossible to pass a
different FrameSinkId into SurfaceFactory; the API simply does not allow 
for it.

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

[Fady Samuel, 2016-10-05 20:52:10]

fsamuel@chromium.org changed reviewers:
+ bolian@chromium.org, dtrainor@chromium.org, enne@chromium.org,
jam@chromium.org, sky@chromium.org, tsepez@chromium.org

[Fady Samuel, 2016-10-05 20:52:12]

Apologies for the large CL. It's actually fairly mechanical. LocalFrameId and
SurfaceFactory is where the meat of this patch is, and the rest is the fallout
of the change. 

+enne@ for cc
+sky@ for ui + services
+jam@ for content (once enne@ is happy)
+dtrainor@ for blimp
+boliu@ for android_webview
+tsepez@ for ipc review

[Fady Samuel, 2016-10-05 20:52:32]

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

[Fady Samuel, 2016-10-05 20:52:34]

+reveman@ for exo

[Fady Samuel, 2016-10-05 20:53:03]

fsamuel@chromium.org changed reviewers:
+ boliu@chromium.org
- bolian@chromium.org

[Fady Samuel, 2016-10-05 20:53:04]

-bolian +boliu

[Fady Samuel, 2016-10-05 20:57:07]

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

[commit-bot: I haz the power, 2016-10-05 20:57:42]

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

[Tom Sepez, 2016-10-05 21:08:21]

ipc lgtm

[commit-bot: I haz the power, 2016-10-05 22:38:09]

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

[commit-bot: I haz the power, 2016-10-05 22:38:10]

Dry run: This issue passed the CQ dry run.

[enne (OOO), 2016-10-06 00:28:11]

lgtm

https://codereview.chromium.org/2388753003/diff/260001/cc/surfaces/surface_fa...
File cc/surfaces/surface_factory.h (right):

https://codereview.chromium.org/2388753003/diff/260001/cc/surfaces/surface_fa...
cc/surfaces/surface_factory.h:50: void Create(const LocalFrameId&
local_frame_id);
Yeah, this is nice.  :)

[jam, 2016-10-06 00:53:45]

lgtm

[Fady Samuel, 2016-10-06 00:55:17]

Description was changed from

==========
Introduce cc::LocalFrameId and use in SurfaceFactory

This CL introduces cc::LocalFrameId which is the (local_id, nonce)
part of the SurfaceId. This is the part that can be allocated by
the renderer asynchronously on resize or initial creation. The 
FrameSinkId component of a SurfaceId remains constant across resizes
as it identifies a FrameSink/FrameSource as opposed to individual
surfaces.

Since SurfaceFactory is tied to a single FrameSink now, passing in
SurfaceIds including FrameSinkIds is redundant and invites API
abuse by passing in SurfaceIds that don't belong to the given 
FrameSink. This will result in a DCHECK, but in production, once
SurfaceIds are allocated in the renderer, then one client could
clobber a surface of another client (albeit this is unlikely due to the 
nonce), or it could confuse surface ID routing and fool the parent
to embed a surface ID belonging to a different client (since 
FrameSinkIds are guessable).

This change eliminates this concern by making it impossible to pass a
different FrameSinkId into SurfaceFactory; the API simply does not allow 
for it.

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

to

==========
Introduce cc::LocalFrameId and use in SurfaceFactory

This CL introduces cc::LocalFrameId which is the (local_id, nonce)
part of the SurfaceId. This is the part that can be allocated by
the renderer asynchronously on resize or initial creation. The 
FrameSinkId component of a SurfaceId remains constant across resizes
as it identifies a FrameSink/FrameSource as opposed to individual
surfaces.

Since SurfaceFactory is tied to a single FrameSink now, passing in
SurfaceIds including FrameSinkIds is redundant and invites API
abuse by passing in SurfaceIds that don't belong to the given 
FrameSink. This will result in a DCHECK, but in production, once
SurfaceIds are allocated in the renderer, then one client could
clobber a surface of another client (albeit this is unlikely due to the 
nonce), or it could confuse surface ID routing and fool the parent
to embed a surface ID belonging to a different client (since 
FrameSinkIds are guessable).

This change eliminates this concern by making it impossible to pass a
different FrameSinkId into SurfaceFactory; the API simply does not allow 
for it.

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

[Fady Samuel, 2016-10-06 00:57:02]

I'm going to TBR everyone else (dtrainor@, reveman@, sky@, boliu@) as these
files are changed largely as a fallout of a cc change.

[Fady Samuel, 2016-10-06 00:58:01]

Description was changed from

==========
Introduce cc::LocalFrameId and use in SurfaceFactory

This CL introduces cc::LocalFrameId which is the (local_id, nonce)
part of the SurfaceId. This is the part that can be allocated by
the renderer asynchronously on resize or initial creation. The 
FrameSinkId component of a SurfaceId remains constant across resizes
as it identifies a FrameSink/FrameSource as opposed to individual
surfaces.

Since SurfaceFactory is tied to a single FrameSink now, passing in
SurfaceIds including FrameSinkIds is redundant and invites API
abuse by passing in SurfaceIds that don't belong to the given 
FrameSink. This will result in a DCHECK, but in production, once
SurfaceIds are allocated in the renderer, then one client could
clobber a surface of another client (albeit this is unlikely due to the 
nonce), or it could confuse surface ID routing and fool the parent
to embed a surface ID belonging to a different client (since 
FrameSinkIds are guessable).

This change eliminates this concern by making it impossible to pass a
different FrameSinkId into SurfaceFactory; the API simply does not allow 
for it.

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

to

==========
Introduce cc::LocalFrameId and use in SurfaceFactory

This CL introduces cc::LocalFrameId which is the (local_id, nonce)
part of the SurfaceId. This is the part that can be allocated by
the renderer asynchronously on resize or initial creation. The 
FrameSinkId component of a SurfaceId remains constant across resizes
as it identifies a FrameSink/FrameSource as opposed to individual
surfaces.

Since SurfaceFactory is tied to a single FrameSink now, passing in
SurfaceIds including FrameSinkIds is redundant and invites API
abuse by passing in SurfaceIds that don't belong to the given 
FrameSink. This will result in a DCHECK, but in production, once
SurfaceIds are allocated in the renderer, then one client could
clobber a surface of another client (albeit this is unlikely due to the 
nonce), or it could confuse surface ID routing and fool the parent
to embed a surface ID belonging to a different client (since 
FrameSinkIds are guessable).

This change eliminates this concern by making it impossible to pass a
different FrameSinkId into SurfaceFactory; the API simply does not allow 
for it.

BUG=647852
TBR=dtrainor@chromium.org, reveman@chromium.org, sky@chromium.org,
boliu@chromium.org
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_precise_blink_rel
==========

[Fady Samuel, 2016-10-06 00:58:15]

The CQ bit was checked by fsamuel@chromium.org

[commit-bot: I haz the power, 2016-10-06 00:58: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-10-06 01:08:54]

Description was changed from

==========
Introduce cc::LocalFrameId and use in SurfaceFactory

This CL introduces cc::LocalFrameId which is the (local_id, nonce)
part of the SurfaceId. This is the part that can be allocated by
the renderer asynchronously on resize or initial creation. The 
FrameSinkId component of a SurfaceId remains constant across resizes
as it identifies a FrameSink/FrameSource as opposed to individual
surfaces.

Since SurfaceFactory is tied to a single FrameSink now, passing in
SurfaceIds including FrameSinkIds is redundant and invites API
abuse by passing in SurfaceIds that don't belong to the given 
FrameSink. This will result in a DCHECK, but in production, once
SurfaceIds are allocated in the renderer, then one client could
clobber a surface of another client (albeit this is unlikely due to the 
nonce), or it could confuse surface ID routing and fool the parent
to embed a surface ID belonging to a different client (since 
FrameSinkIds are guessable).

This change eliminates this concern by making it impossible to pass a
different FrameSinkId into SurfaceFactory; the API simply does not allow 
for it.

BUG=647852
TBR=dtrainor@chromium.org, reveman@chromium.org, sky@chromium.org,
boliu@chromium.org
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_precise_blink_rel
==========

to

==========
Introduce cc::LocalFrameId and use in SurfaceFactory

This CL introduces cc::LocalFrameId which is the (local_id, nonce)
part of the SurfaceId. This is the part that can be allocated by
the renderer asynchronously on resize or initial creation. The 
FrameSinkId component of a SurfaceId remains constant across resizes
as it identifies a FrameSink/FrameSource as opposed to individual
surfaces.

Since SurfaceFactory is tied to a single FrameSink now, passing in
SurfaceIds including FrameSinkIds is redundant and invites API
abuse by passing in SurfaceIds that don't belong to the given 
FrameSink. This will result in a DCHECK, but in production, once
SurfaceIds are allocated in the renderer, then one client could
clobber a surface of another client (albeit this is unlikely due to the 
nonce), or it could confuse surface ID routing and fool the parent
to embed a surface ID belonging to a different client (since 
FrameSinkIds are guessable).

This change eliminates this concern by making it impossible to pass a
different FrameSinkId into SurfaceFactory; the API simply does not allow 
for it.

BUG=647852
TBR=dtrainor@chromium.org, reveman@chromium.org, sky@chromium.org,
boliu@chromium.org
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_precise_blink_rel
==========

[commit-bot: I haz the power, 2016-10-06 01:08:56]

Committed patchset #14 (id:260001)

[commit-bot: I haz the power, 2016-10-06 01:10:30]

Description was changed from

==========
Introduce cc::LocalFrameId and use in SurfaceFactory

This CL introduces cc::LocalFrameId which is the (local_id, nonce)
part of the SurfaceId. This is the part that can be allocated by
the renderer asynchronously on resize or initial creation. The 
FrameSinkId component of a SurfaceId remains constant across resizes
as it identifies a FrameSink/FrameSource as opposed to individual
surfaces.

Since SurfaceFactory is tied to a single FrameSink now, passing in
SurfaceIds including FrameSinkIds is redundant and invites API
abuse by passing in SurfaceIds that don't belong to the given 
FrameSink. This will result in a DCHECK, but in production, once
SurfaceIds are allocated in the renderer, then one client could
clobber a surface of another client (albeit this is unlikely due to the 
nonce), or it could confuse surface ID routing and fool the parent
to embed a surface ID belonging to a different client (since 
FrameSinkIds are guessable).

This change eliminates this concern by making it impossible to pass a
different FrameSinkId into SurfaceFactory; the API simply does not allow 
for it.

BUG=647852
TBR=dtrainor@chromium.org, reveman@chromium.org, sky@chromium.org,
boliu@chromium.org
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_precise_blink_rel
==========

to

==========
Introduce cc::LocalFrameId and use in SurfaceFactory

This CL introduces cc::LocalFrameId which is the (local_id, nonce)
part of the SurfaceId. This is the part that can be allocated by
the renderer asynchronously on resize or initial creation. The
FrameSinkId component of a SurfaceId remains constant across resizes
as it identifies a FrameSink/FrameSource as opposed to individual
surfaces.

Since SurfaceFactory is tied to a single FrameSink now, passing in
SurfaceIds including FrameSinkIds is redundant and invites API
abuse by passing in SurfaceIds that don't belong to the given
FrameSink. This will result in a DCHECK, but in production, once
SurfaceIds are allocated in the renderer, then one client could
clobber a surface of another client (albeit this is unlikely due to the
nonce), or it could confuse surface ID routing and fool the parent
to embed a surface ID belonging to a different client (since
FrameSinkIds are guessable).

This change eliminates this concern by making it impossible to pass a
different FrameSinkId into SurfaceFactory; the API simply does not allow
for it.

BUG=647852
TBR=dtrainor@chromium.org, reveman@chromium.org, sky@chromium.org,
boliu@chromium.org
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_precise_blink_rel

Committed: https://crrev.com/01f3620439dc6473c838e8e4700e4be84d1b4423
Cr-Commit-Position: refs/heads/master@{#423371}
==========

[commit-bot: I haz the power, 2016-10-06 01:10:32]

Patchset 14 (id:??) landed as
https://crrev.com/01f3620439dc6473c838e8e4700e4be84d1b4423
Cr-Commit-Position: refs/heads/master@{#423371}

[boliu, 2016-10-06 02:15:51]

belated lgtm

[reveman, 2016-10-06 08:04:00]

lgtm