Update liftetime management of GpuClient

services/ui/ws/gpu_host_unittest.cc

Index: services/ui/ws/gpu_host_unittest.cc
diff --git a/services/ui/ws/gpu_host_unittest.cc b/services/ui/ws/gpu_host_unittest.cc
new file mode 100644
index 0000000000000000000000000000000000000000..c6804e77a828d686994e70c3deb5137a2be9dd9d
--- /dev/null
+++ b/services/ui/ws/gpu_host_unittest.cc
@@ -0,0 +1,201 @@
+// Copyright 2017 The Chromium Authors. All rights reserved.
+// Use of this source code is governed by a BSD-style license that can be
+// found in the LICENSE file.
+
+#include "services/ui/ws/gpu_host.h"
+
+#include "base/macros.h"
+#include "base/memory/ptr_util.h"
+#include "base/memory/ref_counted.h"
+#include "base/memory/weak_ptr.h"
+#include "base/message_loop/message_loop.h"
+#include "base/test/test_simple_task_runner.h"
+#include "gpu/config/gpu_info.h"
+#include "gpu/ipc/service/gpu_watchdog_thread.h"
+#include "services/ui/gpu/gpu_service.h"
+#include "services/ui/public/interfaces/gpu.mojom.h"
+#include "services/ui/ws/gpu_host_delegate.h"
+#include "testing/gtest/include/gtest/gtest.h"
+
+namespace ui {
+namespace ws {
+namespace test {
+namespace {
+
+// No-opt implementation of GpuHostDelegate.
+class TestGpuHostDelegate : public GpuHostDelegate {
+ public:
+  TestGpuHostDelegate() {}
+  ~TestGpuHostDelegate() override {}
+
+  void OnGpuServiceInitialized() override {}
+
+ private:
+  DISALLOW_COPY_AND_ASSIGN(TestGpuHostDelegate);
+};
+
+// Test implementation of GpuService. For testing behaviour of calls made by
+// GpuHost::GpuClient
+class TestGpuService : public GpuService {
+ public:
+  TestGpuService(scoped_refptr<base::TestSimpleTaskRunner> task_runner);
+  ~TestGpuService() override {}
+
+  bool establish_gpu_called() { return establish_gpu_called_; }
+
+  // Sets callback for providing test implementation of EstablishGpuChannel/
+  void SetEstablishGpuChannelTestCallback(base::Closure callback) {
+    establish_gpu_channel_callback_ = callback;
+  }
+
+  // GpuService:
+  void EstablishGpuChannel(
+      int32_t client_id,
+      uint64_t client_tracing_id,
+      bool is_gpu_host,
+      const EstablishGpuChannelCallback& callback) override;
+
+ private:
+  base::Closure establish_gpu_channel_callback_;
+
+  bool establish_gpu_called_ = false;
+
+  DISALLOW_COPY_AND_ASSIGN(TestGpuService);
+};
+
+TestGpuService::TestGpuService(
+    scoped_refptr<base::TestSimpleTaskRunner> task_runner)
+    : GpuService(gpu::GPUInfo(),
+                 gpu::GpuWatchdogThread::Create(),
+                 nullptr,
+                 task_runner,
+                 gpu::GpuFeatureInfo()) {}
+
+void TestGpuService::EstablishGpuChannel(
+    int32_t client_id,
+    uint64_t client_tracing_id,
+    bool is_gpu_host,
+    const EstablishGpuChannelCallback& callback) {
+  establish_gpu_called_ = true;
+  EXPECT_FALSE(establish_gpu_channel_callback_.is_null());
+  EXPECT_FALSE(callback.is_null());
+  EXPECT_FALSE(callback.IsCancelled());
+  establish_gpu_channel_callback_.Run();
+  // Calling should not lead to a crash.
+  callback.Run(mojo::ScopedMessagePipeHandle());
+}
+
+// While provide to GpuClient as a callback, actually calling into this during
+// the test would lead to a use-after-free. This will explicitly crash to detect
+// the error on non-asan builds.
+void CrashOnCallback(int32_t client_id,
+                     mojo::ScopedMessagePipeHandle channel_handle,
+                     const gpu::GPUInfo& gpu_info) {
+  CHECK(false);
+}
+
+}  // namespace
+
+class GpuHostTest : public testing::Test {
+ public:
+  GpuHostTest();
+  ~GpuHostTest() override {}
+
+  GpuHost* gpu_host() { return gpu_host_.get(); }
+
+  // Implementations for corresponding tests.
+  void TestGpuClientDestructionOrder();
+  void TestHostDeletionInvalidatesGpuClientCallback();
+
+  // Deletes |gpu_host_| and verifies that |client_ref_| has cleaned up.
+  void DeleteHostAndVerifyClientDeleted();
+
+  // testing::Test
+  void SetUp() override;
+
+ private:
+  base::MessageLoop message_loop_;
+  scoped_refptr<base::TestSimpleTaskRunner> task_runner_;
+
+  base::WeakPtr<GpuHost::GpuClient> client_ref_;
+
+  TestGpuHostDelegate gpu_host_delegate_;
+  TestGpuService gpu_service_;
+  ui::mojom::GpuServicePtr gpu_service_ptr_;
+  std::unique_ptr<GpuHost> gpu_host_;
+
+  DISALLOW_COPY_AND_ASSIGN(GpuHostTest);
+};
+
+GpuHostTest::GpuHostTest()
+    : task_runner_(new base::TestSimpleTaskRunner), gpu_service_(task_runner_) {

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

Is there a reason to set this task runner? Can you just
base::ThreadTaskRunnerHandle::Get()?

[jonross, 2017/03/23 00:38:29]

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.

+  message_loop_.SetTaskRunner(task_runner_);
+}
+
+void GpuHostTest::TestGpuClientDestructionOrder() {
+  mojom::GpuRequest request;
+  GpuHost::GpuClient* client = static_cast<GpuHost::GpuClient*>(
+      gpu_host_->AddInternal(std::move(request)));
+  client_ref_ = client->AsWeakPtr();
+  // Closing the host should shutdown the GpuClient.
+  gpu_host_.reset();
+  // If |client_ref| is not deleted the subsequent call will reproduce the
+  // use-after-free of GpuHost's GpuInfo.
+  if (client_ref_) {
+    // Passing in a callback which crashes to identify the error on non-asan
+    // builds.
+    client_ref_->OnGpuChannelEstablished(base::Bind(&CrashOnCallback),
+                                         mojo::ScopedMessagePipeHandle());
+  }
+  EXPECT_EQ(nullptr, client_ref_);

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

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)

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

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.

[jonross, 2017/03/23 00:38:29]

Done.

+}
+
+void GpuHostTest::TestHostDeletionInvalidatesGpuClientCallback() {
+  mojom::GpuRequest request;
+  GpuHost::GpuClient* client = static_cast<GpuHost::GpuClient*>(
+      gpu_host_->AddInternal(std::move(request)));

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

Make the return type GpuClient* so you don't need the cast here?

[jonross, 2017/03/23 00:38:29]

Done.

+  client_ref_ = client->AsWeakPtr();
+
+  // Second stage of test is DeleteHostAndVerifyClientDeleted invoked by
+  // TestGpuService.
+  gpu_service_.SetEstablishGpuChannelTestCallback(base::Bind(
+      &GpuHostTest::DeleteHostAndVerifyClientDeleted, base::Unretained(this)));
+  client->EstablishGpuChannel(base::Bind(&CrashOnCallback));
+  // TestGpuService will be notified of above in a posted task.
+  task_runner_->RunPendingTasks();
+  EXPECT_TRUE(gpu_service_.establish_gpu_called());

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

This is still essentially testing that destroying a GpuHost takes down all
GpuClients. Maybe we can have a simpler test to verify that?

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

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.

[jonross, 2017/03/23 00:38:29]

Done.

+}
+
+void GpuHostTest::DeleteHostAndVerifyClientDeleted() {
+  EXPECT_NE(nullptr, client_ref_);
+  gpu_host_.reset();
+  task_runner_->RunPendingTasks();
+  // Verifies cleanup of the GpuClient, which prevents callbacks from resolving.
+  EXPECT_EQ(nullptr, client_ref_);
+}
+
+void GpuHostTest::SetUp() {
+  testing::Test::SetUp();
+  gpu_host_ = base::MakeUnique<GpuHost>(&gpu_host_delegate_);
+
+  ui::mojom::GpuServiceRequest request(&gpu_service_ptr_);
+  gpu_service_.Bind(std::move(request));
+  gpu_host_->gpu_service_ = std::move(gpu_service_ptr_);
+}
+
+// Tests to verify, that if a GpuHost is deleted before GpuClient receives a
+// callback, that GpuClient is torn down and does not attempt to use GpuInfo
+// after deletion. This should not crash on asan-builds.
+TEST_F(GpuHostTest, GpuClientDestructionOrder) {
+  TestGpuClientDestructionOrder();

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

Why a separate function for each test?

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

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:29]

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.

+}
+
+// Tests that if GpuHost is deleted that callbacks provided by GpuClient are
+// invalidated and do not cause crashes.
+TEST_F(GpuHostTest, HostDeletionInvalidatesGpuClientCallback) {
+  TestHostDeletionInvalidatesGpuClientCallback();
+}
+
+}  // namespace test
+}  // namespace ws
+}  // namespace ui