[Remoting Android] Fix thread issue with OpenGL drawable

remoting/client/jni/jni_gl_display_handler.cc

Index: remoting/client/jni/jni_gl_display_handler.cc
diff --git a/remoting/client/jni/jni_gl_display_handler.cc b/remoting/client/jni/jni_gl_display_handler.cc
index 85c0641e79384e73e8776fcb1caf67fcd38e4d66..118c60b5f12f10c170336be1f0d59676a6a600ac 100644
--- a/remoting/client/jni/jni_gl_display_handler.cc
+++ b/remoting/client/jni/jni_gl_display_handler.cc
@@ -41,6 +41,9 @@ class JniGlDisplayHandler::Core : public protocol::CursorShapeStub,
   // thread but no more than once.
   std::unique_ptr<protocol::FrameConsumer> GrabFrameConsumer();
 
+  void OnFrameReceived(std::unique_ptr<webrtc::DesktopFrame> frame,
+                       const base::Closure& done);
+
   void SurfaceCreated(base::android::ScopedJavaGlobalRef<jobject> surface);
   void SurfaceChanged(int width, int height);
   void SurfaceDestroyed();
@@ -53,6 +56,9 @@ class JniGlDisplayHandler::Core : public protocol::CursorShapeStub,
   base::WeakPtr<Core> GetWeakPtr();
 
  private:
+  // Initializes the core on the display thread. Can be called on any thread.
+  void Initialize();
+
   ChromotingJniRuntime* runtime_;
   base::WeakPtr<JniGlDisplayHandler> shell_;
 
@@ -75,11 +81,11 @@ class JniGlDisplayHandler::Core : public protocol::CursorShapeStub,
 JniGlDisplayHandler::Core::Core(ChromotingJniRuntime* runtime,
                                 base::WeakPtr<JniGlDisplayHandler> shell)
     : runtime_(runtime), shell_(shell), weak_factory_(this) {
-  renderer_ = GlRenderer::CreateGlRendererWithDesktop();
   weak_ptr_ = weak_factory_.GetWeakPtr();
-  renderer_->SetDelegate(weak_ptr_);
+  Initialize();

[Sergey Ulanov, 2017/01/18 22:33:40]

runtime_->display_task_runner()->PostTask(
         FROM_HERE, base::Bind(&JniGlDisplayHandler::Core::Initialize,
                               base::Unretained(this)));

[Yuwei, 2017/01/18 23:11:56]

Since we still need the weak pointer for other purposes, why not just use weak
pointer here so that we don't need to check the thread safety in mind when
reading this code?

[Sergey Ulanov, 2017/01/18 23:32:43]

The "Core" class used here to switch to a different thread is a common pattern
that is used in many other places. This pattern doesn't need WeakPtr. Some
issues I see with using weak_ptr here:
 1. Someone reads this code, recognizes the pattern, but gets confused by
WeakPtr.
 2. Someone removes weak_ptr dependency from DualBufferFrameConsumer, but
assumes it's still needed for this PostTask() keeps weak_factory_.
 3. Someone uses this code as reference for the pattern, assumes that weak_ptr
is meaningful here, and copies it in a new class.

Overall WeakPtr is not a silver-bullet. If it's used here it doesn't
automatically make this PostTask call safe (particularly because WeakPtr is not
thread-safe). You are just trading one set of potential issues for another.

[Yuwei, 2017/01/19 00:27:33]

Acknowledged. Done.

+  // Do not bind GlRenderer::OnFrameReceived. |renderer_| is not ready yet.
   owned_frame_consumer_.reset(new DualBufferFrameConsumer(
-      base::Bind(&GlRenderer::OnFrameReceived, renderer_->GetWeakPtr()),
+      base::Bind(&JniGlDisplayHandler::Core::OnFrameReceived, weak_ptr_),
       runtime_->display_task_runner(),
       protocol::FrameConsumer::PixelFormat::FORMAT_RGBA));
   frame_consumer_ = owned_frame_consumer_->GetWeakPtr();
@@ -118,6 +124,13 @@ JniGlDisplayHandler::Core::GrabFrameConsumer() {
   return std::move(owned_frame_consumer_);
 }
 
+void JniGlDisplayHandler::Core::OnFrameReceived(
+    std::unique_ptr<webrtc::DesktopFrame> frame,
+    const base::Closure& done) {
+  DCHECK(runtime_->display_task_runner()->BelongsToCurrentThread());
+  renderer_->OnFrameReceived(std::move(frame), done);
+}
+
 void JniGlDisplayHandler::Core::SurfaceCreated(
     base::android::ScopedJavaGlobalRef<jobject> surface) {
   DCHECK(runtime_->display_task_runner()->BelongsToCurrentThread());
@@ -180,6 +193,18 @@ JniGlDisplayHandler::Core::GetWeakPtr() {
   return weak_ptr_;
 }
 
+void JniGlDisplayHandler::Core::Initialize() {
+  if (!runtime_->display_task_runner()->BelongsToCurrentThread()) {

[Sergey Ulanov, 2017/01/18 22:33:40]

PostTask in the constructor and replace this code with
DCHECK(runtime_->display_task_runner()->BelongsToCurrentThread()).

[Yuwei, 2017/01/19 00:27:33]

Done.

+    runtime_->display_task_runner()->PostTask(
+        FROM_HERE, base::Bind(&JniGlDisplayHandler::Core::Initialize,
+                              weak_ptr_));

[Sergey Ulanov, 2017/01/18 22:33:40]

Don't need to use weak pointer here because Core is destroyed on the same
thread.

[Yuwei, 2017/01/19 00:27:33]

Done.

+    return;
+  }
+
+  renderer_ = GlRenderer::CreateGlRendererWithDesktop();
+  renderer_->SetDelegate(weak_ptr_);
+}
+
 // Shell implementations.
 
 JniGlDisplayHandler::JniGlDisplayHandler(