Fix threading issue in StreamTextureProxyImpl

content/renderer/media/android/stream_texture_factory_impl.cc

 // Copyright 2014 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 "content/renderer/media/android/stream_texture_factory_impl.h"
 
 #include "cc/output/context_provider.h"
 #include "content/common/gpu/client/gpu_channel_host.h"
 #include "content/common/gpu/gpu_messages.h"
 #include "content/renderer/gpu/stream_texture_host_android.h"
@@ skipping 14 matching lines @@
   virtual void BindToLoop(int32 stream_id,
                           cc::VideoFrameProvider::Client* client,
                           scoped_refptr<base::MessageLoopProxy> loop) OVERRIDE;
   virtual void Release() OVERRIDE;
 
   // StreamTextureHost::Listener implementation:
   virtual void OnFrameAvailable() OVERRIDE;
   virtual void OnMatrixChanged(const float matrix[16]) OVERRIDE;
 
  private:
-  void SetClient(cc::VideoFrameProvider::Client* client);
-  void BindOnThread(int32 stream_id,
-                    scoped_refptr<base::MessageLoopProxy> loop);
+  void BindOnThread(int32 stream_id);
 
   const scoped_ptr<StreamTextureHost> host_;
+
+  base::Lock lock_;
+  cc::VideoFrameProvider::Client* client_;
   scoped_refptr<base::MessageLoopProxy> loop_;
 
-  base::Lock client_lock_;
-  cc::VideoFrameProvider::Client* client_;
-
   DISALLOW_IMPLICIT_CONSTRUCTORS(StreamTextureProxyImpl);
 };
 
 StreamTextureProxyImpl::StreamTextureProxyImpl(StreamTextureHost* host)
     : host_(host), client_(NULL) {}
 
 StreamTextureProxyImpl::~StreamTextureProxyImpl() {}
 
 void StreamTextureProxyImpl::Release() {
-  // Assumes this is the last reference to this object. So no need to acquire
-  // lock.
-  SetClient(NULL);
-  if (!loop_.get() || loop_->BelongsToCurrentThread() ||
-      !loop_->DeleteSoon(FROM_HERE, this)) {
+  scoped_refptr<base::MessageLoopProxy> loop;

[qinmin, 2014/09/18 02:01:30]

why do we need this, cannot we use loop_?

if bindToloop() is called on the impl thread, it will block the main thread and
Release() cannot be called at the same time, am I missing something here?

[boliu, 2014/09/18 02:04:23]

I guess you can argue it both ways.

One side is "all access to client_ and loop_ must be protected".

Another side is "this is release, so the last thing to be called on this thread,
so no need to require any locks".

I chose the first one. It doesn't matter in practice I guess, but it's hard to
understand the second argument.

+  {
+    base::AutoLock lock(lock_);
+    loop = loop_;
+    client_ = NULL;
+  }
+  if (!loop.get() || loop->BelongsToCurrentThread() ||
+      !loop->DeleteSoon(FROM_HERE, this)) {
     delete this;
   }
 }
 
-void StreamTextureProxyImpl::SetClient(cc::VideoFrameProvider::Client* client) {
-  base::AutoLock lock(client_lock_);
-  client_ = client;
-}
-
 void StreamTextureProxyImpl::BindToLoop(
     int32 stream_id,
     cc::VideoFrameProvider::Client* client,
     scoped_refptr<base::MessageLoopProxy> loop) {
   DCHECK(loop);
-  SetClient(client);
+
+  {
+    base::AutoLock lock(lock_);
+    DCHECK(!loop_ || (loop == loop_));
+    loop_ = loop;
+    client_ = client;
+  }
+
   if (loop->BelongsToCurrentThread()) {
-    BindOnThread(stream_id, loop);
+    BindOnThread(stream_id);
     return;
   }
   // Unretained is safe here only because the object is deleted on |loop_|
   // thread.
   loop->PostTask(FROM_HERE,
                  base::Bind(&StreamTextureProxyImpl::BindOnThread,
                             base::Unretained(this),
-                            stream_id,
-                            loop));
+                            stream_id));
 }
 
-void StreamTextureProxyImpl::BindOnThread(
-    int32 stream_id,
-    scoped_refptr<base::MessageLoopProxy> loop) {
-  DCHECK(!loop_ || (loop == loop_));
-  loop_ = loop;
+void StreamTextureProxyImpl::BindOnThread(int32 stream_id) {
   host_->BindToCurrentThread(stream_id, this);
 }
 
 void StreamTextureProxyImpl::OnFrameAvailable() {
-  base::AutoLock lock(client_lock_);
+  base::AutoLock lock(lock_);
   if (client_)
     client_->DidReceiveFrame();
 }
 
 void StreamTextureProxyImpl::OnMatrixChanged(const float matrix[16]) {
-  base::AutoLock lock(client_lock_);
+  base::AutoLock lock(lock_);
   if (client_)
     client_->DidUpdateMatrix(matrix);
 }
 
 }  // namespace
 
 // static
 scoped_refptr<StreamTextureFactoryImpl> StreamTextureFactoryImpl::Create(
     const scoped_refptr<cc::ContextProvider>& context_provider,
     GpuChannelHost* channel,
@@ skipping 52 matching lines @@
 
 void StreamTextureFactoryImpl::AddObserver(
     StreamTextureFactoryContextObserver* obs) {
 }
 
 void StreamTextureFactoryImpl::RemoveObserver(
     StreamTextureFactoryContextObserver* obs) {
 }
 
 }  // namespace content