Add null check on GpuChannel in StreamTextureHost.

1. StreamTextureHost's |channel_| could be null while
StreamTextureHost is still alive because |channel_| can be
set to null on a OnChannelError() call.

2. Add a new unit test to test and make sure StreamTextureHost
can handle a null |channel_|.

3. An invalid mailbox needs to be checked by calling
Mailbox::IsZero().

4. Remove previously added CHECK's for debugging.

Bug: 984309
Change-Id: I1fb42855877540f1ec0ddbcec4fe5713071727a0
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1740168
Commit-Queue: vikas soni <vikassoni@chromium.org>
Reviewed-by: Eric Karl <ericrk@chromium.org>
Reviewed-by: Stefan Zager <szager@chromium.org>
Reviewed-by: Frank Liberato <liberato@chromium.org>
Cr-Commit-Position: refs/heads/master@{#686642}

content/renderer/media/android/stream_texture_factory.cc

@@ -77,12 +77,9 @@ void StreamTextureProxy::CreateSharedImage(
     const gfx::Size& size,
     gpu::Mailbox* mailbox,
     gpu::SyncToken* unverified_sync_token) {
-  // TODO(vikassoni): Remove CHECK after debugging.
-  CHECK(host_);
   *mailbox = host_->CreateSharedImage(size);
-  if (!mailbox)
+  if (mailbox->IsZero())
     return;
-  CHECK(host_);
   *unverified_sync_token = host_->GenUnverifiedSyncToken();
 }
 

content/renderer/media/android/stream_texture_factory.h

@@ -31,7 +31,7 @@ class StreamTextureFactory;
 
 // The proxy class for the gpu thread to notify the compositor thread
 // when a new video frame is available.
-class StreamTextureProxy : public StreamTextureHost::Listener {
+class CONTENT_EXPORT StreamTextureProxy : public StreamTextureHost::Listener {
  public:
   ~StreamTextureProxy() override;
 
@@ -68,6 +68,7 @@ class StreamTextureProxy : public StreamTextureHost::Listener {
   };
  private:
   friend class StreamTextureFactory;
+  friend class StreamTextureProxyTest;
   explicit StreamTextureProxy(std::unique_ptr<StreamTextureHost> host);
 
   void BindOnThread();

content/renderer/media/android/stream_texture_proxy_unittest.cc

+// Copyright (c) 2019 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 "base/memory/ptr_util.h"
+#include "base/memory/scoped_refptr.h"
+#include "base/test/test_simple_task_runner.h"
+#include "base/threading/thread_task_runner_handle.h"
+#include "content/renderer/media/android/stream_texture_factory.h"
+#include "content/renderer/stream_texture_host_android.h"
+#include "gpu/command_buffer/common/mailbox.h"
+#include "gpu/command_buffer/common/sync_token.h"
+#include "gpu/ipc/client/gpu_channel_host.h"
+#include "gpu/ipc/common/gpu_messages.h"
+#include "gpu/ipc/common/surface_handle.h"
+#include "mojo/public/cpp/system/message_pipe.h"
+#include "testing/gtest/include/gtest/gtest.h"
+
+namespace content {
+
+// GpuChannelHost is expected to be created on the IO thread, and posts tasks to
+// setup its IPC listener, so it must be created after the thread task runner
+// handle is set.
+class TestGpuChannelHost : public gpu::GpuChannelHost {
+ public:
+  TestGpuChannelHost()
+      : gpu::GpuChannelHost(
+            0 /* channel_id */,
+            gpu::GPUInfo(),
+            gpu::GpuFeatureInfo(),
+            mojo::ScopedMessagePipeHandle(
+                mojo::MessagePipeHandle(mojo::kInvalidHandleValue))) {}
+
+ protected:
+  ~TestGpuChannelHost() override {}
+};
+
+class StreamTextureProxyTest : public testing::Test {
+ public:
+  StreamTextureProxyTest()
+      : task_runner_(base::MakeRefCounted<base::TestSimpleTaskRunner>()),
+        thread_task_runner_handle_override_(
+            base::ThreadTaskRunnerHandle::OverrideForTesting(task_runner_)),
+        channel_(base::MakeRefCounted<TestGpuChannelHost>()) {}
+
+  ~StreamTextureProxyTest() override { channel_ = nullptr; }
+
+  std::unique_ptr<StreamTextureProxy> CreateProxyWithNullGpuChannelHost() {
+    // Create the StreamTextureHost with a valid |channel_|. Note that route_id
+    // does not matter here for the test we are writing.
+    auto host = std::make_unique<StreamTextureHost>(channel_, 1 /* route_id */);
+
+    // Force the StreamTextureHost's |channel_| to be null by calling
+    // OnChannelError.
+    host->OnChannelError();
+    auto proxy = base::WrapUnique(new StreamTextureProxy(std::move(host)));
+    return proxy;
+  }
+
+ protected:
+  scoped_refptr<base::TestSimpleTaskRunner> task_runner_;
+  base::ScopedClosureRunner thread_task_runner_handle_override_;
+  scoped_refptr<TestGpuChannelHost> channel_;
+};
+
+// This test is to make sure CreateSharedImage() do not crash even if
+// StreamTextureHost's |channel_| is null.
+TEST_F(StreamTextureProxyTest,
+       CreateSharedImageDoesNotCrashWithNullGpuChannelHost) {
+  auto proxy = CreateProxyWithNullGpuChannelHost();
+  gpu::Mailbox mailbox;
+  gpu::SyncToken texture_mailbox_sync_token;
+
+  // This method should not crash even if the StreamTextureHost's |channel_| is
+  // null.
+  proxy->CreateSharedImage(gfx::Size(1, 1), &mailbox,
+                           &texture_mailbox_sync_token);
+}
+
+}  // namespace content

content/renderer/stream_texture_host_android.cc

@@ -85,8 +85,10 @@ gpu::Mailbox StreamTextureHost::CreateSharedImage(const gfx::Size& size) {
 }
 
 gpu::SyncToken StreamTextureHost::GenUnverifiedSyncToken() {
-  // TODO(vikassoni): Remove CHECK after debugging.
-  CHECK(channel_);
+  // |channel_| can be set to null via OnChannelError() which means
+  // StreamTextureHost could still be alive when |channel_| is gone.
+  if (!channel_)
+    return gpu::SyncToken();
 
   return gpu::SyncToken(gpu::CommandBufferNamespace::GPU_IO,
                         gpu::CommandBufferIdFromChannelAndRoute(

content/renderer/stream_texture_host_android.h

@@ -9,6 +9,7 @@
 
 #include "base/macros.h"
 #include "base/memory/weak_ptr.h"
+#include "content/common/content_export.h"
 #include "ipc/ipc_listener.h"
 #include "ipc/ipc_message.h"
 
@@ -30,7 +31,7 @@ namespace content {
 
 // Class for handling all the IPC messages between the GPU process and
 // StreamTextureProxy.
-class StreamTextureHost : public IPC::Listener {
+class CONTENT_EXPORT StreamTextureHost : public IPC::Listener {
  public:
   explicit StreamTextureHost(scoped_refptr<gpu::GpuChannelHost> channel,
                              int32_t route_id);

content/test/BUILD.gn

@@ -2248,6 +2248,7 @@ test("content_unittests") {
       "../browser/renderer_host/render_widget_host_view_android_unittest.cc",
       "../browser/sms/sms_provider_android_unittest.cc",
       "../renderer/java/gin_java_bridge_value_converter_unittest.cc",
+      "../renderer/media/android/stream_texture_proxy_unittest.cc",
       "../renderer/media/android/stream_texture_wrapper_impl_unittest.cc",
     ]
     sources -= [ "../browser/tracing/tracing_ui_unittest.cc" ]