Bug 1443329 - Ensure we always call NotifyActivity on ForcePaint BHR r?mconley draft
authorDoug Thayer <dothayer@mozilla.com>
Sun, 18 Mar 2018 11:44:19 -0700
changeset 769590 475c3eda424602f6466dad8ff4c1352466166600
parent 769528 aa07caa8a38da941b0d8aed609f20edae3cec07f
push id103175
push userbmo:dothayer@mozilla.com
push dateMon, 19 Mar 2018 20:41:12 +0000
reviewersmconley
bugs1443329
milestone61.0a1
Bug 1443329 - Ensure we always call NotifyActivity on ForcePaint BHR r?mconley MozReview-Commit-ID: GomQWREr7vG
dom/ipc/ProcessHangMonitor.cpp
dom/ipc/ProcessHangMonitor.h
dom/ipc/TabChild.cpp
--- a/dom/ipc/ProcessHangMonitor.cpp
+++ b/dom/ipc/ProcessHangMonitor.cpp
@@ -92,16 +92,22 @@ class HangMonitorChild
 
   void NotifyPluginHang(uint32_t aPluginId);
   void NotifyPluginHangAsync(uint32_t aPluginId);
 
   void ClearHang();
   void ClearHangAsync();
   void ClearForcePaint(uint64_t aLayerObserverEpoch);
 
+  // MaybeStartForcePaint will notify the background hang monitor of activity
+  // if this is the first time calling it since ClearForcePaint. It should be
+  // callable from any thread, but you must be holding mMonitor if using it off
+  // the main thread, since it could race with ClearForcePaint.
+  void MaybeStartForcePaint();
+
   mozilla::ipc::IPCResult RecvTerminateScript(const bool& aTerminateGlobal) override;
   mozilla::ipc::IPCResult RecvBeginStartingDebugger() override;
   mozilla::ipc::IPCResult RecvEndStartingDebugger() override;
 
   mozilla::ipc::IPCResult RecvForcePaint(const TabId& aTabId, const uint64_t& aLayerObserverEpoch) override;
 
   void ActorDestroy(ActorDestroyReason aWhy) override;
 
@@ -136,16 +142,20 @@ class HangMonitorChild
   bool mForcePaint;
   TabId mForcePaintTab;
   MOZ_INIT_OUTSIDE_CTOR uint64_t mForcePaintEpoch;
   JSContext* mContext;
   bool mShutdownDone;
 
   // This field is only accessed on the hang thread.
   bool mIPCOpen;
+
+  // Allows us to ensure we NotifyActivity only once, allowing
+  // either thread to do so.
+  Atomic<bool> mBHRMonitorActive;
 };
 
 Atomic<HangMonitorChild*> HangMonitorChild::sInstance;
 
 /* Parent process objects */
 
 class HangMonitorParent;
 
@@ -425,43 +435,60 @@ HangMonitorChild::RecvForcePaint(const T
   {
     MonitorAutoLock lock(mMonitor);
     // If we lose our race, and the main thread has already painted,
     // the NotifyActivity call below would result in an indefinite
     // hang, since it wouldn't have a matching NotifyWait()
     if (mForcePaintEpoch >= aLayerObserverEpoch) {
       return IPC_OK();
     }
-    mForcePaintMonitor->NotifyActivity();
+    MaybeStartForcePaint();
     mForcePaint = true;
     mForcePaintTab = aTabId;
     mForcePaintEpoch = aLayerObserverEpoch;
   }
 
   JS_RequestInterruptCallback(mContext);
 
   return IPC_OK();
 }
 
 void
+HangMonitorChild::MaybeStartForcePaint()
+{
+  if (!NS_IsMainThread()) {
+    mMonitor.AssertCurrentThreadOwns();
+  }
+
+  if (!mBHRMonitorActive.exchange(true)) {
+    mForcePaintMonitor->NotifyActivity();
+  }
+}
+
+void
 HangMonitorChild::ClearForcePaint(uint64_t aLayerObserverEpoch)
 {
   MOZ_RELEASE_ASSERT(NS_IsMainThread());
   MOZ_RELEASE_ASSERT(XRE_IsContentProcess());
 
   {
     MonitorAutoLock lock(mMonitor);
     // Set the epoch, so that if the forcepaint loses its race, it
     // knows it and can exit appropriately. However, ensure we don't
     // overwrite an even newer mForcePaintEpoch which could have
     // come in in a ForcePaint notification while we were painting.
     if (aLayerObserverEpoch > mForcePaintEpoch) {
       mForcePaintEpoch = aLayerObserverEpoch;
     }
     mForcePaintMonitor->NotifyWait();
+
+    // ClearForcePaint must be called on the main thread, and the
+    // hang monitor thread only sets this with mMonitor held, so there
+    // should be no risk of missing NotifyActivity calls here.
+    mBHRMonitorActive = false;
   }
 }
 
 void
 HangMonitorChild::Bind(Endpoint<PProcessHangMonitorChild>&& aEndpoint)
 {
   MOZ_RELEASE_ASSERT(IsOnThread());
 
@@ -1376,8 +1403,19 @@ ProcessHangMonitor::ClearForcePaint(uint
 {
   MOZ_RELEASE_ASSERT(NS_IsMainThread());
   MOZ_RELEASE_ASSERT(XRE_IsContentProcess());
 
   if (HangMonitorChild* child = HangMonitorChild::Get()) {
     child->ClearForcePaint(aLayerObserverEpoch);
   }
 }
+
+/* static */ void
+ProcessHangMonitor::MaybeStartForcePaint()
+{
+  MOZ_RELEASE_ASSERT(NS_IsMainThread());
+  MOZ_RELEASE_ASSERT(XRE_IsContentProcess());
+
+  if (HangMonitorChild* child = HangMonitorChild::Get()) {
+    child->MaybeStartForcePaint();
+  }
+}
--- a/dom/ipc/ProcessHangMonitor.h
+++ b/dom/ipc/ProcessHangMonitor.h
@@ -44,16 +44,17 @@ class ProcessHangMonitor final
   static void RemoveProcess(PProcessHangMonitorParent* aParent);
 
   static void ClearHang();
 
   static void ForcePaint(PProcessHangMonitorParent* aParent,
                          dom::TabParent* aTab,
                          uint64_t aLayerObserverEpoch);
   static void ClearForcePaint(uint64_t aLayerObserverEpoch);
+  static void MaybeStartForcePaint();
 
   enum SlowScriptAction {
     Continue,
     Terminate,
     StartDebugger,
     TerminateGlobal,
   };
   SlowScriptAction NotifySlowScript(nsITabChild* aTabChild,
--- a/dom/ipc/TabChild.cpp
+++ b/dom/ipc/TabChild.cpp
@@ -2577,16 +2577,20 @@ TabChild::RecvRenderLayers(const bool& a
     // ProcessHangMonitor that we've finished responding to what might have
     // been a request to force paint. This is so that the BackgroundHangMonitor
     // for force painting can be made to wait again.
     if (aEnabled) {
       ProcessHangMonitor::ClearForcePaint(mLayerObserverEpoch);
     }
   });
 
+  if (aEnabled) {
+    ProcessHangMonitor::MaybeStartForcePaint();
+  }
+
   if (mCompositorOptions) {
     MOZ_ASSERT(mPuppetWidget);
     RefPtr<LayerManager> lm = mPuppetWidget->GetLayerManager();
     MOZ_ASSERT(lm);
 
     // We send the current layer observer epoch to the compositor so that
     // TabParent knows whether a layer update notification corresponds to the
     // latest RecvRenderLayers request that was made.