Bug 1549177 - Fix a race condition when cancelling content JS during navigation; r=smaug
authorJim Porter <jporter@mozilla.com>
Tue, 07 May 2019 21:56:58 +0000
changeset 531958 a9dab8054a98402f14ce37e40cf9e4fdeed0b95d
parent 531957 d9cf7d539c45e2895193e507840bf1bdec6cb21b
child 531959 db318bbb7517c815d85df318f955747dbdb8d698
push id11265
push userffxbld-merge
push dateMon, 13 May 2019 10:53:39 +0000
treeherdermozilla-beta@77e0fe8dbdd3 [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewerssmaug
bugs1549177
milestone68.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 1549177 - Fix a race condition when cancelling content JS during navigation; r=smaug This patch fixes a race condition where we could request that a content page's JS be cancelled during navigation in `HangMonitorChild::InterruptCallback`, but the new page has already started loading by the time the JS is cancelled in `XPCJSContext::InterruptCallback`, thus cancelling the JS of the new page. To fix this, we now handle everything in `HangMonitorChild::InterruptCallback`, making sure to only do anything for *content* scripts (to avoid inadvertently cancelling the browser's JS). Differential Revision: https://phabricator.services.mozilla.com/D30272
dom/ipc/ProcessHangMonitor.cpp
js/xpconnect/src/XPCJSContext.cpp
js/xpconnect/src/xpcprivate.h
--- a/dom/ipc/ProcessHangMonitor.cpp
+++ b/dom/ipc/ProcessHangMonitor.cpp
@@ -118,17 +118,17 @@ class HangMonitorChild : public PProcess
   mozilla::ipc::IPCResult RecvCancelContentJSExecutionIfRunning(
       const TabId& aTabId, const nsIRemoteTab::NavigationType& aNavigationType,
       const int32_t& aNavigationIndex,
       const mozilla::Maybe<nsCString>& aNavigationURI,
       const int32_t& aEpoch) override;
 
   void ActorDestroy(ActorDestroyReason aWhy) override;
 
-  void InterruptCallback();
+  bool InterruptCallback();
   void Shutdown();
 
   static HangMonitorChild* Get() { return sInstance; }
 
   void Dispatch(already_AddRefed<nsIRunnable> aRunnable) {
     mHangMonitor->Dispatch(std::move(aRunnable));
   }
   bool IsOnThread() { return mHangMonitor->IsOnThread(); }
@@ -341,94 +341,112 @@ HangMonitorChild::HangMonitorChild(Proce
 }
 
 HangMonitorChild::~HangMonitorChild() {
   MOZ_RELEASE_ASSERT(NS_IsMainThread());
   MOZ_ASSERT(sInstance == this);
   sInstance = nullptr;
 }
 
-void HangMonitorChild::InterruptCallback() {
+bool HangMonitorChild::InterruptCallback() {
   MOZ_RELEASE_ASSERT(NS_IsMainThread());
 
   // The interrupt callback is triggered at non-deterministic points when
   // recording/replaying, so don't perform any operations that can interact
   // with the recording.
   if (recordreplay::IsRecordingOrReplaying()) {
-    return;
+    return true;
   }
 
   bool paintWhileInterruptingJS;
   bool paintWhileInterruptingJSForce;
   TabId paintWhileInterruptingJSTab;
   LayersObserverEpoch paintWhileInterruptingJSEpoch;
 
+  {
+    MonitorAutoLock lock(mMonitor);
+    paintWhileInterruptingJS = mPaintWhileInterruptingJS;
+    paintWhileInterruptingJSForce = mPaintWhileInterruptingJSForce;
+    paintWhileInterruptingJSTab = mPaintWhileInterruptingJSTab;
+    paintWhileInterruptingJSEpoch = mPaintWhileInterruptingJSEpoch;
+
+    mPaintWhileInterruptingJS = false;
+  }
+
+  if (paintWhileInterruptingJS) {
+    RefPtr<BrowserChild> browserChild =
+        BrowserChild::FindBrowserChild(paintWhileInterruptingJSTab);
+    if (browserChild) {
+      js::AutoAssertNoContentJS nojs(mContext);
+      browserChild->PaintWhileInterruptingJS(paintWhileInterruptingJSEpoch,
+                                             paintWhileInterruptingJSForce);
+    }
+  }
+
+  // Only handle the interrupt for cancelling content JS if we have an actual
+  // window associated with this context...
+  JS::RootedObject global(mContext, JS::CurrentGlobalOrNull(mContext));
+  RefPtr<nsGlobalWindowInner> win = xpc::WindowOrNull(global);
+  if (!win) {
+    return true;
+  }
+
   bool cancelContentJS;
   TabId cancelContentJSTab;
   nsIRemoteTab::NavigationType cancelContentJSNavigationType;
   int32_t cancelContentJSNavigationIndex;
   mozilla::Maybe<nsCString> cancelContentJSNavigationURI;
   int32_t cancelContentJSEpoch;
 
   {
     MonitorAutoLock lock(mMonitor);
-    paintWhileInterruptingJS = mPaintWhileInterruptingJS;
-    paintWhileInterruptingJSForce = mPaintWhileInterruptingJSForce;
-    paintWhileInterruptingJSTab = mPaintWhileInterruptingJSTab;
-    paintWhileInterruptingJSEpoch = mPaintWhileInterruptingJSEpoch;
-
     cancelContentJS = mCancelContentJS;
     cancelContentJSTab = mCancelContentJSTab;
     cancelContentJSNavigationType = mCancelContentJSNavigationType;
     cancelContentJSNavigationIndex = mCancelContentJSNavigationIndex;
     cancelContentJSNavigationURI = std::move(mCancelContentJSNavigationURI);
     cancelContentJSEpoch = mCancelContentJSEpoch;
 
-    mPaintWhileInterruptingJS = false;
     mCancelContentJS = false;
   }
 
-  if (paintWhileInterruptingJS) {
-    RefPtr<BrowserChild> browserChild =
-        BrowserChild::FindBrowserChild(paintWhileInterruptingJSTab);
-    if (browserChild) {
-      js::AutoAssertNoContentJS nojs(mContext);
-      browserChild->PaintWhileInterruptingJS(paintWhileInterruptingJSEpoch,
-                                             paintWhileInterruptingJSForce);
-    }
-  }
-
   if (cancelContentJS) {
     RefPtr<BrowserChild> browserChild =
         BrowserChild::FindBrowserChild(cancelContentJSTab);
     if (browserChild) {
       js::AutoAssertNoContentJS nojs(mContext);
       nsresult rv;
       nsCOMPtr<nsIURI> uri;
 
       if (cancelContentJSNavigationURI) {
         rv = NS_NewURI(getter_AddRefs(uri),
                        cancelContentJSNavigationURI.value());
         if (NS_FAILED(rv)) {
-          return;
+          return true;
         }
       }
 
       bool canCancel;
       rv = browserChild->CanCancelContentJS(cancelContentJSNavigationType,
                                             cancelContentJSNavigationIndex, uri,
                                             cancelContentJSEpoch, &canCancel);
       if (NS_SUCCEEDED(rv) && canCancel) {
-        // Tell xpconnect that we want to cancel the content JS in this tab
-        // during the next interrupt callback.
-        XPCJSContext::SetTabIdToCancelContentJS(cancelContentJSTab);
-        JS_RequestInterruptCallback(mContext);
+        // Don't add this page to the BF cache, since we're cancelling its JS.
+        if (Document* doc = win->GetExtantDoc()) {
+          if (Document* topLevelDoc = doc->GetTopLevelContentDocument()) {
+            topLevelDoc->DisallowBFCaching();
+          }
+        }
+
+        return false;
       }
     }
   }
+
+  return true;
 }
 
 void HangMonitorChild::AnnotateHang(BackgroundHangAnnotations& aAnnotations) {
   if (mPaintWhileInterruptingJSActive) {
     aAnnotations.AddAnnotation(NS_LITERAL_STRING("PaintWhileInterruptingJS"),
                                true);
   }
 }
@@ -1174,17 +1192,17 @@ HangMonitoredProcess::UserCanceled() {
     uint32_t id = mHangData.get_PluginHangData().pluginId();
     mActor->CleanupPluginHang(id, true);
   }
   return NS_OK;
 }
 
 static bool InterruptCallback(JSContext* cx) {
   if (HangMonitorChild* child = HangMonitorChild::Get()) {
-    child->InterruptCallback();
+    return child->InterruptCallback();
   }
 
   return true;
 }
 
 ProcessHangMonitor* ProcessHangMonitor::sInstance;
 
 ProcessHangMonitor::ProcessHangMonitor() : mCPOWTimeout(false) {
--- a/js/xpconnect/src/XPCJSContext.cpp
+++ b/js/xpconnect/src/XPCJSContext.cpp
@@ -658,34 +658,16 @@ bool XPCJSContext::InterruptCallback(JSC
     }
   }
 
   if (!win) {
     NS_WARNING("No active window");
     return true;
   }
 
-  // Check if we're waiting to cancel JS when going back/forward in a tab.
-  if (sTabIdToCancelContentJS) {
-    if (nsIBrowserChild* browserChild = win->GetBrowserChild()) {
-      uint64_t tabId;
-      browserChild->GetTabId(&tabId);
-      if (sTabIdToCancelContentJS == tabId) {
-        // Don't add this page to the BF cache, since we're cancelling its JS.
-        if (Document* doc = win->GetExtantDoc()) {
-          if (Document* topLevelDoc = doc->GetTopLevelContentDocument()) {
-            topLevelDoc->DisallowBFCaching();
-          }
-        }
-        sTabIdToCancelContentJS = 0;
-        return false;
-      }
-    }
-  }
-
   // If there's no limit, or we're within the limit, let it go.
   if (limit == 0 || duration.ToSeconds() < limit / 2.0) {
     return true;
   }
 
   self->mSlowScriptActualWait += duration;
 
   // In order to guard against time changes or laptops going to sleep, we
@@ -1231,21 +1213,16 @@ nsresult XPCJSContext::Initialize(XPCJSC
 #ifdef FUZZING
   Preferences::RegisterCallback(ReloadPrefsCallback, "fuzzing.enabled", this);
 #endif
 
   return NS_OK;
 }
 
 // static
-void XPCJSContext::SetTabIdToCancelContentJS(uint64_t aTabId) {
-  sTabIdToCancelContentJS = aTabId;
-}
-
-// static
 uint32_t XPCJSContext::sInstanceCount;
 
 // static
 StaticAutoPtr<WatchdogManager> XPCJSContext::sWatchdogInstance;
 
 // static
 WatchdogManager* XPCJSContext::GetWatchdogManager() {
   if (sWatchdogInstance) {
@@ -1253,19 +1230,16 @@ WatchdogManager* XPCJSContext::GetWatchd
   }
 
   MOZ_ASSERT(sInstanceCount == 0);
   sWatchdogInstance = new WatchdogManager();
   return sWatchdogInstance;
 }
 
 // static
-mozilla::Atomic<uint64_t> XPCJSContext::sTabIdToCancelContentJS(0);
-
-// static
 void XPCJSContext::InitTLS() { MOZ_RELEASE_ASSERT(gTlsContext.init()); }
 
 // static
 XPCJSContext* XPCJSContext::NewXPCJSContext(XPCJSContext* aPrimaryContext) {
   XPCJSContext* self = new XPCJSContext();
   nsresult rv = self->Initialize(aPrimaryContext);
   if (NS_FAILED(rv)) {
     MOZ_CRASH("new XPCJSContext failed to initialize.");
--- a/js/xpconnect/src/xpcprivate.h
+++ b/js/xpconnect/src/xpcprivate.h
@@ -416,18 +416,16 @@ class XPCJSContext final : public mozill
     IDX_INTERFACE_ID,
     IDX_INITIALIZER,
     IDX_TOTAL_COUNT  // just a count of the above
   };
 
   inline JS::HandleId GetStringID(unsigned index) const;
   inline const char* GetStringName(unsigned index) const;
 
-  static void SetTabIdToCancelContentJS(uint64_t aTabId);
-
  private:
   XPCJSContext();
 
   MOZ_IS_CLASS_INIT
   nsresult Initialize(XPCJSContext* aPrimaryContext);
 
   XPCCallContext* mCallContext;
   AutoMarkingPtr* mAutoRoots;
@@ -435,18 +433,16 @@ class XPCJSContext final : public mozill
   XPCWrappedNative* mResolvingWrapper;
   WatchdogManager* mWatchdogManager;
 
   // Number of XPCJSContexts currently alive.
   static uint32_t sInstanceCount;
   static mozilla::StaticAutoPtr<WatchdogManager> sWatchdogInstance;
   static WatchdogManager* GetWatchdogManager();
 
-  static mozilla::Atomic<uint64_t> sTabIdToCancelContentJS;
-
   // If we spend too much time running JS code in an event handler, then we
   // want to show the slow script UI. The timeout T is controlled by prefs. We
   // invoke the interrupt callback once after T/2 seconds and set
   // mSlowScriptSecondHalf to true. After another T/2 seconds, we invoke the
   // interrupt callback again. Since mSlowScriptSecondHalf is now true, it
   // shows the slow script UI. The reason we invoke the callback twice is to
   // ensure that putting the computer to sleep while running a script doesn't
   // cause the UI to be shown. If the laptop goes to sleep during one of the