Bug 1173219 - Return nsresults from TabParent::RecvCreateWindow to make opening windows more robust. r=billm, a=kglazko
authorMike Conley <mconley@mozilla.com>
Mon, 29 Jun 2015 14:37:57 -0400
changeset 281457 66660a969cdd8ca3144b0b389f4e107b9946185a
parent 281456 d7eea16d75d9034d297d9aa7ad08b4d631ef6d08
child 281458 b40e564b656b546348e6494038506e8de15450a6
push id4932
push userjlund@mozilla.com
push dateMon, 10 Aug 2015 18:23:06 +0000
treeherdermozilla-beta@6dd5a4f5f745 [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewersbillm, kglazko
bugs1173219
milestone41.0a2
Bug 1173219 - Return nsresults from TabParent::RecvCreateWindow to make opening windows more robust. r=billm, a=kglazko We were returning false from TabParent::RecvCreateWindow whenever anything went wrong. Returning false in response to an IPC message results in the content process being killed, which is a terrible user experience. With this patch, instead of returning false, we more often return the nsresult of operations occurring within TabParent.
dom/ipc/PBrowser.ipdl
dom/ipc/TabChild.cpp
dom/ipc/TabParent.cpp
dom/ipc/TabParent.h
--- a/dom/ipc/PBrowser.ipdl
+++ b/dom/ipc/PBrowser.ipdl
@@ -153,17 +153,20 @@ parent:
                       uint32_t aChromeFlags,
                       bool aCalledFromJS,
                       bool aPositionSpecified,
                       bool aSizeSpecified,
                       nsString aURI,
                       nsString aName,
                       nsString aFeatures,
                       nsString aBaseURI)
-      returns (bool windowOpened, FrameScriptInfo[] frameScripts, nsCString urlToLoad);
+      returns (nsresult rv,
+               bool windowOpened,
+               FrameScriptInfo[] frameScripts,
+               nsCString urlToLoad);
 
     sync SyncMessage(nsString aMessage, ClonedMessageData aData,
                      CpowEntry[] aCpows, Principal aPrincipal)
       returns (OwningSerializedStructuredCloneBuffer[] retval);
 
     prio(high) sync RpcMessage(nsString aMessage, ClonedMessageData aData,
                                CpowEntry[] aCpows, Principal aPrincipal)
       returns (OwningSerializedStructuredCloneBuffer[] retval);
--- a/dom/ipc/TabChild.cpp
+++ b/dom/ipc/TabChild.cpp
@@ -1528,26 +1528,33 @@ TabChild::ProvideWindowCommon(nsIDOMWind
 
     nsAutoCString baseURIString;
     baseURI->GetSpec(baseURIString);
 
     // We can assume that if content is requesting to open a window from a remote
     // tab, then we want to enforce that the new window is also a remote tab.
     features.AppendLiteral(",remote");
 
+    nsresult rv;
+
     if (!SendCreateWindow(newChild,
                           aChromeFlags, aCalledFromJS, aPositionSpecified,
                           aSizeSpecified, url,
                           name, NS_ConvertUTF8toUTF16(features),
                           NS_ConvertUTF8toUTF16(baseURIString),
+                          &rv,
                           aWindowIsNew,
                           &frameScripts,
                           &urlToLoad)) {
       return NS_ERROR_NOT_AVAILABLE;
     }
+
+    if (NS_FAILED(rv)) {
+      return rv;
+    }
   }
   if (!*aWindowIsNew) {
     PBrowserChild::Send__delete__(newChild);
     return NS_ERROR_ABORT;
   }
 
   TextureFactoryIdentifier textureFactoryIdentifier;
   uint64_t layersId = 0;
--- a/dom/ipc/TabParent.cpp
+++ b/dom/ipc/TabParent.cpp
@@ -605,31 +605,32 @@ TabParent::RecvCreateWindow(PBrowserPare
                             const uint32_t& aChromeFlags,
                             const bool& aCalledFromJS,
                             const bool& aPositionSpecified,
                             const bool& aSizeSpecified,
                             const nsString& aURI,
                             const nsString& aName,
                             const nsString& aFeatures,
                             const nsString& aBaseURI,
+                            nsresult* aResult,
                             bool* aWindowIsNew,
                             InfallibleTArray<FrameScriptInfo>* aFrameScripts,
                             nsCString* aURLToLoad)
 {
   // We always expect to open a new window here. If we don't, it's an error.
   *aWindowIsNew = true;
 
-  if (IsBrowserOrApp()) {
+  if (NS_WARN_IF(IsBrowserOrApp()))
     return false;
-  }
-
-  nsresult rv;
+
   nsCOMPtr<nsPIWindowWatcher> pwwatch =
-    do_GetService(NS_WINDOWWATCHER_CONTRACTID, &rv);
-  NS_ENSURE_SUCCESS(rv, false);
+    do_GetService(NS_WINDOWWATCHER_CONTRACTID, aResult);
+
+  if (NS_WARN_IF(NS_FAILED(*aResult)))
+    return true;
 
   TabParent* newTab = TabParent::GetFrom(aNewTab);
   MOZ_ASSERT(newTab);
 
   // Content has requested that we open this new content window, so
   // we must have an opener.
   newTab->SetHasContentOpener(true);
 
@@ -650,18 +651,19 @@ TabParent::RecvCreateWindow(PBrowserPare
   }
 
   nsCOMPtr<nsIBrowserDOMWindow> browserDOMWin = mBrowserDOMWindow;
 
   // If we haven't found a chrome window to open in, just use the most recently
   // opened one.
   if (!parent) {
     parent = FindMostRecentOpenWindow();
-    if (!parent) {
-      return false;
+    if (NS_WARN_IF(!parent)) {
+      *aResult = NS_ERROR_FAILURE;
+      return true;
     }
 
     nsCOMPtr<nsIDOMChromeWindow> rootChromeWin = do_QueryInterface(parent);
     if (rootChromeWin) {
       rootChromeWin->GetBrowserDOMWindow(getter_AddRefs(browserDOMWin));
     }
   }
 
@@ -669,17 +671,20 @@ TabParent::RecvCreateWindow(PBrowserPare
     nsWindowWatcher::GetWindowOpenLocation(parent, aChromeFlags, aCalledFromJS,
                                            aPositionSpecified, aSizeSpecified);
 
   MOZ_ASSERT(openLocation == nsIBrowserDOMWindow::OPEN_NEWTAB ||
              openLocation == nsIBrowserDOMWindow::OPEN_NEWWINDOW);
 
   // Opening new tabs is the easy case...
   if (openLocation == nsIBrowserDOMWindow::OPEN_NEWTAB) {
-    NS_ENSURE_TRUE(browserDOMWin, false);
+    if (NS_WARN_IF(!browserDOMWin)) {
+      *aResult = NS_ERROR_FAILURE;
+      return true;
+    }
 
     bool isPrivate;
     nsCOMPtr<nsILoadContext> loadContext = GetLoadContext();
     loadContext->GetUsePrivateBrowsing(&isPrivate);
 
     nsCOMPtr<nsIOpenURIInFrameParams> params = new nsOpenURIInFrameParams();
     params->SetReferrer(aBaseURI);
     params->SetIsPrivate(isPrivate);
@@ -701,56 +706,75 @@ TabParent::RecvCreateWindow(PBrowserPare
 
   // WindowWatcher is going to expect a valid URI to open a window
   // to. If it can't find one, it's going to attempt to figure one
   // out on its own, which is problematic because it can't access
   // the document for the remote browser we're opening. Luckily,
   // TabChild has sent us a baseURI with which we can ensure that
   // the URI we pass to WindowWatcher is valid.
   nsCOMPtr<nsIURI> baseURI;
-  rv = NS_NewURI(getter_AddRefs(baseURI), aBaseURI);
-  NS_ENSURE_SUCCESS(rv, false);
+  *aResult = NS_NewURI(getter_AddRefs(baseURI), aBaseURI);
+
+  if (NS_WARN_IF(NS_FAILED(*aResult)))
+    return true;
 
   nsAutoCString finalURIString;
   if (!aURI.IsEmpty()) {
     nsCOMPtr<nsIURI> finalURI;
-    rv = NS_NewURI(getter_AddRefs(finalURI), NS_ConvertUTF16toUTF8(aURI).get(), baseURI);
-    NS_ENSURE_SUCCESS(rv, false);
+    *aResult = NS_NewURI(getter_AddRefs(finalURI), NS_ConvertUTF16toUTF8(aURI).get(), baseURI);
+
+    if (NS_WARN_IF(NS_FAILED(*aResult)))
+      return true;
+
     finalURI->GetSpec(finalURIString);
   }
 
   nsCOMPtr<nsIDOMWindow> window;
 
   AutoUseNewTab aunt(newTab, aWindowIsNew, aURLToLoad);
 
-  rv = pwwatch->OpenWindow2(parent, finalURIString.get(),
-                            NS_ConvertUTF16toUTF8(aName).get(),
-                            NS_ConvertUTF16toUTF8(aFeatures).get(), aCalledFromJS,
-                            false, false, this, nullptr, getter_AddRefs(window));
-  NS_ENSURE_SUCCESS(rv, false);
+  *aResult = pwwatch->OpenWindow2(parent, finalURIString.get(),
+                                  NS_ConvertUTF16toUTF8(aName).get(),
+                                  NS_ConvertUTF16toUTF8(aFeatures).get(), aCalledFromJS,
+                                  false, false, this, nullptr, getter_AddRefs(window));
+
+  if (NS_WARN_IF(NS_FAILED(*aResult)))
+    return true;
+
+  *aResult = NS_ERROR_FAILURE;
 
   nsCOMPtr<nsPIDOMWindow> pwindow = do_QueryInterface(window);
-  NS_ENSURE_TRUE(pwindow, false);
+  if (NS_WARN_IF(!pwindow)) {
+    return true;
+  }
 
   nsCOMPtr<nsIDocShell> windowDocShell = pwindow->GetDocShell();
-  NS_ENSURE_TRUE(windowDocShell, false);
+  if (NS_WARN_IF(!windowDocShell)) {
+    return true;
+  }
 
   nsCOMPtr<nsIDocShellTreeOwner> treeOwner;
   windowDocShell->GetTreeOwner(getter_AddRefs(treeOwner));
 
   nsCOMPtr<nsIXULWindow> xulWin = do_GetInterface(treeOwner);
-  NS_ENSURE_TRUE(xulWin, false);
+  if (NS_WARN_IF(!xulWin)) {
+    return true;
+  }
 
   nsCOMPtr<nsIXULBrowserWindow> xulBrowserWin;
   xulWin->GetXULBrowserWindow(getter_AddRefs(xulBrowserWin));
-  NS_ENSURE_TRUE(xulBrowserWin, false);
+  if (NS_WARN_IF(!xulBrowserWin)) {
+    return true;
+  }
 
   nsCOMPtr<nsITabParent> newRemoteTab;
-  rv = xulBrowserWin->ForceInitialBrowserRemote(getter_AddRefs(newRemoteTab));
-  NS_ENSURE_SUCCESS(rv, false);
+  *aResult = xulBrowserWin->ForceInitialBrowserRemote(getter_AddRefs(newRemoteTab));
+
+  if (NS_WARN_IF(NS_FAILED(*aResult)))
+    return true;
 
   MOZ_ASSERT(TabParent::GetFrom(newRemoteTab) == newTab);
 
   aFrameScripts->SwapElements(newTab->mDelayedFrameScripts);
   return true;
 }
 
 TabParent* TabParent::sNextTabParent;
--- a/dom/ipc/TabParent.h
+++ b/dom/ipc/TabParent.h
@@ -138,16 +138,17 @@ public:
                                   const uint32_t& aChromeFlags,
                                   const bool& aCalledFromJS,
                                   const bool& aPositionSpecified,
                                   const bool& aSizeSpecified,
                                   const nsString& aURI,
                                   const nsString& aName,
                                   const nsString& aFeatures,
                                   const nsString& aBaseURI,
+                                  nsresult* aResult,
                                   bool* aWindowIsNew,
                                   InfallibleTArray<FrameScriptInfo>* aFrameScripts,
                                   nsCString* aURLToLoad) override;
     virtual bool RecvSyncMessage(const nsString& aMessage,
                                  const ClonedMessageData& aData,
                                  InfallibleTArray<CpowEntry>&& aCpows,
                                  const IPC::Principal& aPrincipal,
                                  nsTArray<OwningSerializedStructuredCloneBuffer>* aRetVal) override;