Bug 1455302 - Robustify the IsSamplerThread() check similarly to the updater code. r=botond

Same as the previous patch, but adapted for the sampler thread.

MozReview-Commit-ID: 7PVaPl38FkM

--HG--
extra : rebase_source : b7637270fea226cde15b9351a4ef8ac7ffab5796

gfx/layers/apz/public/APZSampler.h

@@ -42,7 +42,8 @@ class APZSampler {
   NS_INLINE_DECL_THREADSAFE_REFCOUNTING(APZSampler)
 
 public:
-  explicit APZSampler(const RefPtr<APZCTreeManager>& aApz);
+  APZSampler(const RefPtr<APZCTreeManager>& aApz,
+             bool aIsUsingWebRender);
 
   void SetWebRenderWindowId(const wr::WindowId& aWindowId);
 
@@ -103,11 +104,11 @@ public:
 protected:
   virtual ~APZSampler();
 
-  bool UsingWebRenderSamplerThread() const;
   static already_AddRefed<APZSampler> GetSampler(const wr::WrWindowId& aWindowId);
 
 private:
   RefPtr<APZCTreeManager> mApz;
+  bool mIsUsingWebRender;
 
   // Used to manage the mapping from a WR window id to APZSampler. These are only
   // used if WebRender is enabled. Both sWindowIdMap and mWindowId should only
@@ -119,18 +120,12 @@ private:
   static StaticAutoPtr<std::unordered_map<uint64_t, APZSampler*>> sWindowIdMap;
   Maybe<wr::WrWindowId> mWindowId;
 
+  // Lock used to protected mSamplerThreadId
+  mutable Mutex mThreadIdLock;
   // If WebRender is enabled, this holds the thread id of the render backend
   // thread (which is the sampler thread) for the compositor associated with
   // this APZSampler instance.
-  // 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> mSamplerThreadId;
-#ifdef DEBUG
-  // This flag is used to ensure that we don't ever try to do sampler-thread
-  // stuff before the updater thread has been properly initialized.
-  mutable bool mSamplerThreadQueried;
-#endif
 
   Mutex mSampleTimeLock;
   // Can only be accessed or modified while holding mSampleTimeLock.

gfx/layers/apz/src/APZSampler.cpp

@@ -22,11 +22,11 @@ StaticMutex APZSampler::sWindowIdLock;
 StaticAutoPtr<std::unordered_map<uint64_t, APZSampler*>> APZSampler::sWindowIdMap;
 
 
-APZSampler::APZSampler(const RefPtr<APZCTreeManager>& aApz)
+APZSampler::APZSampler(const RefPtr<APZCTreeManager>& aApz,
+                       bool aIsUsingWebRender)
   : mApz(aApz)
-#ifdef DEBUG
-  , mSamplerThreadQueried(false)
-#endif
+  , mIsUsingWebRender(aIsUsingWebRender)
+  , mThreadIdLock("APZSampler::mThreadIdLock")
   , mSampleTimeLock("APZSampler::mSampleTimeLock")
 {
   MOZ_ASSERT(aApz);
@@ -60,8 +60,7 @@ APZSampler::SetWebRenderWindowId(const wr::WindowId& aWindowId)
 APZSampler::SetSamplerThread(const wr::WrWindowId& aWindowId)
 {
   if (RefPtr<APZSampler> sampler = GetSampler(aWindowId)) {
-    // Ensure nobody tried to use the updater thread before this point.
-    MOZ_ASSERT(!sampler->mSamplerThreadQueried);
+    MutexAutoLock lock(sampler->mThreadIdLock);
     sampler->mSamplerThreadId = Some(PlatformThread::CurrentId());
   }
 }
@@ -218,29 +217,17 @@ APZSampler::AssertOnSamplerThread() const
 bool
 APZSampler::IsSamplerThread() const
 {
-  if (UsingWebRenderSamplerThread()) {
-    return PlatformThread::CurrentId() == *mSamplerThreadId;
+  if (mIsUsingWebRender) {
+    // If the sampler thread id isn't set yet then we cannot be running on the
+    // sampler thread (because we will have the thread id before we run any
+    // other C++ code on it, and this function is only ever invoked from C++
+    // code), so return false in that scenario.
+    MutexAutoLock lock(mThreadIdLock);
+    return mSamplerThreadId && PlatformThread::CurrentId() == *mSamplerThreadId;
   }
   return CompositorThreadHolder::IsInCompositorThread();
 }
 
-bool
-APZSampler::UsingWebRenderSamplerThread() const
-{
-  // If mSamplerThreadId 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 APZSampler is attached or (b) we are attempting to do
-  // something sampler-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 sampler-related thing so early. We catch this
-  // case by setting the mSamplerThreadQueried flag and asserting on WR
-  // initialization.
-#ifdef DEBUG
-  mSamplerThreadQueried = true;
-#endif
-  return mSamplerThreadId.isSome();
-}
-
 /*static*/ already_AddRefed<APZSampler>
 APZSampler::GetSampler(const wr::WrWindowId& aWindowId)
 {

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

@@ -32,7 +32,7 @@ protected:
 
     tm = new TestAPZCTreeManager(mcc);
     updater = new APZUpdater(tm, false);
-    sampler = new APZSampler(tm);
+    sampler = new APZSampler(tm, false);
     apzc = new TestAsyncPanZoomController(LayersId{0}, mcc, tm, mGestureBehavior);
     apzc->SetFrameMetrics(TestFrameMetrics());
     apzc->GetScrollMetadata().SetIsLayersIdRoot(true);

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

@@ -28,7 +28,7 @@ protected:
 
     manager = new TestAPZCTreeManager(mcc);
     updater = new APZUpdater(manager, false);
-    sampler = new APZSampler(manager);
+    sampler = new APZSampler(manager, false);
   }
 
   virtual void TearDown() {

gfx/layers/ipc/CompositorBridgeParent.cpp

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