Bug 1455302 - Allow scheduling updater thread tasks before we have the updater thread id. r=botond

This is possible if we just let the APZUpdater know during construction
if WR is enabled or not, and that information combined with the pref
will allow it to know whether to use the scene builder thread task queue
or just use the compositor thread as the updater thread.

MozReview-Commit-ID: 7IGMMtl7iFP

--HG--
extra : rebase_source : 3950adf77f4b48906b29cdb36f0437df1540bec6

gfx/layers/apz/public/APZUpdater.h

@@ -44,7 +44,8 @@ class APZUpdater {
   NS_INLINE_DECL_THREADSAFE_REFCOUNTING(APZUpdater)
 
 public:
-  explicit APZUpdater(const RefPtr<APZCTreeManager>& aApz);
+  APZUpdater(const RefPtr<APZCTreeManager>& aApz,
+             bool aIsUsingWebRender);
 
   bool HasTreeManager(const RefPtr<APZCTreeManager>& aApz);
   void SetWebRenderWindowId(const wr::WindowId& aWindowId);
@@ -145,6 +146,7 @@ protected:
 
 private:
   RefPtr<APZCTreeManager> mApz;
+  bool mIsUsingWebRender;
 
   // Map from layers id to WebRenderScrollData. This can only be touched on
   // the updater thread.
@@ -193,20 +195,14 @@ private:
   static StaticAutoPtr<std::unordered_map<uint64_t, APZUpdater*>> sWindowIdMap;
   Maybe<wr::WrWindowId> mWindowId;
 
+  // Lock used to protected mUpdaterThreadId;
+  mutable Mutex mThreadIdLock;
   // If WebRender and async scene building are enabled, this holds the thread id
   // of the scene builder thread (which is the updater thread) for the
   // compositor associated with this APZUpdater instance. It may be populated
   // even if async scene building is not enabled, but in that case we don't
   // care about the contents.
-  // This is written to once during init and never cleared, and so reading it
-  // from multiple threads during normal operation (after initialization)
-  // without locking should be fine.
   Maybe<PlatformThreadId> mUpdaterThreadId;
-#ifdef DEBUG
-  // This flag is used to ensure that we don't ever try to do updater-thread
-  // stuff before the updater thread has been properly initialized.
-  mutable bool mUpdaterThreadQueried;
-#endif
 
   // Helper struct that pairs each queued runnable with the layers id that it
   // is associated with. This allows us to easily implement the conceptual

gfx/layers/apz/src/APZUpdater.cpp

@@ -22,11 +22,11 @@ StaticMutex APZUpdater::sWindowIdLock;
 StaticAutoPtr<std::unordered_map<uint64_t, APZUpdater*>> APZUpdater::sWindowIdMap;
 
 
-APZUpdater::APZUpdater(const RefPtr<APZCTreeManager>& aApz)
+APZUpdater::APZUpdater(const RefPtr<APZCTreeManager>& aApz,
+                       bool aIsUsingWebRender)
   : mApz(aApz)
-#ifdef DEBUG
-  , mUpdaterThreadQueried(false)
-#endif
+  , mIsUsingWebRender(aIsUsingWebRender)
+  , mThreadIdLock("APZUpdater::ThreadIdLock")
   , mQueueLock("APZUpdater::QueueLock")
 {
   MOZ_ASSERT(aApz);
@@ -67,8 +67,7 @@ APZUpdater::SetWebRenderWindowId(const wr::WindowId& aWindowId)
 APZUpdater::SetUpdaterThread(const wr::WrWindowId& aWindowId)
 {
   if (RefPtr<APZUpdater> updater = GetUpdater(aWindowId)) {
-    // Ensure nobody tried to use the updater thread before this point.
-    MOZ_ASSERT(!updater->mUpdaterThreadQueried);
+    MutexAutoLock lock(updater->mThreadIdLock);
     updater->mUpdaterThreadId = Some(PlatformThread::CurrentId());
   }
 }
@@ -339,6 +338,13 @@ APZUpdater::RunOnUpdaterThread(LayersId aLayersId, already_AddRefed<Runnable> aT
 {
   RefPtr<Runnable> task = aTask;
 
+  // In the scenario where UsingWebRenderUpdaterThread() is true, this function
+  // might get called early (before mUpdaterThreadId is set). In that case
+  // IsUpdaterThread() will return false and we'll queue the task onto
+  // mUpdaterQueue. This is fine; the task is still guaranteed to run (barring
+  // catastrophic failure) because the WakeSceneBuilder call will still trigger
+  // the callback to run tasks.
+
   if (IsUpdaterThread()) {
     task->Run();
     return;
@@ -380,7 +386,12 @@ bool
 APZUpdater::IsUpdaterThread() const
 {
   if (UsingWebRenderUpdaterThread()) {
-    return PlatformThread::CurrentId() == *mUpdaterThreadId;
+    // If the updater thread id isn't set yet then we cannot be running on the
+    // updater thread (because we will have the thread id before we run any
+    // C++ code on it, and this function is only ever invoked from C++ code),
+    // so return false in that scenario.
+    MutexAutoLock lock(mThreadIdLock);
+    return mUpdaterThreadId && PlatformThread::CurrentId() == *mUpdaterThreadId;
   }
   return CompositorThreadHolder::IsInCompositorThread();
 }
@@ -399,21 +410,7 @@ APZUpdater::RunOnControllerThread(LayersId aLayersId, already_AddRefed<Runnable>
 bool
 APZUpdater::UsingWebRenderUpdaterThread() const
 {
-  if (!gfxPrefs::WebRenderAsyncSceneBuild()) {
-    return false;
-  }
-  // If mUpdaterThreadId is not set at the point that this is called, then
-  // that means that either (a) WebRender is not enabled for the compositor
-  // to which this APZUpdater is attached or (b) we are attempting to do
-  // something updater-related before WebRender is up and running. In case
-  // (a) falling back to the compositor thread is correct, and in case (b)
-  // we should stop doing the updater-related thing so early. We catch this
-  // case by setting the mUpdaterThreadQueried flag and asserting on WR
-  // initialization.
-#ifdef DEBUG
-  mUpdaterThreadQueried = true;
-#endif
-  return mUpdaterThreadId.isSome();
+  return (mIsUsingWebRender && gfxPrefs::WebRenderAsyncSceneBuild());
 }
 
 /*static*/ already_AddRefed<APZUpdater>

gfx/layers/apz/test/gtest/APZCBasicTester.h

@@ -31,7 +31,7 @@ protected:
     APZThreadUtils::SetControllerThread(MessageLoop::current());
 
     tm = new TestAPZCTreeManager(mcc);
-    updater = new APZUpdater(tm);
+    updater = new APZUpdater(tm, false);
     sampler = new APZSampler(tm);
     apzc = new TestAsyncPanZoomController(LayersId{0}, mcc, tm, mGestureBehavior);
     apzc->SetFrameMetrics(TestFrameMetrics());

gfx/layers/apz/test/gtest/APZCTreeManagerTester.h

@@ -27,7 +27,7 @@ protected:
     APZThreadUtils::SetControllerThread(MessageLoop::current());
 
     manager = new TestAPZCTreeManager(mcc);
-    updater = new APZUpdater(manager);
+    updater = new APZUpdater(manager, false);
     sampler = new APZSampler(manager);
   }
 

gfx/layers/ipc/CompositorBridgeParent.cpp

@@ -386,7 +386,7 @@ CompositorBridgeParent::Initialize()
     MOZ_ASSERT(!mApzUpdater);
     mApzcTreeManager = new APZCTreeManager(mRootLayerTreeID);
     mApzSampler = new APZSampler(mApzcTreeManager);
-    mApzUpdater = new APZUpdater(mApzcTreeManager);
+    mApzUpdater = new APZUpdater(mApzcTreeManager, mOptions.UseWebRender());
   }
 
   mCompositorBridgeID = 0;

gfx/layers/ipc/CrossProcessCompositorBridgeParent.cpp

@@ -135,7 +135,7 @@ CrossProcessCompositorBridgeParent::AllocPAPZCTreeManagerParent(const LayersId&
     // retain a reference to itself, through the checkerboard observer.
     LayersId dummyId{0};
     RefPtr<APZCTreeManager> temp = new APZCTreeManager(dummyId);
-    RefPtr<APZUpdater> tempUpdater = new APZUpdater(temp);
+    RefPtr<APZUpdater> tempUpdater = new APZUpdater(temp, false);
     tempUpdater->ClearTree(dummyId);
     return new APZCTreeManagerParent(aLayersId, temp, tempUpdater);
   }