Support mojo applications in GPU process.

Support mojo applications in GPU process.

BUG=521755
TEST=Manually tested MojoMediaApplication in GPU process and GPU thread in the browser process (--in-process-gpu).

Committed: https://crrev.com/9c8e128c413c383fc9d4b9cd8175e0d5673214f2
Cr-Commit-Position: refs/heads/master@{#353453}

[xhwang, 2015-08-20 19:01:15]

xhwang@chromium.org changed reviewers:
+ jam@chromium.org, rockot@chromium.org

[xhwang, 2015-08-20 19:01:16]

I am basically copying what we are doing in the utility process to the GPU
process. I tested it with MojoMediaApplication and it's working :)

But I am very new to the GPU world so please pay extra attention when reviewing
this CL.

PTAL

[xhwang, 2015-10-02 21:08:55]

This CL is rebased and tested. It's ready for full review. PTAL!

[Ken Rockot(use gerrit already), 2015-10-02 21:34:51]

lgtm

https://codereview.chromium.org/1297953004/diff/40001/content/gpu/gpu_child_t...
File content/gpu/gpu_child_thread.h (right):

https://codereview.chromium.org/1297953004/diff/40001/content/gpu/gpu_child_t...
content/gpu/gpu_child_thread.h:22: #include
"content/gpu/gpu_process_control_impl.h"
nit: could just forward declare this?

[xhwang, 2015-10-02 21:43:42]

rebase only

[xhwang, 2015-10-02 22:01:07]

comments addressed

[xhwang, 2015-10-02 22:07:48]

https://codereview.chromium.org/1297953004/diff/40001/content/gpu/gpu_child_t...
File content/gpu/gpu_child_thread.h (right):

https://codereview.chromium.org/1297953004/diff/40001/content/gpu/gpu_child_t...
content/gpu/gpu_child_thread.h:22: #include
"content/gpu/gpu_process_control_impl.h"
On 2015/10/02 21:34:51, Ken Rockot wrote:
> nit: could just forward declare this?

Done.

[xhwang, 2015-10-02 22:08:04]

jam: Please OWNERS review!

[xhwang, 2015-10-05 21:14:36]

On 2015/10/02 22:08:04, xhwang wrote:
> jam: Please OWNERS review!

jam: kindly ping :)

[jam, 2015-10-06 19:08:05]

jam@chromium.org changed reviewers:
+ piman@chromium.org, sievers@chromium.org

[jam, 2015-10-06 19:08:05]

either sievers or piman would be better reviewers as it's related to gpu

[xhwang, 2015-10-06 19:13:23]

On 2015/10/06 19:08:05, jam wrote:
> either sievers or piman would be better reviewers as it's related to gpu

jam: Thanks for finding better reviews.

sievers: PTAL as piman@ is marked as "slow to review" :)

[piman, 2015-10-06 19:59:36]

https://codereview.chromium.org/1297953004/diff/60002/content/browser/gpu/gpu...
File content/browser/gpu/gpu_process_host.cc (right):

https://codereview.chromium.org/1297953004/diff/60002/content/browser/gpu/gpu...
content/browser/gpu/gpu_process_host.cc:590: CHECK(!mojo_application_host_);
DCHECK

https://codereview.chromium.org/1297953004/diff/60002/content/browser/gpu/gpu...
content/browser/gpu/gpu_process_host.cc:888: if (mojo_application_host_) {
When would this be false?

https://codereview.chromium.org/1297953004/diff/60002/content/browser/gpu/gpu...
content/browser/gpu/gpu_process_host.cc:911: if (mojo_application_host_)
When would this be false?

https://codereview.chromium.org/1297953004/diff/60002/content/browser/mojo/mo...
File content/browser/mojo/mojo_shell_context.cc (right):

https://codereview.chromium.org/1297953004/diff/60002/content/browser/mojo/mo...
content/browser/mojo/mojo_shell_context.cc:112: CAUSE_FOR_GPU_LAUNCH_NO_LAUNCH);
What happens if the GPU process is not launched yet? What happens when the GPU
process dies and is restarted?

https://codereview.chromium.org/1297953004/diff/60002/content/browser/mojo/mo...
content/browser/mojo/mojo_shell_context.cc:114: LOG(ERROR) << "GPU process host
not available.";
DLOG

https://codereview.chromium.org/1297953004/diff/60002/content/browser/mojo/mo...
content/browser/mojo/mojo_shell_context.cc:119: if (!service_registry) {
When would this be false?

https://codereview.chromium.org/1297953004/diff/60002/content/browser/mojo/mo...
content/browser/mojo/mojo_shell_context.cc:120: LOG(ERROR) << "ServiceRegistry
not available in GPU process host.";
DLOG

https://codereview.chromium.org/1297953004/diff/60002/content/gpu/gpu_child_t...
File content/gpu/gpu_child_thread.cc (right):

https://codereview.chromium.org/1297953004/diff/60002/content/gpu/gpu_child_t...
content/gpu/gpu_child_thread.cc:189: &GpuChildThread::BindProcessControlRequest,
base::Unretained(this)));
What makes Unretained safe?

[xhwang, 2015-10-07 19:07:58]

piman: PTAL again!

https://chromiumcodereview.appspot.com/1297953004/diff/60002/content/browser/...
File content/browser/gpu/gpu_process_host.cc (right):

https://chromiumcodereview.appspot.com/1297953004/diff/60002/content/browser/...
content/browser/gpu/gpu_process_host.cc:590: CHECK(!mojo_application_host_);
On 2015/10/06 19:59:35, piman (slow to review) wrote:
> DCHECK

Done.

https://chromiumcodereview.appspot.com/1297953004/diff/60002/content/browser/...
content/browser/gpu/gpu_process_host.cc:888: if (mojo_application_host_) {
On 2015/10/06 19:59:35, piman (slow to review) wrote:
> When would this be false?

This should never be false. Removed the if statement.

https://chromiumcodereview.appspot.com/1297953004/diff/60002/content/browser/...
content/browser/gpu/gpu_process_host.cc:911: if (mojo_application_host_)
On 2015/10/06 19:59:35, piman (slow to review) wrote:
> When would this be false?

Done.

https://chromiumcodereview.appspot.com/1297953004/diff/60002/content/browser/...
File content/browser/mojo/mojo_shell_context.cc (right):

https://chromiumcodereview.appspot.com/1297953004/diff/60002/content/browser/...
content/browser/mojo/mojo_shell_context.cc:112: CAUSE_FOR_GPU_LAUNCH_NO_LAUNCH);
On 2015/10/06 19:59:35, piman (slow to review) wrote:
> What happens if the GPU process is not launched yet? 

Great question. If the process is not launched yet, |process_host| will be null
and the |request| will be dropped. 

Actually I am not familiar with these flags, should whoever needs the GPU
process start it if it's not launched yet? If so, I can add another cause,
something like CAUSE_FOR_GPU_LAUNCH_MOJO_SETUP

> What happens when the GPU process dies and is restarted?

When the GPU process dies, all mojo applications hosted in it will be gone and
all clients of those apps (in other processes) will get notified (via a
connection error). It's the client's responsibility to reconnect to the
services. Actually this is a great case where we want to launch the GPU process
if it's not launched yet (on line 112).

https://chromiumcodereview.appspot.com/1297953004/diff/60002/content/browser/...
content/browser/mojo/mojo_shell_context.cc:114: LOG(ERROR) << "GPU process host
not available.";
On 2015/10/06 19:59:35, piman (slow to review) wrote:
> DLOG

Done.

https://chromiumcodereview.appspot.com/1297953004/diff/60002/content/browser/...
content/browser/mojo/mojo_shell_context.cc:119: if (!service_registry) {
On 2015/10/06 19:59:35, piman (slow to review) wrote:
> When would this be false?

Done.

https://chromiumcodereview.appspot.com/1297953004/diff/60002/content/browser/...
content/browser/mojo/mojo_shell_context.cc:120: LOG(ERROR) << "ServiceRegistry
not available in GPU process host.";
On 2015/10/06 19:59:35, piman (slow to review) wrote:
> DLOG

Obsolete now.

https://chromiumcodereview.appspot.com/1297953004/diff/60002/content/gpu/gpu_...
File content/gpu/gpu_child_thread.cc (right):

https://chromiumcodereview.appspot.com/1297953004/diff/60002/content/gpu/gpu_...
content/gpu/gpu_child_thread.cc:189: &GpuChildThread::BindProcessControlRequest,
base::Unretained(this)));
On 2015/10/06 19:59:36, piman (slow to review) wrote:
> What makes Unretained safe?

Added comment.

https://chromiumcodereview.appspot.com/1297953004/diff/110001/content/common/...
File content/common/gpu/gpu_process_launch_causes.h (right):

https://chromiumcodereview.appspot.com/1297953004/diff/110001/content/common/...
content/common/gpu/gpu_process_launch_causes.h:11: //
http://cs/file:chrome/histograms.xml
I can't find this histograms.xml file and I can't fin where the UMA for
CauseForGpuLaunch is reported. I think I am missing something...

[piman, 2015-10-08 22:37:01]

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

[piman, 2015-10-08 22:37:02]

LGTM, +asviktine for guidance about the internal histograms.xml

https://chromiumcodereview.appspot.com/1297953004/diff/60002/content/browser/...
File content/browser/mojo/mojo_shell_context.cc (right):

https://chromiumcodereview.appspot.com/1297953004/diff/60002/content/browser/...
content/browser/mojo/mojo_shell_context.cc:112: CAUSE_FOR_GPU_LAUNCH_NO_LAUNCH);
On 2015/10/07 19:07:58, xhwang wrote:
> On 2015/10/06 19:59:35, piman (slow to review) wrote:
> > What happens if the GPU process is not launched yet? 
> 
> Great question. If the process is not launched yet, |process_host| will be
null
> and the |request| will be dropped. 
> 
> Actually I am not familiar with these flags, should whoever needs the GPU
> process start it if it's not launched yet? If so, I can add another cause,
> something like CAUSE_FOR_GPU_LAUNCH_MOJO_SETUP

Yes, this is typically what we do in places we need to talk to the GPU process.
See e.g. the code in BrowserGpuChannelHostFactory (in particular
BrowserGpuChannelHostFactory::EstablishRequest::EstablishOnIO).

> > What happens when the GPU process dies and is restarted?
> 
> When the GPU process dies, all mojo applications hosted in it will be gone and
> all clients of those apps (in other processes) will get notified (via a
> connection error). It's the client's responsibility to reconnect to the
> services. Actually this is a great case where we want to launch the GPU
process
> if it's not launched yet (on line 112).

Ok. Keep in mind that the code to deal with lost GPU processes can be tricky to
avoid races. Some things to keep in mind:
1- a GPU process may be already dead but not known by GpuProcessHost. We detect
the GPU process death by seeing that the channel is lost, typically when sending
an IPC. That means GpuProcessHost::Get may return a non-NULL pointer, but that
GpuProcessHost may be already dead, and further IPCs on the channel will fail.
This is why we have retry logic in
BrowserGpuChannelHostFactory::EstablishRequest
2- it is often the case that 2 subsystems need to communicate to the same GPU
process. When each of these subsystems try to launch the GPU process, but a lost
GPU process happens in the middle, one of the subsystems may get an old,
invalid-but-not-known-yet GpuProcessHost, and the other subsystem may get a
now-valid GpuProcessHost corresponding to a relaunch. One way to detect this is
via the GpuProcessHost::host_id(), which would then be different between the 2.

I don't see code here to handle these cases. I don't know whether (2) is
relevant, though I suspect it will be given the context of the CL. (1) most
likely applies here (for example, could ConnectToRemoteService fail? If so what
happens?).

I'm not going to block the CL on it, but I strongly suggest adding a TODO and/or
filing a bug.

https://chromiumcodereview.appspot.com/1297953004/diff/110001/content/common/...
File content/common/gpu/gpu_process_launch_causes.h (right):

https://chromiumcodereview.appspot.com/1297953004/diff/110001/content/common/...
content/common/gpu/gpu_process_launch_causes.h:11: //
http://cs/file:chrome/histograms.xml
On 2015/10/07 19:07:58, xhwang wrote:
> I can't find this histograms.xml file and I can't fin where the UMA for
> CauseForGpuLaunch is reported. I think I am missing something...

It's in the internal google3 repository, follow the link as is on a corp machine
- I'm not quite sure why and/or whether it should be migrated to the open-source
repo, but I assume asviktine@ can help you there.

https://chromiumcodereview.appspot.com/1297953004/diff/110001/content/gpu/gpu...
File content/gpu/gpu_child_thread.cc (right):

https://chromiumcodereview.appspot.com/1297953004/diff/110001/content/gpu/gpu...
content/gpu/gpu_child_thread.cc:189: // and the message loop will be destroyed
before GpuChildThread is destructed.
To be clear, does that mean BindProcessControlRequest is called on a separate
thread? If so, can you add comments about this? It sounds like process_control_
is accessed from 2 threads. Is that ok?

[Alexei Svitkine (slow), 2015-10-09 16:58:09]

https://chromiumcodereview.appspot.com/1297953004/diff/110001/content/common/...
File content/common/gpu/gpu_process_launch_causes.h (right):

https://chromiumcodereview.appspot.com/1297953004/diff/110001/content/common/...
content/common/gpu/gpu_process_launch_causes.h:11: //
http://cs/file:chrome/histograms.xml
On 2015/10/08 22:37:02, piman (slow to review) wrote:
> On 2015/10/07 19:07:58, xhwang wrote:
> > I can't find this histograms.xml file and I can't fin where the UMA for
> > CauseForGpuLaunch is reported. I think I am missing something...
> 
> It's in the internal google3 repository, follow the link as is on a corp
machine
> - I'm not quite sure why and/or whether it should be migrated to the
open-source
> repo, but I assume asviktine@ can help you there.

The histogram should be migrated to the open source repo, given it's implemented
in the open source code and there's nothing confidential about it (which is
generally the case).

To do so, please do it in a separate CL - one that adds it to the open source
histograms.xml and another CL to remove from the internal file. Thanks!

[xhwang, 2015-10-09 23:36:30]

comments addressed

[xhwang, 2015-10-09 23:36:35]

https://chromiumcodereview.appspot.com/1297953004/diff/60002/content/browser/...
File content/browser/mojo/mojo_shell_context.cc (right):

https://chromiumcodereview.appspot.com/1297953004/diff/60002/content/browser/...
content/browser/mojo/mojo_shell_context.cc:112: CAUSE_FOR_GPU_LAUNCH_NO_LAUNCH);
On 2015/10/08 22:37:02, piman (slow to review) wrote:
> On 2015/10/07 19:07:58, xhwang wrote:
> > On 2015/10/06 19:59:35, piman (slow to review) wrote:
> > > What happens if the GPU process is not launched yet? 
> > 
> > Great question. If the process is not launched yet, |process_host| will be
> null
> > and the |request| will be dropped. 
> > 
> > Actually I am not familiar with these flags, should whoever needs the GPU
> > process start it if it's not launched yet? If so, I can add another cause,
> > something like CAUSE_FOR_GPU_LAUNCH_MOJO_SETUP
> 
> Yes, this is typically what we do in places we need to talk to the GPU
process.
> See e.g. the code in BrowserGpuChannelHostFactory (in particular
> BrowserGpuChannelHostFactory::EstablishRequest::EstablishOnIO).
> 
> > > What happens when the GPU process dies and is restarted?
> > 
> > When the GPU process dies, all mojo applications hosted in it will be gone
and
> > all clients of those apps (in other processes) will get notified (via a
> > connection error). It's the client's responsibility to reconnect to the
> > services. Actually this is a great case where we want to launch the GPU
> process
> > if it's not launched yet (on line 112).
> 
> Ok. Keep in mind that the code to deal with lost GPU processes can be tricky
to
> avoid races. Some things to keep in mind:
> 1- a GPU process may be already dead but not known by GpuProcessHost. We
detect
> the GPU process death by seeing that the channel is lost, typically when
sending
> an IPC. That means GpuProcessHost::Get may return a non-NULL pointer, but that
> GpuProcessHost may be already dead, and further IPCs on the channel will fail.
> This is why we have retry logic in
> BrowserGpuChannelHostFactory::EstablishRequest
> 2- it is often the case that 2 subsystems need to communicate to the same GPU
> process. When each of these subsystems try to launch the GPU process, but a
lost
> GPU process happens in the middle, one of the subsystems may get an old,
> invalid-but-not-known-yet GpuProcessHost, and the other subsystem may get a
> now-valid GpuProcessHost corresponding to a relaunch. One way to detect this
is
> via the GpuProcessHost::host_id(), which would then be different between the
2.
> 
> I don't see code here to handle these cases. I don't know whether (2) is
> relevant, though I suspect it will be given the context of the CL. (1) most
> likely applies here (for example, could ConnectToRemoteService fail? If so
what
> happens?).
> 
> I'm not going to block the CL on it, but I strongly suggest adding a TODO
and/or
> filing a bug.

Thanks for all the info! It seems to me in both (1) and (2),
ConnectToRemoteService will fail, and the ServiceRequest will be dropped,
causing a connection error at the client side. The individual client needs to
handle this case.

Added TODO.

https://chromiumcodereview.appspot.com/1297953004/diff/110001/content/common/...
File content/common/gpu/gpu_process_launch_causes.h (right):

https://chromiumcodereview.appspot.com/1297953004/diff/110001/content/common/...
content/common/gpu/gpu_process_launch_causes.h:11: //
http://cs/file:chrome/histograms.xml
On 2015/10/09 16:58:08, Alexei Svitkine(OOO til Oct13) wrote:
> On 2015/10/08 22:37:02, piman (slow to review) wrote:
> > On 2015/10/07 19:07:58, xhwang wrote:
> > > I can't find this histograms.xml file and I can't fin where the UMA for
> > > CauseForGpuLaunch is reported. I think I am missing something...
> > 
> > It's in the internal google3 repository, follow the link as is on a corp
> machine
> > - I'm not quite sure why and/or whether it should be migrated to the
> open-source
> > repo, but I assume asviktine@ can help you there.
> 
> The histogram should be migrated to the open source repo, given it's
implemented
> in the open source code and there's nothing confidential about it (which is
> generally the case).
> 
> To do so, please do it in a separate CL - one that adds it to the open source
> histograms.xml and another CL to remove from the internal file. Thanks!

Will do in a separate CL.

https://chromiumcodereview.appspot.com/1297953004/diff/110001/content/gpu/gpu...
File content/gpu/gpu_child_thread.cc (right):

https://chromiumcodereview.appspot.com/1297953004/diff/110001/content/gpu/gpu...
content/gpu/gpu_child_thread.cc:189: // and the message loop will be destroyed
before GpuChildThread is destructed.
On 2015/10/08 22:37:02, piman (slow to review) wrote:
> To be clear, does that mean BindProcessControlRequest is called on a separate
> thread? If so, can you add comments about this? It sounds like
process_control_
> is accessed from 2 threads. Is that ok?

Sorry I didn't make it clear. BindProcessControlRequest() is only called on the
main/UI thread. I was just saying even if
GpuChildThread::BindProcessControlRequest() is posted to the message loop we are
still okay. But now I look at ServiceRegistryImpl's implementation that's not
gonna happen.

[xhwang, 2015-10-10 00:02:11]

rebase only

[xhwang, 2015-10-10 00:23:16]

The CQ bit was checked by xhwang@chromium.org

[xhwang, 2015-10-10 00:23:17]

The patchset sent to the CQ was uploaded after l-g-t-m from rockot@chromium.org,
piman@chromium.org
Link to the patchset:
https://chromiumcodereview.appspot.com/1297953004/#ps150001 (title: "rebase
only")

[piman, 2015-10-10 00:24:14]

Ok, LGTM

[commit-bot: I haz the power, 2015-10-10 00:24:37]

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

[commit-bot: I haz the power, 2015-10-10 01:54:21]

Committed patchset #9 (id:150001)

[commit-bot: I haz the power, 2015-10-10 01:55:31]

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