Bug 1562292: Part 1a - Consider requesting context in BrowsingContext lookup methods. r=farre
authorKris Maglione <maglione.k@gmail.com>
Tue, 30 Jul 2019 17:30:55 -0700
changeset 488060 d513683e1fdbfdb79f5112e9745f2fc0fb4e92fe
parent 488059 99d4d3837b3ba32d26ece9fe77e337b968199283
child 488061 f1fc2382346d163fe18a8fe5d323aafa759e1317
push id36435
push usercbrindusan@mozilla.com
push dateThu, 15 Aug 2019 09:46:49 +0000
treeherdermozilla-central@0db07ff50ab5 [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewersfarre
bugs1562292
milestone70.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 1562292: Part 1a - Consider requesting context in BrowsingContext lookup methods. r=farre Access to a particular named browsing context depends on the caller who is attempting the access. For a call to `parent.open(..., name)`, for instance, it's the privileges of the sub-frame making the open() call that matter, even though the name resolution happens relative to the parent. The current BrowsingContext FindWithName logic always considers only the access of the BrowsingContext it's searching relative to, regardless of the caller, while the corresponding DocShell logic correctly takes the caller into account. This patch updates the APIs to allow passing a specific accessing BrowsingContext, and falls back to the target when one isn't passed (e.g., by WebIDL callers, to which the new parameter is not exposed). Differential Revision: https://phabricator.services.mozilla.com/D40492
docshell/base/BrowsingContext.cpp
docshell/base/BrowsingContext.h
docshell/test/browser/browser_browsingContext-02.js
docshell/test/browser/browser_browsingContext-03.js
dom/base/nsGlobalWindowOuter.cpp
dom/chrome-webidl/BrowsingContext.webidl
--- a/docshell/base/BrowsingContext.cpp
+++ b/docshell/base/BrowsingContext.cpp
@@ -457,114 +457,123 @@ void BrowsingContext::GetChildren(Childr
 // 4) Siblings and their children, both in insertion order
 // 5) After this we iteratively follow the parent chain, repeating 3
 //    and 4 until
 // 6) If there is no parent, consider all other top level browsing
 //    contexts and their children, both in insertion order
 //
 // See
 // https://html.spec.whatwg.org/multipage/browsers.html#the-rules-for-choosing-a-browsing-context-given-a-browsing-context-name
-BrowsingContext* BrowsingContext::FindWithName(const nsAString& aName) {
+BrowsingContext* BrowsingContext::FindWithName(
+    const nsAString& aName, BrowsingContext& aRequestingContext) {
   BrowsingContext* found = nullptr;
   if (aName.IsEmpty()) {
     // You can't find a browsing context with an empty name.
     found = nullptr;
-  } else if (BrowsingContext* special = FindWithSpecialName(aName)) {
+  } else if (BrowsingContext* special =
+                 FindWithSpecialName(aName, aRequestingContext)) {
     found = special;
   } else if (aName.LowerCaseEqualsLiteral("_blank")) {
     // Just return null. Caller must handle creating a new window with
     // a blank name.
     found = nullptr;
-  } else if (BrowsingContext* child = FindWithNameInSubtree(aName, this)) {
+  } else if (BrowsingContext* child =
+                 FindWithNameInSubtree(aName, aRequestingContext)) {
     found = child;
   } else {
     BrowsingContext* current = this;
 
     do {
       Children* siblings;
       BrowsingContext* parent = current->mParent;
 
       if (!parent) {
         // We've reached the root of the tree, consider browsing
         // contexts in the same browsing context group.
         siblings = &mGroup->Toplevels();
-      } else if (parent->NameEquals(aName) && CanAccess(parent) &&
+      } else if (parent->NameEquals(aName) &&
+                 aRequestingContext.CanAccess(parent) &&
                  parent->IsTargetable()) {
         found = parent;
         break;
       } else {
         siblings = &parent->mChildren;
       }
 
       for (BrowsingContext* sibling : *siblings) {
         if (sibling == current) {
           continue;
         }
 
         if (BrowsingContext* relative =
-                sibling->FindWithNameInSubtree(aName, this)) {
+                sibling->FindWithNameInSubtree(aName, aRequestingContext)) {
           found = relative;
           // Breaks the outer loop
           parent = nullptr;
           break;
         }
       }
 
       current = parent;
     } while (current);
   }
 
   // Helpers should perform access control checks, which means that we
   // only need to assert that we can access found.
-  MOZ_DIAGNOSTIC_ASSERT(!found || CanAccess(found));
+  MOZ_DIAGNOSTIC_ASSERT(!found || aRequestingContext.CanAccess(found));
 
   return found;
 }
 
-BrowsingContext* BrowsingContext::FindChildWithName(const nsAString& aName) {
+BrowsingContext* BrowsingContext::FindChildWithName(
+    const nsAString& aName, BrowsingContext& aRequestingContext) {
   if (aName.IsEmpty()) {
     // You can't find a browsing context with the empty name.
     return nullptr;
   }
 
   for (BrowsingContext* child : mChildren) {
-    if (child->NameEquals(aName) && CanAccess(child) && child->IsTargetable()) {
+    if (child->NameEquals(aName) && aRequestingContext.CanAccess(child) &&
+        child->IsTargetable()) {
       return child;
     }
   }
 
   return nullptr;
 }
 
-BrowsingContext* BrowsingContext::FindWithSpecialName(const nsAString& aName) {
+BrowsingContext* BrowsingContext::FindWithSpecialName(
+    const nsAString& aName, BrowsingContext& aRequestingContext) {
   // TODO(farre): Neither BrowsingContext nor nsDocShell checks if the
   // browsing context pointed to by a special name is active. Should
   // it be? See Bug 1527913.
   if (aName.LowerCaseEqualsLiteral("_self")) {
     return this;
   }
 
   if (aName.LowerCaseEqualsLiteral("_parent")) {
-    return mParent && CanAccess(mParent.get()) ? mParent.get() : this;
+    return mParent && aRequestingContext.CanAccess(mParent.get())
+               ? mParent.get()
+               : this;
   }
 
   if (aName.LowerCaseEqualsLiteral("_top")) {
     BrowsingContext* top = Top();
 
-    return CanAccess(top) ? top : nullptr;
+    return aRequestingContext.CanAccess(top) ? top : nullptr;
   }
 
   return nullptr;
 }
 
 BrowsingContext* BrowsingContext::FindWithNameInSubtree(
-    const nsAString& aName, BrowsingContext* aRequestingContext) {
+    const nsAString& aName, BrowsingContext& aRequestingContext) {
   MOZ_DIAGNOSTIC_ASSERT(!aName.IsEmpty());
 
-  if (NameEquals(aName) && aRequestingContext->CanAccess(this) &&
+  if (NameEquals(aName) && aRequestingContext.CanAccess(this) &&
       IsTargetable()) {
     return this;
   }
 
   for (BrowsingContext* child : mChildren) {
     if (BrowsingContext* found =
             child->FindWithNameInSubtree(aName, aRequestingContext)) {
       return found;
--- a/docshell/base/BrowsingContext.h
+++ b/docshell/base/BrowsingContext.h
@@ -218,23 +218,27 @@ class BrowsingContext : public nsWrapper
   // transitively reachable browsing contexts. Performs access control
   // with regards to this.
   // See
   // https://html.spec.whatwg.org/multipage/browsers.html#the-rules-for-choosing-a-browsing-context-given-a-browsing-context-name.
   //
   // BrowsingContext::FindWithName(const nsAString&) is equivalent to
   // calling nsIDocShellTreeItem::FindItemWithName(aName, nullptr,
   // nullptr, false, <return value>).
-  BrowsingContext* FindWithName(const nsAString& aName);
+  BrowsingContext* FindWithName(const nsAString& aName,
+                                BrowsingContext& aRequestingContext);
+
 
   // Find a browsing context in this context's list of
   // children. Doesn't consider the special names, '_self', '_parent',
   // '_top', or '_blank'. Performs access control with regard to
   // 'this'.
-  BrowsingContext* FindChildWithName(const nsAString& aName);
+  BrowsingContext* FindChildWithName(const nsAString& aName,
+                                     BrowsingContext& aRequestingContext);
+
 
   nsISupports* GetParentObject() const;
   JSObject* WrapObject(JSContext* aCx,
                        JS::Handle<JSObject*> aGivenProto) override;
 
   // This function would be called when its corresponding document is activated
   // by user gesture, and we would set the flag in the top level browsing
   // context.
@@ -387,24 +391,25 @@ class BrowsingContext : public nsWrapper
  protected:
   virtual ~BrowsingContext();
   BrowsingContext(BrowsingContext* aParent, BrowsingContextGroup* aGroup,
                   uint64_t aBrowsingContextId, Type aType);
 
  private:
   // Find the special browsing context if aName is '_self', '_parent',
   // '_top', but not '_blank'. The latter is handled in FindWithName
-  BrowsingContext* FindWithSpecialName(const nsAString& aName);
+  BrowsingContext* FindWithSpecialName(const nsAString& aName,
+                                       BrowsingContext& aRequestingContext);
 
   // Find a browsing context in the subtree rooted at 'this' Doesn't
   // consider the special names, '_self', '_parent', '_top', or
   // '_blank'. Performs access control with regard to
   // 'aRequestingContext'.
   BrowsingContext* FindWithNameInSubtree(const nsAString& aName,
-                                         BrowsingContext* aRequestingContext);
+                                         BrowsingContext& aRequestingContext);
 
   // Removes the context from its group and sets mIsDetached to true.
   void Unregister();
 
   friend class ::nsOuterWindowProxy;
   friend class ::nsGlobalWindowOuter;
   // Update the window proxy object that corresponds to this browsing context.
   // This should be called from the window proxy object's objectMoved hook, if
--- a/docshell/test/browser/browser_browsingContext-02.js
+++ b/docshell/test/browser/browser_browsingContext-02.js
@@ -95,26 +95,26 @@ add_task(async function() {
 
           function bc(frame) {
             return (frame.contentWindow || frame).docShell.browsingContext;
           }
 
           function reachable(start, targets) {
             for (let target of targets) {
               is(
-                bc(start).findWithName(target.name),
+                bc(start).findWithName(target.name, bc(start)),
                 bc(target),
                 [bc(start).name, "can reach", target.name].join(" ")
               );
             }
           }
 
           function unreachable(start, target) {
             is(
-              bc(start).findWithName(target.name),
+              bc(start).findWithName(target.name, bc(start)),
               null,
               [bc(start).name, "can't reach", target.name].join(" ")
             );
           }
 
           let first = await addFrame(top, "first");
           info("first");
           let second = await addFrame(top, "second");
--- a/docshell/test/browser/browser_browsingContext-03.js
+++ b/docshell/test/browser/browser_browsingContext-03.js
@@ -133,34 +133,40 @@ add_task(async function() {
 
             for (let i = 0; i < docShells.length; ++i) {
               let docShell = docShells[i].findItemWithName(
                 "target",
                 null,
                 null,
                 false
               );
-              let browsingContext = browsingContexts[i].findWithName("target");
+              let browsingContext = browsingContexts[i].findWithName(
+                "target",
+                browsingContexts[i]
+              );
               is(
                 docShell ? docShell.browsingContext : null,
                 browsingContext,
                 "findItemWithName should find same browsing context as findWithName"
               );
             }
           }
 
           for (let target of ["_self", "_top", "_parent", "_blank"]) {
             for (let i = 0; i < docShells.length; ++i) {
               let docShell = docShells[i].findItemWithName(
                 target,
                 null,
                 null,
                 false
               );
-              let browsingContext = browsingContexts[i].findWithName(target);
+              let browsingContext = browsingContexts[i].findWithName(
+                target,
+                browsingContexts[i]
+              );
               is(
                 docShell ? docShell.browsingContext : null,
                 browsingContext,
                 "findItemWithName should find same browsing context as findWithName for " +
                   target
               );
             }
           }
--- a/dom/base/nsGlobalWindowOuter.cpp
+++ b/dom/base/nsGlobalWindowOuter.cpp
@@ -4034,17 +4034,17 @@ Nullable<WindowProxyHolder> nsGlobalWind
   BrowsingContext* bc = GetBrowsingContext();
   return bc ? bc->GetTop(IgnoreErrors()) : nullptr;
 }
 
 already_AddRefed<BrowsingContext> nsGlobalWindowOuter::GetChildWindow(
     const nsAString& aName) {
   NS_ENSURE_TRUE(mBrowsingContext, nullptr);
 
-  return do_AddRef(mBrowsingContext->FindChildWithName(aName));
+  return do_AddRef(mBrowsingContext->FindChildWithName(aName, *mBrowsingContext));
 }
 
 bool nsGlobalWindowOuter::DispatchCustomEvent(const nsAString& aEventName) {
   bool defaultActionEnabled = true;
   nsContentUtils::DispatchTrustedEvent(mDoc, ToSupports(this), aEventName,
                                        CanBubble::eYes, Cancelable::eYes,
                                        &defaultActionEnabled);
 
--- a/dom/chrome-webidl/BrowsingContext.webidl
+++ b/dom/chrome-webidl/BrowsingContext.webidl
@@ -6,18 +6,18 @@
 interface nsIDocShell;
 
 [Exposed=Window, ChromeOnly]
 interface BrowsingContext {
   static BrowsingContext? get(unsigned long long aId);
 
   static BrowsingContext? getFromWindow(WindowProxy window);
 
-  BrowsingContext? findChildWithName(DOMString name);
-  BrowsingContext? findWithName(DOMString name);
+  BrowsingContext? findChildWithName(DOMString name, BrowsingContext accessor);
+  BrowsingContext? findWithName(DOMString name, BrowsingContext accessor);
 
   readonly attribute DOMString name;
 
   readonly attribute BrowsingContext? parent;
 
   readonly attribute BrowsingContext top;
 
   sequence<BrowsingContext> getChildren();