Bug 1196682 - DebugDataSender is not thread safe. r=kamidphish
authorCJKu <cku@mozilla.com>
Mon, 24 Aug 2015 02:00:00 -0400
changeset 259030 3e0ba8a66ed401eb3b423fa7d4c4a69763df5e09
parent 259029 dd9b9887e889d2d41a72b0b715ac886f849cb4e1
child 259031 c678e1317fa0b592169260b7e98a8985abc267ec
push id29268
push userryanvm@gmail.com
push dateTue, 25 Aug 2015 00:37:23 +0000
treeherdermozilla-central@08015770c9d6 [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewerskamidphish
bugs1196682
milestone43.0a1
first release with
nightly linux32
nightly linux64
nightly mac
nightly win32
nightly win64
last release without
nightly linux32
nightly linux64
nightly mac
nightly win32
nightly win64
Bug 1196682 - DebugDataSender is not thread safe. r=kamidphish
gfx/layers/LayerScope.cpp
--- a/gfx/layers/LayerScope.cpp
+++ b/gfx/layers/LayerScope.cpp
@@ -82,18 +82,16 @@ public:
         MOZ_ASSERT(NS_IsMainThread());
 
         MutexAutoLock lock(mHandlerMutex);
         mHandlers.Clear();
     }
 
     bool WriteAll(void *ptr, uint32_t size)
     {
-        MOZ_ASSERT(NS_IsMainThread());
-
         for (int32_t i = mHandlers.Length() - 1; i >= 0; --i) {
             if (!mHandlers[i]->WriteToStream(ptr, size)) {
                 // Send failed, remove this handler
                 RemoveConnection(i);
             }
         }
 
         return true;
@@ -120,17 +118,22 @@ private:
 
         nsRefPtr<SocketHandler> temp = new SocketHandler();
         temp->OpenStream(aTransport);
         mHandlers.AppendElement(temp.get());
     }
 
     void RemoveConnection(uint32_t aIndex)
     {
-        MOZ_ASSERT(NS_IsMainThread());
+        // TBD: RemoveConnection is executed on the compositor thread and
+        // AddConntection is executed on the main thread, which might be
+        // a problem if a user disconnect and connect readlly quickly at
+        // viewer side.
+
+        // We should dispatch RemoveConnection onto main thead.
         MOZ_ASSERT(aIndex < mHandlers.Length());
 
         MutexAutoLock lock(mHandlerMutex);
         mHandlers.RemoveElementAt(aIndex);
     }
 
     friend class SocketListener;
     class SocketListener : public nsIServerSocketListener
@@ -738,67 +741,135 @@ protected:
     gfx::Matrix4x4 mMVMatrix;
     size_t mRects;
     gfx::Rect mLayerRects[4];
     gfx::Rect mTextureRects[4];
     std::list<GLuint> mTexIDs;
     uint64_t mLayerRef;
 };
 
-class DebugDataSender : public nsIRunnable
+class DebugDataSender
 {
-    virtual ~DebugDataSender() {
-        Cleanup();
-    }
+public:
+   NS_INLINE_DECL_THREADSAFE_REFCOUNTING(DebugDataSender)
+
+    // Append a DebugData into mList on mThread
+    class AppendTask: public nsIRunnable
+    {
+    public:
+        NS_DECL_THREADSAFE_ISUPPORTS
+
+        AppendTask(DebugDataSender *host, DebugGLData *d)
+            : mData(d),
+              mHost(host)
+        {  }
+
+        NS_IMETHODIMP Run() override {
+            mHost->mList.insertBack(mData);
+            return NS_OK;
+        }
+
+    private:
+        virtual ~AppendTask() { }
+
+        DebugGLData *mData;
+        // Keep a strong reference to DebugDataSender to prevent this object
+        // accessing mHost on mThread, when it's been destroyed on the main
+        // thread.
+        nsRefPtr<DebugDataSender> mHost;
+    };
+
+    // Clear all DebugData in mList on mThead.
+    class ClearTask: public nsIRunnable
+    {
+    public:
+        NS_DECL_THREADSAFE_ISUPPORTS
+        ClearTask(DebugDataSender *host)
+            : mHost(host)
+        {  }
 
-public:
+        NS_IMETHODIMP Run() override {
+            mHost->RemoveData();
+            return NS_OK;
+        }
+
+    private:
+        virtual ~ClearTask() { }
+
+        nsRefPtr<DebugDataSender> mHost;
+    };
+
+    // Send all DebugData in mList via websocket, and then, clean up
+    // mList on mThread.
+    class SendTask: public nsIRunnable
+    {
+    public:
+        NS_DECL_THREADSAFE_ISUPPORTS
+
+        SendTask(DebugDataSender *host)
+            : mHost(host)
+        {  }
 
-    NS_DECL_THREADSAFE_ISUPPORTS
+        NS_IMETHODIMP Run() override {
+            // Sendout all appended debug data.
+            DebugGLData *d = nullptr;
+            while ((d = mHost->mList.popFirst()) != nullptr) {
+                UniquePtr<DebugGLData> cleaner(d);
+                if (!d->Write()) {
+                    gLayerScopeManager.DestroyServerSocket();
+                    break;
+                }
+            }
 
-    DebugDataSender() { }
+            // Cleanup.
+            mHost->RemoveData();
+            return NS_OK;
+        }
+    private:
+        virtual ~SendTask() { }
+
+        nsRefPtr<DebugDataSender> mHost;
+    };
+
+    DebugDataSender(nsIThread *thread)
+        : mThread(thread)
+    {  }
 
     void Append(DebugGLData *d) {
-        mList.insertBack(d);
+        mThread->Dispatch(new AppendTask(this, d), NS_DISPATCH_NORMAL);
     }
 
     void Cleanup() {
+        mThread->Dispatch(new ClearTask(this), NS_DISPATCH_NORMAL);
+    }
+
+    void Send() {
+        mThread->Dispatch(new SendTask(this), NS_DISPATCH_NORMAL);
+    }
+
+protected:
+    virtual ~DebugDataSender() {}
+    void RemoveData() {
+        MOZ_ASSERT(NS_GetCurrentThread() == mThread);
         if (mList.isEmpty())
             return;
 
         DebugGLData *d;
         while ((d = mList.popFirst()) != nullptr)
             delete d;
     }
 
-    NS_IMETHODIMP Run() override {
-        DebugGLData *d;
-        nsresult rv = NS_OK;
-
-        while ((d = mList.popFirst()) != nullptr) {
-            UniquePtr<DebugGLData> cleaner(d);
-            if (!d->Write()) {
-                rv = NS_ERROR_FAILURE;
-                break;
-            }
-        }
-
-        Cleanup();
-
-        if (NS_FAILED(rv)) {
-            gLayerScopeManager.DestroyServerSocket();
-        }
-
-        return NS_OK;
-    }
-
-protected:
+    // We can only modify or aceess mList on mThread.
     LinkedList<DebugGLData> mList;
+    nsCOMPtr<nsIThread>     mThread;
 };
 
-NS_IMPL_ISUPPORTS(DebugDataSender, nsIRunnable);
+NS_IMPL_ISUPPORTS(DebugDataSender::AppendTask, nsIRunnable);
+NS_IMPL_ISUPPORTS(DebugDataSender::ClearTask, nsIRunnable);
+NS_IMPL_ISUPPORTS(DebugDataSender::SendTask, nsIRunnable);
 
 
 /*
  * LayerScope SendXXX Structure
  * 1. SendLayer
  * 2. SendEffectChain
  *   1. SendTexturedEffect
  *      -> SendTextureSource
@@ -1575,34 +1646,36 @@ LayerScopeWebSocketManager::~LayerScopeW
 {
     mServerSocket->Close();
 }
 
 void
 LayerScopeWebSocketManager::AppendDebugData(DebugGLData *aDebugData)
 {
     if (!mCurrentSender) {
-        mCurrentSender = new DebugDataSender();
+        mCurrentSender = new DebugDataSender(mDebugSenderThread);
     }
 
     mCurrentSender->Append(aDebugData);
 }
 
 void
 LayerScopeWebSocketManager::CleanDebugData()
 {
     if (mCurrentSender) {
         mCurrentSender->Cleanup();
     }
 }
 
 void
 LayerScopeWebSocketManager::DispatchDebugData()
 {
-    mDebugSenderThread->Dispatch(mCurrentSender, NS_DISPATCH_NORMAL);
+    MOZ_ASSERT(mCurrentSender.get() != nullptr);
+
+    mCurrentSender->Send();
     mCurrentSender = nullptr;
 }
 
 NS_IMETHODIMP LayerScopeWebSocketManager::SocketListener::OnSocketAccepted(
                                      nsIServerSocket *aServ,
                                      nsISocketTransport *aTransport)
 {
     if (!gLayerScopeManager.GetSocketManager())