Merge r221808 - WebDriver: ensure we close all windows handles when c…

…losing 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.

Source/WebDriver/ChangeLog

@@ -1,3 +1,23 @@
+2017-09-09  Carlos Garcia Campos  <cgarcia@igalia.com>
+
+        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  <cgarcia@igalia.com>
 
         WebDriver: implement screen capture commands

Source/WebDriver/Session.cpp

@@ -57,35 +57,47 @@ const Capabilities& Session::capabilities() const
     return m_host->capabilities();
 }
 
+static std::optional<String> firstWindowHandleInResult(InspectorValue& result)
+{
+    RefPtr<InspectorArray> 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<void (CommandResult&&)>&& completionHandler)
 {
     closeTopLevelBrowsingContext(toplevelBrowsingContext, [this, protectedThis = makeRef(*this), completionHandler = WTFMove(completionHandler)](CommandResult&& result) mutable {
         if (result.isError()) {
             completionHandler(WTFMove(result));
             return;
         }
-        RefPtr<InspectorArray> 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());
     });
 }
 
 void Session::close(Function<void (CommandResult&&)>&& 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<void (CommandResult&&)>&& completionHandler)