Added an extra sync token field for extra command buffer identification.

Added an extra sync token field for extra command buffer identification.

This CL adds an extra field within sync tokens without changing the size
of the sync token. Currently this extra field is only used for the
GPU channel implementation, storing the stream of the identified command
buffer.

BUG=514815
Committed: https://crrev.com/f21e7e6e2b260d56fa54373dd662c81c9d955331
Cr-Commit-Position: refs/heads/master@{#363231}

[David Yen, 2015-11-30 20:24:11]

Description was changed from

==========
Added an extra sync token field for extra command buffer identification.

BUG=514815
==========

to

==========
Added an extra sync token field for extra command buffer identification.

This CL adds an extra field within sync tokens without changing the size
of the sync token. Currently this extra field is only used for the
GPU channel implementation, storing the stream of the identified command
buffer.

BUG=514815
==========

[David Yen, 2015-11-30 20:26:27]

dyen@chromium.org changed reviewers:
+ dalecurtis@chromium.org, dcheng@chromium.org, piman@chromium.org,
sky@chromium.org

[David Yen, 2015-11-30 20:26:28]

+piman@ for general review

+dcheng@ command buffer param trait changes.

+sky@ for mojo changes, this should just add an extra field which does not do
anything.

+dalecurtis@ for media review, only the unit test was changed for the extra
field in the constructor.

[DaleCurtis, 2015-11-30 20:51:05]

media/base lgtm

[piman, 2015-11-30 20:59:24]

https://codereview.chromium.org/1489573003/diff/1/content/common/gpu/client/c...
File content/common/gpu/client/command_buffer_proxy_impl.cc (right):

https://codereview.chromium.org/1489573003/diff/1/content/common/gpu/client/c...
content/common/gpu/client/command_buffer_proxy_impl.cc:585: if
(sync_token->namespace_id() != gpu::CommandBufferNamespace::GPU_IO &&
|| instead of &&

https://codereview.chromium.org/1489573003/diff/1/content/common/gpu/gpu_comm...
File content/common/gpu/gpu_command_buffer_stub.cc (right):

https://codereview.chromium.org/1489573003/diff/1/content/common/gpu/gpu_comm...
content/common/gpu/gpu_command_buffer_stub.cc:217:
DCHECK(context_group_->bind_generatse_resource() ==
??

https://codereview.chromium.org/1489573003/diff/1/gpu/ipc/gpu_command_buffer_...
File gpu/ipc/gpu_command_buffer_traits.cc (right):

https://codereview.chromium.org/1489573003/diff/1/gpu/ipc/gpu_command_buffer_...
gpu/ipc/gpu_command_buffer_traits.cc:79: !ReadParam(m, iter, &extra_data_field)
||
Actually, I have a security concern: the rest of the code will assume that the
stream id stored in this field is the one that corresponds to command_buffer_id,
but we have no way to validate it here. I'm worried that we'd make
security-sensitive decisions on it when an untrusted client could craft a
SyncToken where they don't match.

My preference would actually be to not send this over IPC, and handle the
possibility that it's 0 (which is invalid) in e.g. CanWaitUnverifiedSyncToken
(if stream_id == 0, then no, we can't wait on that token).

The argument is that it's only used on the client to stash the stream_id for
same-process synchronization, but as soon as we're talking cross-process
synchronization, the stream_id shouldn't be relevant anymore.

[David Yen, 2015-11-30 22:45:07]

https://codereview.chromium.org/1489573003/diff/1/content/common/gpu/client/c...
File content/common/gpu/client/command_buffer_proxy_impl.cc (right):

https://codereview.chromium.org/1489573003/diff/1/content/common/gpu/client/c...
content/common/gpu/client/command_buffer_proxy_impl.cc:585: if
(sync_token->namespace_id() != gpu::CommandBufferNamespace::GPU_IO &&
On 2015/11/30 20:59:23, piman (slow to review) wrote:
> || instead of &&

Done.

https://codereview.chromium.org/1489573003/diff/1/content/common/gpu/gpu_comm...
File content/common/gpu/gpu_command_buffer_stub.cc (right):

https://codereview.chromium.org/1489573003/diff/1/content/common/gpu/gpu_comm...
content/common/gpu/gpu_command_buffer_stub.cc:217:
DCHECK(context_group_->bind_generatse_resource() ==
On 2015/11/30 20:59:23, piman (slow to review) wrote:
> ??

Strange... not sure how that happened. Reverted.

https://codereview.chromium.org/1489573003/diff/1/gpu/ipc/gpu_command_buffer_...
File gpu/ipc/gpu_command_buffer_traits.cc (right):

https://codereview.chromium.org/1489573003/diff/1/gpu/ipc/gpu_command_buffer_...
gpu/ipc/gpu_command_buffer_traits.cc:79: !ReadParam(m, iter, &extra_data_field)
||
On 2015/11/30 20:59:23, piman (slow to review) wrote:
> Actually, I have a security concern: the rest of the code will assume that the
> stream id stored in this field is the one that corresponds to
command_buffer_id,
> but we have no way to validate it here. I'm worried that we'd make
> security-sensitive decisions on it when an untrusted client could craft a
> SyncToken where they don't match.
> 
> My preference would actually be to not send this over IPC, and handle the
> possibility that it's 0 (which is invalid) in e.g. CanWaitUnverifiedSyncToken
> (if stream_id == 0, then no, we can't wait on that token).
> 
> The argument is that it's only used on the client to stash the stream_id for
> same-process synchronization, but as soon as we're talking cross-process
> synchronization, the stream_id shouldn't be relevant anymore.

Since I have created this variable as a "generic store what you need into sync
token" I'm worried someone might come up with a use later in mojo or something
which they expect it to make it across process. This variable only being
available within a single process is a huge gotcha.

From the point of view of security, you're right that we do always have to
account for invalid data. I think all the data in a sync token should always be
viewed as "untrusted" though, and we already have to account for that invalid
data for all the other values. Having said that, you reminded me that the
FlushPendingStream needs to account for having an invalid stream id. I will
change the FlushPendingStream() to use find instead of operator [].

[dcheng, 2015-12-01 01:29:14]

https://codereview.chromium.org/1489573003/diff/40001/content/common/gpu/clie...
File content/common/gpu/client/command_buffer_proxy_impl.cc (right):

https://codereview.chromium.org/1489573003/diff/40001/content/common/gpu/clie...
content/common/gpu/client/command_buffer_proxy_impl.cc:523: return
static_cast<uint32_t>(stream_id_);
Wouldn't it be easier just to make the signs match throughout?

[David Yen, 2015-12-01 21:29:45]

https://codereview.chromium.org/1489573003/diff/40001/content/common/gpu/clie...
File content/common/gpu/client/command_buffer_proxy_impl.cc (right):

https://codereview.chromium.org/1489573003/diff/40001/content/common/gpu/clie...
content/common/gpu/client/command_buffer_proxy_impl.cc:523: return
static_cast<uint32_t>(stream_id_);
On 2015/12/01 01:29:14, dcheng wrote:
> Wouldn't it be easier just to make the signs match throughout?

Done.

[dcheng, 2015-12-01 22:39:17]

lgtm

[sky, 2015-12-01 22:42:44]

LGTM

[piman, 2015-12-02 02:25:19]

On 2015/11/30 22:45:07, David Yen wrote:
>
https://codereview.chromium.org/1489573003/diff/1/content/common/gpu/client/c...
> File content/common/gpu/client/command_buffer_proxy_impl.cc (right):
> 
>
https://codereview.chromium.org/1489573003/diff/1/content/common/gpu/client/c...
> content/common/gpu/client/command_buffer_proxy_impl.cc:585: if
> (sync_token->namespace_id() != gpu::CommandBufferNamespace::GPU_IO &&
> On 2015/11/30 20:59:23, piman (slow to review) wrote:
> > || instead of &&
> 
> Done.
> 
>
https://codereview.chromium.org/1489573003/diff/1/content/common/gpu/gpu_comm...
> File content/common/gpu/gpu_command_buffer_stub.cc (right):
> 
>
https://codereview.chromium.org/1489573003/diff/1/content/common/gpu/gpu_comm...
> content/common/gpu/gpu_command_buffer_stub.cc:217:
> DCHECK(context_group_->bind_generatse_resource() ==
> On 2015/11/30 20:59:23, piman (slow to review) wrote:
> > ??
> 
> Strange... not sure how that happened. Reverted.
> 
>
https://codereview.chromium.org/1489573003/diff/1/gpu/ipc/gpu_command_buffer_...
> File gpu/ipc/gpu_command_buffer_traits.cc (right):
> 
>
https://codereview.chromium.org/1489573003/diff/1/gpu/ipc/gpu_command_buffer_...
> gpu/ipc/gpu_command_buffer_traits.cc:79: !ReadParam(m, iter,
&extra_data_field)
> ||
> On 2015/11/30 20:59:23, piman (slow to review) wrote:
> > Actually, I have a security concern: the rest of the code will assume that
the
> > stream id stored in this field is the one that corresponds to
> command_buffer_id,
> > but we have no way to validate it here. I'm worried that we'd make
> > security-sensitive decisions on it when an untrusted client could craft a
> > SyncToken where they don't match.
> > 
> > My preference would actually be to not send this over IPC, and handle the
> > possibility that it's 0 (which is invalid) in e.g.
CanWaitUnverifiedSyncToken
> > (if stream_id == 0, then no, we can't wait on that token).
> > 
> > The argument is that it's only used on the client to stash the stream_id for
> > same-process synchronization, but as soon as we're talking cross-process
> > synchronization, the stream_id shouldn't be relevant anymore.
> 
> Since I have created this variable as a "generic store what you need into sync
> token" I'm worried someone might come up with a use later in mojo or something
> which they expect it to make it across process. This variable only being
> available within a single process is a huge gotcha.

But I think this would be a bogus use. Namespace ID + command buffer ID uniquely
identifies the command buffer. If that extra data is meant as a shortcut to
identify a property of the command buffer (as is the case here), it is invalid
(unsafe) to use it from another process without validating it. Validating it
means looking up the command buffer and checking that the property is identical.
At that point, the shortcut is pointless.

I'm particularly worried that it would be very tempting to lookup the stream
from the GPU process using this (e.g. for the scheduler to track dependencies
between streams), but it would be absolutely invalid and unsafe.

Is there a concrete cross-process use case where the validation would not also
make the extra data redundant?

[David Yen, 2015-12-02 18:48:46]

On 2015/12/02 02:25:19, piman (slow to review) wrote:
> On 2015/11/30 22:45:07, David Yen wrote:
> >
>
https://codereview.chromium.org/1489573003/diff/1/content/common/gpu/client/c...
> > File content/common/gpu/client/command_buffer_proxy_impl.cc (right):
> > 
> >
>
https://codereview.chromium.org/1489573003/diff/1/content/common/gpu/client/c...
> > content/common/gpu/client/command_buffer_proxy_impl.cc:585: if
> > (sync_token->namespace_id() != gpu::CommandBufferNamespace::GPU_IO &&
> > On 2015/11/30 20:59:23, piman (slow to review) wrote:
> > > || instead of &&
> > 
> > Done.
> > 
> >
>
https://codereview.chromium.org/1489573003/diff/1/content/common/gpu/gpu_comm...
> > File content/common/gpu/gpu_command_buffer_stub.cc (right):
> > 
> >
>
https://codereview.chromium.org/1489573003/diff/1/content/common/gpu/gpu_comm...
> > content/common/gpu/gpu_command_buffer_stub.cc:217:
> > DCHECK(context_group_->bind_generatse_resource() ==
> > On 2015/11/30 20:59:23, piman (slow to review) wrote:
> > > ??
> > 
> > Strange... not sure how that happened. Reverted.
> > 
> >
>
https://codereview.chromium.org/1489573003/diff/1/gpu/ipc/gpu_command_buffer_...
> > File gpu/ipc/gpu_command_buffer_traits.cc (right):
> > 
> >
>
https://codereview.chromium.org/1489573003/diff/1/gpu/ipc/gpu_command_buffer_...
> > gpu/ipc/gpu_command_buffer_traits.cc:79: !ReadParam(m, iter,
> &extra_data_field)
> > ||
> > On 2015/11/30 20:59:23, piman (slow to review) wrote:
> > > Actually, I have a security concern: the rest of the code will assume that
> the
> > > stream id stored in this field is the one that corresponds to
> > command_buffer_id,
> > > but we have no way to validate it here. I'm worried that we'd make
> > > security-sensitive decisions on it when an untrusted client could craft a
> > > SyncToken where they don't match.
> > > 
> > > My preference would actually be to not send this over IPC, and handle the
> > > possibility that it's 0 (which is invalid) in e.g.
> CanWaitUnverifiedSyncToken
> > > (if stream_id == 0, then no, we can't wait on that token).
> > > 
> > > The argument is that it's only used on the client to stash the stream_id
for
> > > same-process synchronization, but as soon as we're talking cross-process
> > > synchronization, the stream_id shouldn't be relevant anymore.
> > 
> > Since I have created this variable as a "generic store what you need into
sync
> > token" I'm worried someone might come up with a use later in mojo or
something
> > which they expect it to make it across process. This variable only being
> > available within a single process is a huge gotcha.
> 
> But I think this would be a bogus use. Namespace ID + command buffer ID
uniquely
> identifies the command buffer. If that extra data is meant as a shortcut to
> identify a property of the command buffer (as is the case here), it is invalid
> (unsafe) to use it from another process without validating it. Validating it
> means looking up the command buffer and checking that the property is
identical.
> At that point, the shortcut is pointless.
> 
> I'm particularly worried that it would be very tempting to lookup the stream
> from the GPU process using this (e.g. for the scheduler to track dependencies
> between streams), but it would be absolutely invalid and unsafe.
> 
> Is there a concrete cross-process use case where the validation would not also
> make the extra data redundant?

That's fair that there is currently no cross-process use case. I will make it
not IPC the extra data field, I also added a comment in sync_token.h above the
extra_data_field accessor about this behavior in case anyone tries to use it in
a cross process way.

[David Yen, 2015-12-03 23:19:43]

On 2015/12/02 18:48:46, David Yen wrote:
> On 2015/12/02 02:25:19, piman (slow to review) wrote:
> > On 2015/11/30 22:45:07, David Yen wrote:
> > >
> >
>
https://codereview.chromium.org/1489573003/diff/1/content/common/gpu/client/c...
> > > File content/common/gpu/client/command_buffer_proxy_impl.cc (right):
> > > 
> > >
> >
>
https://codereview.chromium.org/1489573003/diff/1/content/common/gpu/client/c...
> > > content/common/gpu/client/command_buffer_proxy_impl.cc:585: if
> > > (sync_token->namespace_id() != gpu::CommandBufferNamespace::GPU_IO &&
> > > On 2015/11/30 20:59:23, piman (slow to review) wrote:
> > > > || instead of &&
> > > 
> > > Done.
> > > 
> > >
> >
>
https://codereview.chromium.org/1489573003/diff/1/content/common/gpu/gpu_comm...
> > > File content/common/gpu/gpu_command_buffer_stub.cc (right):
> > > 
> > >
> >
>
https://codereview.chromium.org/1489573003/diff/1/content/common/gpu/gpu_comm...
> > > content/common/gpu/gpu_command_buffer_stub.cc:217:
> > > DCHECK(context_group_->bind_generatse_resource() ==
> > > On 2015/11/30 20:59:23, piman (slow to review) wrote:
> > > > ??
> > > 
> > > Strange... not sure how that happened. Reverted.
> > > 
> > >
> >
>
https://codereview.chromium.org/1489573003/diff/1/gpu/ipc/gpu_command_buffer_...
> > > File gpu/ipc/gpu_command_buffer_traits.cc (right):
> > > 
> > >
> >
>
https://codereview.chromium.org/1489573003/diff/1/gpu/ipc/gpu_command_buffer_...
> > > gpu/ipc/gpu_command_buffer_traits.cc:79: !ReadParam(m, iter,
> > &extra_data_field)
> > > ||
> > > On 2015/11/30 20:59:23, piman (slow to review) wrote:
> > > > Actually, I have a security concern: the rest of the code will assume
that
> > the
> > > > stream id stored in this field is the one that corresponds to
> > > command_buffer_id,
> > > > but we have no way to validate it here. I'm worried that we'd make
> > > > security-sensitive decisions on it when an untrusted client could craft
a
> > > > SyncToken where they don't match.
> > > > 
> > > > My preference would actually be to not send this over IPC, and handle
the
> > > > possibility that it's 0 (which is invalid) in e.g.
> > CanWaitUnverifiedSyncToken
> > > > (if stream_id == 0, then no, we can't wait on that token).
> > > > 
> > > > The argument is that it's only used on the client to stash the stream_id
> for
> > > > same-process synchronization, but as soon as we're talking cross-process
> > > > synchronization, the stream_id shouldn't be relevant anymore.
> > > 
> > > Since I have created this variable as a "generic store what you need into
> sync
> > > token" I'm worried someone might come up with a use later in mojo or
> something
> > > which they expect it to make it across process. This variable only being
> > > available within a single process is a huge gotcha.
> > 
> > But I think this would be a bogus use. Namespace ID + command buffer ID
> uniquely
> > identifies the command buffer. If that extra data is meant as a shortcut to
> > identify a property of the command buffer (as is the case here), it is
invalid
> > (unsafe) to use it from another process without validating it. Validating it
> > means looking up the command buffer and checking that the property is
> identical.
> > At that point, the shortcut is pointless.
> > 
> > I'm particularly worried that it would be very tempting to lookup the stream
> > from the GPU process using this (e.g. for the scheduler to track
dependencies
> > between streams), but it would be absolutely invalid and unsafe.
> > 
> > Is there a concrete cross-process use case where the validation would not
also
> > make the extra data redundant?
> 
> That's fair that there is currently no cross-process use case. I will make it
> not IPC the extra data field, I also added a comment in sync_token.h above the
> extra_data_field accessor about this behavior in case anyone tries to use it
in
> a cross process way.

ping

[piman, 2015-12-03 23:42:44]

https://codereview.chromium.org/1489573003/diff/100001/components/mus/public/...
File components/mus/public/interfaces/compositor_frame.mojom (right):

https://codereview.chromium.org/1489573003/diff/100001/components/mus/public/...
components/mus/public/interfaces/compositor_frame.mojom:36: int32
extra_data_field;
Please remove this one too, for consistency with IPC.

[David Yen, 2015-12-03 23:53:27]

https://codereview.chromium.org/1489573003/diff/100001/components/mus/public/...
File components/mus/public/interfaces/compositor_frame.mojom (right):

https://codereview.chromium.org/1489573003/diff/100001/components/mus/public/...
components/mus/public/interfaces/compositor_frame.mojom:36: int32
extra_data_field;
On 2015/12/03 23:42:44, piman (slow to review) wrote:
> Please remove this one too, for consistency with IPC.

Done.

[piman, 2015-12-04 00:02:02]

LGTM after one last thing

https://codereview.chromium.org/1489573003/diff/120001/content/common/gpu/cli...
File content/common/gpu/client/command_buffer_proxy_impl.cc (right):

https://codereview.chromium.org/1489573003/diff/120001/content/common/gpu/cli...
content/common/gpu/client/command_buffer_proxy_impl.cc:590: const int32_t
release_stream_id = sync_token->extra_data_field();
nit:
if (release_stream_id == 0)
  return false;

[David Yen, 2015-12-04 00:12:28]

https://codereview.chromium.org/1489573003/diff/120001/content/common/gpu/cli...
File content/common/gpu/client/command_buffer_proxy_impl.cc (right):

https://codereview.chromium.org/1489573003/diff/120001/content/common/gpu/cli...
content/common/gpu/client/command_buffer_proxy_impl.cc:590: const int32_t
release_stream_id = sync_token->extra_data_field();
On 2015/12/04 00:02:01, piman (slow to review) wrote:
> nit:
> if (release_stream_id == 0)
>   return false;
>  

Done.

[David Yen, 2015-12-04 00:12:46]

The CQ bit was checked by dyen@chromium.org

[David Yen, 2015-12-04 00:12:46]

The patchset sent to the CQ was uploaded after l-g-t-m from
dalecurtis@chromium.org, sky@chromium.org, dcheng@chromium.org,
piman@chromium.org
Link to the patchset: https://codereview.chromium.org/1489573003/#ps140001
(title: "Return False on invalid stream id")

[commit-bot: I haz the power, 2015-12-04 00:14:25]

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

[commit-bot: I haz the power, 2015-12-04 02:22:39]

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

[commit-bot: I haz the power, 2015-12-04 02:22:40]

Try jobs failed on following builders:
  android_chromium_gn_compile_dbg on tryserver.chromium.linux (JOB_TIMED_OUT, no
build URL)
  android_chromium_gn_compile_rel on tryserver.chromium.linux (JOB_TIMED_OUT, no
build URL)
  android_clang_dbg_recipe on tryserver.chromium.linux (JOB_TIMED_OUT, no build
URL)
  android_compile_dbg on tryserver.chromium.linux (JOB_TIMED_OUT, no build URL)
  cast_shell_android on tryserver.chromium.linux (JOB_TIMED_OUT, no build URL)
  cast_shell_linux on tryserver.chromium.linux (JOB_TIMED_OUT, no build URL)
  chromeos_amd64-generic_chromium_compile_only_ng on tryserver.chromium.linux
(JOB_TIMED_OUT, no build URL)
  chromeos_daisy_chromium_compile_only_ng on tryserver.chromium.linux
(JOB_TIMED_OUT, no build URL)
  chromeos_x86-generic_chromium_compile_only_ng on tryserver.chromium.linux
(JOB_TIMED_OUT, no build URL)
  linux_android_rel_ng on tryserver.chromium.linux (JOB_TIMED_OUT, no build URL)
  linux_chromium_asan_rel_ng on tryserver.chromium.linux (JOB_TIMED_OUT, no
build URL)
  linux_chromium_chromeos_ozone_rel_ng on tryserver.chromium.linux
(JOB_TIMED_OUT, no build URL)
  linux_chromium_clobber_rel_ng on tryserver.chromium.linux (JOB_TIMED_OUT, no
build URL)
  linux_chromium_compile_dbg_32_ng on tryserver.chromium.linux (JOB_TIMED_OUT,
no build URL)
  linux_chromium_compile_dbg_ng on tryserver.chromium.linux (JOB_TIMED_OUT, no
build URL)
  linux_chromium_gn_chromeos_rel on tryserver.chromium.linux (JOB_TIMED_OUT, no
build URL)
  linux_chromium_rel_ng on tryserver.chromium.linux (JOB_TIMED_OUT, no build
URL)

[David Yen, 2015-12-04 16:41:07]

The CQ bit was checked by dyen@chromium.org

[commit-bot: I haz the power, 2015-12-04 16:41:37]

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

[commit-bot: I haz the power, 2015-12-04 16:47:31]

Description was changed from

==========
Added an extra sync token field for extra command buffer identification.

This CL adds an extra field within sync tokens without changing the size
of the sync token. Currently this extra field is only used for the
GPU channel implementation, storing the stream of the identified command
buffer.

BUG=514815
==========

to

==========
Added an extra sync token field for extra command buffer identification.

This CL adds an extra field within sync tokens without changing the size
of the sync token. Currently this extra field is only used for the
GPU channel implementation, storing the stream of the identified command
buffer.

BUG=514815
==========

[commit-bot: I haz the power, 2015-12-04 16:47:32]

Committed patchset #8 (id:140001)

[commit-bot: I haz the power, 2015-12-04 16:48:31]

Description was changed from

==========
Added an extra sync token field for extra command buffer identification.

This CL adds an extra field within sync tokens without changing the size
of the sync token. Currently this extra field is only used for the
GPU channel implementation, storing the stream of the identified command
buffer.

BUG=514815
==========

to

==========
Added an extra sync token field for extra command buffer identification.

This CL adds an extra field within sync tokens without changing the size
of the sync token. Currently this extra field is only used for the
GPU channel implementation, storing the stream of the identified command
buffer.

BUG=514815

Committed: https://crrev.com/f21e7e6e2b260d56fa54373dd662c81c9d955331
Cr-Commit-Position: refs/heads/master@{#363231}
==========

[commit-bot: I haz the power, 2015-12-04 16:48:32]

Patchset 8 (id:??) landed as
https://crrev.com/f21e7e6e2b260d56fa54373dd662c81c9d955331
Cr-Commit-Position: refs/heads/master@{#363231}

[ajuma, 2015-12-04 20:50:44]

A revert of this CL (patchset #8 id:140001) has been created in
https://codereview.chromium.org/1495893005/ by ajuma@chromium.org.

The reason for reverting is: This is causing a large number of layout tests in
virtual/threaded/animations to crash.

They're hitting the following assertion:
FATAL:scheduler.cc(203)] Check failed: state_machine_.pending_swaps() > 0 (0 vs.
0){"compositor_timing_history":

e.g. see
https://build.chromium.org/p/chromium.webkit/builders/WebKit%20Mac10.7%20%28d...

Confirmed with a local bisect..