Merge r220794 - WebDriver: fix return value of close window command

https://bugs.webkit.org/show_bug.cgi?id=174861 Reviewed by Brian Burg. We are currently returning null, but we should return the list of window handles, and try to close the session if there aren't more window handles. 10.2 Close Window https://w3c.github.io/webdriver/webdriver-spec.html#close-window 3. If there are no more open top-level browsing contexts, then try to close the session. 4. Return the result of running the remote end steps for the Get Window Handles command. * Session.cpp: (WebDriver::Session::closeAllToplevelBrowsingContexts): Helper function to close the given toplevel browsing context and the next one if there are more. (WebDriver::Session::close): Call closeAllToplevelBrowsingContexts() to delete all toplevel browsing contexts of the session. (WebDriver::Session::closeTopLevelBrowsingContext): Close the given toplevel browsing context and call getWindowHandles() when done. (WebDriver::Session::closeWindow): Call closeTopLevelBrowsingContext() passing the current toplevel browsing context. (WebDriver::Session::getWindowHandles): Remove the early return, this command doesn't depend on a current toplevel browsing context. * Session.h: * SessionHost.h: * WebDriverService.cpp: (WebDriver::WebDriverService::run): Disconnect the server when main loop quits. (WebDriver::WebDriverService::deleteSession): Do not fail if the given session is not active. (WebDriver::WebDriverService::closeWindow): Remove the session if the closed window was the last one. * WebDriverService.h: Remove unused quit() method. * glib/SessionHostGlib.cpp: (WebDriver::SessionHost::isConnected): Return whether host is connected to a browser instance. (WebDriver::SessionHost::dbusConnectionClosedCallback): Delete m_browser.
WebKit · Aug 17, 2017 · f09890e · f09890e
1 parent fb5873b
commit f09890e
Show file tree

Hide file tree

Showing 8 changed files with 130 additions and 43 deletions.
diff --git a/Source/WebDriver/ChangeLog b/Source/WebDriver/ChangeLog
@@ -1,3 +1,40 @@
+2017-07-28  Carlos Garcia Campos  <cgarcia@igalia.com>
+
+        WebDriver: fix return value of close window command
+        https://bugs.webkit.org/show_bug.cgi?id=174861
+
+        Reviewed by Brian Burg.
+
+        We are currently returning null, but we should return the list of window handles, and try to close the session
+        if there aren't more window handles.
+
+        10.2 Close Window
+        https://w3c.github.io/webdriver/webdriver-spec.html#close-window
+
+        3. If there are no more open top-level browsing contexts, then try to close the session.
+        4. Return the result of running the remote end steps for the Get Window Handles command.
+
+        * Session.cpp:
+        (WebDriver::Session::closeAllToplevelBrowsingContexts): Helper function to close the given toplevel browsing
+        context and the next one if there are more.
+        (WebDriver::Session::close): Call closeAllToplevelBrowsingContexts() to delete all toplevel browsing contexts of
+        the session.
+        (WebDriver::Session::closeTopLevelBrowsingContext): Close the given toplevel browsing context and call
+        getWindowHandles() when done.
+        (WebDriver::Session::closeWindow): Call closeTopLevelBrowsingContext() passing the current toplevel browsing context.
+        (WebDriver::Session::getWindowHandles): Remove the early return, this command doesn't depend on a current
+        toplevel browsing context.
+        * Session.h:
+        * SessionHost.h:
+        * WebDriverService.cpp:
+        (WebDriver::WebDriverService::run): Disconnect the server when main loop quits.
+        (WebDriver::WebDriverService::deleteSession): Do not fail if the given session is not active.
+        (WebDriver::WebDriverService::closeWindow): Remove the session if the closed window was the last one.
+        * WebDriverService.h: Remove unused quit() method.
+        * glib/SessionHostGlib.cpp:
+        (WebDriver::SessionHost::isConnected): Return whether host is connected to a browser instance.
+        (WebDriver::SessionHost::dbusConnectionClosedCallback): Delete m_browser.
+
 2017-08-14  Carlos Garcia Campos  <cgarcia@igalia.com>
 
         WebDriver: handle click events on option elements

diff --git a/Source/WebDriver/Session.cpp b/Source/WebDriver/Session.cpp
@@ -57,23 +57,35 @@ const Capabilities& Session::capabilities() const
     return m_host->capabilities();
 }
 
+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;
+            }
+        }
+        completionHandler(CommandResult::success());
+    });
+}
+
 void Session::close(Function<void (CommandResult&&)>&& completionHandler)
 {
     if (!m_toplevelBrowsingContext) {
         completionHandler(CommandResult::success());
         return;
     }
 
-    RefPtr<InspectorObject> parameters = InspectorObject::create();
-    parameters->setString(ASCIILiteral("handle"), m_toplevelBrowsingContext.value());
-    m_host->sendCommandToBackend(ASCIILiteral("closeBrowsingContext"), WTFMove(parameters), [this, protectedThis = makeRef(*this), completionHandler = WTFMove(completionHandler)](SessionHost::CommandResponse&& response) {
-        if (response.isError) {
-            completionHandler(CommandResult::fail(WTFMove(response.responseObject)));
-            return;
-        }
-        switchToTopLevelBrowsingContext(std::nullopt);
-        completionHandler(CommandResult::success());
-    });
+    auto toplevelBrowsingContext = std::exchange(m_toplevelBrowsingContext, std::nullopt);
+    closeAllToplevelBrowsingContexts(toplevelBrowsingContext.value(), WTFMove(completionHandler));
 }
 
 void Session::setTimeouts(const Timeouts& timeouts, Function<void (CommandResult&&)>&& completionHandler)
@@ -426,6 +438,32 @@ void Session::getWindowHandle(Function<void (CommandResult&&)>&& completionHandl
     });
 }
 
+void Session::closeTopLevelBrowsingContext(const String& toplevelBrowsingContext, Function<void (CommandResult&&)>&& completionHandler)
+{
+    RefPtr<InspectorObject> parameters = InspectorObject::create();
+    parameters->setString(ASCIILiteral("handle"), toplevelBrowsingContext);
+    m_host->sendCommandToBackend(ASCIILiteral("closeBrowsingContext"), WTFMove(parameters), [this, protectedThis = makeRef(*this), completionHandler = WTFMove(completionHandler)](SessionHost::CommandResponse&& response) mutable {
+        if (!m_host->isConnected()) {
+            // Closing the browsing context made the browser quit.
+            completionHandler(CommandResult::success(InspectorArray::create()));
+            return;
+        }
+        if (response.isError) {
+            completionHandler(CommandResult::fail(WTFMove(response.responseObject)));
+            return;
+        }
+
+        getWindowHandles([this, completionHandler = WTFMove(completionHandler)](CommandResult&& result) {
+            if (!m_host->isConnected()) {
+                // Closing the browsing context made the browser quit.
+                completionHandler(CommandResult::success(InspectorArray::create()));
+                return;
+            }
+            completionHandler(WTFMove(result));
+        });
+    });
+}
+
 void Session::closeWindow(Function<void (CommandResult&&)>&& completionHandler)
 {
     if (!m_toplevelBrowsingContext) {
@@ -438,7 +476,8 @@ void Session::closeWindow(Function<void (CommandResult&&)>&& completionHandler)
             completionHandler(WTFMove(result));
             return;
         }
-        close(WTFMove(completionHandler));
+        auto toplevelBrowsingContext = std::exchange(m_toplevelBrowsingContext, std::nullopt);
+        closeTopLevelBrowsingContext(toplevelBrowsingContext.value(), WTFMove(completionHandler));
     });
 }
 
@@ -458,11 +497,6 @@ void Session::switchToWindow(const String& windowHandle, Function<void (CommandR
 
 void Session::getWindowHandles(Function<void (CommandResult&&)>&& completionHandler)
 {
-    if (!m_toplevelBrowsingContext) {
-        completionHandler(CommandResult::fail(CommandResult::ErrorCode::NoSuchWindow));
-        return;
-    }
-
     m_host->sendCommandToBackend(ASCIILiteral("getBrowsingContexts"), InspectorObject::create(), [this, protectedThis = makeRef(*this), completionHandler = WTFMove(completionHandler)](SessionHost::CommandResponse&& response) {
         if (response.isError || !response.responseObject) {
             completionHandler(CommandResult::fail(WTFMove(response.responseObject)));

diff --git a/Source/WebDriver/Session.h b/Source/WebDriver/Session.h
@@ -104,6 +104,8 @@ class Session : public RefCounted<Session> {
 
     void switchToTopLevelBrowsingContext(std::optional<String>);
     void switchToBrowsingContext(std::optional<String>);
+    void closeTopLevelBrowsingContext(const String& toplevelBrowsingContext, Function<void (CommandResult&&)>&&);
+    void closeAllToplevelBrowsingContexts(const String& toplevelBrowsingContext, Function<void (CommandResult&&)>&&);
 
     std::optional<String> pageLoadStrategyString() const;
 

diff --git a/Source/WebDriver/SessionHost.cpp b/Source/WebDriver/SessionHost.cpp
@@ -35,22 +35,20 @@ namespace WebDriver {
 
 void SessionHost::inspectorDisconnected()
 {
-    if (!m_closeMessageID)
-        return;
-
-    // Closing the browsing context made the browser quit.
-    auto responseHandler = m_commandRequests.take(m_closeMessageID);
-    m_closeMessageID = 0;
-    responseHandler({ });
+    // Browser closed or crashed, finish all pending commands with error.
+    Vector<long> messages;
+    copyKeysToVector(m_commandRequests, messages);
+    for (auto messageID : messages) {
+        auto responseHandler = m_commandRequests.take(messageID);
+        responseHandler({ nullptr, true });
+    }
 }
 
 long SessionHost::sendCommandToBackend(const String& command, RefPtr<InspectorObject>&& parameters, Function<void (CommandResponse&&)>&& responseHandler)
 {
     static long lastSequenceID = 0;
     long sequenceID = ++lastSequenceID;
     m_commandRequests.add(sequenceID, WTFMove(responseHandler));
-    if (command == "closeBrowsingContext")
-        m_closeMessageID = sequenceID;
     StringBuilder messageBuilder;
     messageBuilder.appendLiteral("{\"id\":");
     messageBuilder.appendNumber(sequenceID);
@@ -81,9 +79,6 @@ void SessionHost::dispatchMessage(const String& message)
     if (!messageObject->getInteger(ASCIILiteral("id"), sequenceID))
         return;
 
-    if (m_closeMessageID == sequenceID)
-        m_closeMessageID = 0;
-
     auto responseHandler = m_commandRequests.take(sequenceID);
     ASSERT(responseHandler);
 

diff --git a/Source/WebDriver/SessionHost.h b/Source/WebDriver/SessionHost.h
@@ -52,6 +52,8 @@ class SessionHost {
     }
     ~SessionHost();
 
+    bool isConnected() const;
+
     const Capabilities& capabilities() const { return m_capabilities; }
 
     enum class Succeeded { No, Yes };
@@ -92,7 +94,6 @@ class SessionHost {
     Target m_target;
 
     HashMap<long, Function<void (CommandResponse&&)>> m_commandRequests;
-    long m_closeMessageID { 0 };
 
 #if USE(GLIB)
     Function<void (std::optional<String>)> m_startSessionCompletionHandler;

diff --git a/Source/WebDriver/WebDriverService.cpp b/Source/WebDriver/WebDriverService.cpp
@@ -95,13 +95,9 @@ int WebDriverService::run(int argc, char** argv)
 
     RunLoop::run();
 
-    return EXIT_SUCCESS;
-}
-
-void WebDriverService::quit()
-{
     m_server.disconnect();
-    RunLoop::main().stop();
+
+    return EXIT_SUCCESS;
 }
 
 const WebDriverService::Command WebDriverService::s_commands[] = {
@@ -637,14 +633,15 @@ void WebDriverService::deleteSession(RefPtr<InspectorObject>&& parameters, Funct
 
     auto session = m_sessions.take(sessionID);
     if (!session) {
-        completionHandler(CommandResult::fail(CommandResult::ErrorCode::InvalidSessionID));
+        completionHandler(CommandResult::success());
         return;
     }
 
-    if (m_activeSession == session.get())
-        m_activeSession = nullptr;
-
-    session->close(WTFMove(completionHandler));
+    session->close([this, session, completionHandler = WTFMove(completionHandler)](CommandResult&& result) mutable {
+        if (m_activeSession == session.get())
+            m_activeSession = nullptr;
+        completionHandler(WTFMove(result));
+    });
 }
 
 void WebDriverService::setTimeouts(RefPtr<InspectorObject>&& parameters, Function<void (CommandResult&&)>&& completionHandler)
@@ -838,8 +835,24 @@ void WebDriverService::closeWindow(RefPtr<InspectorObject>&& parameters, Functio
 {
     // §10.2 Close Window.
     // https://www.w3.org/TR/webdriver/#close-window
-    if (auto session = findSessionOrCompleteWithError(*parameters, completionHandler))
-        session->closeWindow(WTFMove(completionHandler));
+    auto session = findSessionOrCompleteWithError(*parameters, completionHandler);
+    if (!session)
+        return;
+
+    session->closeWindow([this, session, completionHandler = WTFMove(completionHandler)](CommandResult&& result) mutable {
+        if (result.isError()) {
+            completionHandler(WTFMove(result));
+            return;
+        }
+
+        RefPtr<InspectorArray> handles;
+        if (result.result()->asArray(handles) && !handles->length()) {
+            m_sessions.remove(session->id());
+            if (m_activeSession == session.get())
+                m_activeSession = nullptr;
+        }
+        completionHandler(WTFMove(result));
+    });
 }
 
 void WebDriverService::switchToWindow(RefPtr<InspectorObject>&& parameters, Function<void (CommandResult&&)>&& completionHandler)

diff --git a/Source/WebDriver/WebDriverService.h b/Source/WebDriver/WebDriverService.h
@@ -48,7 +48,6 @@ class WebDriverService final : public HTTPRequestHandler {
     ~WebDriverService() = default;
 
     int run(int argc, char** argv);
-    void quit();
 
     static bool platformCompareBrowserVersions(const String&, const String&);
 

diff --git a/Source/WebDriver/glib/SessionHostGlib.cpp b/Source/WebDriver/glib/SessionHostGlib.cpp
@@ -103,6 +103,11 @@ void SessionHost::connectToBrowser(Function<void (Succeeded)>&& completionHandle
     launchBrowser(WTFMove(completionHandler));
 }
 
+bool SessionHost::isConnected() const
+{
+    return !!m_browser;
+}
+
 struct ConnectToBrowserAsyncData {
     ConnectToBrowserAsyncData(SessionHost* sessionHost, GUniquePtr<char>&& dbusAddress, GCancellable* cancellable, Function<void (SessionHost::Succeeded)>&& completionHandler)
         : sessionHost(sessionHost)
@@ -195,6 +200,7 @@ void SessionHost::connectToBrowser(std::unique_ptr<ConnectToBrowserAsyncData>&&
 
 void SessionHost::dbusConnectionClosedCallback(SessionHost* sessionHost)
 {
+    sessionHost->m_browser = nullptr;
     sessionHost->inspectorDisconnected();
 }