content: Remove base::WaitableEvent from ChildIOSurfaceManager.

content: Remove base::WaitableEvent from ChildIOSurfaceManager.

The token should always be set before AcquireIOSurface is called
as the message with the token is handled on the same thread as
messages that create render widgets and AcquireIOSurface should
never be called before a render widget has been created.

BUG=

Committed: https://crrev.com/c31b72bcf79c48426d3854abd5c4ee5f16abf752
Cr-Commit-Position: refs/heads/master@{#345653}

[reveman, 2015-08-26 01:53:26]

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

[reveman, 2015-08-26 01:53:34]

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

[commit-bot: I haz the power, 2015-08-26 01:53:51]

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

[commit-bot: I haz the power, 2015-08-26 02:26:40]

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

[commit-bot: I haz the power, 2015-08-26 02:26:41]

Dry run: This issue passed the CQ dry run.

[dshwang, 2015-08-26 13:07:18]

dongseong.hwang@intel.com changed reviewers:
+ dongseong.hwang@intel.com

[dshwang, 2015-08-26 13:07:18]

https://codereview.chromium.org/1312353002/diff/1/content/child/child_io_surf...
File content/child/child_io_surface_manager_mac.h (left):

https://codereview.chromium.org/1312353002/diff/1/content/child/child_io_surf...
content/child/child_io_surface_manager_mac.h:40: token_ = token;
Don't we need lock? IO thread set it and any thread (e.g. worker, impl) will
read it.
Currently WaitableEvent makes sure visibility between threads, but not anymore.

[reveman, 2015-08-26 14:38:21]

https://codereview.chromium.org/1312353002/diff/1/content/child/child_io_surf...
File content/child/child_io_surface_manager_mac.h (left):

https://codereview.chromium.org/1312353002/diff/1/content/child/child_io_surf...
content/child/child_io_surface_manager_mac.h:40: token_ = token;
On 2015/08/26 at 13:07:18, dshwang_ooo_5.9-27.9 wrote:
> Don't we need lock? IO thread set it and any thread (e.g. worker, impl) will
read it.
> Currently WaitableEvent makes sure visibility between threads, but not
anymore.

A mutex here would be sad as all other code is thread safe. I think the user of
this API should make sure there's no race. It's currently guaranteed by the fact
that both setting this token and anything that can result in an IOSurface being
acquired is a result of some processing on the same thread.

[Robert Sesek, 2015-08-26 17:51:26]

Offline discussion with David: this used to be a possible race condition, but
since switching to an IPC message filter ensures that we don't have that race
potential anymore.

https://codereview.chromium.org/1312353002/diff/1/content/child/child_io_surf...
File content/child/child_io_surface_manager_mac.h (left):

https://codereview.chromium.org/1312353002/diff/1/content/child/child_io_surf...
content/child/child_io_surface_manager_mac.h:40: token_ = token;
On 2015/08/26 14:38:21, reveman wrote:
> On 2015/08/26 at 13:07:18, dshwang_ooo_5.9-27.9 wrote:
> > Don't we need lock? IO thread set it and any thread (e.g. worker, impl) will
> read it.
> > Currently WaitableEvent makes sure visibility between threads, but not
> anymore.
> 
> A mutex here would be sad as all other code is thread safe. I think the user
of
> this API should make sure there's no race. It's currently guaranteed by the
fact
> that both setting this token and anything that can result in an IOSurface
being
> acquired is a result of some processing on the same thread.

Can you DCHECK that this is supposed to be called on a current thread, then?

[reveman, 2015-08-26 19:40:33]

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

[reveman, 2015-08-26 19:40:34]

+avi for owner rs

https://codereview.chromium.org/1312353002/diff/1/content/child/child_io_surf...
File content/child/child_io_surface_manager_mac.h (left):

https://codereview.chromium.org/1312353002/diff/1/content/child/child_io_surf...
content/child/child_io_surface_manager_mac.h:40: token_ = token;
On 2015/08/26 at 17:51:26, Robert Sesek wrote:
> On 2015/08/26 14:38:21, reveman wrote:
> > On 2015/08/26 at 13:07:18, dshwang_ooo_5.9-27.9 wrote:
> > > Don't we need lock? IO thread set it and any thread (e.g. worker, impl)
will
> > read it.
> > > Currently WaitableEvent makes sure visibility between threads, but not
> > anymore.
> > 
> > A mutex here would be sad as all other code is thread safe. I think the user
of
> > this API should make sure there's no race. It's currently guaranteed by the
fact
> > that both setting this token and anything that can result in an IOSurface
being
> > acquired is a result of some processing on the same thread.
> 
> Can you DCHECK that this is supposed to be called on a current thread, then?

As discussed, we can't add a DCHECK for this but I made it more clear by adding
a comment to the header.

[Avi (use Gerrit), 2015-08-26 19:45:02]

lgtm

OK

[reveman, 2015-08-26 19:58:08]

The CQ bit was checked by reveman@chromium.org

[commit-bot: I haz the power, 2015-08-26 19:59:05]

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

[Robert Sesek, 2015-08-26 20:15:05]

lgtm

[commit-bot: I haz the power, 2015-08-26 20:15:21]

Committed patchset #2 (id:20001)

[commit-bot: I haz the power, 2015-08-26 20:16:04]

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