Update liftetime management of GpuClient

While testing expanded mash_browser_tests a flaky shutdown crash was found in GpuClient.

GpuClient::EstablishGpuChannel could be called after GpuHost or GpuClient had been deleted.

GpuClient was holding a raw pointer back to GpuHost. It's lifetime can exceed GpuHost as it exists in a StrongBindingPtr. It also was registering a callback with and Unretained pointer to itself. So it could be called post delete.

This change updates GpuHost to hold all GpuClients in a StrongBindingSet which deletes them upon shutdown.

This also updates GpuClient to register the callback with a WeakPtr to itself, so that the callback can be invalidated upon deletion.

TEST=GpuHostTest.GpuClientDestructionOrder,
GpuHostTest.HostDeletionInvalidatesGpuClientCallback
TEST=BrowserTests
BUG=701380
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.android:android_optional_gpu_tests_rel;master.tryserver.chromium.linux:linux_optional_gpu_tests_rel;master.tryserver.chromium.mac:mac_optional_gpu_tests_rel;master.tryserver.chromium.win:win_optional_gpu_tests_rel

Review-Url: https://codereview.chromium.org/2741343003
Cr-Commit-Position: refs/heads/master@{#459248}
Committed: https://chromium.googlesource.com/chromium/src/+/cead65052697a1c3ec0d2fbfb7ee0ee9a9e52963

[jonross, 2017-03-17 15:49:16]

Description was changed from

==========
Enable more Mash Browser Tests

Enabling more tests within mash_browser_tests.

TEST=BrowserTests
BUG=678687
==========

to

==========
Enable more Mash Browser Tests

Enabling more tests within mash_browser_tests.

TEST=BrowserTests
BUG=678687
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.android:android_optional_gpu_tests_rel;master.tryserver.chromium.linux:linux_optional_gpu_tests_rel;master.tryserver.chromium.mac:mac_optional_gpu_tests_rel;master.tryserver.chromium.win:win_optional_gpu_tests_rel
==========

[jonross, 2017-03-20 22:08:35]

jonross@chromium.org changed reviewers:
+ sadrul@chromium.org

[jonross, 2017-03-20 22:08:35]

Hey Sadrul,

This change includes some testing work on GpuClient, where deleting the GpuHost
invalidates the Callback provided. Could you PTAL?

[jonross, 2017-03-20 22:09:02]

https://codereview.chromium.org/2741343003/diff/220001/services/ui/ws/gpu_hos...
File services/ui/ws/gpu_host_unittest.cc (right):

https://codereview.chromium.org/2741343003/diff/220001/services/ui/ws/gpu_hos...
services/ui/ws/gpu_host_unittest.cc:72: // host_deletion_callback_.Run();
With this turned on the callback doesn't run.

[sadrul, 2017-03-20 22:28:22]

https://codereview.chromium.org/2741343003/diff/220001/services/ui/ws/gpu_hos...
File services/ui/ws/gpu_host_unittest.cc (right):

https://codereview.chromium.org/2741343003/diff/220001/services/ui/ws/gpu_hos...
services/ui/ws/gpu_host_unittest.cc:110: : task_runner_(new
base::TestSimpleTaskRunner),
Don't you need to actually install the task-runner so that it runs (e.g. using
MessageLoop::SetTaskRunner(), or something like that)?

[jonross, 2017-03-22 02:12:55]

Description was changed from

==========
Enable more Mash Browser Tests

Enabling more tests within mash_browser_tests.

TEST=BrowserTests
BUG=678687
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.android:android_optional_gpu_tests_rel;master.tryserver.chromium.linux:linux_optional_gpu_tests_rel;master.tryserver.chromium.mac:mac_optional_gpu_tests_rel;master.tryserver.chromium.win:win_optional_gpu_tests_rel
==========

to

==========
While testing expanded mash_browser_tests a flaky shutdown crash was found in
GpuClient.

GpuClient::EstablishGpuChannel could be called after GpuHost or GpuClient had
been deleted.

GpuClient was holding a raw pointer back to GpuHost. It's lifetime can exceed
GpuHost as it exists in a StrongBindingPtr. It also was registering a callback
with and Unretained pointer to itself. So it could be called post delete.

This change updates GpuHost to hold all GpuClients in a StrongBindingSet which
deletes them upon shutdown.

This also updates GpuClient to register the callback with a WeakPtr to itself,
so that the callback can be invalidated upon deletion.

TEST=GpuHostTest.GpuClientDestructionOrder,
GpuHostTest.HostDeletionInvalidatesGpuClientCallback
TEST=BrowserTests
BUG=701380
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.android:android_optional_gpu_tests_rel;master.tryserver.chromium.linux:linux_optional_gpu_tests_rel;master.tryserver.chromium.mac:mac_optional_gpu_tests_rel;master.tryserver.chromium.win:win_optional_gpu_tests_rel
==========

[jonross, 2017-03-22 02:15:29]

On 2017/03/20 22:28:22, sadrul wrote:
>
https://codereview.chromium.org/2741343003/diff/220001/services/ui/ws/gpu_hos...
> File services/ui/ws/gpu_host_unittest.cc (right):
> 
>
https://codereview.chromium.org/2741343003/diff/220001/services/ui/ws/gpu_hos...
> services/ui/ws/gpu_host_unittest.cc:110: : task_runner_(new
> base::TestSimpleTaskRunner),
> Don't you need to actually install the task-runner so that it runs (e.g. using
> MessageLoop::SetTaskRunner(), or something like that)?

I've redone the GpuHost/GpuClient change from the initial debug version uploaded
in #13

I no longer see the crash locally, but am running the trybot which originally
found it.

I plan to revert the mash.browser_tests.filter before landing this.

PTAL

[sadrul, 2017-03-22 16:41:13]

https://codereview.chromium.org/2741343003/diff/280001/services/ui/ws/gpu_hos...
File services/ui/ws/gpu_host.cc (right):

https://codereview.chromium.org/2741343003/diff/280001/services/ui/ws/gpu_hos...
services/ui/ws/gpu_host.cc:134: std::unique_ptr<GpuClient>
client(base::MakeUnique<GpuClient>(
auto client = base::MakeUnique<...

https://codereview.chromium.org/2741343003/diff/280001/services/ui/ws/gpu_host.h
File services/ui/ws/gpu_host.h (right):

https://codereview.chromium.org/2741343003/diff/280001/services/ui/ws/gpu_hos...
services/ui/ws/gpu_host.h:55: class GpuClient : public
base::SupportsWeakPtr<GpuClient>, public mojom::Gpu {
Use a WeakPtrFactory<> instead

https://codereview.chromium.org/2741343003/diff/280001/services/ui/ws/gpu_hos...
services/ui/ws/gpu_host.h:86: DISALLOW_COPY_AND_ASSIGN(GpuClient);
Move this into a separate file.

https://codereview.chromium.org/2741343003/diff/280001/services/ui/ws/gpu_hos...
File services/ui/ws/gpu_host_unittest.cc (right):

https://codereview.chromium.org/2741343003/diff/280001/services/ui/ws/gpu_hos...
services/ui/ws/gpu_host_unittest.cc:150: EXPECT_EQ(nullptr, client_ref_);
This the expectation, right? i.e. when GpuHost is destroyed, it should take down
all associated GpuClients. The part about installing CrashOnCallback() isn't
useful, I think. (the test would fail in both asan/non-asan builds; so that
should be sufficient)

https://codereview.chromium.org/2741343003/diff/280001/services/ui/ws/gpu_hos...
services/ui/ws/gpu_host_unittest.cc:156:
gpu_host_->AddInternal(std::move(request)));
Make the return type GpuClient* so you don't need the cast here?

https://codereview.chromium.org/2741343003/diff/280001/services/ui/ws/gpu_hos...
services/ui/ws/gpu_host_unittest.cc:166:
EXPECT_TRUE(gpu_service_.establish_gpu_called());
This is still essentially testing that destroying a GpuHost takes down all
GpuClients. Maybe we can have a simpler test to verify that?

https://codereview.chromium.org/2741343003/diff/280001/services/ui/ws/gpu_hos...
services/ui/ws/gpu_host_unittest.cc:190: TestGpuClientDestructionOrder();
Why a separate function for each test?

[sadrul, 2017-03-22 16:42:04]

https://codereview.chromium.org/2741343003/diff/280001/services/ui/ws/gpu_hos...
File services/ui/ws/gpu_host_unittest.cc (right):

https://codereview.chromium.org/2741343003/diff/280001/services/ui/ws/gpu_hos...
services/ui/ws/gpu_host_unittest.cc:131: : task_runner_(new
base::TestSimpleTaskRunner), gpu_service_(task_runner_) {
Is there a reason to set this task runner? Can you just
base::ThreadTaskRunnerHandle::Get()?

[jonross, 2017-03-22 16:49:43]

https://codereview.chromium.org/2741343003/diff/280001/services/ui/ws/gpu_hos...
File services/ui/ws/gpu_host_unittest.cc (right):

https://codereview.chromium.org/2741343003/diff/280001/services/ui/ws/gpu_hos...
services/ui/ws/gpu_host_unittest.cc:150: EXPECT_EQ(nullptr, client_ref_);
On 2017/03/22 16:41:13, sadrul wrote:
> This the expectation, right? i.e. when GpuHost is destroyed, it should take
down
> all associated GpuClients. The part about installing CrashOnCallback() isn't
> useful, I think. (the test would fail in both asan/non-asan builds; so that
> should be sufficient)

I had originally used the CrashOnCallback to confirm the matching stack trace,
and had considered keeping it around. However this could be simplified into just
the single expectation. Will remove this.

https://codereview.chromium.org/2741343003/diff/280001/services/ui/ws/gpu_hos...
services/ui/ws/gpu_host_unittest.cc:166:
EXPECT_TRUE(gpu_service_.establish_gpu_called());
On 2017/03/22 16:41:13, sadrul wrote:
> This is still essentially testing that destroying a GpuHost takes down all
> GpuClients. Maybe we can have a simpler test to verify that?

The other test is a simple verification of that.

This is just to verify that callbacks posted don't run. They are now bound with
WeakPtrs. So this test could be considered redundant.

https://codereview.chromium.org/2741343003/diff/280001/services/ui/ws/gpu_hos...
services/ui/ws/gpu_host_unittest.cc:190: TestGpuClientDestructionOrder();
On 2017/03/22 16:41:13, sadrul wrote:
> Why a separate function for each test?

They each are calling several private methods.
I could just build a GpuHostTestApi instead and move the tests here.

[jonross, 2017-03-23 00:38:30]

https://codereview.chromium.org/2741343003/diff/280001/services/ui/ws/gpu_hos...
File services/ui/ws/gpu_host.cc (right):

https://codereview.chromium.org/2741343003/diff/280001/services/ui/ws/gpu_hos...
services/ui/ws/gpu_host.cc:134: std::unique_ptr<GpuClient>
client(base::MakeUnique<GpuClient>(
On 2017/03/22 16:41:13, sadrul wrote:
> auto client = base::MakeUnique<...

Done.

https://codereview.chromium.org/2741343003/diff/280001/services/ui/ws/gpu_host.h
File services/ui/ws/gpu_host.h (right):

https://codereview.chromium.org/2741343003/diff/280001/services/ui/ws/gpu_hos...
services/ui/ws/gpu_host.h:55: class GpuClient : public
base::SupportsWeakPtr<GpuClient>, public mojom::Gpu {
On 2017/03/22 16:41:13, sadrul wrote:
> Use a WeakPtrFactory<> instead

Done.

https://codereview.chromium.org/2741343003/diff/280001/services/ui/ws/gpu_hos...
services/ui/ws/gpu_host.h:86: DISALLOW_COPY_AND_ASSIGN(GpuClient);
On 2017/03/22 16:41:13, sadrul wrote:
> Move this into a separate file.

Done.

https://codereview.chromium.org/2741343003/diff/280001/services/ui/ws/gpu_hos...
File services/ui/ws/gpu_host_unittest.cc (right):

https://codereview.chromium.org/2741343003/diff/280001/services/ui/ws/gpu_hos...
services/ui/ws/gpu_host_unittest.cc:131: : task_runner_(new
base::TestSimpleTaskRunner), gpu_service_(task_runner_) {
On 2017/03/22 16:42:04, sadrul wrote:
> Is there a reason to set this task runner? Can you just
> base::ThreadTaskRunnerHandle::Get()?

Allows for manually advancing posted tasks in an independent way. There's been
concerns about base::RunLoop().RunUntilIdle() behaving differently for different
platforms.

But the removal of the second test means I don't need this anymore.

https://codereview.chromium.org/2741343003/diff/280001/services/ui/ws/gpu_hos...
services/ui/ws/gpu_host_unittest.cc:150: EXPECT_EQ(nullptr, client_ref_);
On 2017/03/22 16:49:43, jonross wrote:
> On 2017/03/22 16:41:13, sadrul wrote:
> > This the expectation, right? i.e. when GpuHost is destroyed, it should take
> down
> > all associated GpuClients. The part about installing CrashOnCallback() isn't
> > useful, I think. (the test would fail in both asan/non-asan builds; so that
> > should be sufficient)
> 
> I had originally used the CrashOnCallback to confirm the matching stack trace,
> and had considered keeping it around. However this could be simplified into
just
> the single expectation. Will remove this.

Done.

https://codereview.chromium.org/2741343003/diff/280001/services/ui/ws/gpu_hos...
services/ui/ws/gpu_host_unittest.cc:156:
gpu_host_->AddInternal(std::move(request)));
On 2017/03/22 16:41:13, sadrul wrote:
> Make the return type GpuClient* so you don't need the cast here?

Done.

https://codereview.chromium.org/2741343003/diff/280001/services/ui/ws/gpu_hos...
services/ui/ws/gpu_host_unittest.cc:166:
EXPECT_TRUE(gpu_service_.establish_gpu_called());
On 2017/03/22 16:49:43, jonross wrote:
> On 2017/03/22 16:41:13, sadrul wrote:
> > This is still essentially testing that destroying a GpuHost takes down all
> > GpuClients. Maybe we can have a simpler test to verify that?
> 
> The other test is a simple verification of that.
> 
> This is just to verify that callbacks posted don't run. They are now bound
with
> WeakPtrs. So this test could be considered redundant.

Done.

https://codereview.chromium.org/2741343003/diff/280001/services/ui/ws/gpu_hos...
services/ui/ws/gpu_host_unittest.cc:190: TestGpuClientDestructionOrder();
On 2017/03/22 16:49:43, jonross wrote:
> On 2017/03/22 16:41:13, sadrul wrote:
> > Why a separate function for each test?
> 
> They each are calling several private methods.
> I could just build a GpuHostTestApi instead and move the tests here.

Actually the reduced test has less requirements of private access. So I've
switched to smaller helpers here in the test suite. With the test case holding
the implementation again.

[sadrul, 2017-03-23 15:26:57]

Cool! lgtm

https://codereview.chromium.org/2741343003/diff/300001/services/ui/ws/DEPS
File services/ui/ws/DEPS (right):

https://codereview.chromium.org/2741343003/diff/300001/services/ui/ws/DEPS#ne...
services/ui/ws/DEPS:7: "+services/ui/gpu",
Hrm. I guess these are for the tests. Can you move that to a file-specific rule?
We would want to avoid accidental use of these code in the window-server.

[jonross, 2017-03-23 16:41:13]

Patchset #17 (id:320001) has been deleted

[jonross, 2017-03-23 16:41:39]

https://codereview.chromium.org/2741343003/diff/300001/services/ui/ws/DEPS
File services/ui/ws/DEPS (right):

https://codereview.chromium.org/2741343003/diff/300001/services/ui/ws/DEPS#ne...
services/ui/ws/DEPS:7: "+services/ui/gpu",
On 2017/03/23 15:26:56, sadrul wrote:
> Hrm. I guess these are for the tests. Can you move that to a file-specific
rule?
> We would want to avoid accidental use of these code in the window-server.

Done.

[jonross, 2017-03-23 16:42:15]

jonross@chromium.org changed reviewers:
+ sky@chromium.org

[jonross, 2017-03-23 16:42:16]

sky@chromium.org: Please review changes in 

services/ui/ws/BUILD and DEPS?

Thanks,
Jon

[sky, 2017-03-23 18:45:59]

LGTM

[jonross, 2017-03-23 18:51:24]

The CQ bit was checked by jonross@chromium.org

[jonross, 2017-03-23 18:51:25]

The patchset sent to the CQ was uploaded after l-g-t-m from sadrul@chromium.org
Link to the patchset: https://codereview.chromium.org/2741343003/#ps340001
(title: "Update Deps. Remove Test Filter")

[commit-bot: I haz the power, 2017-03-23 18:51:51]

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-03-23 19:01:36]

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

[commit-bot: I haz the power, 2017-03-23 19:01:37]

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

[jonross, 2017-03-23 19:59:27]

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

[jonross, 2017-03-23 19:59:28]

Hi piman@,

As an owner of /gpu/ipc/service, could you review this new DEPS?

It is a test specific DEPS so that the unittests can create a GpuWatchdogThread.

Thanks,
Jon

[sadrul, 2017-03-23 20:00:20]

https://codereview.chromium.org/2741343003/diff/340001/services/ui/ws/gpu_hos...
File services/ui/ws/gpu_host_unittest.cc (right):

https://codereview.chromium.org/2741343003/diff/340001/services/ui/ws/gpu_hos...
services/ui/ws/gpu_host_unittest.cc:50: gpu::GpuWatchdogThread::Create(),
Just use nullptr here?

[jonross, 2017-03-23 20:09:49]

jonross@chromium.org changed reviewers:
- piman@chromium.org

[jonross, 2017-03-23 20:09:50]

Hi piman@,

My mistake, sadrul@ noticed that the new DEPS I was adding was actually not
needed. It has been removed.

Thanks,
Jon

https://codereview.chromium.org/2741343003/diff/340001/services/ui/ws/gpu_hos...
File services/ui/ws/gpu_host_unittest.cc (right):

https://codereview.chromium.org/2741343003/diff/340001/services/ui/ws/gpu_hos...
services/ui/ws/gpu_host_unittest.cc:50: gpu::GpuWatchdogThread::Create(),
On 2017/03/23 20:00:20, sadrul wrote:
> Just use nullptr here?

Actually now yeah. At one point a part of the test was hitting it, but not
anymore.

DEPS removed.

[jonross, 2017-03-23 20:09:58]

The CQ bit was checked by jonross@chromium.org

[jonross, 2017-03-23 20:09:58]

The patchset sent to the CQ was uploaded after l-g-t-m from sadrul@chromium.org,
sky@chromium.org
Link to the patchset: https://codereview.chromium.org/2741343003/#ps360001
(title: "Remove gpu/ipc/service deps")

[commit-bot: I haz the power, 2017-03-23 20:10:51]

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-03-23 20:33:20]

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

[commit-bot: I haz the power, 2017-03-23 20:33:21]

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

[jonross, 2017-03-23 20:57:01]

The CQ bit was checked by jonross@chromium.org

[jonross, 2017-03-23 20:57:01]

The patchset sent to the CQ was uploaded after l-g-t-m from sadrul@chromium.org,
sky@chromium.org
Link to the patchset: https://codereview.chromium.org/2741343003/#ps380001
(title: "fix x11 re-definition")

[commit-bot: I haz the power, 2017-03-23 20:57:58]

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-03-23 22:12:40]

CQ is committing da patch.

Bot data: {"patchset_id": 380001, "attempt_start_ts": 1490302621351360,
"parent_rev": "0b6de43d2f326542de3756c46bb3c98b63880f62", "commit_rev":
"cead65052697a1c3ec0d2fbfb7ee0ee9a9e52963"}

[commit-bot: I haz the power, 2017-03-23 22:13:48]

Description was changed from

==========
While testing expanded mash_browser_tests a flaky shutdown crash was found in
GpuClient.

GpuClient::EstablishGpuChannel could be called after GpuHost or GpuClient had
been deleted.

GpuClient was holding a raw pointer back to GpuHost. It's lifetime can exceed
GpuHost as it exists in a StrongBindingPtr. It also was registering a callback
with and Unretained pointer to itself. So it could be called post delete.

This change updates GpuHost to hold all GpuClients in a StrongBindingSet which
deletes them upon shutdown.

This also updates GpuClient to register the callback with a WeakPtr to itself,
so that the callback can be invalidated upon deletion.

TEST=GpuHostTest.GpuClientDestructionOrder,
GpuHostTest.HostDeletionInvalidatesGpuClientCallback
TEST=BrowserTests
BUG=701380
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.android:android_optional_gpu_tests_rel;master.tryserver.chromium.linux:linux_optional_gpu_tests_rel;master.tryserver.chromium.mac:mac_optional_gpu_tests_rel;master.tryserver.chromium.win:win_optional_gpu_tests_rel
==========

to

==========
While testing expanded mash_browser_tests a flaky shutdown crash was found in
GpuClient.

GpuClient::EstablishGpuChannel could be called after GpuHost or GpuClient had
been deleted.

GpuClient was holding a raw pointer back to GpuHost. It's lifetime can exceed
GpuHost as it exists in a StrongBindingPtr. It also was registering a callback
with and Unretained pointer to itself. So it could be called post delete.

This change updates GpuHost to hold all GpuClients in a StrongBindingSet which
deletes them upon shutdown.

This also updates GpuClient to register the callback with a WeakPtr to itself,
so that the callback can be invalidated upon deletion.

TEST=GpuHostTest.GpuClientDestructionOrder,
GpuHostTest.HostDeletionInvalidatesGpuClientCallback
TEST=BrowserTests
BUG=701380
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.android:android_optional_gpu_tests_rel;master.tryserver.chromium.linux:linux_optional_gpu_tests_rel;master.tryserver.chromium.mac:mac_optional_gpu_tests_rel;master.tryserver.chromium.win:win_optional_gpu_tests_rel

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

[commit-bot: I haz the power, 2017-03-23 22:13:49]

Committed patchset #19 (id:380001) as
https://chromium.googlesource.com/chromium/src/+/cead65052697a1c3ec0d2fbfb7ee...