[CRD iOS] Fix thread issue of RendererProxy and QueuedTaskPoster

RendererProxy is used mainly on the UI thread, but it is deleted on the
display thread because GlDisplayRenderer binds its WeakPtrFactory to
the display thread when initializing it. This forces QueuedTaskPoster
to be deleted on the wrong thread, which may be a source of crash. You
can see more details in the bug.

This CL resolves this by making GlDisplayRenderer::Core initialize
RendererProxy on the UI thread.

Bug: 872944
Change-Id: I37dbd20aad80dd05e4eb0ad410c339fdfa5053c5
Reviewed-on: https://chromium-review.googlesource.com/1170133
Commit-Queue: Yuwei Huang <yuweih@chromium.org>
Reviewed-by: Jamie Walch <jamiewalch@chromium.org>
Reviewed-by: Joe Downing <joedow@chromium.org>
Cr-Commit-Position: refs/heads/master@{#582674}

remoting/client/display/renderer_proxy.cc

@@ -15,12 +15,12 @@ namespace remoting {
 RendererProxy::RendererProxy(
     scoped_refptr<base::SingleThreadTaskRunner> task_runner)
     : task_runner_(task_runner),
-      ui_task_poster_(new remoting::QueuedTaskPoster(task_runner_)),
-      weak_factory_(this) {}
+      ui_task_poster_(new remoting::QueuedTaskPoster(task_runner_)) {}
 
 RendererProxy::~RendererProxy() = default;
 
 void RendererProxy::Initialize(base::WeakPtr<GlRenderer> renderer) {
+  DCHECK_CALLED_ON_VALID_THREAD(thread_checker_);
   renderer_ = renderer;
 }
 
@@ -49,12 +49,9 @@ void RendererProxy::StartInputFeedback(float x, float y, float diameter) {
       false);
 }
 
-base::WeakPtr<RendererProxy> RendererProxy::GetWeakPtr() {
-  return weak_factory_.GetWeakPtr();
-}
-
 void RendererProxy::RunTaskOnProperThread(const base::Closure& task,
                                           bool needs_synchronization) {
+  DCHECK_CALLED_ON_VALID_THREAD(thread_checker_);
   if (task_runner_->BelongsToCurrentThread()) {
     task.Run();
     return;

remoting/client/display/renderer_proxy.h

@@ -5,9 +5,11 @@
 #ifndef REMOTING_CLIENT_UI_RENDERER_PROXY_H_
 #define REMOTING_CLIENT_UI_RENDERER_PROXY_H_
 
+#include "base/macros.h"
 #include "base/memory/ref_counted.h"
 #include "base/memory/weak_ptr.h"
 #include "base/single_thread_task_runner.h"
+#include "base/threading/thread_checker.h"
 
 namespace remoting {
 
@@ -15,7 +17,8 @@ class GlRenderer;
 class QueuedTaskPoster;
 class ViewMatrix;
 
-// A class to proxy calls to GlRenderer from one thread to another.
+// A class to proxy calls to GlRenderer from one thread to another. Must be
+// created and used on the same thread.
 // TODO(yuweih): This should be removed once we have moved Drawables out of
 // GlRenderer.
 class RendererProxy {
@@ -32,8 +35,6 @@ class RendererProxy {
   void SetCursorVisibility(bool visible);
   void StartInputFeedback(float x, float y, float diameter);
 
-  base::WeakPtr<RendererProxy> GetWeakPtr();
-
  private:
   // Runs the |task| on the thread of |task_runner_|. All tasks run with
   // |needs_synchronization| set to true inside the same tick will be run on
@@ -45,11 +46,9 @@ class RendererProxy {
   scoped_refptr<base::SingleThreadTaskRunner> task_runner_;
   std::unique_ptr<remoting::QueuedTaskPoster> ui_task_poster_;
 
-  base::WeakPtrFactory<RendererProxy> weak_factory_;
+  THREAD_CHECKER(thread_checker_);
 
-  // RenderStubProxy is neither copyable nor movable.
-  RendererProxy(const RendererProxy&) = delete;
-  RendererProxy& operator=(const RendererProxy&) = delete;
+  DISALLOW_COPY_AND_ASSIGN(RendererProxy);
 };
 
 }  // namespace remoting

remoting/client/queued_task_poster.cc

@@ -16,7 +16,11 @@ QueuedTaskPoster::QueuedTaskPoster(
     : target_task_runner_(target_task_runner),
       weak_factory_(this) {}
 
-QueuedTaskPoster::~QueuedTaskPoster() = default;
+QueuedTaskPoster::~QueuedTaskPoster() {
+  if (source_task_runner_) {
+    DCHECK(source_task_runner_->BelongsToCurrentThread());
+  }
+}
 
 void QueuedTaskPoster::AddTask(const base::Closure& closure) {
   if (!source_task_runner_) {

remoting/ios/display/gl_display_handler.h

@@ -54,9 +54,10 @@ class CursorShapeStub;
 - (void)setSurfaceSize:(const CGRect&)frame;
 
 // Must be called immediately after the object is constructed.
-- (std::unique_ptr<remoting::RendererProxy>)CreateRendererProxy;
-- (std::unique_ptr<remoting::protocol::VideoRenderer>)CreateVideoRenderer;
-- (std::unique_ptr<remoting::protocol::CursorShapeStub>)CreateCursorShapeStub;
+- (std::unique_ptr<remoting::protocol::VideoRenderer>)createVideoRenderer;
+- (std::unique_ptr<remoting::protocol::CursorShapeStub>)createCursorShapeStub;
+
+@property(readonly) remoting::RendererProxy* rendererProxy;
 
 // This is write-only but @property doesn't support write-only modifier.
 @property id<GlDisplayHandlerDelegate> delegate;

remoting/ios/display/gl_display_handler.mm

@@ -44,8 +44,6 @@ class Core : public protocol::CursorShapeStub, public GlRendererDelegate {
 
   void SetHandlerDelegate(id<GlDisplayHandlerDelegate> delegate);
 
-  std::unique_ptr<RendererProxy> GrabRendererProxy();
-
   // CursorShapeStub interface.
   void SetCursorShape(const protocol::CursorShapeInfo& cursor_shape) override;
 
@@ -62,16 +60,15 @@ class Core : public protocol::CursorShapeStub, public GlRendererDelegate {
 
   std::unique_ptr<protocol::FrameConsumer> GrabFrameConsumer();
 
+  // The renderer proxy should be used on the UI thread.
+  RendererProxy* renderer_proxy() { return renderer_proxy_.get(); }
+
   // Returns a weak pointer to be used on the display thread.
   base::WeakPtr<Core> GetWeakPtr();
 
  private:
   remoting::ChromotingClientRuntime* runtime_;
 
-  // Will be std::move'd when GrabRendererProxy() is called.
-  std::unique_ptr<RendererProxy> owned_renderer_proxy_;
-  base::WeakPtr<RendererProxy> renderer_proxy_;
-
   // Will be std::move'd when GrabFrameConsumer() is called.
   std::unique_ptr<DualBufferFrameConsumer> owned_frame_consumer_;
   base::WeakPtr<DualBufferFrameConsumer> frame_consumer_;
@@ -80,6 +77,9 @@ class Core : public protocol::CursorShapeStub, public GlRendererDelegate {
   std::unique_ptr<GlRenderer> renderer_;
   __weak id<GlDisplayHandlerDelegate> handler_delegate_;
 
+  // This should be used and deleted on the UI thread.
+  std::unique_ptr<RendererProxy> renderer_proxy_;
+
   // Valid only when the surface is created.
   __weak EAGLView* view_;
 
@@ -92,7 +92,7 @@ class Core : public protocol::CursorShapeStub, public GlRendererDelegate {
 
 Core::Core() : weak_factory_(this) {
   runtime_ = ChromotingClientRuntime::GetInstance();
-  DCHECK(!runtime_->display_task_runner()->BelongsToCurrentThread());
+  DCHECK(runtime_->ui_task_runner()->BelongsToCurrentThread());
 
   weak_ptr_ = weak_factory_.GetWeakPtr();
 
@@ -103,9 +103,8 @@ Core::Core() : weak_factory_(this) {
       protocol::FrameConsumer::PixelFormat::FORMAT_RGBA));
   frame_consumer_ = owned_frame_consumer_->GetWeakPtr();
 
-  owned_renderer_proxy_.reset(
-      new RendererProxy(runtime_->display_task_runner()));
-  renderer_proxy_ = owned_renderer_proxy_->GetWeakPtr();
+  renderer_proxy_ =
+      std::make_unique<RendererProxy>(runtime_->display_task_runner());
 
   runtime_->display_task_runner()->PostTask(
       FROM_HERE, base::Bind(&Core::Initialize, GetWeakPtr()));
@@ -113,6 +112,7 @@ Core::Core() : weak_factory_(this) {
 
 Core::~Core() {
   DCHECK(runtime_->display_task_runner()->BelongsToCurrentThread());
+  runtime_->ui_task_runner()->DeleteSoon(FROM_HERE, renderer_proxy_.release());
 }
 
 void Core::Initialize() {
@@ -134,9 +134,14 @@ void Core::Initialize() {
 
   renderer_ = remoting::GlRenderer::CreateGlRendererWithDesktop();
 
-  renderer_proxy_->Initialize(renderer_->GetWeakPtr());
-
   renderer_->SetDelegate(weak_ptr_);
+
+  // Safe to use base::Unretained because |renderer_proxy_| is destroyed on UI
+  // after Core is destroyed.
+  runtime_->ui_task_runner()->PostTask(
+      FROM_HERE, base::BindOnce(&RendererProxy::Initialize,
+                                base::Unretained(renderer_proxy_.get()),
+                                renderer_->GetWeakPtr()));
 }
 
 void Core::SetHandlerDelegate(id<GlDisplayHandlerDelegate> delegate) {
@@ -144,11 +149,6 @@ void Core::SetHandlerDelegate(id<GlDisplayHandlerDelegate> delegate) {
   handler_delegate_ = delegate;
 }
 
-std::unique_ptr<RendererProxy> Core::GrabRendererProxy() {
-  DCHECK(owned_renderer_proxy_);
-  return std::move(owned_renderer_proxy_);
-}
-
 void Core::SetCursorShape(const protocol::CursorShapeInfo& cursor_shape) {
   DCHECK(runtime_->display_task_runner()->BelongsToCurrentThread());
   renderer_->OnCursorShapeChanged(cursor_shape);
@@ -265,16 +265,12 @@ base::WeakPtr<remoting::GlDisplayHandler::Core> Core::GetWeakPtr() {
 
 #pragma mark - Public
 
-- (std::unique_ptr<remoting::RendererProxy>)CreateRendererProxy {
-  return _core->GrabRendererProxy();
-}
-
-- (std::unique_ptr<remoting::protocol::VideoRenderer>)CreateVideoRenderer {
+- (std::unique_ptr<remoting::protocol::VideoRenderer>)createVideoRenderer {
   return std::make_unique<remoting::SoftwareVideoRenderer>(
       _core->GrabFrameConsumer());
 }
 
-- (std::unique_ptr<remoting::protocol::CursorShapeStub>)CreateCursorShapeStub {
+- (std::unique_ptr<remoting::protocol::CursorShapeStub>)createCursorShapeStub {
   return std::make_unique<remoting::CursorShapeStubProxy>(
       _core->GetWeakPtr(), _runtime->display_task_runner());
 }
@@ -302,6 +298,10 @@ base::WeakPtr<remoting::GlDisplayHandler::Core> Core::GetWeakPtr() {
 
 #pragma mark - Properties
 
+- (remoting::RendererProxy*)rendererProxy {
+  return _core->renderer_proxy();
+}
+
 - (void)setDelegate:(id<GlDisplayHandlerDelegate>)delegate {
   _runtime->display_task_runner()->PostTask(
       FROM_HERE,

remoting/ios/session/remoting_client.mm

@@ -23,7 +23,6 @@
 #include "remoting/client/chromoting_client_runtime.h"
 #include "remoting/client/chromoting_session.h"
 #include "remoting/client/connect_to_host_info.h"
-#include "remoting/client/display/renderer_proxy.h"
 #include "remoting/client/gesture_interpreter.h"
 #include "remoting/client/input/keyboard_interpreter.h"
 #import "remoting/ios/facade/remoting_authentication.h"
@@ -67,7 +66,6 @@ static void ResolveFeedbackDataCallback(
   remoting::protocol::SecretFetchedCallback _secretFetchedCallback;
   remoting::GestureInterpreter _gestureInterpreter;
   remoting::KeyboardInterpreter _keyboardInterpreter;
-  std::unique_ptr<remoting::RendererProxy> _renderer;
 
   // _session is valid only when the session is connected.
   std::unique_ptr<remoting::ChromotingSession> _session;
@@ -142,10 +140,9 @@ static void ResolveFeedbackDataCallback(
   _displayHandler.delegate = self;
 
   _session.reset(new remoting::ChromotingSession(
-      _sessonDelegate->GetWeakPtr(), [_displayHandler CreateCursorShapeStub],
-      [_displayHandler CreateVideoRenderer], std::move(audioStream), info));
-  _renderer = [_displayHandler CreateRendererProxy];
-  _gestureInterpreter.SetContext(_renderer.get(), _session.get());
+      _sessonDelegate->GetWeakPtr(), [_displayHandler createCursorShapeStub],
+      [_displayHandler createVideoRenderer], std::move(audioStream), info));
+  _gestureInterpreter.SetContext(_displayHandler.rendererProxy, _session.get());
   _keyboardInterpreter.SetContext(_session.get());
 
   _session->Connect();
@@ -156,14 +153,6 @@ static void ResolveFeedbackDataCallback(
 
   _displayHandler = nil;
 
-  // This needs to be deleted on the display thread since GlDisplayHandler binds
-  // its WeakPtrFactory to the display thread.
-  // TODO(yuweih): Ideally this constraint can be removed once we allow
-  // GlRenderer to be created on the UI thread before being used.
-  if (_renderer) {
-    _runtime->display_task_runner()->DeleteSoon(FROM_HERE, _renderer.release());
-  }
-
   _gestureInterpreter.SetContext(nullptr, nullptr);
   _keyboardInterpreter.SetContext(nullptr);
 }