Bug 1577723: Store BrowsingContext ID in dom::Location rather than DocShell weak reference. r=farre

Storing a DocShell rather than a BrowsingContext causes a number of problems
when dealing with cross-process navigations. The most immediate in this case
is that some cross-origin-allowed operations only work after a local-to-remote
navigation only until the original DocShell is destroyed, which causes
intermittent test failures.

It also means, though, that after a local-to-remote navigation, where the
DocShell has not been destroyed, attempts to read same-origin properties still
end up at the old DocShell, and as a result, lie about the current state of
the BrowsingContext.

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

MANUAL PUSH: Cannot update re-opened Phabricator revisions.

--HG--
extra : source : 6ba3bcede28a51512b8f17606b917024813e98b9
extra : histedit_source : 8dcc4aaabfc3db541a1269dfcd6414c4009d1c47%2C424b940eac29b4e916ea390223103ca41922375d

dom/base/Location.cpp

@@ -40,10 +40,12 @@
 namespace mozilla {
 namespace dom {
 
-Location::Location(nsPIDOMWindowInner* aWindow, nsIDocShell* aDocShell)
+Location::Location(nsPIDOMWindowInner* aWindow, BrowsingContext* aBrowsingContext)
     : mInnerWindow(aWindow) {
-  // aDocShell can be null if it gets called after nsDocShell::Destory().
+  // aBrowsingContext can be null if it gets called after nsDocShell::Destory().
-  mDocShell = do_GetWeakReference(aDocShell);
+  if (aBrowsingContext) {
+    mBrowsingContextId = aBrowsingContext->Id();
+  }
 }
 
 Location::~Location() {}
@@ -60,21 +62,21 @@ NS_IMPL_CYCLE_COLLECTING_ADDREF(Location)
 NS_IMPL_CYCLE_COLLECTING_RELEASE(Location)
 
 BrowsingContext* Location::GetBrowsingContext() {
-  if (nsCOMPtr<nsIDocShell> docShell = GetDocShell()) {
+  RefPtr<BrowsingContext> bc = BrowsingContext::Get(mBrowsingContextId);
-    return docShell->GetBrowsingContext();
+  return bc.get();
-  }
-  return nullptr;
 }
 
 already_AddRefed<nsIDocShell> Location::GetDocShell() {
-  nsCOMPtr<nsIDocShell> docShell = do_QueryReferent(mDocShell);
+  if (RefPtr<BrowsingContext> bc = GetBrowsingContext()) {
-  return docShell.forget();
+    return do_AddRef(bc->GetDocShell());
+  }
+  return nullptr;
 }
 
 nsresult Location::GetURI(nsIURI** aURI, bool aGetInnermostURI) {
   *aURI = nullptr;
 
-  nsCOMPtr<nsIDocShell> docShell(do_QueryReferent(mDocShell));
+  nsCOMPtr<nsIDocShell> docShell(GetDocShell());
   if (!docShell) {
     return NS_OK;
   }
@@ -549,7 +551,7 @@ void Location::SetSearch(const nsAString& aSearch,
 }
 
 void Location::Reload(bool aForceget, ErrorResult& aRv) {
-  nsCOMPtr<nsIDocShell> docShell(do_QueryReferent(mDocShell));
+  nsCOMPtr<nsIDocShell> docShell(GetDocShell());
   if (!docShell) {
     return aRv.Throw(NS_ERROR_FAILURE);
   }
@@ -603,13 +605,26 @@ void Location::Assign(const nsAString& aUrl, nsIPrincipal& aSubjectPrincipal,
 bool Location::CallerSubsumes(nsIPrincipal* aSubjectPrincipal) {
   MOZ_ASSERT(aSubjectPrincipal);
 
+  RefPtr<BrowsingContext> bc(GetBrowsingContext());
+  if (MOZ_UNLIKELY(!bc) || MOZ_UNLIKELY(bc->IsDiscarded())) {
+    // Per spec, operations on a Location object with a discarded BC are no-ops,
+    // not security errors, so we need to return true from the access check and
+    // let the caller do its own discarded docShell check.
+    return true;
+  }
+  if (MOZ_UNLIKELY(!bc->IsInProcess())) {
+    return false;
+  }
+
   // Get the principal associated with the location object.  Note that this is
   // the principal of the page which will actually be navigated, not the
   // principal of the Location object itself.  This is why we need this check
   // even though we only allow limited cross-origin access to Location objects
   // in general.
-  nsCOMPtr<nsPIDOMWindowOuter> outer = mInnerWindow->GetOuterWindow();
+  nsCOMPtr<nsPIDOMWindowOuter> outer = bc->GetDOMWindow();
+  MOZ_DIAGNOSTIC_ASSERT(outer);
   if (MOZ_UNLIKELY(!outer)) return false;
+
   nsCOMPtr<nsIScriptObjectPrincipal> sop = do_QueryInterface(outer);
   bool subsumes = false;
   nsresult rv = aSubjectPrincipal->SubsumesConsideringDomain(

dom/base/Location.h

@@ -34,7 +34,7 @@ class Location final : public nsISupports,
  public:
   typedef BrowsingContext::LocationProxy RemoteProxy;
 
-  Location(nsPIDOMWindowInner* aWindow, nsIDocShell* aDocShell);
+  Location(nsPIDOMWindowInner* aWindow, BrowsingContext* aBrowsingContext);
 
   NS_DECL_CYCLE_COLLECTING_ISUPPORTS
   NS_DECL_CYCLE_COLLECTION_SCRIPT_HOLDER_CLASS(Location)
@@ -144,7 +144,7 @@ class Location final : public nsISupports,
 
   nsString mCachedHash;
   nsCOMPtr<nsPIDOMWindowInner> mInnerWindow;
-  nsWeakPtr mDocShell;
+  uint64_t mBrowsingContextId = 0;
 };
 
 }  // namespace dom

dom/base/nsGlobalWindowInner.cpp

@@ -4001,7 +4001,7 @@ nsGlobalWindowInner::GetExistingDebuggerNotificationManager() {
 
 Location* nsGlobalWindowInner::Location() {
   if (!mLocation) {
-    mLocation = new dom::Location(this, GetDocShell());
+    mLocation = new dom::Location(this, GetBrowsingContext());
   }
 
   return mLocation;

js/xpconnect/tests/mochitest/mochitest.ini

@@ -61,7 +61,6 @@ support-files =
 skip-if = fission # Times out.
 [test_bug629331.html]
 [test_bug636097.html]
-fail-if = fission # Bug 1573621: window.location access after cross-origin navigation.
 [test_bug650273.html]
 [test_bug655297-1.html]
 [test_bug655297-2.html]
@@ -77,7 +76,6 @@ skip-if = toolkit == "android" && debug && !is_fennec
 [test_bug790732.html]
 [test_bug793969.html]
 [test_bug800864.html]
-fail-if = fission # Bug 1573621: window.location access after cross-origin navigation.
 [test_bug802557.html]
 fail-if = fission # Bug 1573621: window.location access after cross-origin navigation.
 [test_bug803730.html]