Bug 1455302 - Robustify the IsSamplerThread() check similarly to the updater code. r=botond
authorKartikaya Gupta <kgupta@mozilla.com>
Thu, 19 Apr 2018 10:10:00 -0400
changeset 468208 9bb64b4e34ce2ec456f4e6084a7b358e5ca500e0
parent 468207 8450bccb1873ca29bd5f226af0dcf5d6186612f0
child 468209 6817a2e394941fa5b46426dc2abde06232062e01
push id9165
push userasasaki@mozilla.com
push dateThu, 26 Apr 2018 21:04:54 +0000
treeherdermozilla-beta@064c3804de2e [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewersbotond
bugs1455302
milestone61.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 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
@@ -37,17 +37,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.
@@ -98,44 +99,38 @@ 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. Note that we use a StaticAutoPtr
   // wrapper on sWindowIdMap to avoid a static initializer for the unordered_map.
   // This also avoids the initializer/memory allocation in cases where we're
   // not using WebRender.
   static StaticMutex sWindowIdLock;
   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.
   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;
 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);
   mApz->SetSampler(this);
 }
 
 APZSampler::~APZSampler()
 {
@@ -55,18 +55,17 @@ APZSampler::SetWebRenderWindowId(const w
   }
   (*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)
 {
@@ -213,39 +212,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);
   if (sWindowIdMap) {
     auto it = sWindowIdMap->find(wr::AsUint64(aWindowId));
     if (it != sWindowIdMap->end()) {
--- 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());