Remove GPU relinquish resources infrastructure and stop GPU process.

Remove GPU relinquish resources infrastructure and stop GPU process.

Previously, chromecast depended on a "Relinquish Resources" code
path that would force the GPU process to release all OpenGL and EGL
resources. Rather than use Relinquish Resources, this CL adopts a
new technique that asks the GPU process to terminate, releasing all
resources.

BUG=468192
R=zmo@chromium.org,dcheng@chromium.org,halliwell@chromium.org

Committed: https://crrev.com/1b0f3bbb05db67ce9e2c9c1d0bf7eb4ca852f5fc
Cr-Commit-Position: refs/heads/master@{#341165}

[derekjchow1, 2015-07-23 19:03:06]

Hi all,

Please take a look. We've played with stopping the GPU process instead of
relinquishing display and have found no major problems.

I've broken this CL down into four parts hoping it would make it easier to
review:

Patchset 1 reverts https://codereview.chromium.org/800103005/

Patchset 2 reverts
https://codereview.chromium.org/712343003/

Patchset 3 removes the unused RelinquishGpuResources from Ozone platforms

Patchset 4 implements stopping the GPU process

PTAL. Most of the try-bots look good (I'm investigating the four failures right
now).

[halliwell, 2015-07-25 16:59:58]

halliwell@chromium.org changed reviewers:
+ alexst@chromium.org

[halliwell, 2015-07-25 17:00:30]

On 2015/07/23 19:03:06, derekjchow1 wrote:
> Hi all,
> 
> Please take a look. We've played with stopping the GPU process instead of
> relinquishing display and have found no major problems.
> 
> I've broken this CL down into four parts hoping it would make it easier to
> review:
> 
> Patchset 1 reverts https://codereview.chromium.org/800103005/
> 
> Patchset 2 reverts
> https://codereview.chromium.org/712343003/
> 
> Patchset 3 removes the unused RelinquishGpuResources from Ozone platforms
> 
> Patchset 4 implements stopping the GPU process
> 
> PTAL. Most of the try-bots look good (I'm investigating the four failures
right
> now).

cast ozone changes lgtm.  +alexst for rest of ozone.

[halliwell, 2015-07-25 17:00:39]

https://codereview.chromium.org/1249313004/diff/60001/content/browser/gpu/gpu...
File content/browser/gpu/gpu_process_host_ui_shim.h (right):

https://codereview.chromium.org/1249313004/diff/60001/content/browser/gpu/gpu...
content/browser/gpu/gpu_process_host_ui_shim.h:69: // TODO(derekjchow): Make
this API prettier.
I don't understand the TODO, can you elaborate?

https://codereview.chromium.org/1249313004/diff/60001/content/common/gpu/gpu_...
File content/common/gpu/gpu_channel_manager.cc (right):

https://codereview.chromium.org/1249313004/diff/60001/content/common/gpu/gpu_...
content/common/gpu/gpu_channel_manager.cc:283:
base::MessageLoop::current()->Quit();
Question: are there any circumstances in which the process doesn't terminate
completely after this?  i.e. on the browser side, do we need to consider a
timeout and force stop?

[alexst (slow to review), 2015-07-27 15:25:44]

On 2015/07/25 17:00:39, halliwell wrote:
>
https://codereview.chromium.org/1249313004/diff/60001/content/browser/gpu/gpu...
> File content/browser/gpu/gpu_process_host_ui_shim.h (right):
> 
>
https://codereview.chromium.org/1249313004/diff/60001/content/browser/gpu/gpu...
> content/browser/gpu/gpu_process_host_ui_shim.h:69: // TODO(derekjchow): Make
> this API prettier.
> I don't understand the TODO, can you elaborate?
> 
>
https://codereview.chromium.org/1249313004/diff/60001/content/common/gpu/gpu_...
> File content/common/gpu/gpu_channel_manager.cc (right):
> 
>
https://codereview.chromium.org/1249313004/diff/60001/content/common/gpu/gpu_...
> content/common/gpu/gpu_channel_manager.cc:283:
> base::MessageLoop::current()->Quit();
> Question: are there any circumstances in which the process doesn't terminate
> completely after this?  i.e. on the browser side, do we need to consider a
> timeout and force stop?

ui/ozone/ changes are straight forward, so lgtm pending the rest of the reviews
in content/ etc.

[Zhenyao Mo, 2015-07-27 16:21:00]

zmo@chromium.org changed reviewers:
+ jbauman@chromium.org

[Zhenyao Mo, 2015-07-27 16:21:39]

The content side looks good to me, but I am not a content/common/gpu owner, so I
delegate this to jbauman.

[Zhenyao Mo, 2015-07-27 16:23:12]

https://codereview.chromium.org/1249313004/diff/100001/content/browser/gpu/gp...
File content/browser/gpu/gpu_process_host_ui_shim.h (right):

https://codereview.chromium.org/1249313004/diff/100001/content/browser/gpu/gp...
content/browser/gpu/gpu_process_host_ui_shim.h:69: void
StopGpuProcess(base::Closure callback);
should callback be const reference type?

[jbauman, 2015-07-27 20:19:04]

lgtm

[derekjchow1, 2015-07-28 15:54:42]

The CQ bit was checked by derekjchow@chromium.org

[derekjchow1, 2015-07-28 15:54:43]

The patchset sent to the CQ was uploaded after l-g-t-m from alexst@chromium.org,
halliwell@chromium.org
Link to the patchset: https://codereview.chromium.org/1249313004/#ps120001
(title: "Use const ref")

[commit-bot: I haz the power, 2015-07-28 15:55:08]

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

[commit-bot: I haz the power, 2015-07-28 16:03:22]

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

[commit-bot: I haz the power, 2015-07-28 16:03:23]

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

[derekjchow1, 2015-07-28 16:14:56]

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

[derekjchow1, 2015-07-28 16:14:57]

+sievers for ui/gl/

https://codereview.chromium.org/1249313004/diff/60001/content/browser/gpu/gpu...
File content/browser/gpu/gpu_process_host_ui_shim.h (right):

https://codereview.chromium.org/1249313004/diff/60001/content/browser/gpu/gpu...
content/browser/gpu/gpu_process_host_ui_shim.h:69: // TODO(derekjchow): Make
this API prettier.
On 2015/07/25 17:00:38, halliwell wrote:
> I don't understand the TODO, can you elaborate?

Done.

[dcheng, 2015-07-28 18:47:21]

https://codereview.chromium.org/1249313004/diff/120001/content/browser/gpu/gp...
File content/browser/gpu/gpu_process_host.cc (right):

https://codereview.chromium.org/1249313004/diff/120001/content/browser/gpu/gp...
content/browser/gpu/gpu_process_host.cc:1045: if (status ==
base::TERMINATION_STATUS_NORMAL_TERMINATION) {
Why does this set gpu_crash_recorded_ to true if it terminated "normally"?

[derekjchow1, 2015-07-28 18:57:28]

https://codereview.chromium.org/1249313004/diff/120001/content/browser/gpu/gp...
File content/browser/gpu/gpu_process_host.cc (right):

https://codereview.chromium.org/1249313004/diff/120001/content/browser/gpu/gp...
content/browser/gpu/gpu_process_host.cc:1045: if (status ==
base::TERMINATION_STATUS_NORMAL_TERMINATION) {
On 2015/07/28 18:47:21, dcheng wrote:
> Why does this set gpu_crash_recorded_ to true if it terminated "normally"?

RecordProcessCrash can be called multiple times for one GPU crash. When
gpu_crash_recorded_ is true, RecordProcessCrash is noop (so no extra crashes are
recorded). 

When stopping the GPU process, RecordProcessCrash can be called multiple times
as well. The first call checks the termination status, and the subsequent calls
are no-op.

Perhaps I should rename gpu_crash_recorded_ to gpu_crash_handled_? Would that be
more clear?

[dcheng, 2015-07-28 19:04:27]

IPC changes LGTM

https://codereview.chromium.org/1249313004/diff/120001/content/browser/gpu/gp...
File content/browser/gpu/gpu_process_host.cc (right):

https://codereview.chromium.org/1249313004/diff/120001/content/browser/gpu/gp...
content/browser/gpu/gpu_process_host.cc:1045: if (status ==
base::TERMINATION_STATUS_NORMAL_TERMINATION) {
On 2015/07/28 at 18:57:28, derekjchow1 wrote:
> On 2015/07/28 18:47:21, dcheng wrote:
> > Why does this set gpu_crash_recorded_ to true if it terminated "normally"?
> 
> RecordProcessCrash can be called multiple times for one GPU crash. When
gpu_crash_recorded_ is true, RecordProcessCrash is noop (so no extra crashes are
recorded). 
> 
> When stopping the GPU process, RecordProcessCrash can be called multiple times
as well. The first call checks the termination status, and the subsequent calls
are no-op.
> 
> Perhaps I should rename gpu_crash_recorded_ to gpu_crash_handled_? Would that
be more clear?

Huh. That's kind of non-intuitive (both the fact that this can be called
multiple times for one crash, and the meaning of the field).

It'd be nice to clean this up, though it could be in a separate patch.

[no sievers, 2015-07-28 19:21:20]

https://codereview.chromium.org/1249313004/diff/120001/content/browser/gpu/gp...
File content/browser/gpu/gpu_process_host.cc (right):

https://codereview.chromium.org/1249313004/diff/120001/content/browser/gpu/gp...
content/browser/gpu/gpu_process_host.cc:1045: if (status ==
base::TERMINATION_STATUS_NORMAL_TERMINATION) {
Actually it's not quite obvious how you get here.
OnProcessCrashed() should only happen for abnormal termination (see
browser_child_process_host_impl.cc). Can we maybe handle this normal shutdown
more explicitly instead of routing it through RecordProcessCrash() to then just
early out?

[derekjchow1, 2015-07-28 21:01:44]

https://codereview.chromium.org/1249313004/diff/120001/content/browser/gpu/gp...
File content/browser/gpu/gpu_process_host.cc (right):

https://codereview.chromium.org/1249313004/diff/120001/content/browser/gpu/gp...
content/browser/gpu/gpu_process_host.cc:1045: if (status ==
base::TERMINATION_STATUS_NORMAL_TERMINATION) {
On 2015/07/28 19:21:20, sievers wrote:
> Actually it's not quite obvious how you get here.
> OnProcessCrashed() should only happen for abnormal termination (see
> browser_child_process_host_impl.cc). Can we maybe handle this normal shutdown
> more explicitly instead of routing it through RecordProcessCrash() to then
just
> early out?

Sure. The only other place RecordProcessCrash is called is in the destructor of
this class. This CL now preforms the check there. PTAL.

[no sievers, 2015-07-28 21:28:50]

https://codereview.chromium.org/1249313004/diff/140001/content/browser/gpu/gp...
File content/browser/gpu/gpu_process_host.cc (right):

https://codereview.chromium.org/1249313004/diff/140001/content/browser/gpu/gp...
content/browser/gpu/gpu_process_host.cc:440: RecordProcessCrash();
That's better, but calling RecordProcessCrash() here at all seems odd.
Can you just remove it?

Then I think it needs a fix for Android in
BrowserChildProcessHostImpl::OnChildDisconnected():

Similarly to CrOS TERMINATION_STATUS_PROCESS_WAS_KILLED_BY_OOM, can you add
TERMINATION_STATUS_OOM_PROTECTED to make sure it calls OnProcessCrashed()?

And then I'd also add an explicit call to RecordProcessCrash() from
GpuProcessHost::Get() where Init() failed and we 'delete host'.
But put a TODO to revisit this behavior. It's not really a crash, but we also
don't want to regress anything in terms of fallback-to-sw behavior.

You can also get rid of |expecting_shutdown_| then.

[no sievers, 2015-07-28 21:31:46]

https://codereview.chromium.org/1249313004/diff/140001/content/browser/gpu/gp...
File content/browser/gpu/gpu_process_host.cc (right):

https://codereview.chromium.org/1249313004/diff/140001/content/browser/gpu/gp...
content/browser/gpu/gpu_process_host.cc:440: RecordProcessCrash();
On 2015/07/28 21:28:50, sievers wrote:
> That's better, but calling RecordProcessCrash() here at all seems odd.
> Can you just remove it?
> 
> Then I think it needs a fix for Android in
> BrowserChildProcessHostImpl::OnChildDisconnected():
> 
> Similarly to CrOS TERMINATION_STATUS_PROCESS_WAS_KILLED_BY_OOM, can you add
> TERMINATION_STATUS_OOM_PROTECTED to make sure it calls OnProcessCrashed()?
> 
> And then I'd also add an explicit call to RecordProcessCrash() from
> GpuProcessHost::Get() where Init() failed and we 'delete host'.
> But put a TODO to revisit this behavior. It's not really a crash, but we also
> don't want to regress anything in terms of fallback-to-sw behavior.
> 
> You can also get rid of |expecting_shutdown_| then.

I forgot one:

You might also want to override OnProcessLaunchFailed() to keep the current
behavior. But it's also a bit questionable to count it as a crash.

[derekjchow1, 2015-07-29 18:35:40]

https://codereview.chromium.org/1249313004/diff/140001/content/browser/gpu/gp...
File content/browser/gpu/gpu_process_host.cc (right):

https://codereview.chromium.org/1249313004/diff/140001/content/browser/gpu/gp...
content/browser/gpu/gpu_process_host.cc:440: RecordProcessCrash();
On 2015/07/28 21:31:46, sievers wrote:
> On 2015/07/28 21:28:50, sievers wrote:
> > That's better, but calling RecordProcessCrash() here at all seems odd.
> > Can you just remove it?

Done.

> > 
> > Then I think it needs a fix for Android in
> > BrowserChildProcessHostImpl::OnChildDisconnected():
> > 
> > Similarly to CrOS TERMINATION_STATUS_PROCESS_WAS_KILLED_BY_OOM, can you add
> > TERMINATION_STATUS_OOM_PROTECTED to make sure it calls OnProcessCrashed()?

Done.

> > 
> > And then I'd also add an explicit call to RecordProcessCrash() from
> > GpuProcessHost::Get() where Init() failed and we 'delete host'.
> > But put a TODO to revisit this behavior. It's not really a crash, but we
also
> > don't want to regress anything in terms of fallback-to-sw behavior.

Done. I've stapled your name to the TODO. Is that ok?

> > 
> > You can also get rid of |expecting_shutdown_| then.
> 

Done.

> I forgot one:
> 
> You might also want to override OnProcessLaunchFailed() to keep the current
> behavior. But it's also a bit questionable to count it as a crash.

Done.

[no sievers, 2015-07-29 19:05:19]

lgtm!

https://codereview.chromium.org/1249313004/diff/180001/content/browser/gpu/gp...
File content/browser/gpu/gpu_process_host.cc (right):

https://codereview.chromium.org/1249313004/diff/180001/content/browser/gpu/gp...
content/browser/gpu/gpu_process_host.cc:1040: if (gpu_crash_recorded_)
You can remove this boolean also now, as with your changes it should only be
called once.

[derekjchow1, 2015-07-30 18:45:06]

The CQ bit was checked by derekjchow@chromium.org

[derekjchow1, 2015-07-30 18:45:06]

The patchset sent to the CQ was uploaded after l-g-t-m from dcheng@chromium.org,
halliwell@chromium.org, jbauman@chromium.org, alexst@chromium.org,
sievers@chromium.org
Link to the patchset: https://codereview.chromium.org/1249313004/#ps200001
(title: "Remove gpu_crash_recorded_.")

[commit-bot: I haz the power, 2015-07-30 18:45:36]

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

[commit-bot: I haz the power, 2015-07-30 18:51:42]

Committed patchset #11 (id:200001)

[commit-bot: I haz the power, 2015-07-30 18:52:29]

Patchset 11 (id:??) landed as
https://crrev.com/1b0f3bbb05db67ce9e2c9c1d0bf7eb4ca852f5fc
Cr-Commit-Position: refs/heads/master@{#341165}