Bug 1246788 - Properly synchronize and manage UI thread tasks; r=rbarker
authorJim Chen <nchen@mozilla.com>
Wed, 10 Feb 2016 18:54:55 -0500
changeset 283897 3bf743cb74216ffb94862ca588973c4a673c4bf2
parent 283896 b1a7043faedd2d1562c22fe8ad10b6356383c5f7
child 283898 f02665b3a963a6df9a598875a0edf29f776e4d0d
push id17557
push usercbook@mozilla.com
push dateThu, 11 Feb 2016 10:57:16 +0000
treeherderfx-team@7a7c04a6d9df [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewersrbarker
bugs1246788
milestone47.0a1
Bug 1246788 - Properly synchronize and manage UI thread tasks; r=rbarker Add proper synchronization to the UI thread task queue so we don't run into race conditions. Also use UniquePtr to manage the contained task, to fix a previous memory leak.
widget/android/AndroidBridge.cpp
widget/android/AndroidBridge.h
--- a/widget/android/AndroidBridge.cpp
+++ b/widget/android/AndroidBridge.cpp
@@ -43,16 +43,18 @@
 #include "nsContentUtils.h"
 #include "nsIScriptError.h"
 #include "nsIHttpChannel.h"
 
 #include "MediaCodec.h"
 #include "SurfaceTexture.h"
 #include "GLContextProvider.h"
 
+#include "mozilla/TimeStamp.h"
+#include "mozilla/UniquePtr.h"
 #include "mozilla/dom/ContentChild.h"
 
 using namespace mozilla;
 using namespace mozilla::gfx;
 using namespace mozilla::jni;
 using namespace mozilla::widget;
 
 AndroidBridge* AndroidBridge::sBridge = nullptr;
@@ -179,19 +181,20 @@ AndroidBridge::DeconstructBridge()
     }
 }
 
 AndroidBridge::~AndroidBridge()
 {
 }
 
 AndroidBridge::AndroidBridge()
-  : mLayerClient(nullptr),
-    mPresentationWindow(nullptr),
-    mPresentationSurface(nullptr)
+  : mLayerClient(nullptr)
+  , mUiTaskQueueLock("UiTaskQueue")
+  , mPresentationWindow(nullptr)
+  , mPresentationSurface(nullptr)
 {
     ALOG_BRIDGE("AndroidBridge::Init");
 
     JNIEnv* const jEnv = jni::GetGeckoThreadEnv();
     AutoLocalJNIFrame jniFrame(jEnv);
 
     mClassLoader = Object::GlobalRef(jEnv, widget::GeckoThread::ClsLoader());
     mClassLoaderLoadClass = GetMethodID(
@@ -2042,63 +2045,115 @@ AndroidBridge::ProgressiveUpdateCallback
 
     aScrollOffset.x = progressiveUpdateData->X();
     aScrollOffset.y = progressiveUpdateData->Y();
     aZoom.scale = progressiveUpdateData->Scale();
 
     return progressiveUpdateData->Abort();
 }
 
+class AndroidBridge::DelayedTask
+{
+    using TimeStamp = mozilla::TimeStamp;
+    using TimeDuration = mozilla::TimeDuration;
+
+public:
+    DelayedTask(Task* aTask)
+        : mTask(aTask)
+        , mRunTime() // Null timestamp representing no delay.
+    {}
+
+    DelayedTask(Task* aTask, int aDelayMs)
+        : mTask(aTask)
+        , mRunTime(TimeStamp::Now() + TimeDuration::FromMilliseconds(aDelayMs))
+    {}
+
+    bool IsEarlierThan(const DelayedTask& aOther) const
+    {
+        if (mRunTime) {
+            return aOther.mRunTime ? mRunTime < aOther.mRunTime : false;
+        }
+        // In the case of no delay, we're earlier if aOther has a delay.
+        // Otherwise, we're not earlier, to maintain task order.
+        return !!aOther.mRunTime;
+    }
+
+    int64_t MillisecondsToRunTime() const
+    {
+        if (mRunTime) {
+            return int64_t((mRunTime - TimeStamp::Now()).ToMilliseconds());
+        }
+        return 0;
+    }
+
+    UniquePtr<Task>&& GetTask()
+    {
+        return static_cast<UniquePtr<Task>&&>(mTask);
+    }
+
+private:
+    UniquePtr<Task> mTask;
+    const TimeStamp mRunTime;
+};
+
+
 void
 AndroidBridge::PostTaskToUiThread(Task* aTask, int aDelayMs)
 {
-    // add the new task into the mDelayedTaskQueue, sorted with
+    // add the new task into the mUiTaskQueue, sorted with
     // the earliest task first in the queue
-    DelayedTask* newTask = new DelayedTask(aTask, aDelayMs);
-    uint32_t i = 0;
-    while (i < mDelayedTaskQueue.Length()) {
-        if (newTask->IsEarlierThan(mDelayedTaskQueue[i])) {
-            mDelayedTaskQueue.InsertElementAt(i, newTask);
-            break;
+    size_t i;
+    DelayedTask newTask(aDelayMs ? DelayedTask(aTask, aDelayMs)
+                                 : DelayedTask(aTask));
+
+    {
+        MutexAutoLock lock(mUiTaskQueueLock);
+
+        for (i = 0; i < mUiTaskQueue.Length(); i++) {
+            if (newTask.IsEarlierThan(mUiTaskQueue[i])) {
+                mUiTaskQueue.InsertElementAt(i, mozilla::Move(newTask));
+                break;
+            }
         }
-        i++;
+
+        if (i == mUiTaskQueue.Length()) {
+            // We didn't insert the task, which means we should append it.
+            mUiTaskQueue.AppendElement(mozilla::Move(newTask));
+        }
     }
-    if (i == mDelayedTaskQueue.Length()) {
-        // this new task will run after all the existing tasks in the queue
-        mDelayedTaskQueue.AppendElement(newTask);
-    }
+
     if (i == 0) {
         // if we're inserting it at the head of the queue, notify Java because
         // we need to get a callback at an earlier time than the last scheduled
         // callback
         GeckoAppShell::RequestUiThreadCallback((int64_t)aDelayMs);
     }
 }
 
 int64_t
 AndroidBridge::RunDelayedUiThreadTasks()
 {
-    while (mDelayedTaskQueue.Length() > 0) {
-        DelayedTask* nextTask = mDelayedTaskQueue[0];
-        int64_t timeLeft = nextTask->MillisecondsToRunTime();
+    MutexAutoLock lock(mUiTaskQueueLock);
+
+    while (!mUiTaskQueue.IsEmpty()) {
+        const int64_t timeLeft = mUiTaskQueue[0].MillisecondsToRunTime();
         if (timeLeft > 0) {
             // this task (and therefore all remaining tasks)
             // have not yet reached their runtime. return the
             // time left until we should be called again
             return timeLeft;
         }
 
-        // we have a delayed task to run. extract it from
-        // the wrapper and free the wrapper
+        // Retrieve task before unlocking/running.
+        const UniquePtr<Task> nextTask(mUiTaskQueue[0].GetTask());
+        mUiTaskQueue.RemoveElementAt(0);
 
-        mDelayedTaskQueue.RemoveElementAt(0);
-        Task* task = nextTask->GetTask();
-        delete nextTask;
-
-        task->Run();
+        // Unlock to allow posting new tasks reentrantly.
+        MutexAutoUnlock unlock(mUiTaskQueueLock);
+        nextTask->Run();
     }
     return -1;
 }
 
 void*
 AndroidBridge::GetPresentationWindow()
 {
     return mPresentationWindow;
--- a/widget/android/AndroidBridge.h
+++ b/widget/android/AndroidBridge.h
@@ -16,27 +16,26 @@
 
 #include "GeneratedJNIWrappers.h"
 #include "AndroidJavaWrappers.h"
 
 #include "nsIMutableArray.h"
 #include "nsIMIMEInfo.h"
 #include "nsColor.h"
 #include "gfxRect.h"
-#include "mozilla/gfx/Point.h"
 
 #include "nsIAndroidBridge.h"
 #include "nsIMobileMessageCallback.h"
 #include "nsIMobileMessageCursorCallback.h"
 #include "nsIDOMDOMCursor.h"
 
 #include "mozilla/Likely.h"
-#include "mozilla/StaticPtr.h"
-#include "mozilla/TimeStamp.h"
+#include "mozilla/Mutex.h"
 #include "mozilla/Types.h"
+#include "mozilla/gfx/Point.h"
 #include "mozilla/jni/Utils.h"
 
 // Some debug #defines
 // #define DEBUG_ANDROID_EVENTS
 // #define DEBUG_ANDROID_WIDGET
 
 class nsIObserver;
 class Task;
@@ -71,41 +70,16 @@ typedef struct AndroidSystemColors {
     nscolor textColorTertiaryInverse;
     nscolor textColorHighlight;
     nscolor colorForeground;
     nscolor colorBackground;
     nscolor panelColorForeground;
     nscolor panelColorBackground;
 } AndroidSystemColors;
 
-class DelayedTask {
-public:
-    DelayedTask(Task* aTask, int aDelayMs) {
-        mTask = aTask;
-        mRunTime = mozilla::TimeStamp::Now() + mozilla::TimeDuration::FromMilliseconds(aDelayMs);
-    }
-
-    bool IsEarlierThan(DelayedTask *aOther) {
-        return mRunTime < aOther->mRunTime;
-    }
-
-    int64_t MillisecondsToRunTime() {
-        mozilla::TimeDuration timeLeft = mRunTime - mozilla::TimeStamp::Now();
-        return (int64_t)timeLeft.ToMilliseconds();
-    }
-
-    Task* GetTask() {
-        return mTask;
-    }
-
-private:
-    Task* mTask;
-    mozilla::TimeStamp mRunTime;
-};
-
 class ThreadCursorContinueCallback : public nsICursorContinueCallback
 {
 public:
     NS_DECL_ISUPPORTS
     NS_DECL_NSICURSORCONTINUECALLBACK
 
     ThreadCursorContinueCallback(int aRequestId)
         : mRequestId(aRequestId)
@@ -433,19 +407,20 @@ protected:
     int (* ANativeWindow_getHeight)(void * window);
 
     int (* Surface_lock)(void* surface, void* surfaceInfo, void* region, bool block);
     int (* Surface_unlockAndPost)(void* surface);
     void (* Region_constructor)(void* region);
     void (* Region_set)(void* region, void* rect);
 
 private:
-    // This will always be accessed from one thread (the Java UI thread),
-    // so we don't need to do locking to touch it.
-    nsTArray<DelayedTask*> mDelayedTaskQueue;
+    class DelayedTask;
+    nsTArray<DelayedTask> mUiTaskQueue;
+    mozilla::Mutex mUiTaskQueueLock;
+
 public:
     void PostTaskToUiThread(Task* aTask, int aDelayMs);
     int64_t RunDelayedUiThreadTasks();
 
     void* GetPresentationWindow();
     void SetPresentationWindow(void* aPresentationWindow);
 
     EGLSurface GetPresentationSurface();