From 0bd8aa5d0d01208e02fd0758f6dc64faa532bc02 Mon Sep 17 00:00:00 2001
From: Carlos Garcia Campos <cgarcia@igalia.com>
Date: Sat, 9 Sep 2017 07:46:30 +0000
Subject: [PATCH] Merge r221808 - 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.
---
 Source/WebDriver/ChangeLog   | 20 ++++++++++++++++++
 Source/WebDriver/Session.cpp | 40 +++++++++++++++++++++++-------------
 2 files changed, 46 insertions(+), 14 deletions(-)

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  <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
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<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 {
@@ -64,14 +76,9 @@ void Session::closeAllToplevelBrowsingContexts(const String& toplevelBrowsingCon
             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());
     });
@@ -79,13 +86,18 @@ void Session::closeAllToplevelBrowsingContexts(const String& toplevelBrowsingCon
 
 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)