Bug 1455302 - Robustify the IsSamplerThread() check similarly to the updater code. r?botond draft
authorKartikaya Gupta <kgupta@mozilla.com>
Thu, 19 Apr 2018 10:10:00 -0400
changeset 785062 d705c06eb0465f8849b06f5a2a719fb2ca873fa8
parent 785061 f999e7145f955e35a1bfc3f451f2e817be9a47b2
push id107112
push userkgupta@mozilla.com
push dateThu, 19 Apr 2018 14:10:25 +0000
reviewersbotond
bugs1455302
milestone61.0a1
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
gfx/layers/apz/public/APZSampler.h
gfx/layers/apz/src/APZSampler.cpp
gfx/layers/apz/test/gtest/APZCBasicTester.h
gfx/layers/apz/test/gtest/APZCTreeManagerTester.h
gfx/layers/ipc/CompositorBridgeParent.cpp
--- a/gfx/layers/apz/public/APZSampler.h
+++ b/gfx/layers/apz/public/APZSampler.h
@@ -36,17 +36,18 @@ struct ScrollbarData;
  * This interface exposes APZ methods related to "sampling" (i.e. reading the
  * async transforms produced by APZ). These methods should all be called on
  * the sampler thread.
  */
 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);
 
   /**
    * This function is invoked from rust on the render backend thread when it
    * is created. It effectively tells the APZSampler "the current thread is
    * the sampler thread for this window id" and allows APZSampler to remember
    * which thread it is.
@@ -97,41 +98,35 @@ public:
   /**
    * Returns true if currently on the APZSampler's "sampler thread".
    */
   bool IsSamplerThread() const;
 
 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
   // be used while holding the sWindowIdLock.
   static StaticMutex sWindowIdLock;
   static 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.
   TimeStamp mSampleTime;
 };
 
 } // namespace layers
 } // namespace mozilla
--- a/gfx/layers/apz/src/APZSampler.cpp
+++ b/gfx/layers/apz/src/APZSampler.cpp
@@ -17,21 +17,21 @@
 
 namespace mozilla {
 namespace layers {
 
 StaticMutex APZSampler::sWindowIdLock;
 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);
   mApz->SetSampler(this);
 }
 
 APZSampler::~APZSampler()
 {
@@ -51,18 +51,17 @@ APZSampler::SetWebRenderWindowId(const w
   mWindowId = Some(aWindowId);
   sWindowIdMap[wr::AsUint64(aWindowId)] = this;
 }
 
 /*static*/ void
 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());
   }
 }
 
 /*static*/ void
 APZSampler::SampleForWebRender(const wr::WrWindowId& aWindowId,
                                wr::Transaction* aTransaction)
 {
@@ -209,39 +208,27 @@ APZSampler::AssertOnSamplerThread() cons
   if (APZThreadUtils::GetThreadAssertionsEnabled()) {
     MOZ_ASSERT(IsSamplerThread());
   }
 }
 
 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)
 {
   RefPtr<APZSampler> sampler;
   StaticMutexAutoLock lock(sWindowIdLock);
   auto it = sWindowIdMap.find(wr::AsUint64(aWindowId));
   if (it != sWindowIdMap.end()) {
     sampler = it->second;
--- a/gfx/layers/apz/test/gtest/APZCBasicTester.h
+++ b/gfx/layers/apz/test/gtest/APZCBasicTester.h
@@ -27,17 +27,17 @@ protected:
   virtual void SetUp()
   {
     gfxPrefs::GetSingleton();
     APZThreadUtils::SetThreadAssertionsEnabled(false);
     APZThreadUtils::SetControllerThread(MessageLoop::current());
 
     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);
   }
 
   /**
    * Get the APZC's scroll range in CSS pixels.
    */
--- a/gfx/layers/apz/test/gtest/APZCTreeManagerTester.h
+++ b/gfx/layers/apz/test/gtest/APZCTreeManagerTester.h
@@ -23,17 +23,17 @@ protected:
   virtual void SetUp() {
     gfxPrefs::GetSingleton();
     gfxPlatform::GetPlatform();
     APZThreadUtils::SetThreadAssertionsEnabled(false);
     APZThreadUtils::SetControllerThread(MessageLoop::current());
 
     manager = new TestAPZCTreeManager(mcc);
     updater = new APZUpdater(manager, false);
-    sampler = new APZSampler(manager);
+    sampler = new APZSampler(manager, false);
   }
 
   virtual void TearDown() {
     while (mcc->RunThroughDelayedTasks());
     manager->ClearTree();
     manager->ClearContentController();
   }
 
--- a/gfx/layers/ipc/CompositorBridgeParent.cpp
+++ b/gfx/layers/ipc/CompositorBridgeParent.cpp
@@ -380,17 +380,17 @@ CompositorBridgeParent::Initialize()
   MOZ_ASSERT(CompositorThread(),
              "The compositor thread must be Initialized before instanciating a CompositorBridgeParent.");
 
   if (mOptions.UseAPZ()) {
     MOZ_ASSERT(!mApzcTreeManager);
     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());
   }
 
   mCompositorBridgeID = 0;
   // FIXME: This holds on the the fact that right now the only thing that
   // can destroy this instance is initialized on the compositor thread after
   // this task has been processed.
   MOZ_ASSERT(CompositorLoop());