Bug 1542415: Part 2 - Treat in-flight messages from incorrect owner as warnings. r=nika

Differential Revision: https://phabricator.services.mozilla.com/D44736

--HG--
extra : moz-landing-system : lando

docshell/base/CanonicalBrowsingContext.cpp

@@ -91,6 +91,13 @@ void CanonicalBrowsingContext::SetOwnerProcessId(uint64_t aProcessId) {
   mProcessId = aProcessId;
 }
 
+void CanonicalBrowsingContext::SetInFlightProcessId(uint64_t aProcessId) {
+  // We can't handle more than one in-flight process change at a time.
+  MOZ_ASSERT_IF(aProcessId, mInFlightProcessId == 0);
+
+  mInFlightProcessId = aProcessId;
+}
+
 void CanonicalBrowsingContext::GetWindowGlobals(
     nsTArray<RefPtr<WindowGlobalParent>>& aWindows) {
   aWindows.SetCapacity(mWindowGlobals.Count());

docshell/base/CanonicalBrowsingContext.h

@@ -41,6 +41,9 @@ class CanonicalBrowsingContext final : public BrowsingContext {
 
   void SetOwnerProcessId(uint64_t aProcessId);
 
+  void SetInFlightProcessId(uint64_t aProcessId);
+  uint64_t GetInFlightProcessId() const { return mInFlightProcessId; }
+
   void GetWindowGlobals(nsTArray<RefPtr<WindowGlobalParent>>& aWindows);
 
   // Called by WindowGlobalParent to register and unregister window globals.
@@ -93,6 +96,10 @@ class CanonicalBrowsingContext final : public BrowsingContext {
   // Indicates which process owns the docshell.
   uint64_t mProcessId;
 
+  // The ID of the former owner process during an ownership change, which may
+  // have in-flight messages that assume it is still the owner.
+  uint64_t mInFlightProcessId = 0;
+
   // All live window globals within this browsing context.
   nsTHashtable<nsRefPtrHashKey<WindowGlobalParent>> mWindowGlobals;
   RefPtr<WindowGlobalParent> mCurrentWindowGlobal;

dom/base/nsFrameLoader.cpp

@@ -3425,7 +3425,29 @@ void nsFrameLoader::SkipBrowsingContextDetach() {
   if (IsRemoteFrame()) {
     // OOP Browser - Go directly over Browser Parent
     if (auto* browserParent = GetBrowserParent()) {
-      Unused << browserParent->SendSkipBrowsingContextDetach();
+      RefPtr<BrowsingContext> bc(GetBrowsingContext());
+      // We're going to be synchronously changing the owner of the
+      // BrowsingContext in the parent process while the current owner may still
+      // have in-flight requests which only the owner is allowed to make. Those
+      // requests will typically trigger assertions if they come from a child
+      // other than the owner.
+      //
+      // To work around this, we record the previous owner at the start of the
+      // process switch, and clear it when we've received a reply from the
+      // child, treating ownership mismatches as warnings in the interim.
+      //
+      // In the future, this sort of issue will probably need to be handled
+      // using ownership epochs, which should be more both flexible and
+      // resilient. For the moment, though, the surrounding process switch code
+      // is enough in flux that we're better off with a workable interim
+      // solution.
+      bc->Canonical()->SetInFlightProcessId(
+          browserParent->Manager()->ChildID());
+      browserParent->SendSkipBrowsingContextDetach(
+          [bc](bool aSuccess) { bc->Canonical()->SetInFlightProcessId(0); },
+          [bc](mozilla::ipc::ResponseRejectReason aReason) {
+            bc->Canonical()->SetInFlightProcessId(0);
+          });
     }
     // OOP IFrame - Through Browser Bridge Parent, set on browser child
     else if (auto* browserBridgeChild = GetBrowserBridgeChild()) {

dom/ipc/BrowserChild.cpp

@@ -1057,14 +1057,13 @@ BrowserChild::~BrowserChild() {
   mozilla::DropJSObjects(this);
 }
 
-mozilla::ipc::IPCResult BrowserChild::RecvSkipBrowsingContextDetach() {
-  nsCOMPtr<nsIDocShell> docShell = do_GetInterface(WebNavigation());
-  if (!docShell) {
-    return IPC_OK();
+mozilla::ipc::IPCResult BrowserChild::RecvSkipBrowsingContextDetach(
+    SkipBrowsingContextDetachResolver&& aResolve) {
+  if (nsCOMPtr<nsIDocShell> docShell = do_GetInterface(WebNavigation())) {
+    RefPtr<nsDocShell> docshell = nsDocShell::Cast(docShell);
+    docshell->SkipBrowsingContextDetach();
   }
-  RefPtr<nsDocShell> docshell = nsDocShell::Cast(docShell);
-  MOZ_ASSERT(docshell);
-  docshell->SkipBrowsingContextDetach();
+  aResolve(true);
   return IPC_OK();
 }
 

dom/ipc/BrowserChild.h

@@ -519,7 +519,8 @@ class BrowserChild final : public nsMessageManagerScriptExecutor,
   mozilla::ipc::IPCResult RecvUpdateNativeWindowHandle(
       const uintptr_t& aNewHandle);
 
-  mozilla::ipc::IPCResult RecvSkipBrowsingContextDetach();
+  mozilla::ipc::IPCResult RecvSkipBrowsingContextDetach(
+      SkipBrowsingContextDetachResolver&& aResolve);
   /**
    * Native widget remoting protocol for use with windowed plugins with e10s.
    */

dom/ipc/ContentParent.cpp

@@ -5838,7 +5838,7 @@ mozilla::ipc::IPCResult ContentParent::RecvAttachBrowsingContext(
 bool ContentParent::CheckBrowsingContextOwnership(
     BrowsingContext* aBC, const char* aOperation) const {
   if (!aBC->Canonical()->IsOwnedByProcess(ChildID())) {
-    MOZ_DIAGNOSTIC_ASSERT(false,
+    MOZ_DIAGNOSTIC_ASSERT(ChildID() == aBC->Canonical()->GetInFlightProcessId(),
                           "Attempt to modify a BrowsingContext from a child "
                           "which doesn't own it");
 

dom/ipc/PBrowser.ipdl

@@ -1026,7 +1026,7 @@ child:
      */
     async GetContentBlockingLog() returns(IPCStream ipcStream, bool success);
 
-    async SkipBrowsingContextDetach();
+    async SkipBrowsingContextDetach() returns (bool success);
 parent:
     /** Records a history visit. */
     async VisitURI(URIParams aURI, URIParams? aLastVisitedURI,