vr/gpu: BrowserGpuChannelHostFactory should be called on the same thread.

vr/gpu: BrowserGpuChannelHostFactory should be called on the same thread.

MailboxToSurfaceBridge in vr code lives in the vr-gl thread. It requests
a cc::ContextProvider from content::Compositor, which in turn gets a gpu
channel from BrowserGpuChannelHostFactory. However, the factory lives in
the UI thread, and is not thread-safe. So, impose a restriction on the
factory and content::Compositor that the relevant functions can only be
called from the UI thread. Update the vr code to do the necessary thread
hopping to make sure it calls the functions on the right threads.

BUG=609316
Review-Url: https://codereview.chromium.org/2969153002
Cr-Commit-Position: refs/heads/master@{#484311}
Committed: https://chromium.googlesource.com/chromium/src/+/a4447c621719211e9d8624de2fe62c59f1e1384e

[sadrul, 2017-07-05 02:42:50]

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

[commit-bot: I haz the power, 2017-07-05 02:43:00]

Dry run: CQ is trying da patch.

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

[sadrul, 2017-07-05 02:57:01]

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

[commit-bot: I haz the power, 2017-07-05 02:57: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, 2017-07-05 03:50:40]

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

[commit-bot: I haz the power, 2017-07-05 03:50:40]

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

[sadrul, 2017-07-05 04:08:09]

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

[commit-bot: I haz the power, 2017-07-05 04:08:23]

Dry run: CQ is trying da patch.

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

[sadrul, 2017-07-05 04:47:02]

The CQ bit was unchecked by sadrul@chromium.org

[sadrul, 2017-07-05 04:55:08]

Description was changed from

==========
exp: BrowserGpuChannelHostFactory should be called on the same thread.

...

BUG=...
==========

to

==========
vr/gpu: BrowserGpuChannelHostFactory should be called on the same thread.

MailboxToSurfaceBridge in vr code lives in the vr-gl thread. It requests
a cc::ContextProvider from content::Compositor, which in turn gets a gpu
channel from BrowserGpuChannelHostFactory. However, the factory lives in
the UI thread, and is not thread-safe. So, impose a restriction on the
factory and content::Compositor that the relevant functions can only be
called from the UI thread. Update the vr code to do the necessary thread
hopping to make sure it calls the functions on the right threads.

BUG=609316
==========

[sadrul, 2017-07-05 04:58:04]

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

[commit-bot: I haz the power, 2017-07-05 04:58:18]

Dry run: CQ is trying da patch.

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

[sadrul, 2017-07-05 04:58:20]

Patchset #1 (id:1) has been deleted

[sadrul, 2017-07-05 04:59:38]

sadrul@chromium.org changed reviewers:
+ mthiesse@chromium.org, piman@chromium.org

[sadrul, 2017-07-05 04:59:38]

+mthiesse@ for chrome/ change.

+piman@ for content/ changes.

Thanks!

[commit-bot: I haz the power, 2017-07-05 06:48:52]

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

[commit-bot: I haz the power, 2017-07-05 06:48:53]

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

[sadrul, 2017-07-05 07:05:38]

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

[commit-bot: I haz the power, 2017-07-05 07:05:46]

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, 2017-07-05 08:04:35]

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

[commit-bot: I haz the power, 2017-07-05 08:04:36]

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

[sadrul, 2017-07-05 12:24:24]

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

[commit-bot: I haz the power, 2017-07-05 12:25:05]

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, 2017-07-05 13:27:07]

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

[commit-bot: I haz the power, 2017-07-05 13:27:08]

Dry run: This issue passed the CQ dry run.

[mthiesse, 2017-07-05 14:45:40]

lgtm

https://codereview.chromium.org/2969153002/diff/100001/chrome/browser/android...
File chrome/browser/android/vr_shell/mailbox_to_surface_bridge.cc (right):

https://codereview.chromium.org/2969153002/diff/100001/chrome/browser/android...
chrome/browser/android/vr_shell/mailbox_to_surface_bridge.cc:189:
weak_ptr_factory_.GetWeakPtr(), base::Passed(&surface));
Are we just passing this surface to keep it alive? If so, please add a comment
to this effect as it looks unused and somebody might remove it later :P

[bshe, 2017-07-05 14:56:34]

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

[bshe, 2017-07-05 14:56:35]

I had a similar fix (https://chromium-review.googlesource.com/c/558397/) but
everything is called on GL thread instead of UI thread (I am under the
impression that it should all live on GL thread). +cc klausw@

[bshe, 2017-07-05 15:00:53]

On 2017/07/05 14:57:00, bshe wrote:

This is a better fix. I will rebase on your patch. :)

[sadrul, 2017-07-05 15:04:28]

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

[commit-bot: I haz the power, 2017-07-05 15:04:39]

Dry run: CQ is trying da patch.

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

[sadrul, 2017-07-05 15:05:41]

https://codereview.chromium.org/2969153002/diff/100001/chrome/browser/android...
File chrome/browser/android/vr_shell/mailbox_to_surface_bridge.cc (right):

https://codereview.chromium.org/2969153002/diff/100001/chrome/browser/android...
chrome/browser/android/vr_shell/mailbox_to_surface_bridge.cc:189:
weak_ptr_factory_.GetWeakPtr(), base::Passed(&surface));
On 2017/07/05 14:45:40, mthiesse wrote:
> Are we just passing this surface to keep it alive? If so, please add a comment
> to this effect as it looks unused and somebody might remove it later :P

Yep, good point. Added a comment.

[sadrul, 2017-07-05 15:11:25]

On 2017/07/05 15:00:53, bshe wrote:
> On 2017/07/05 14:57:00, bshe wrote:
> 
> This is a better fix. I will rebase on your patch. :)

Cool, thanks!

[commit-bot: I haz the power, 2017-07-05 16:34:04]

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

[commit-bot: I haz the power, 2017-07-05 16:34:05]

Dry run: This issue passed the CQ dry run.

[piman, 2017-07-05 17:46:52]

content/ LGTM

[sadrul, 2017-07-05 18:10:07]

The CQ bit was checked by sadrul@chromium.org

[sadrul, 2017-07-05 18:10:07]

The patchset sent to the CQ was uploaded after l-g-t-m from
mthiesse@chromium.org
Link to the patchset: https://codereview.chromium.org/2969153002/#ps120001
(title: "comment")

[commit-bot: I haz the power, 2017-07-05 18:10:26]

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, 2017-07-05 18:17:57]

CQ is committing da patch.

Bot data: {"patchset_id": 120001, "attempt_start_ts": 1499278207126590,
"parent_rev": "d92f0f004dbc7afb8fe78436b8caf9e98012f06c", "commit_rev":
"a4447c621719211e9d8624de2fe62c59f1e1384e"}

[commit-bot: I haz the power, 2017-07-05 18:18:14]

Description was changed from

==========
vr/gpu: BrowserGpuChannelHostFactory should be called on the same thread.

MailboxToSurfaceBridge in vr code lives in the vr-gl thread. It requests
a cc::ContextProvider from content::Compositor, which in turn gets a gpu
channel from BrowserGpuChannelHostFactory. However, the factory lives in
the UI thread, and is not thread-safe. So, impose a restriction on the
factory and content::Compositor that the relevant functions can only be
called from the UI thread. Update the vr code to do the necessary thread
hopping to make sure it calls the functions on the right threads.

BUG=609316
==========

to

==========
vr/gpu: BrowserGpuChannelHostFactory should be called on the same thread.

MailboxToSurfaceBridge in vr code lives in the vr-gl thread. It requests
a cc::ContextProvider from content::Compositor, which in turn gets a gpu
channel from BrowserGpuChannelHostFactory. However, the factory lives in
the UI thread, and is not thread-safe. So, impose a restriction on the
factory and content::Compositor that the relevant functions can only be
called from the UI thread. Update the vr code to do the necessary thread
hopping to make sure it calls the functions on the right threads.

BUG=609316

Review-Url: https://codereview.chromium.org/2969153002
Cr-Commit-Position: refs/heads/master@{#484311}
Committed:
https://chromium.googlesource.com/chromium/src/+/a4447c621719211e9d8624de2fe6...
==========

[commit-bot: I haz the power, 2017-07-05 18:18:18]

Committed patchset #6 (id:120001) as
https://chromium.googlesource.com/chromium/src/+/a4447c621719211e9d8624de2fe6...