Window named getter behaves incorrectly in some cases when there are …

…duplicate frame names https://bugs.webkit.org/show_bug.cgi?id=258108 Reviewed by Darin Adler and Ryosuke Niwa. When several frames gave the same name, WebKit will generate unique frame names internally. As a result, if a frame gets created with the name "foo", it will get "foo" as unique name. If a second frame gets created with the name "foo", we will generate a unique name for this frame (in the "" format) instead if using the already taken "foo" name. The issue is that this is Web-observable when using the named property getter on Window. `window.foo` is supposed to return the *first* frame with the name "foo" in tree order. WebKit was previously looking up the frame "foo" via its unique name, as a result, it would return the frame was assigned the name "foo" first, which is not necessarily the first frame with the name "foo" in tree order. The same issue affects `window.open('', 'foo')`. To address the issue, we now look up the frame by name instead of unique name. This aligns our behavior with both Chrome and Firefox. The newly added tests are passing in Chrome & Firefox but fails in shipping Safari. * LayoutTests/http/tests/frames/frames-with-same-name-cross-origin-expected.txt: Added. * LayoutTests/http/tests/frames/frames-with-same-name-cross-origin.html: Added. * LayoutTests/http/tests/frames/frames-with-same-name-expected.txt: Added. * LayoutTests/http/tests/frames/frames-with-same-name.html: Added. * LayoutTests/http/tests/frames/resources/frames-with-same-name-cross-origin-frame.html: Added. * LayoutTests/http/tests/frames/resources/set-window-name-to-foo.html: Added. * Source/WebCore/bindings/js/JSDOMWindowProperties.cpp: (WebCore::jsDOMWindowPropertiesGetOwnPropertySlotNamedItemGetter): * Source/WebCore/bindings/js/JSLocalDOMWindowCustom.cpp: (WebCore::jsLocalDOMWindowGetOwnPropertySlotRestrictedAccess): * Source/WebCore/page/FrameTree.cpp: (WebCore::FrameTree::scopedChild const): (WebCore::FrameTree::scopedChildByUniqueName const): (WebCore::FrameTree::scopedChildByName const): * Source/WebCore/page/FrameTree.h: * Source/WebKit/WebProcess/Automation/WebAutomationSessionProxy.cpp: (WebKit::WebAutomationSessionProxy::resolveChildFrameWithName): Canonical link: https://commits.webkit.org/265211@main
WebKit · Jun 15, 2023 · d3e11a7 · d3e11a7
1 parent 64cb5c7
commit d3e11a7
Show file tree

Hide file tree

Showing 18 changed files with 144 additions and 45 deletions.
diff --git a/LayoutTests/http/tests/frames/frames-with-same-name-cross-origin-expected.txt b/LayoutTests/http/tests/frames/frames-with-same-name-cross-origin-expected.txt
@@ -0,0 +1,11 @@
+Make sure that frame lookup by name has correct ordering when duplicate names are involved.
+
+On success, you will see a series of "PASS" messages, followed by "TEST COMPLETE".
+
+
+PASS crossOriginWindow.foo === crossOriginWindow[0] is true
+PASS open('', 'foo') === crossOriginWindow.foo is true
+PASS successfullyParsed is true
+
+TEST COMPLETE
+
diff --git a/LayoutTests/http/tests/frames/frames-with-same-name-cross-origin.html b/LayoutTests/http/tests/frames/frames-with-same-name-cross-origin.html
@@ -0,0 +1,18 @@
+<!DOCTYPE html>
+<html>
+<body>
+<script src="/js-test-resources/js-test.js"></script>
+<iframe id="crossOriginFrame" src="http://localhost:8000/frames/resources/frames-with-same-name-cross-origin-frame.html"></iframe>
+<script>
+description("Make sure that frame lookup by name has correct ordering when duplicate names are involved.");
+
+onload = () => {
+    crossOriginWindow = document.getElementById("crossOriginFrame").contentWindow;
+    // Both frames have the same name "foo" so window.foo should return the first one in tree order, even
+    // though the name of the first frame gets set after the name of the second frame.
+    shouldBeTrue("crossOriginWindow.foo === crossOriginWindow[0]");
+    shouldBeTrue("open('', 'foo') === crossOriginWindow.foo");
+};
+</script>
+</body>
+</html>
diff --git a/LayoutTests/http/tests/frames/frames-with-same-name-expected.txt b/LayoutTests/http/tests/frames/frames-with-same-name-expected.txt
@@ -0,0 +1,11 @@
+Make sure that frame lookup by name has correct ordering when duplicate names are involved.
+
+On success, you will see a series of "PASS" messages, followed by "TEST COMPLETE".
+
+
+PASS window.foo is document.getElementById('firstFrame').contentWindow
+PASS open('', 'foo') is window.foo
+PASS successfullyParsed is true
+
+TEST COMPLETE
+
diff --git a/LayoutTests/http/tests/frames/frames-with-same-name.html b/LayoutTests/http/tests/frames/frames-with-same-name.html
@@ -0,0 +1,18 @@
+<!DOCTYPE html>
+<html>
+<body>
+<script src="/js-test-resources/js-test.js"></script>
+<iframe id="firstFrame" src="resources/set-window-name-to-foo.html"></iframe>
+<iframe name="foo"></iframe>
+<script>
+description("Make sure that frame lookup by name has correct ordering when duplicate names are involved.");
+
+onload = () => {
+    // Both frames have the same name "foo" so window.foo should return the first one in tree order, even
+    // though the name of the first frame gets set after the name of the second frame.
+    shouldBe("window.foo", "document.getElementById('firstFrame').contentWindow");
+    shouldBe("open('', 'foo')", "window.foo");
+};
+</script>
+</body>
+</html>
diff --git a/LayoutTests/http/tests/frames/resources/frames-with-same-name-cross-origin-frame.html b/LayoutTests/http/tests/frames/resources/frames-with-same-name-cross-origin-frame.html
@@ -0,0 +1,7 @@
+<!DOCTYPE html>
+<html>
+<body>
+<iframe src="set-window-name-to-foo.html"></iframe>
+<iframe name="foo"></iframe>
+</body>
+</html>
diff --git a/LayoutTests/http/tests/frames/resources/set-window-name-to-foo.html b/LayoutTests/http/tests/frames/resources/set-window-name-to-foo.html
@@ -0,0 +1,4 @@
+<!DOCTYPE html>
+<script>
+window.name = "foo";
+</script>
diff --git a/Source/WebCore/bindings/js/JSDOMWindowProperties.cpp b/Source/WebCore/bindings/js/JSDOMWindowProperties.cpp
@@ -46,7 +46,7 @@ const ClassInfo JSDOMWindowProperties::s_info = { "WindowProperties"_s, &Base::s
 static bool jsDOMWindowPropertiesGetOwnPropertySlotNamedItemGetter(JSDOMWindowProperties* thisObject, LocalDOMWindow& window, JSGlobalObject* lexicalGlobalObject, PropertyName propertyName, PropertySlot& slot)
 {
     if (auto* frame = window.frame()) {
-        if (auto* scopedChild = dynamicDowncast<LocalFrame>(frame->tree().scopedChild(propertyNameToAtomString(propertyName)))) {
+        if (auto* scopedChild = dynamicDowncast<LocalFrame>(frame->tree().scopedChildBySpecifiedName(propertyNameToAtomString(propertyName)))) {
             slot.setValue(thisObject, static_cast<unsigned>(PropertyAttribute::DontEnum), toJS(lexicalGlobalObject, scopedChild->document()->domWindow()));
             return true;
         }

diff --git a/Source/WebCore/bindings/js/JSLocalDOMWindowCustom.cpp b/Source/WebCore/bindings/js/JSLocalDOMWindowCustom.cpp
@@ -158,7 +158,7 @@ bool jsLocalDOMWindowGetOwnPropertySlotRestrictedAccess(JSDOMGlobalObject* thisO
     // FIXME: Add support to named attributes on RemoteFrames.
     auto* frame = window.frame();
     if (frame && is<LocalFrame>(*frame)) {
-        if (auto* scopedChild = dynamicDowncast<LocalFrame>(downcast<LocalFrame>(*frame).tree().scopedChild(propertyNameToAtomString(propertyName)))) {
+        if (auto* scopedChild = dynamicDowncast<LocalFrame>(downcast<LocalFrame>(*frame).tree().scopedChildBySpecifiedName(propertyNameToAtomString(propertyName)))) {
             slot.setValue(thisObject, PropertyAttribute::ReadOnly | PropertyAttribute::DontEnum, toJS(&lexicalGlobalObject, scopedChild->document()->domWindow()));
             return true;
         }

diff --git a/Source/WebCore/loader/FrameLoader.cpp b/Source/WebCore/loader/FrameLoader.cpp
@@ -680,7 +680,7 @@ void FrameLoader::clear(RefPtr<Document>&& newDocument, bool clearWindowProperti
         m_frame.windowProxy().clearJSWindowProxiesNotMatchingDOMWindow(newDocument->domWindow(), m_frame.document()->backForwardCacheState() == Document::AboutToEnterBackForwardCache);
 
         if (shouldClearWindowName(m_frame, *newDocument))
-            m_frame.tree().setName(nullAtom());
+            m_frame.tree().setSpecifiedName(nullAtom());
     }
 
     m_frame.eventHandler().clear();
@@ -3806,7 +3806,7 @@ void FrameLoader::continueLoadAfterNewWindowPolicy(const ResourceRequest& reques
         mainFrame->loader().forceSandboxFlags(sandboxFlags);
 
     if (!isBlankTargetFrameName(frameName))
-        mainFrame->tree().setName(frameName);
+        mainFrame->tree().setSpecifiedName(frameName);
 
     mainFrame->page()->setOpenedByDOM();
     mainFrame->loader().m_client->dispatchShow();
@@ -3969,7 +3969,7 @@ LocalFrame* FrameLoader::findFrameForNavigation(const AtomString& name, Document
     if (!activeDocument)
         return nullptr;
 
-    auto* frame = dynamicDowncast<LocalFrame>(m_frame.tree().find(name, activeDocument->frame() ? *activeDocument->frame() : m_frame));
+    auto* frame = dynamicDowncast<LocalFrame>(m_frame.tree().findBySpecifiedName(name, activeDocument->frame() ? *activeDocument->frame() : m_frame));
 
     if (!activeDocument->canNavigate(frame))
         return nullptr;
@@ -4399,7 +4399,7 @@ RefPtr<LocalFrame> createWindow(LocalFrame& openerFrame, LocalFrame& lookupFrame
         frame->loader().forceSandboxFlags(openerFrame.document()->sandboxFlags());
 
     if (!isBlankTargetFrameName(request.frameName()))
-        frame->tree().setName(request.frameName());
+        frame->tree().setSpecifiedName(request.frameName());
 
     page->chrome().setToolbarsVisible(features.toolBarVisible || features.locationBarVisible);
 

diff --git a/Source/WebCore/loader/HistoryController.cpp b/Source/WebCore/loader/HistoryController.cpp
@@ -763,7 +763,7 @@ void FrameLoader::HistoryController::recursiveSetProvisionalItem(HistoryItem& it
         if (!fromChildItem)
             continue;
 
-        if (auto* childFrame = dynamicDowncast<LocalFrame>(m_frame.tree().child(childFrameName)))
+        if (auto* childFrame = dynamicDowncast<LocalFrame>(m_frame.tree().childByUniqueName(childFrameName)))
             childFrame->loader().history().recursiveSetProvisionalItem(childItem, fromChildItem);
     }
 }
@@ -785,7 +785,7 @@ void FrameLoader::HistoryController::recursiveGoToItem(HistoryItem& item, Histor
         if (!fromChildItem)
             continue;
 
-        if (auto* childFrame = dynamicDowncast<LocalFrame>(m_frame.tree().child(childFrameName)))
+        if (auto* childFrame = dynamicDowncast<LocalFrame>(m_frame.tree().childByUniqueName(childFrameName)))
             childFrame->loader().history().recursiveGoToItem(childItem, fromChildItem, type, shouldTreatAsContinuingLoad);
     }
 }
@@ -816,7 +816,7 @@ bool FrameLoader::HistoryController::currentFramesMatchItem(HistoryItem& item) c
         return false;
 
     for (auto& item : childItems) {
-        if (!m_frame.tree().child(item->target()))
+        if (!m_frame.tree().childByUniqueName(item->target()))
             return false;
     }
 

diff --git a/Source/WebCore/page/FrameTree.cpp b/Source/WebCore/page/FrameTree.cpp
@@ -50,20 +50,20 @@ FrameTree::~FrameTree()
     }
 }
 
-void FrameTree::setName(const AtomString& name) 
+void FrameTree::setSpecifiedName(const AtomString& specifiedName)
 {
-    m_name = name;
+    m_specifiedName = specifiedName;
     if (!parent()) {
-        m_uniqueName = name;
+        m_uniqueName = specifiedName;
         return;
     }
     m_uniqueName = nullAtom(); // Remove our old frame name so it's not considered in uniqueChildName.
-    m_uniqueName = parent()->tree().uniqueChildName(name);
+    m_uniqueName = parent()->tree().uniqueChildName(specifiedName);
 }
 
 void FrameTree::clearName()
 {
-    m_name = nullAtom();
+    m_specifiedName = nullAtom();
     m_uniqueName = nullAtom();
 }
 
@@ -105,7 +105,7 @@ void FrameTree::removeChild(Frame& child)
 AtomString FrameTree::uniqueChildName(const AtomString& requestedName) const
 {
     // If the requested name (the frame's "name" attribute) is unique, just use that.
-    if (!requestedName.isEmpty() && !child(requestedName) && !isBlankTargetFrameName(requestedName))
+    if (!requestedName.isEmpty() && !childByUniqueName(requestedName) && !isBlankTargetFrameName(requestedName))
         return requestedName;
 
     // The "name" attribute was not unique or absent. Generate a name based on a counter on the main frame that gets reset
@@ -153,13 +153,13 @@ Frame* FrameTree::scopedChild(unsigned index, TreeScope* scope) const
     return nullptr;
 }
 
-Frame* FrameTree::scopedChild(const AtomString& name, TreeScope* scope) const
+inline Frame* FrameTree::scopedChild(const Function<bool(const FrameTree&)>& isMatch, TreeScope* scope) const
 {
     if (!scope)
         return nullptr;
 
     for (auto* child = firstChild(); child; child = child->tree().nextSibling()) {
-        if (child->tree().uniqueName() == name && inScope(*child, *scope))
+        if (isMatch(child->tree()) && inScope(*child, *scope))
             return child;
     }
     return nullptr;
@@ -187,12 +187,24 @@ Frame* FrameTree::scopedChild(unsigned index) const
     return scopedChild(index, localFrame->document());
 }
 
-Frame* FrameTree::scopedChild(const AtomString& name) const
+Frame* FrameTree::scopedChildByUniqueName(const AtomString& uniqueName) const
 {
     auto* localFrame = dynamicDowncast<LocalFrame>(m_thisFrame);
     if (!localFrame)
         return nullptr;
-    return scopedChild(name, localFrame->document());
+    return scopedChild([&](auto& frameTree) {
+        return frameTree.uniqueName() == uniqueName;
+    }, localFrame->document());
+}
+
+Frame* FrameTree::scopedChildBySpecifiedName(const AtomString& specifiedName) const
+{
+    auto* localFrame = dynamicDowncast<LocalFrame>(m_thisFrame);
+    if (!localFrame)
+        return nullptr;
+    return scopedChild([&](auto& frameTree) {
+        return frameTree.specifiedName() == specifiedName;
+    }, localFrame->document());
 }
 
 unsigned FrameTree::scopedChildCount() const
@@ -228,7 +240,7 @@ Frame* FrameTree::child(unsigned index) const
     return result;
 }
 
-Frame* FrameTree::child(const AtomString& name) const
+Frame* FrameTree::childByUniqueName(const AtomString& name) const
 {
     for (auto* child = firstChild(); child; child = child->tree().nextSibling()) {
         if (child->tree().uniqueName() == name)
@@ -259,14 +271,14 @@ static bool isFrameFamiliarWith(Frame& abstractFrameA, Frame& abstractFrameB)
     return (frameAOpener && frameAOpener->page() == frameB->page()) || (frameBOpener && frameBOpener->page() == frameA->page()) || (frameAOpener && frameBOpener && frameAOpener->page() == frameBOpener->page());
 }
 
-Frame* FrameTree::find(const AtomString& name, Frame& activeFrame) const
+inline Frame* FrameTree::find(const AtomString& name, const Function<const AtomString&(const FrameTree&)>& nameGetter, Frame& activeFrame) const
 {
     if (isSelfTargetFrameName(name))
         return &m_thisFrame;
-    
+
     if (isTopTargetFrameName(name))
         return &top();
-    
+
     if (isParentTargetFrameName(name))
         return parent() ? parent() : &m_thisFrame;
 
@@ -276,35 +288,49 @@ Frame* FrameTree::find(const AtomString& name, Frame& activeFrame) const
 
     // Search subtree starting with this frame first.
     for (auto* frame = &m_thisFrame; frame; frame = frame->tree().traverseNext(&m_thisFrame)) {
-        if (frame->tree().uniqueName() == name)
+        if (nameGetter(frame->tree()) == name)
             return frame;
     }
 
     // Then the rest of the tree.
     auto* localFrame = dynamicDowncast<LocalFrame>(m_thisFrame);
     for (Frame* frame = localFrame ? dynamicDowncast<LocalFrame>(localFrame->mainFrame()) : nullptr; frame; frame = frame->tree().traverseNext()) {
-        if (frame->tree().uniqueName() == name)
+        if (nameGetter(frame->tree()) == name)
             return frame;
     }
 
     // Search the entire tree of each of the other pages in this namespace.
     Page* page = m_thisFrame.page();
     if (!page)
         return nullptr;
-    
+
     // FIXME: These pages are searched in random order; that doesn't seem good. Maybe use order of creation?
     for (auto& otherPage : page->group().pages()) {
         if (&otherPage == page || otherPage.isClosing())
             continue;
         for (Frame* frame = &otherPage.mainFrame(); frame; frame = frame->tree().traverseNext()) {
-            if (frame->tree().uniqueName() == name && isFrameFamiliarWith(activeFrame, *frame))
+            if (nameGetter(frame->tree()) == name && isFrameFamiliarWith(activeFrame, *frame))
                 return frame;
         }
     }
 
     return nullptr;
 }
 
+Frame* FrameTree::findByUniqueName(const AtomString& uniqueName, Frame& activeFrame) const
+{
+    return find(uniqueName, [&](auto& frameTree) -> const AtomString& {
+        return frameTree.uniqueName();
+    }, activeFrame);
+}
+
+Frame* FrameTree::findBySpecifiedName(const AtomString& specifiedName, Frame& activeFrame) const
+{
+    return find(specifiedName, [&](auto& frameTree) -> const AtomString& {
+        return frameTree.specifiedName();
+    }, activeFrame);
+}
+
 bool FrameTree::isDescendantOf(const Frame* ancestor) const
 {
     if (!ancestor)

diff --git a/Source/WebCore/page/FrameTree.h b/Source/WebCore/page/FrameTree.h
@@ -41,9 +41,9 @@ class FrameTree {
 
     ~FrameTree();
 
-    const AtomString& name() const { return m_name; }
+    const AtomString& specifiedName() const { return m_specifiedName; }
     const AtomString& uniqueName() const { return m_uniqueName; }
-    WEBCORE_EXPORT void setName(const AtomString&);
+    WEBCORE_EXPORT void setSpecifiedName(const AtomString&);
     WEBCORE_EXPORT void clearName();
     WEBCORE_EXPORT Frame* parent() const;
 
@@ -71,15 +71,17 @@ class FrameTree {
     WEBCORE_EXPORT void removeChild(Frame&);
 
     Frame* child(unsigned index) const;
-    Frame* child(const AtomString& name) const;
-    WEBCORE_EXPORT Frame* find(const AtomString& name, Frame& activeFrame) const;
+    Frame* childByUniqueName(const AtomString& name) const;
+    WEBCORE_EXPORT Frame* findByUniqueName(const AtomString&, Frame& activeFrame) const;
+    WEBCORE_EXPORT Frame* findBySpecifiedName(const AtomString&, Frame& activeFrame) const;
     WEBCORE_EXPORT unsigned childCount() const;
     unsigned descendantCount() const;
     WEBCORE_EXPORT Frame& top() const;
     unsigned depth() const;
 
     WEBCORE_EXPORT Frame* scopedChild(unsigned index) const;
-    WEBCORE_EXPORT Frame* scopedChild(const AtomString& name) const;
+    WEBCORE_EXPORT Frame* scopedChildByUniqueName(const AtomString&) const;
+    Frame* scopedChildBySpecifiedName(const AtomString& name) const;
     unsigned scopedChildCount() const;
 
     void resetFrameIdentifiers() { m_frameIDGenerator = 0; }
@@ -91,16 +93,18 @@ class FrameTree {
 
     bool scopedBy(TreeScope*) const;
     Frame* scopedChild(unsigned index, TreeScope*) const;
-    Frame* scopedChild(const AtomString& name, TreeScope*) const;
+    Frame* scopedChild(const Function<bool(const FrameTree&)>& isMatch, TreeScope*) const;
     unsigned scopedChildCount(TreeScope*) const;
 
+    Frame* find(const AtomString& name, const Function<const AtomString&(const FrameTree&)>& nameGetter, Frame& activeFrame) const;
+
     AtomString uniqueChildName(const AtomString& requestedName) const;
     AtomString generateUniqueName() const;
 
     Frame& m_thisFrame;
 
     WeakPtr<Frame> m_parent;
-    AtomString m_name; // The actual frame name (may be empty).
+    AtomString m_specifiedName; // The actual frame name (may be empty).
     AtomString m_uniqueName;
 
     RefPtr<Frame> m_nextSibling;

diff --git a/Source/WebCore/page/LocalDOMWindow.cpp b/Source/WebCore/page/LocalDOMWindow.cpp
@@ -1472,7 +1472,7 @@ AtomString LocalDOMWindow::name() const
     if (!frame)
         return nullAtom();
 
-    return frame->tree().name();
+    return frame->tree().specifiedName();
 }
 
 void LocalDOMWindow::setName(const AtomString& string)
@@ -1481,7 +1481,7 @@ void LocalDOMWindow::setName(const AtomString& string)
     if (!frame)
         return;
 
-    frame->tree().setName(string);
+    frame->tree().setSpecifiedName(string);
 }
 
 void LocalDOMWindow::setStatus(const String& string)

diff --git a/Source/WebCore/rendering/HitTestResult.cpp b/Source/WebCore/rendering/HitTestResult.cpp
@@ -192,7 +192,7 @@ LocalFrame* HitTestResult::targetFrame() const
     if (!frame)
         return nullptr;
 
-    return dynamicDowncast<LocalFrame>(frame->tree().find(m_innerURLElement->target(), *frame));
+    return dynamicDowncast<LocalFrame>(frame->tree().findBySpecifiedName(m_innerURLElement->target(), *frame));
 }
 
 bool HitTestResult::isSelected() const