Fix threading issue in StreamTextureProxyImpl

content/renderer/media/android/stream_texture_factory_impl.cc

Index: content/renderer/media/android/stream_texture_factory_impl.cc
diff --git a/content/renderer/media/android/stream_texture_factory_impl.cc b/content/renderer/media/android/stream_texture_factory_impl.cc
index bf95b143faada382b44d2993bce1742cfa9c32ed..a8b9a32d90d9df9ecf6baad7a72ebf4cc4e3a79f 100644
--- a/content/renderer/media/android/stream_texture_factory_impl.cc
+++ b/content/renderer/media/android/stream_texture_factory_impl.cc
@@ -32,15 +32,13 @@ class StreamTextureProxyImpl : public StreamTextureProxy,
   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_;
-  scoped_refptr<base::MessageLoopProxy> loop_;
 
-  base::Lock client_lock_;
+  base::Lock lock_;

[no sievers, 2014/09/18 18:46:53]

nit: put a comment 'protects access to client_ and loop_.

[boliu, 2014/09/18 19:15:49]

Done.

   cc::VideoFrameProvider::Client* client_;
+  scoped_refptr<base::MessageLoopProxy> loop_;
 
   DISALLOW_IMPLICIT_CONSTRUCTORS(StreamTextureProxyImpl);
 };
@@ -51,28 +49,33 @@ StreamTextureProxyImpl::StreamTextureProxyImpl(StreamTextureHost* host)
 StreamTextureProxyImpl::~StreamTextureProxyImpl() {}
 
 void StreamTextureProxyImpl::Release() {
+  {

[no sievers, 2014/09/18 18:46:53]

nit: // we cannot call into client anymore (from any thread) after returning
from here

[boliu, 2014/09/18 19:15:49]

Done.

+    base::AutoLock lock(lock_);
+    client_ = NULL;
+  }
   // Assumes this is the last reference to this object. So no need to acquire
   // lock.

[no sievers, 2014/09/18 18:46:52]

nit: maybe rephrase this comment to that outside calls into this object are not
allowed/expected anymore from either thread when Release() is called.

Assuming that we don't allow binding to a yet completely different thread
(DCHECK in line 72 below), you could simply handle it here though by just
putting loop_ into a local variable while holding the lock above. It might not
make too much sense to handle this case though, since it gets a bit silly.

[boliu, 2014/09/18 19:15:49]

Done.
I had that in PS1, then Min said it's not needed, which is true.

-  SetClient(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);

[qinmin, 2014/09/18 16:45:11]

loop is not always non-null.
For Layout tests, RenderThreadImpl::current()->compositor_message_loop_proxy()
actually returns NULL

[no sievers, 2014/09/18 18:46:53]

should we be passing the main thread's loop then instead for those cases?

[boliu, 2014/09/18 19:15:49]

In another CL after I reproduce the problem.

-  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_|
@@ -80,26 +83,21 @@ void StreamTextureProxyImpl::BindToLoop(
   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);
 }