diff --git a/Source/WebDriver/ChangeLog b/Source/WebDriver/ChangeLog index 2f7dc03daa48..84e8446ae8b3 100644 --- a/Source/WebDriver/ChangeLog +++ b/Source/WebDriver/ChangeLog @@ -1,3 +1,23 @@ +2017-09-09 Carlos Garcia Campos + + WebDriver: ensure we close all windows handles when closing the session + https://bugs.webkit.org/show_bug.cgi?id=176508 + + Reviewed by Brian Burg. + + The spec says that when closing the session all top level browsing contexts should be closed. We are currently + checking if we have an active top level browsing context and then we try to close it before trying with the + rest. It can happen that we are in an inconsistent state, for example if the current top level browsing context + has been closed by JavaScript or another action and the user didn't switch to another one before closing the + session. In such case, closing the session will fail with NoSuchwindow and any other window open will not be + closed. It's safer to always ask for all window handles and close them, which is what the spec says too. + + * Session.cpp: + (WebDriver::firstWindowHandleInResult): Helper class to get the first window handle in the result array. + (WebDriver::Session::closeAllToplevelBrowsingContexts): Use firstWindowHandleInResult(). + (WebDriver::Session::close): Close the current top level browsing context and get all window handles to close + them all if needed. + 2017-08-28 Carlos Garcia Campos WebDriver: implement screen capture commands diff --git a/Source/WebDriver/Session.cpp b/Source/WebDriver/Session.cpp index 18b69a8347a2..5eceb9c5c415 100644 --- a/Source/WebDriver/Session.cpp +++ b/Source/WebDriver/Session.cpp @@ -57,6 +57,18 @@ const Capabilities& Session::capabilities() const return m_host->capabilities(); } +static std::optional firstWindowHandleInResult(InspectorValue& result) +{ + RefPtr handles; + if (result.asArray(handles) && handles->length()) { + auto handleValue = handles->get(0); + String handle; + if (handleValue->asString(handle)) + return handle; + } + return std::nullopt; +} + void Session::closeAllToplevelBrowsingContexts(const String& toplevelBrowsingContext, Function&& completionHandler) { closeTopLevelBrowsingContext(toplevelBrowsingContext, [this, protectedThis = makeRef(*this), completionHandler = WTFMove(completionHandler)](CommandResult&& result) mutable { @@ -64,14 +76,9 @@ void Session::closeAllToplevelBrowsingContexts(const String& toplevelBrowsingCon completionHandler(WTFMove(result)); return; } - RefPtr handles; - if (result.result()->asArray(handles) && handles->length()) { - auto handleValue = handles->get(0); - String handle; - if (handleValue->asString(handle)) { - closeAllToplevelBrowsingContexts(handle, WTFMove(completionHandler)); - return; - } + if (auto handle = firstWindowHandleInResult(*result.result())) { + closeAllToplevelBrowsingContexts(handle.value(), WTFMove(completionHandler)); + return; } completionHandler(CommandResult::success()); }); @@ -79,13 +86,18 @@ void Session::closeAllToplevelBrowsingContexts(const String& toplevelBrowsingCon void Session::close(Function&& completionHandler) { - if (!m_toplevelBrowsingContext) { + m_toplevelBrowsingContext = std::nullopt; + getWindowHandles([this, completionHandler = WTFMove(completionHandler)](CommandResult&& result) mutable { + if (result.isError()) { + completionHandler(WTFMove(result)); + return; + } + if (auto handle = firstWindowHandleInResult(*result.result())) { + closeAllToplevelBrowsingContexts(handle.value(), WTFMove(completionHandler)); + return; + } completionHandler(CommandResult::success()); - return; - } - - auto toplevelBrowsingContext = std::exchange(m_toplevelBrowsingContext, std::nullopt); - closeAllToplevelBrowsingContexts(toplevelBrowsingContext.value(), WTFMove(completionHandler)); + }); } void Session::setTimeouts(const Timeouts& timeouts, Function&& completionHandler)