Re-land: cc: Implement shared worker contexts.

Re-land: cc: Implement shared worker contexts.

This moves the responsibility to call BindToCurrentThread/SetupLock out
of cc::OutputSurface and to the maintainer of the (possibly) shared
context.

OutputSurface now needs to be destroyed on the same thread they
were created. OutputSurface::DetachFromClient() can be used to
destroy any resources that need to be destroyed on the thread
that the OutputSurface has been bound to.

BUG=523411, 525811
CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel

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

[reveman, 2015-09-11 19:03:41]

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

[reveman, 2015-09-11 19:03:42]

Here's an attempt at implementing proper support for shared worker context as
discussed. Please take a look.

[danakj, 2015-09-11 19:49:54]

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

[danakj, 2015-09-11 19:49:55]

https://codereview.chromium.org/1336733002/diff/60001/cc/trees/layer_tree_host.h
File cc/trees/layer_tree_host.h (right):

https://codereview.chromium.org/1336733002/diff/60001/cc/trees/layer_tree_hos...
cc/trees/layer_tree_host.h:437: scoped_ptr<OutputSurface> new_output_surface_;
What's the reasoning for these variables? Why own the output surface here? These
own the output surface for the entire time it's used by the compositor? Can you
explain in some comments?

[reveman, 2015-09-11 22:26:13]

https://codereview.chromium.org/1336733002/diff/60001/cc/trees/layer_tree_host.h
File cc/trees/layer_tree_host.h (right):

https://codereview.chromium.org/1336733002/diff/60001/cc/trees/layer_tree_hos...
cc/trees/layer_tree_host.h:437: scoped_ptr<OutputSurface> new_output_surface_;
On 2015/09/11 at 19:49:55, danakj wrote:
> What's the reasoning for these variables? Why own the output surface here?
These own the output surface for the entire time it's used by the compositor?
Can you explain in some comments?

This is just moving ownership from the compositor thread to the main thread to
ensure that the output surface is always destroyed on the correct thread. I made
sure current_output_surface_ is reset if init fails and added a comment here in
latest patch. Also updated the description to include the most important part of
this patch: output surfaces now have to be destroyed on the thread they were
created on.

https://codereview.chromium.org/1336733002/diff/80001/content/child/child_pro...
File content/child/child_process.cc (right):

https://codereview.chromium.org/1336733002/diff/80001/content/child/child_pro...
content/child/child_process.cc:66: shutdown_event_.Signal();
This needs to happen after Shutdown() as that might destroy a worker context and
that can require sync IPC if a transfer buffer allocation is required to flush
the context.

https://codereview.chromium.org/1336733002/diff/80001/content/renderer/render...
File content/renderer/render_process_impl.cc (left):

https://codereview.chromium.org/1336733002/diff/80001/content/renderer/render...
content/renderer/render_process_impl.cc:78: GetShutDownEvent()->Signal();
See similar change in ChildProcess::~ChildProcess. This is removed so
RenderThreadImpl::Shutdown() can use sync IPC.

[reveman, 2015-09-11 22:26:19]

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

[commit-bot: I haz the power, 2015-09-11 22:27:07]

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

[commit-bot: I haz the power, 2015-09-11 22:45:30]

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

[commit-bot: I haz the power, 2015-09-11 22:45:30]

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

[danakj, 2015-09-12 00:09:14]

cc LGTM % piman

https://codereview.chromium.org/1336733002/diff/80001/cc/output/output_surface.h
File cc/output/output_surface.h (right):

https://codereview.chromium.org/1336733002/diff/80001/cc/output/output_surfac...
cc/output/output_surface.h:119: // Called by the compositor on the compositor
thread. This is a place where
nit: whitespace before the comment please

https://codereview.chromium.org/1336733002/diff/80001/cc/output/output_surfac...
cc/output/output_surface.h:123: bool CalledOnValidClientThread();
Do we need a public method?

https://codereview.chromium.org/1336733002/diff/80001/cc/output/output_surfac...
cc/output/output_surface.h:202: base::ThreadChecker client_thread_checker_;
just make this protected?

https://codereview.chromium.org/1336733002/diff/80001/cc/trees/layer_tree_hos...
File cc/trees/layer_tree_host.cc (right):

https://codereview.chromium.org/1336733002/diff/80001/cc/trees/layer_tree_hos...
cc/trees/layer_tree_host.cc:411: current_output_surface_ = nullptr;
can you leave a comment that if initialize fails, the compositor thread will not
keep around a pointer to either output surface, so it's safe to destroy the old
one here too. else this looks a bit sketchy :)

https://codereview.chromium.org/1336733002/diff/80001/cc/trees/layer_tree_host.h
File cc/trees/layer_tree_host.h (right):

https://codereview.chromium.org/1336733002/diff/80001/cc/trees/layer_tree_hos...
cc/trees/layer_tree_host.h:442: scoped_ptr<OutputSurface> new_output_surface_;
+sievers: From what I can tell there's no good reason that LTH even owns these
after this, we could just have the embedders own the output surface entirely and
tell them when cc is done with it or something.

[reveman, 2015-09-12 12:49:57]

https://codereview.chromium.org/1336733002/diff/80001/cc/output/output_surface.h
File cc/output/output_surface.h (right):

https://codereview.chromium.org/1336733002/diff/80001/cc/output/output_surfac...
cc/output/output_surface.h:119: // Called by the compositor on the compositor
thread. This is a place where
On 2015/09/12 at 00:09:14, danakj wrote:
> nit: whitespace before the comment please

Done.

https://codereview.chromium.org/1336733002/diff/80001/cc/output/output_surfac...
cc/output/output_surface.h:123: bool CalledOnValidClientThread();
On 2015/09/12 at 00:09:14, danakj wrote:
> Do we need a public method?

Good point. Removed.

https://codereview.chromium.org/1336733002/diff/80001/cc/output/output_surfac...
cc/output/output_surface.h:202: base::ThreadChecker client_thread_checker_;
On 2015/09/12 at 00:09:14, danakj wrote:
> just make this protected?

Done.

https://codereview.chromium.org/1336733002/diff/80001/cc/trees/layer_tree_hos...
File cc/trees/layer_tree_host.cc (right):

https://codereview.chromium.org/1336733002/diff/80001/cc/trees/layer_tree_hos...
cc/trees/layer_tree_host.cc:411: current_output_surface_ = nullptr;
On 2015/09/12 at 00:09:14, danakj wrote:
> can you leave a comment that if initialize fails, the compositor thread will
not keep around a pointer to either output surface, so it's safe to destroy the
old one here too. else this looks a bit sketchy :)

Yes, thanks for pointing that out. Added a comment here and in LTHI.

https://codereview.chromium.org/1336733002/diff/80001/cc/trees/layer_tree_host.h
File cc/trees/layer_tree_host.h (right):

https://codereview.chromium.org/1336733002/diff/80001/cc/trees/layer_tree_hos...
cc/trees/layer_tree_host.h:442: scoped_ptr<OutputSurface> new_output_surface_;
On 2015/09/12 at 00:09:14, danakj wrote:
> +sievers: From what I can tell there's no good reason that LTH even owns these
after this, we could just have the embedders own the output surface entirely and
tell them when cc is done with it or something.

Sgtm but I think I prefer to save that for a follow up patch. Let me know if you
want me to add a TODO here.

[reveman, 2015-09-12 12:50:06]

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

[commit-bot: I haz the power, 2015-09-12 12:50:12]

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

[commit-bot: I haz the power, 2015-09-12 13:22:46]

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

[commit-bot: I haz the power, 2015-09-12 13:22:47]

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

[reveman, 2015-09-12 19:47:54]

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

[commit-bot: I haz the power, 2015-09-12 19:48:13]

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

[commit-bot: I haz the power, 2015-09-12 22:23:25]

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

[commit-bot: I haz the power, 2015-09-12 22:23:27]

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

[danakj, 2015-09-14 18:05:46]

https://codereview.chromium.org/1336733002/diff/80001/cc/trees/layer_tree_host.h
File cc/trees/layer_tree_host.h (right):

https://codereview.chromium.org/1336733002/diff/80001/cc/trees/layer_tree_hos...
cc/trees/layer_tree_host.h:442: scoped_ptr<OutputSurface> new_output_surface_;
On 2015/09/12 12:49:57, reveman wrote:
> On 2015/09/12 at 00:09:14, danakj wrote:
> > +sievers: From what I can tell there's no good reason that LTH even owns
these
> after this, we could just have the embedders own the output surface entirely
and
> tell them when cc is done with it or something.
> 
> Sgtm but I think I prefer to save that for a follow up patch. Let me know if
you
> want me to add a TODO here.

Ya that's all I mean. No TODO is fine I think.

I also was thinking that if we made the RequestNewSurface pass back ownership of
the OS (ie make the LTHI pointer null at the time of loss, not at the time of
re-create-and-fail) then we could avoid having scoped_ptrs here on the LTH at
all, and just pass around ownership instead of "ok you can delete it now LTH"
more-or-less, which would be nice.

It changes when the LTHI::OS* pointer is null though, which might be a bit
tricky. Def not suggesting that here.

[no sievers, 2015-09-14 21:18:04]

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

[no sievers, 2015-09-14 21:18:04]

ay

https://codereview.chromium.org/1336733002/diff/80001/cc/trees/layer_tree_host.h
File cc/trees/layer_tree_host.h (right):

https://codereview.chromium.org/1336733002/diff/80001/cc/trees/layer_tree_hos...
cc/trees/layer_tree_host.h:442: scoped_ptr<OutputSurface> new_output_surface_;
Are you suggesting as a follow-up to move the ownership of the active OS back to
LTHI, but release (or return) it early on context lost so that we don't have to
track ownership of two surfaces?

Or are you saying that embedders should really 'own the output surface'
entirely, in which case we wouldn't pass ownership around? But that probably
isn't better, and we'd still need the same APIs to coordinate who is doing what
with it.

[danakj, 2015-09-14 21:22:47]

https://codereview.chromium.org/1336733002/diff/80001/cc/trees/layer_tree_host.h
File cc/trees/layer_tree_host.h (right):

https://codereview.chromium.org/1336733002/diff/80001/cc/trees/layer_tree_hos...
cc/trees/layer_tree_host.h:442: scoped_ptr<OutputSurface> new_output_surface_;
On 2015/09/14 21:18:05, sievers wrote:
> Are you suggesting as a follow-up to move the ownership of the active OS back
to
> LTHI, but release (or return) it early on context lost so that we don't have
to
> track ownership of two surfaces?
> 
> Or are you saying that embedders should really 'own the output surface'
> entirely, in which case we wouldn't pass ownership around? But that probably
> isn't better, and we'd still need the same APIs to coordinate who is doing
what
> with it.

I was thinking the former. Documenting ownership/lifetime with scoped_ptr
passing is nice.

[no sievers, 2015-09-14 21:33:12]

On 2015/09/14 21:22:47, danakj wrote:
>
https://codereview.chromium.org/1336733002/diff/80001/cc/trees/layer_tree_host.h
> File cc/trees/layer_tree_host.h (right):
> 
>
https://codereview.chromium.org/1336733002/diff/80001/cc/trees/layer_tree_hos...
> cc/trees/layer_tree_host.h:442: scoped_ptr<OutputSurface> new_output_surface_;
> On 2015/09/14 21:18:05, sievers wrote:
> > Are you suggesting as a follow-up to move the ownership of the active OS
back
> to
> > LTHI, but release (or return) it early on context lost so that we don't have
> to
> > track ownership of two surfaces?
> > 
> > Or are you saying that embedders should really 'own the output surface'
> > entirely, in which case we wouldn't pass ownership around? But that probably
> > isn't better, and we'd still need the same APIs to coordinate who is doing
> what
> > with it.
> 
> I was thinking the former. Documenting ownership/lifetime with scoped_ptr
> passing is nice.

That sounds great. And the behavioral change (clean up and release OS early on
context lost) unifies the code better with the suggested ReleaseOutputSurface()
API (https://codereview.chromium.org/1336813002/), which I think you had
suggested earlier.

[no sievers, 2015-09-14 21:35:57]

On 2015/09/14 21:33:12, sievers wrote:
> On 2015/09/14 21:22:47, danakj wrote:
> >
>
https://codereview.chromium.org/1336733002/diff/80001/cc/trees/layer_tree_host.h
> > File cc/trees/layer_tree_host.h (right):
> > 
> >
>
https://codereview.chromium.org/1336733002/diff/80001/cc/trees/layer_tree_hos...
> > cc/trees/layer_tree_host.h:442: scoped_ptr<OutputSurface>
new_output_surface_;
> > On 2015/09/14 21:18:05, sievers wrote:
> > > Are you suggesting as a follow-up to move the ownership of the active OS
> back
> > to
> > > LTHI, but release (or return) it early on context lost so that we don't
have
> > to
> > > track ownership of two surfaces?
> > > 
> > > Or are you saying that embedders should really 'own the output surface'
> > > entirely, in which case we wouldn't pass ownership around? But that
probably
> > > isn't better, and we'd still need the same APIs to coordinate who is doing
> > what
> > > with it.
> > 
> > I was thinking the former. Documenting ownership/lifetime with scoped_ptr
> > passing is nice.
> 
> That sounds great. And the behavioral change (clean up and release OS early on
> context lost) unifies the code better with the suggested
ReleaseOutputSurface()
> API (https://codereview.chromium.org/1336813002/), which I think you had
> suggested earlier.

sorry, i meant https://codereview.chromium.org/1287043002/

[piman, 2015-09-14 21:54:39]

I like this approach. One main thing (the shutdown_event_ signal), the rest is
nits.

https://codereview.chromium.org/1336733002/diff/120001/cc/trees/layer_tree_ho...
File cc/trees/layer_tree_host.cc (right):

https://codereview.chromium.org/1336733002/diff/120001/cc/trees/layer_tree_ho...
cc/trees/layer_tree_host.cc:393:
proxy_->SetOutputSurface(new_output_surface_.get());
Would it be possible to add a test that verifies that it's possible to destroy
the LTH after SetOutputSurface was called but before we got a
DidInitializeOutputSurface/DidFailToInitializeOutputSurface?

https://codereview.chromium.org/1336733002/diff/120001/content/browser/compos...
File content/browser/compositor/gpu_process_transport_factory.cc (right):

https://codereview.chromium.org/1336733002/diff/120001/content/browser/compos...
content/browser/compositor/gpu_process_transport_factory.cc:252:
shared_worker_context_provider_lost = true;
nit: why the change? As opposed to just resetting
shared_worker_context_provider_? It'll be reset below anyway. I feel like I may
be missing something (order of operations?) in which case a comment would be
really useful.

https://codereview.chromium.org/1336733002/diff/120001/content/child/child_pr...
File content/child/child_process.cc (right):

https://codereview.chromium.org/1336733002/diff/120001/content/child/child_pr...
content/child/child_process.cc:60: main_thread_->Shutdown();
I think you still want to do this after shutdown_event_.Signal(), which is there
to avoid deadlocks (e.g. some thread sending a sync message via a
SyncMessageFilter, and the main thread waiting for that thread to finish).

[reveman, 2015-09-15 20:38:35]

PTAL

https://codereview.chromium.org/1336733002/diff/120001/cc/trees/layer_tree_ho...
File cc/trees/layer_tree_host.cc (right):

https://codereview.chromium.org/1336733002/diff/120001/cc/trees/layer_tree_ho...
cc/trees/layer_tree_host.cc:393:
proxy_->SetOutputSurface(new_output_surface_.get());
On 2015/09/14 at 21:54:38, piman (slow to review) wrote:
> Would it be possible to add a test that verifies that it's possible to destroy
the LTH after SetOutputSurface was called but before we got a
DidInitializeOutputSurface/DidFailToInitializeOutputSurface?

Done. LayerTreeHostTestDestroyWhileInitializingOutputSurface.

https://codereview.chromium.org/1336733002/diff/120001/content/browser/compos...
File content/browser/compositor/gpu_process_transport_factory.cc (right):

https://codereview.chromium.org/1336733002/diff/120001/content/browser/compos...
content/browser/compositor/gpu_process_transport_factory.cc:252:
shared_worker_context_provider_lost = true;
On 2015/09/14 at 21:54:38, piman (slow to review) wrote:
> nit: why the change? As opposed to just resetting
shared_worker_context_provider_? It'll be reset below anyway. I feel like I may
be missing something (order of operations?) in which case a comment would be
really useful.

To avoid deleting the context while holding the lock and deleting a lock that is
currently acquired. Added a comment.

https://codereview.chromium.org/1336733002/diff/120001/content/child/child_pr...
File content/child/child_process.cc (right):

https://codereview.chromium.org/1336733002/diff/120001/content/child/child_pr...
content/child/child_process.cc:60: main_thread_->Shutdown();
On 2015/09/14 at 21:54:38, piman (slow to review) wrote:
> I think you still want to do this after shutdown_event_.Signal(), which is
there to avoid deadlocks (e.g. some thread sending a sync message via a
SyncMessageFilter, and the main thread waiting for that thread to finish).

Done. But that meant allowing ChildThreadImpl::AllocateSharedMemory to fail
without a NOTREACHED during shutdown.

[reveman, 2015-09-15 20:38:41]

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

[commit-bot: I haz the power, 2015-09-15 20:39:28]

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

[piman, 2015-09-15 22:40:43]

lgtm

[commit-bot: I haz the power, 2015-09-15 23:30:34]

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

[commit-bot: I haz the power, 2015-09-15 23:30:35]

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

[reveman, 2015-09-16 18:59:51]

The CQ bit was checked by reveman@chromium.org

[reveman, 2015-09-16 18:59:52]

The patchset sent to the CQ was uploaded after l-g-t-m from danakj@chromium.org
Link to the patchset: https://codereview.chromium.org/1336733002/#ps140001
(title: "unit test + comments + shutdown change")

[commit-bot: I haz the power, 2015-09-16 19:01:19]

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

[commit-bot: I haz the power, 2015-09-16 21:18:10]

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

[commit-bot: I haz the power, 2015-09-16 21:18:11]

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

[reveman, 2015-09-16 21:42:49]

The CQ bit was checked by reveman@chromium.org

[reveman, 2015-09-16 21:42:49]

The patchset sent to the CQ was uploaded after l-g-t-m from danakj@chromium.org,
piman@chromium.org
Link to the patchset: https://codereview.chromium.org/1336733002/#ps160001
(title: "add missing changes to
MailboxOutputSurface+SynchronousCompositorOutputSurface")

[commit-bot: I haz the power, 2015-09-16 21:43:26]

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

[commit-bot: I haz the power, 2015-09-16 23:23:39]

Committed patchset #9 (id:160001)

[commit-bot: I haz the power, 2015-09-16 23:24:22]

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

[Ken Russell (switch to Gerrit), 2015-09-17 01:41:03]

A revert of this CL (patchset #9 id:160001) has been created in
https://codereview.chromium.org/1356463002/ by kbr@chromium.org.

The reason for reverting is: Broke GPU rasterization on Mac OS; please see
https://code.google.com/p/chromium/issues/detail?id=523411#c26 .

I'm really sorry -- I don't know how this got past the CQ.
.

[Justin Donnelly, 2015-09-17 04:25:45]

On 2015/09/17 01:41:03, Ken Russell wrote:
> A revert of this CL (patchset #9 id:160001) has been created in
> https://codereview.chromium.org/1356463002/ by mailto:kbr@chromium.org.
> 
> The reason for reverting is: Broke GPU rasterization on Mac OS; please see
> https://code.google.com/p/chromium/issues/detail?id=523411#c26 .
> 
> I'm really sorry -- I don't know how this got past the CQ.
> .

Thanks for the revert
-- Chromium Sheriff

[boliu, 2015-09-17 20:58:09]

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

[boliu, 2015-09-17 20:58:10]

https://codereview.chromium.org/1336733002/diff/180001/content/browser/androi...
File content/browser/android/in_process/synchronous_compositor_factory_impl.cc
(right):

https://codereview.chromium.org/1336733002/diff/180001/content/browser/androi...
content/browser/android/in_process/synchronous_compositor_factory_impl.cc:184:
if (use_ipc_command_buffer_ &&
FYI we found a blocker in android webview for ipc command buffer
(crbug.com/532620), so ipc command buffer is being reverted again, and
!use_ipc_command_buffer_ need to keep working for awhile longer :(

[reveman, 2015-09-18 14:51:33]

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

[reveman, 2015-09-18 14:51:35]

PTAL.

Previous patch caused GPU raster tests to fail on some Macs. That has been fixed
in latest patch. See my comment below.

https://codereview.chromium.org/1336733002/diff/180001/content/browser/androi...
File content/browser/android/in_process/synchronous_compositor_factory_impl.cc
(right):

https://codereview.chromium.org/1336733002/diff/180001/content/browser/androi...
content/browser/android/in_process/synchronous_compositor_factory_impl.cc:184:
if (use_ipc_command_buffer_ &&
On 2015/09/17 at 20:58:10, boliu wrote:
> FYI we found a blocker in android webview for ipc command buffer
(crbug.com/532620), so ipc command buffer is being reverted again, and
!use_ipc_command_buffer_ need to keep working for awhile longer :(

Added !use_ipc_command_buffer_ support to latest patch.

https://codereview.chromium.org/1336733002/diff/220001/content/child/child_th...
File content/child/child_thread_impl.cc (right):

https://codereview.chromium.org/1336733002/diff/220001/content/child/child_th...
content/child/child_thread_impl.cc:609: return nullptr;
Note: I can't have a ChildProcess::current() call here to like the previous
patch as this will be called from multiple threads.

https://codereview.chromium.org/1336733002/diff/220001/content/common/gpu/gpu...
File content/common/gpu/gpu_channel.cc (right):

https://codereview.chromium.org/1336733002/diff/220001/content/common/gpu/gpu...
content/common/gpu/gpu_channel.cc:1022: init_params.attribs,
init_params.gpu_preference, kUseVirtualizedGLContext,
Not sure exactly why non-virtualized contexts are causing drawing artifacts on
some mac machines when using GPU raster but we probably want to virtualize these
worker contexts for performance reasons anyhow. Do we need to limit this to
worker contexts or is it ok to virtualize all offscreen contexts like I'm doing
in this patch?

[reveman, 2015-09-18 14:51:41]

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

[commit-bot: I haz the power, 2015-09-18 14:51:58]

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

[commit-bot: I haz the power, 2015-09-18 14:54:17]

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

[commit-bot: I haz the power, 2015-09-18 14:54:19]

Dry run: Try jobs failed on following builders:
  linux_chromium_compile_dbg_32_ng on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  ios_dbg_simulator_ninja on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_dbg_simulator...)
  ios_rel_device_ninja on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_rel_device_ni...)
  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_...)

[reveman, 2015-09-18 15:02:19]

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

[commit-bot: I haz the power, 2015-09-18 15:02:59]

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

[commit-bot: I haz the power, 2015-09-18 15:21:06]

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

[commit-bot: I haz the power, 2015-09-18 15:21:07]

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

[reveman, 2015-09-18 18:49:56]

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

[commit-bot: I haz the power, 2015-09-18 18:50:13]

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

[commit-bot: I haz the power, 2015-09-18 20:03:32]

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

[commit-bot: I haz the power, 2015-09-18 20:03:33]

Dry run: This issue passed the CQ dry run.

[ccameron, 2015-09-18 20:18:53]

lgtm % nits -- also should ping kbr@ about any concerns about putting WebGL (et
al) in the shared virtualized GL context -- it has the potential to be fairly
disruptive (hence my preference to do it in a separate patch).

https://codereview.chromium.org/1336733002/diff/220001/content/common/gpu/gpu...
File content/common/gpu/gpu_channel.cc (right):

https://codereview.chromium.org/1336733002/diff/220001/content/common/gpu/gpu...
content/common/gpu/gpu_channel.cc:1022: init_params.attribs,
init_params.gpu_preference, kUseVirtualizedGLContext,
On 2015/09/18 14:51:35, reveman wrote:
> Not sure exactly why non-virtualized contexts are causing drawing artifacts on
> some mac machines when using GPU raster but we probably want to virtualize
these
> worker contexts for performance reasons anyhow. Do we need to limit this to
> worker contexts or is it ok to virtualize all offscreen contexts like I'm
doing
> in this patch?

When we initially switched to virtualized contexts, we virtualized only
compositor contexts because only they were causing problems WRT creating too
many GL contexts.

I have any good reason to keep doing things this way (kbr may have thoughts on
the issue, though). It may be wise to make this change in a separate patch, to
isolate failures / separate moving parts.

https://codereview.chromium.org/1336733002/diff/260001/content/common/gpu/gpu...
File content/common/gpu/gpu_channel.cc (right):

https://codereview.chromium.org/1336733002/diff/260001/content/common/gpu/gpu...
content/common/gpu/gpu_channel.cc:73: // Virtualize contexts by defualt on OS X
to prevent performance regressions
s/defualt/default

[boliu, 2015-09-18 20:31:25]

New changes to support use_ipc_command_buffer_ in webview lgtm

[piman, 2015-09-18 21:07:31]

On Fri, Sep 18, 2015 at 1:18 PM, <ccameron@chromium.org> wrote:

> lgtm % nits -- also should ping kbr@ about any concerns about putting
> WebGL (et
> al) in the shared virtualized GL context -- it has the potential to be
> fairly
> disruptive (hence my preference to do it in a separate patch).


I share the concern, in particular in relation to GPU switching.
See comments in:
https://code.google.com/p/chromium/codesearch#chromium/src/gpu/command_buffer...
https://code.google.com/p/chromium/codesearch#chromium/src/gpu/command_buffer...

Basically, I think we get away with it for onscreen contexts because they
all allow switching and they're the only ones in the virtual group. I
suspect putting everybody into the same virtual group will start causing
issues because of the forced switches.

Also, the forced switches may negate the potential savings of virtual
contexts.

I suspect the GPU switching logic would need some work to accommodate this
(e.g. have separate virtual groups for discrete vs integrated, and avoid
forced switches that way?).



>
>
https://codereview.chromium.org/1336733002/diff/220001/content/common/gpu/gpu...
> File content/common/gpu/gpu_channel.cc (right):
>
>
>
https://codereview.chromium.org/1336733002/diff/220001/content/common/gpu/gpu...
> content/common/gpu/gpu_channel.cc:1022: init_params.attribs,
> init_params.gpu_preference, kUseVirtualizedGLContext,
> On 2015/09/18 14:51:35, reveman wrote:
>
>> Not sure exactly why non-virtualized contexts are causing drawing
>>
> artifacts on
>
>> some mac machines when using GPU raster but we probably want to
>>
> virtualize these
>
>> worker contexts for performance reasons anyhow. Do we need to limit
>>
> this to
>
>> worker contexts or is it ok to virtualize all offscreen contexts like
>>
> I'm doing
>
>> in this patch?
>>
>
> When we initially switched to virtualized contexts, we virtualized only
> compositor contexts because only they were causing problems WRT creating
> too many GL contexts.
>
> I have any good reason to keep doing things this way (kbr may have
> thoughts on the issue, though). It may be wise to make this change in a
> separate patch, to isolate failures / separate moving parts.
>
>
>
https://codereview.chromium.org/1336733002/diff/260001/content/common/gpu/gpu...
> File content/common/gpu/gpu_channel.cc (right):
>
>
>
https://codereview.chromium.org/1336733002/diff/260001/content/common/gpu/gpu...
> content/common/gpu/gpu_channel.cc:73: // Virtualize contexts by defualt
> on OS X to prevent performance regressions
> s/defualt/default
>
> https://codereview.chromium.org/1336733002/
>

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[reveman, 2015-09-23 15:04:16]

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

[commit-bot: I haz the power, 2015-09-23 15:04:39]

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

[commit-bot: I haz the power, 2015-09-23 15:13:22]

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

[commit-bot: I haz the power, 2015-09-23 15:13:23]

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

[reveman, 2015-09-23 16:07:41]

The CQ bit was checked by reveman@chromium.org

[reveman, 2015-09-23 16:07:42]

FYI, I've verified that latest patch does not cause the issue that was the
reason for the revert and will now try to re-land this.

[reveman, 2015-09-23 16:07:43]

The patchset sent to the CQ was uploaded after l-g-t-m from danakj@chromium.org,
piman@chromium.org, boliu@chromium.org, ccameron@chromium.org
Link to the patchset: https://codereview.chromium.org/1336733002/#ps300001
(title: "rebase")

[commit-bot: I haz the power, 2015-09-23 16:08:31]

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

[commit-bot: I haz the power, 2015-09-23 16:15:39]

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

[commit-bot: I haz the power, 2015-09-23 16:15:40]

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

[reveman, 2015-09-23 16:52:25]

The CQ bit was checked by reveman@chromium.org

[reveman, 2015-09-23 16:52:26]

The patchset sent to the CQ was uploaded after l-g-t-m from danakj@chromium.org,
ccameron@chromium.org, boliu@chromium.org, piman@chromium.org
Link to the patchset: https://codereview.chromium.org/1336733002/#ps320001
(title: "Fix LayerTreeHostClientTakeAwayOutputSurface test. Content provider is
always destroyed on the clie…")

[commit-bot: I haz the power, 2015-09-23 16:53:11]

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

[commit-bot: I haz the power, 2015-09-23 17:31:49]

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

[commit-bot: I haz the power, 2015-09-23 17:31:50]

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

[reveman, 2015-09-23 18:06:59]

The CQ bit was checked by reveman@chromium.org

[commit-bot: I haz the power, 2015-09-23 18:12:15]

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

[commit-bot: I haz the power, 2015-09-23 19:21:22]

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

[commit-bot: I haz the power, 2015-09-23 19:21:24]

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

[reveman, 2015-09-23 19:47:52]

The CQ bit was checked by reveman@chromium.org

[commit-bot: I haz the power, 2015-09-23 19:48:55]

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

[commit-bot: I haz the power, 2015-09-23 20:20:14]

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

[commit-bot: I haz the power, 2015-09-23 20:20:15]

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

[reveman, 2015-09-23 23:20:50]

The CQ bit was checked by reveman@chromium.org

[commit-bot: I haz the power, 2015-09-23 23:21:46]

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

[commit-bot: I haz the power, 2015-09-24 00:19:57]

Committed patchset #17 (id:320001)

[commit-bot: I haz the power, 2015-09-24 00:35:13]

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

[msw, 2015-09-24 02:26:07]

On 2015/09/24 00:35:13, commit-bot: I haz the power wrote:
> Patchset 17 (id:??) landed as
> https://crrev.com/d180dfc3198c1e58a8c10f7349f3d949e22c5f3d
> Cr-Commit-Position: refs/heads/master@{#350409}

Is it possible that this broke mojo_apptests on Linux GN (sorry but there hasn't
been CQ coverage for a short time), here's the log and sample failure output:
http://build.chromium.org/p/chromium.linux/builders/Linux%20GN/builds/32378/s...

[  FAILED  ] Command:
/mnt/data/b/build/slave/Linux_GN/build/src/out/Release/mojo_runner
--use-temporary-user-data-dir --enable-html-viewer-test-interface
--use-x11-test-config --override-use-gl-with-osmesa-for-tests
--gtest_filter=AXProviderTest.HelloWorld mojo:html_viewer_apptests
------------------------------------------------------------------------
Note: Google Test filter = AXProviderTest.HelloWorld
[==========] Running 1 test from 1 test case.
[----------] Global test environment set-up.
[----------] 1 test from AXProviderTest
[ RUN      ] AXProviderTest.HelloWorld
HTTP server started on http://127.0.0.1:37323...
sending server_data: {"host": "127.0.0.1", "port": 37323} (36 bytes)

Error: Test exited with code: -11
[0923/180718:ERROR:command_buffer_local.cc(219)] Not implemented reached in
virtual uint64_t mus::CommandBufferLocal::GetCommandBufferID() const
[ERROR:command_buffer_client_impl.cc(407)] Not implemented reached in virtual
uint64_t gles2::CommandBufferClientImpl::GetCommandBufferID() const
[30763:30796:0923/180720:ERROR:layer_tree_host_impl.cc(2106)] Forcing zero-copy
tile initialization as worker context is missing
[30763:30775:0923/180720:ERROR:nss_util.cc(853)] After loading Root Certs,
loaded==false: NSS error code: -8018
Received signal 11 SEGV_MAPERR 000000000020

------------------------------------------------------------------------

[msw, 2015-09-24 05:24:49]

On 2015/09/24 02:26:07, msw wrote:
> On 2015/09/24 00:35:13, commit-bot: I haz the power wrote:
> > Patchset 17 (id:??) landed as
> > https://crrev.com/d180dfc3198c1e58a8c10f7349f3d949e22c5f3d
> > Cr-Commit-Position: refs/heads/master@{#350409}
> 
> Is it possible that this broke mojo_apptests on Linux GN (sorry but there
hasn't
> been CQ coverage for a short time), here's the log and sample failure output:
>
http://build.chromium.org/p/chromium.linux/builders/Linux%20GN/builds/32378/s...
> 
> [  FAILED  ] Command:
> /mnt/data/b/build/slave/Linux_GN/build/src/out/Release/mojo_runner
> --use-temporary-user-data-dir --enable-html-viewer-test-interface
> --use-x11-test-config --override-use-gl-with-osmesa-for-tests
> --gtest_filter=AXProviderTest.HelloWorld mojo:html_viewer_apptests
> ------------------------------------------------------------------------
> Note: Google Test filter = AXProviderTest.HelloWorld
> [==========] Running 1 test from 1 test case.
> [----------] Global test environment set-up.
> [----------] 1 test from AXProviderTest
> [ RUN      ] AXProviderTest.HelloWorld
> HTTP server started on http://127.0.0.1:37323...
> sending server_data: {"host": "127.0.0.1", "port": 37323} (36 bytes)
> 
> Error: Test exited with code: -11
> [0923/180718:ERROR:command_buffer_local.cc(219)] Not implemented reached in
> virtual uint64_t mus::CommandBufferLocal::GetCommandBufferID() const
> [ERROR:command_buffer_client_impl.cc(407)] Not implemented reached in virtual
> uint64_t gles2::CommandBufferClientImpl::GetCommandBufferID() const
> [30763:30796:0923/180720:ERROR:layer_tree_host_impl.cc(2106)] Forcing
zero-copy
> tile initialization as worker context is missing
> [30763:30775:0923/180720:ERROR:nss_util.cc(853)] After loading Root Certs,
> loaded==false: NSS error code: -8018
> Received signal 11 SEGV_MAPERR 000000000020
> 
> ------------------------------------------------------------------------

Those errors should be fixed by https://codereview.chromium.org/1362243002/