Log Message
WebDriver: several issues when switching to new browser context https://bugs.webkit.org/show_bug.cgi?id=217217
Reviewed by Brian Burg.
Source/WebDriver:
There are several issues to fix when switching to a browser context:
1- The spec has changed and now we should always keep the current parent browsing context.
2- The spec says we should focus the new frame after switching to a frame or parent frame, but we are just
resolving the frame and updating the handle internally.
3- We are keeping stale frame handles and ids in the automation session, they should be removed when frames
are destroyed.
4- We are clearing all frame references in the automation session when a navigation happens in any main
frame. We should only clear the frames of the page that completed the navigation.
All theses cases are covered by new tests added to imported/w3c/webdriver/tests/switch_to_parent_frame/
* Session.cpp:
(WebDriver::Session::close): Close the current parent browsing context too.
(WebDriver::Session::switchToTopLevelBrowsingContext): Initialize the current parent browsing context too.
(WebDriver::Session::switchToBrowsingContext): Resolve the parent frame handle and set the current parent browsing context.
(WebDriver::Session::go): Pass completion handler to switchToBrowsingContext().
(WebDriver::Session::back): Ditto.
(WebDriver::Session::forward): Ditto.
(WebDriver::Session::refresh): Ditto.
(WebDriver::Session::closeWindow): Close the current parent browsing context too.
(WebDriver::Session::switchToBrowsingContext): Send switchToBrowsingContext message to the browser.
(WebDriver::Session::switchToWindow): Use switchToBrowsingContext() to send the message to the browser.
(WebDriver::Session::switchToFrame): Call switchToBrowsingContext() after the child frame handle is resolved.
(WebDriver::Session::switchToParentFrame): Check current parent browsing context is still open and call
switchToBrowsingContext() to switch to the current parent browsing context.
(WebDriver::Session::waitForNavigationToComplete): Close the current parent browsing context too when the window
is closed due to the navigation.
* Session.h: Add m_currentParentBrowsingContext.
Source/WebKit:
* UIProcess/Automation/WebAutomationSession.cpp:
(WebKit::WebAutomationSession::didDestroyFrame): Remove the frame references from maps.
(WebKit::WebAutomationSession::navigationOccurredForFrame): Only clear the frame references from the maps for
the frames in the given frame's page.
* UIProcess/Automation/WebAutomationSession.h:
* UIProcess/WebProcessProxy.cpp:
(WebKit::WebProcessProxy::didDestroyFrame): Notify automation session about frame being destroyed.
Modified Paths
- trunk/Source/WebDriver/ChangeLog
- trunk/Source/WebDriver/Session.cpp
- trunk/Source/WebDriver/Session.h
- trunk/Source/WebKit/ChangeLog
- trunk/Source/WebKit/UIProcess/Automation/WebAutomationSession.cpp
- trunk/Source/WebKit/UIProcess/Automation/WebAutomationSession.h
- trunk/Source/WebKit/UIProcess/WebProcessProxy.cpp
Diff
Modified: trunk/Source/WebDriver/ChangeLog (267917 => 267918)
--- trunk/Source/WebDriver/ChangeLog 2020-10-03 06:00:27 UTC (rev 267917)
+++ trunk/Source/WebDriver/ChangeLog 2020-10-03 06:26:08 UTC (rev 267918)
@@ -1,5 +1,42 @@
2020-10-02 Carlos Garcia Campos <[email protected]>
+ WebDriver: several issues when switching to new browser context
+ https://bugs.webkit.org/show_bug.cgi?id=217217
+
+ Reviewed by Brian Burg.
+
+ There are several issues to fix when switching to a browser context:
+
+ 1- The spec has changed and now we should always keep the current parent browsing context.
+ 2- The spec says we should focus the new frame after switching to a frame or parent frame, but we are just
+ resolving the frame and updating the handle internally.
+ 3- We are keeping stale frame handles and ids in the automation session, they should be removed when frames
+ are destroyed.
+ 4- We are clearing all frame references in the automation session when a navigation happens in any main
+ frame. We should only clear the frames of the page that completed the navigation.
+
+ All theses cases are covered by new tests added to imported/w3c/webdriver/tests/switch_to_parent_frame/
+
+ * Session.cpp:
+ (WebDriver::Session::close): Close the current parent browsing context too.
+ (WebDriver::Session::switchToTopLevelBrowsingContext): Initialize the current parent browsing context too.
+ (WebDriver::Session::switchToBrowsingContext): Resolve the parent frame handle and set the current parent browsing context.
+ (WebDriver::Session::go): Pass completion handler to switchToBrowsingContext().
+ (WebDriver::Session::back): Ditto.
+ (WebDriver::Session::forward): Ditto.
+ (WebDriver::Session::refresh): Ditto.
+ (WebDriver::Session::closeWindow): Close the current parent browsing context too.
+ (WebDriver::Session::switchToBrowsingContext): Send switchToBrowsingContext message to the browser.
+ (WebDriver::Session::switchToWindow): Use switchToBrowsingContext() to send the message to the browser.
+ (WebDriver::Session::switchToFrame): Call switchToBrowsingContext() after the child frame handle is resolved.
+ (WebDriver::Session::switchToParentFrame): Check current parent browsing context is still open and call
+ switchToBrowsingContext() to switch to the current parent browsing context.
+ (WebDriver::Session::waitForNavigationToComplete): Close the current parent browsing context too when the window
+ is closed due to the navigation.
+ * Session.h: Add m_currentParentBrowsingContext.
+
+2020-10-02 Carlos Garcia Campos <[email protected]>
+
WebDriver: check the right browser context is open in all commands
https://bugs.webkit.org/show_bug.cgi?id=217177
Modified: trunk/Source/WebDriver/Session.cpp (267917 => 267918)
--- trunk/Source/WebDriver/Session.cpp 2020-10-03 06:00:27 UTC (rev 267917)
+++ trunk/Source/WebDriver/Session.cpp 2020-10-03 06:26:08 UTC (rev 267918)
@@ -111,6 +111,7 @@
{
m_toplevelBrowsingContext = WTF::nullopt;
m_currentBrowsingContext = WTF::nullopt;
+ m_currentParentBrowsingContext = WTF::nullopt;
getWindowHandles([this, completionHandler = WTFMove(completionHandler)](CommandResult&& result) mutable {
if (result.isError()) {
completionHandler(WTFMove(result));
@@ -151,11 +152,26 @@
{
m_toplevelBrowsingContext = toplevelBrowsingContext;
m_currentBrowsingContext = String();
+ m_currentParentBrowsingContext = String();
}
-void Session::switchToBrowsingContext(const String& browsingContext)
+void Session::switchToBrowsingContext(const String& browsingContext, Function<void(CommandResult&&)>&& completionHandler)
{
m_currentBrowsingContext = browsingContext;
+ if (browsingContext.isEmpty()) {
+ m_currentParentBrowsingContext = String();
+ completionHandler(CommandResult::success());
+ return;
+ }
+
+ auto parameters = JSON::Object::create();
+ parameters->setString("browsingContextHandle"_s, m_toplevelBrowsingContext.value());
+ parameters->setString("frameHandle"_s, m_currentBrowsingContext.value());
+ m_host->sendCommandToBackend("resolveParentFrameHandle"_s, WTFMove(parameters), [this, completionHandler = WTFMove(completionHandler)](SessionHost::CommandResponse&& response) {
+ if (!response.isError && response.responseObject)
+ m_currentParentBrowsingContext = response.responseObject->getString("result"_s);
+ completionHandler(CommandResult::success());
+ });
}
Optional<String> Session::pageLoadStrategyString() const
@@ -305,13 +321,12 @@
parameters->setDouble("pageLoadTimeout"_s, m_pageLoadTimeout);
if (auto pageLoadStrategy = pageLoadStrategyString())
parameters->setString("pageLoadStrategy"_s, pageLoadStrategy.value());
- m_host->sendCommandToBackend("navigateBrowsingContext"_s, WTFMove(parameters), [this, protectedThis, completionHandler = WTFMove(completionHandler)](SessionHost::CommandResponse&& response) {
+ m_host->sendCommandToBackend("navigateBrowsingContext"_s, WTFMove(parameters), [this, protectedThis, completionHandler = WTFMove(completionHandler)](SessionHost::CommandResponse&& response) mutable {
if (response.isError) {
completionHandler(CommandResult::fail(WTFMove(response.responseObject)));
return;
}
- switchToBrowsingContext({ });
- completionHandler(CommandResult::success());
+ switchToBrowsingContext({ }, WTFMove(completionHandler));
});
});
}
@@ -371,13 +386,12 @@
parameters->setDouble("pageLoadTimeout"_s, m_pageLoadTimeout);
if (auto pageLoadStrategy = pageLoadStrategyString())
parameters->setString("pageLoadStrategy"_s, pageLoadStrategy.value());
- m_host->sendCommandToBackend("goBackInBrowsingContext"_s, WTFMove(parameters), [this, protectedThis, completionHandler = WTFMove(completionHandler)](SessionHost::CommandResponse&& response) {
+ m_host->sendCommandToBackend("goBackInBrowsingContext"_s, WTFMove(parameters), [this, protectedThis, completionHandler = WTFMove(completionHandler)](SessionHost::CommandResponse&& response) mutable {
if (response.isError) {
completionHandler(CommandResult::fail(WTFMove(response.responseObject)));
return;
}
- switchToBrowsingContext({ });
- completionHandler(CommandResult::success());
+ switchToBrowsingContext({ }, WTFMove(completionHandler));
});
});
}
@@ -399,13 +413,12 @@
parameters->setDouble("pageLoadTimeout"_s, m_pageLoadTimeout);
if (auto pageLoadStrategy = pageLoadStrategyString())
parameters->setString("pageLoadStrategy"_s, pageLoadStrategy.value());
- m_host->sendCommandToBackend("goForwardInBrowsingContext"_s, WTFMove(parameters), [this, protectedThis, completionHandler = WTFMove(completionHandler)](SessionHost::CommandResponse&& response) {
+ m_host->sendCommandToBackend("goForwardInBrowsingContext"_s, WTFMove(parameters), [this, protectedThis, completionHandler = WTFMove(completionHandler)](SessionHost::CommandResponse&& response) mutable {
if (response.isError) {
completionHandler(CommandResult::fail(WTFMove(response.responseObject)));
return;
}
- switchToBrowsingContext({ });
- completionHandler(CommandResult::success());
+ switchToBrowsingContext({ }, WTFMove(completionHandler));
});
});
}
@@ -427,13 +440,12 @@
parameters->setDouble("pageLoadTimeout"_s, m_pageLoadTimeout);
if (auto pageLoadStrategy = pageLoadStrategyString())
parameters->setString("pageLoadStrategy"_s, pageLoadStrategy.value());
- m_host->sendCommandToBackend("reloadBrowsingContext"_s, WTFMove(parameters), [this, protectedThis, completionHandler = WTFMove(completionHandler)](SessionHost::CommandResponse&& response) {
+ m_host->sendCommandToBackend("reloadBrowsingContext"_s, WTFMove(parameters), [this, protectedThis, completionHandler = WTFMove(completionHandler)](SessionHost::CommandResponse&& response) mutable {
if (response.isError) {
completionHandler(CommandResult::fail(WTFMove(response.responseObject)));
return;
}
- switchToBrowsingContext({ });
- completionHandler(CommandResult::success());
+ switchToBrowsingContext({ }, WTFMove(completionHandler));
});
});
}
@@ -548,19 +560,32 @@
}
auto toplevelBrowsingContext = std::exchange(m_toplevelBrowsingContext, WTF::nullopt);
m_currentBrowsingContext = WTF::nullopt;
+ m_currentParentBrowsingContext = WTF::nullopt;
closeTopLevelBrowsingContext(toplevelBrowsingContext.value(), WTFMove(completionHandler));
});
}
-void Session::switchToWindow(const String& windowHandle, Function<void (CommandResult&&)>&& completionHandler)
+void Session::switchToBrowsingContext(const String& toplevelBrowsingContext, const String& browsingContext, Function<void(CommandResult&&)>&& completionHandler)
{
auto parameters = JSON::Object::create();
- parameters->setString("browsingContextHandle"_s, windowHandle);
- m_host->sendCommandToBackend("switchToBrowsingContext"_s, WTFMove(parameters), [this, protectedThis = makeRef(*this), windowHandle, completionHandler = WTFMove(completionHandler)](SessionHost::CommandResponse&& response) {
+ parameters->setString("browsingContextHandle"_s, toplevelBrowsingContext);
+ parameters->setString("frameHandle"_s, browsingContext);
+ m_host->sendCommandToBackend("switchToBrowsingContext"_s, WTFMove(parameters), [this, completionHandler = WTFMove(completionHandler)](SessionHost::CommandResponse&& response) {
if (response.isError) {
completionHandler(CommandResult::fail(WTFMove(response.responseObject)));
return;
}
+ completionHandler(CommandResult::success());
+ });
+}
+
+void Session::switchToWindow(const String& windowHandle, Function<void(CommandResult&&)>&& completionHandler)
+{
+ switchToBrowsingContext(windowHandle, { }, [this, protectedThis = makeRef(*this), windowHandle, completionHandler = WTFMove(completionHandler)](CommandResult&& result) mutable {
+ if (result.isError()) {
+ completionHandler(WTFMove(result));
+ return;
+ }
switchToTopLevelBrowsingContext(windowHandle);
completionHandler(CommandResult::success());
});
@@ -651,8 +676,7 @@
completionHandler(CommandResult::fail(CommandResult::ErrorCode::NoSuchWindow));
return;
}
- switchToBrowsingContext({ });
- completionHandler(CommandResult::success());
+ switchToBrowsingContext({ }, WTFMove(completionHandler));
return;
}
@@ -680,7 +704,7 @@
parameters->setString("nodeHandle"_s, frameElementID);
}
- m_host->sendCommandToBackend("resolveChildFrameHandle"_s, WTFMove(parameters), [this, protectedThis, completionHandler = WTFMove(completionHandler)](SessionHost::CommandResponse&& response) {
+ m_host->sendCommandToBackend("resolveChildFrameHandle"_s, WTFMove(parameters), [this, protectedThis, completionHandler = WTFMove(completionHandler)](SessionHost::CommandResponse&& response) mutable {
if (response.isError || !response.responseObject) {
completionHandler(CommandResult::fail(WTFMove(response.responseObject)));
return;
@@ -692,8 +716,13 @@
return;
}
- switchToBrowsingContext(frameHandle);
- completionHandler(CommandResult::success());
+ switchToBrowsingContext(m_toplevelBrowsingContext.value(), frameHandle, [this, protectedThis, frameHandle, completionHandler = WTFMove(completionHandler)](CommandResult&& result) mutable {
+ if (result.isError()) {
+ completionHandler(WTFMove(result));
+ return;
+ }
+ switchToBrowsingContext(frameHandle, WTFMove(completionHandler));
+ });
});
});
}
@@ -700,38 +729,27 @@
void Session::switchToParentFrame(Function<void (CommandResult&&)>&& completionHandler)
{
- if (!m_toplevelBrowsingContext) {
+ if (!m_currentParentBrowsingContext) {
completionHandler(CommandResult::fail(CommandResult::ErrorCode::NoSuchWindow));
return;
}
- if (!m_currentBrowsingContext) {
- completionHandler(CommandResult::success());
- return;
- }
-
handleUserPrompts([this, protectedThis = makeRef(*this), completionHandler = WTFMove(completionHandler)](CommandResult&& result) mutable {
if (result.isError()) {
completionHandler(WTFMove(result));
return;
}
- auto parameters = JSON::Object::create();
- parameters->setString("browsingContextHandle"_s, m_toplevelBrowsingContext.value());
- parameters->setString("frameHandle"_s, m_currentBrowsingContext.value());
- m_host->sendCommandToBackend("resolveParentFrameHandle"_s, WTFMove(parameters), [this, protectedThis, completionHandler = WTFMove(completionHandler)](SessionHost::CommandResponse&& response) {
- if (response.isError || !response.responseObject) {
- completionHandler(CommandResult::fail(WTFMove(response.responseObject)));
- return;
- }
- auto frameHandle = response.responseObject->getString("result"_s);
- if (!frameHandle) {
- completionHandler(CommandResult::fail(CommandResult::ErrorCode::UnknownError));
+ switchToBrowsingContext(m_toplevelBrowsingContext.value(), m_currentParentBrowsingContext.value(), [this, protectedThis, completionHandler = WTFMove(completionHandler)](CommandResult&& result) mutable {
+ if (result.isError()) {
+ if (result.errorCode() == CommandResult::ErrorCode::NoSuchFrame)
+ completionHandler(CommandResult::fail(CommandResult::ErrorCode::NoSuchWindow));
+ else
+ completionHandler(WTFMove(result));
return;
}
- switchToBrowsingContext(frameHandle);
- completionHandler(CommandResult::success());
+ switchToBrowsingContext(m_currentParentBrowsingContext.value(), WTFMove(completionHandler));
});
});
}
@@ -1599,6 +1617,7 @@
// Window was closed, reset the top level browsing context and ignore the error.
m_toplevelBrowsingContext = WTF::nullopt;
m_currentBrowsingContext = WTF::nullopt;
+ m_currentParentBrowsingContext = WTF::nullopt;
break;
case CommandResult::ErrorCode::NoSuchFrame:
// Navigation destroyed the current frame, reset the current browsing context and ignore the error.
Modified: trunk/Source/WebDriver/Session.h (267917 => 267918)
--- trunk/Source/WebDriver/Session.h 2020-10-03 06:00:27 UTC (rev 267917)
+++ trunk/Source/WebDriver/Session.h 2020-10-03 06:26:08 UTC (rev 267918)
@@ -128,7 +128,8 @@
Session(std::unique_ptr<SessionHost>&&);
void switchToTopLevelBrowsingContext(const String&);
- void switchToBrowsingContext(const String&);
+ void switchToBrowsingContext(const String&, Function<void(CommandResult&&)>&&);
+ void switchToBrowsingContext(const String& toplevelBrowsingContext, const String& browsingContext, Function<void(CommandResult&&)>&&);
void closeTopLevelBrowsingContext(const String& toplevelBrowsingContext, Function<void (CommandResult&&)>&&);
void closeAllToplevelBrowsingContexts(const String& toplevelBrowsingContext, Function<void (CommandResult&&)>&&);
@@ -213,6 +214,7 @@
double m_implicitWaitTimeout;
Optional<String> m_toplevelBrowsingContext;
Optional<String> m_currentBrowsingContext;
+ Optional<String> m_currentParentBrowsingContext;
HashMap<String, InputSource> m_activeInputSources;
HashMap<String, InputSourceState> m_inputStateTable;
};
Modified: trunk/Source/WebKit/ChangeLog (267917 => 267918)
--- trunk/Source/WebKit/ChangeLog 2020-10-03 06:00:27 UTC (rev 267917)
+++ trunk/Source/WebKit/ChangeLog 2020-10-03 06:26:08 UTC (rev 267918)
@@ -1,3 +1,18 @@
+2020-10-02 Carlos Garcia Campos <[email protected]>
+
+ WebDriver: several issues when switching to new browser context
+ https://bugs.webkit.org/show_bug.cgi?id=217217
+
+ Reviewed by Brian Burg.
+
+ * UIProcess/Automation/WebAutomationSession.cpp:
+ (WebKit::WebAutomationSession::didDestroyFrame): Remove the frame references from maps.
+ (WebKit::WebAutomationSession::navigationOccurredForFrame): Only clear the frame references from the maps for
+ the frames in the given frame's page.
+ * UIProcess/Automation/WebAutomationSession.h:
+ * UIProcess/WebProcessProxy.cpp:
+ (WebKit::WebProcessProxy::didDestroyFrame): Notify automation session about frame being destroyed.
+
2020-10-02 Simon Fraser <[email protected]>
Move WebEvent subclass declarations to their own files
Modified: trunk/Source/WebKit/UIProcess/Automation/WebAutomationSession.cpp (267917 => 267918)
--- trunk/Source/WebKit/UIProcess/Automation/WebAutomationSession.cpp 2020-10-03 06:00:27 UTC (rev 267917)
+++ trunk/Source/WebKit/UIProcess/Automation/WebAutomationSession.cpp 2020-10-03 06:26:08 UTC (rev 267918)
@@ -203,6 +203,13 @@
return handle;
}
+void WebAutomationSession::didDestroyFrame(FrameIdentifier frameID)
+{
+ auto handle = m_webFrameHandleMap.take(frameID);
+ if (!handle.isEmpty())
+ m_handleWebFrameMap.remove(handle);
+}
+
Optional<FrameIdentifier> WebAutomationSession::webFrameIDForHandle(const String& handle, bool& frameNotFound)
{
if (handle.isEmpty())
@@ -738,9 +745,19 @@
void WebAutomationSession::navigationOccurredForFrame(const WebFrameProxy& frame)
{
if (frame.isMainFrame()) {
- // New page loaded, clear frame handles previously cached.
- m_handleWebFrameMap.clear();
- m_webFrameHandleMap.clear();
+ // New page loaded, clear frame handles previously cached for frame's page.
+ HashSet<String> handlesToRemove;
+ for (const auto& iter : m_handleWebFrameMap) {
+ auto* webFrame = frame.page()->process().webFrame(iter.value);
+ if (webFrame && webFrame->page() == frame.page()) {
+ handlesToRemove.add(iter.key);
+ m_webFrameHandleMap.remove(iter.value);
+ }
+ }
+ m_handleWebFrameMap.removeIf([&](auto& iter) {
+ return handlesToRemove.contains(iter.key);
+ });
+
if (auto callback = m_pendingNormalNavigationInBrowsingContextCallbacksPerPage.take(frame.page()->identifier())) {
m_loadTimer.stop();
callback->sendSuccess(JSON::Object::create());
Modified: trunk/Source/WebKit/UIProcess/Automation/WebAutomationSession.h (267917 => 267918)
--- trunk/Source/WebKit/UIProcess/Automation/WebAutomationSession.h 2020-10-03 06:00:27 UTC (rev 267917)
+++ trunk/Source/WebKit/UIProcess/Automation/WebAutomationSession.h 2020-10-03 06:26:08 UTC (rev 267918)
@@ -216,6 +216,8 @@
void markEventAsSynthesizedForAutomation(NSEvent *);
#endif
+ void didDestroyFrame(WebCore::FrameIdentifier);
+
private:
WebPageProxy* webPageProxyForHandle(const String&);
String handleForWebPageProxy(const WebPageProxy&);
Modified: trunk/Source/WebKit/UIProcess/WebProcessProxy.cpp (267917 => 267918)
--- trunk/Source/WebKit/UIProcess/WebProcessProxy.cpp 2020-10-03 06:00:27 UTC (rev 267917)
+++ trunk/Source/WebKit/UIProcess/WebProcessProxy.cpp 2020-10-03 06:26:08 UTC (rev 267918)
@@ -1018,6 +1018,8 @@
page->websiteDataStore().authenticatorManager().cancelRequest(page->webPageID(), frameID);
}
#endif
+ if (auto* automationSession = m_processPool->automationSession())
+ automationSession->didDestroyFrame(frameID);
m_frameMap.remove(frameID);
}
_______________________________________________ webkit-changes mailing list [email protected] https://lists.webkit.org/mailman/listinfo/webkit-changes
