Merge r227290 - [GTK] WebDriver: test imported/w3c/webdriver/tests/se…

…ssions/new_session/response.py is crashing in the bots https://bugs.webkit.org/show_bug.cgi?id=181904 Reviewed by Carlos Alberto Lopez Perez. Source/WebDriver: Handle the case of failing to launch the browser. The test is actually failing because it's sending wrong capabilities, the driver tries to fall back to the default driver, but since WebKit is not installed in the bots, it fails to find the MiniBrowser. The test needs to be fixed, but we shouldn't crash when the browser can't be spawned for whatever reason in any case. This patch handles that case and changes the boolean result of connectToBrowser to be an optional error string instead. This way we can provide more detailed error message when we reject the session creation because the browser failed to start. * SessionHost.h: * WebDriverService.cpp: (WebDriver::WebDriverService::newSession): * glib/SessionHostGlib.cpp: (WebDriver::SessionHost::connectToBrowser): (WebDriver::ConnectToBrowserAsyncData::ConnectToBrowserAsyncData): (WebDriver::SessionHost::launchBrowser): (WebDriver::SessionHost::setupConnection): WebDriverTests: Unskip imported/w3c/webdriver/tests/sessions/new_session/response.py. * TestExpectations.json:
WebKit · Jan 24, 2018 · f07be2b · f07be2b
1 parent 8296b78
commit f07be2b
Show file tree

Hide file tree

Showing 5 changed files with 55 additions and 17 deletions.
diff --git a/Source/WebDriver/ChangeLog b/Source/WebDriver/ChangeLog
@@ -1,3 +1,26 @@
+2018-01-22  Carlos Garcia Campos  <cgarcia@igalia.com>
+
+        [GTK] WebDriver: test imported/w3c/webdriver/tests/sessions/new_session/response.py is crashing in the bots
+        https://bugs.webkit.org/show_bug.cgi?id=181904
+
+        Reviewed by Carlos Alberto Lopez Perez.
+
+        Handle the case of failing to launch the browser. The test is actually failing because it's sending wrong
+        capabilities, the driver tries to fall back to the default driver, but since WebKit is not installed in the
+        bots, it fails to find the MiniBrowser. The test needs to be fixed, but we shouldn't crash when the browser
+        can't be spawned for whatever reason in any case. This patch handles that case and changes the boolean result of
+        connectToBrowser to be an optional error string instead. This way we can provide more detailed error message
+        when we reject the session creation because the browser failed to start.
+
+        * SessionHost.h:
+        * WebDriverService.cpp:
+        (WebDriver::WebDriverService::newSession):
+        * glib/SessionHostGlib.cpp:
+        (WebDriver::SessionHost::connectToBrowser):
+        (WebDriver::ConnectToBrowserAsyncData::ConnectToBrowserAsyncData):
+        (WebDriver::SessionHost::launchBrowser):
+        (WebDriver::SessionHost::setupConnection):
+
 2018-01-11  Carlos Garcia Campos  <cgarcia@igalia.com>
 
         WebDriver: implement get timeouts command

diff --git a/Source/WebDriver/SessionHost.h b/Source/WebDriver/SessionHost.h
@@ -53,8 +53,7 @@ class SessionHost {
 
     const Capabilities& capabilities() const { return m_capabilities; }
 
-    enum class Succeeded { No, Yes };
-    void connectToBrowser(Function<void (Succeeded)>&&);
+    void connectToBrowser(Function<void (std::optional<String> error)>&&);
     void startAutomationSession(const String& sessionID, Function<void (std::optional<String>)>&&);
 
     struct CommandResponse {
@@ -77,10 +76,10 @@ class SessionHost {
 #if USE(GLIB)
     static void dbusConnectionClosedCallback(SessionHost*);
     static const GDBusInterfaceVTable s_interfaceVTable;
-    void launchBrowser(Function<void (Succeeded)>&&);
+    void launchBrowser(Function<void (std::optional<String> error)>&&);
     void connectToBrowser(std::unique_ptr<ConnectToBrowserAsyncData>&&);
     std::optional<String> matchCapabilities(GVariant*);
-    void setupConnection(GRefPtr<GDBusConnection>&&, Function<void (Succeeded)>&&);
+    void setupConnection(GRefPtr<GDBusConnection>&&);
     void setTargetList(uint64_t connectionID, Vector<Target>&&);
     void sendMessageToFrontend(uint64_t connectionID, uint64_t targetID, const char* message);
 #endif

diff --git a/Source/WebDriver/WebDriverService.cpp b/Source/WebDriver/WebDriverService.cpp
@@ -611,9 +611,9 @@ void WebDriverService::newSession(RefPtr<JSON::Object>&& parameters, Function<vo
     parseCapabilities(*matchedCapabilities, capabilities);
     auto sessionHost = std::make_unique<SessionHost>(WTFMove(capabilities));
     auto* sessionHostPtr = sessionHost.get();
-    sessionHostPtr->connectToBrowser([this, sessionHost = WTFMove(sessionHost), completionHandler = WTFMove(completionHandler)](SessionHost::Succeeded succeeded) mutable {
-        if (succeeded == SessionHost::Succeeded::No) {
-            completionHandler(CommandResult::fail(CommandResult::ErrorCode::SessionNotCreated, String("Failed to connect to browser")));
+    sessionHostPtr->connectToBrowser([this, sessionHost = WTFMove(sessionHost), completionHandler = WTFMove(completionHandler)](std::optional<String> error) mutable {
+        if (error) {
+            completionHandler(CommandResult::fail(CommandResult::ErrorCode::SessionNotCreated, makeString("Failed to connect to browser: ", error.value())));
             return;
         }
 

diff --git a/Source/WebDriver/glib/SessionHostGlib.cpp b/Source/WebDriver/glib/SessionHostGlib.cpp
@@ -98,7 +98,7 @@ const GDBusInterfaceVTable SessionHost::s_interfaceVTable = {
     { 0 }
 };
 
-void SessionHost::connectToBrowser(Function<void (Succeeded)>&& completionHandler)
+void SessionHost::connectToBrowser(Function<void (std::optional<String> error)>&& completionHandler)
 {
     launchBrowser(WTFMove(completionHandler));
 }
@@ -109,7 +109,7 @@ bool SessionHost::isConnected() const
 }
 
 struct ConnectToBrowserAsyncData {
-    ConnectToBrowserAsyncData(SessionHost* sessionHost, GUniquePtr<char>&& dbusAddress, GCancellable* cancellable, Function<void (SessionHost::Succeeded)>&& completionHandler)
+    ConnectToBrowserAsyncData(SessionHost* sessionHost, GUniquePtr<char>&& dbusAddress, GCancellable* cancellable, Function<void (std::optional<String> error)>&& completionHandler)
         : sessionHost(sessionHost)
         , dbusAddress(WTFMove(dbusAddress))
         , cancellable(cancellable)
@@ -120,7 +120,7 @@ struct ConnectToBrowserAsyncData {
     SessionHost* sessionHost;
     GUniquePtr<char> dbusAddress;
     GRefPtr<GCancellable> cancellable;
-    Function<void (SessionHost::Succeeded)> completionHandler;
+    Function<void (std::optional<String> error)> completionHandler;
 };
 
 static guint16 freePort()
@@ -135,7 +135,7 @@ static guint16 freePort()
     return g_inet_socket_address_get_port(G_INET_SOCKET_ADDRESS(address.get()));
 }
 
-void SessionHost::launchBrowser(Function<void (Succeeded)>&& completionHandler)
+void SessionHost::launchBrowser(Function<void (std::optional<String> error)>&& completionHandler)
 {
     m_cancellable = adoptGRef(g_cancellable_new());
     GRefPtr<GSubprocessLauncher> launcher = adoptGRef(g_subprocess_launcher_new(G_SUBPROCESS_FLAGS_NONE));
@@ -152,7 +152,13 @@ void SessionHost::launchBrowser(Function<void (Succeeded)>&& completionHandler)
     for (unsigned i = 0; i < browserArguments.size(); ++i)
         args.get()[i + 1] = g_strdup(browserArguments[i].utf8().data());
 
-    m_browser = adoptGRef(g_subprocess_launcher_spawnv(launcher.get(), args.get(), nullptr));
+    GUniqueOutPtr<GError> error;
+    m_browser = adoptGRef(g_subprocess_launcher_spawnv(launcher.get(), args.get(), &error.outPtr()));
+    if (error) {
+        completionHandler(String::fromUTF8(error->message));
+        return;
+    }
+
     g_subprocess_wait_async(m_browser.get(), m_cancellable.get(), [](GObject* browser, GAsyncResult* result, gpointer userData) {
         GUniqueOutPtr<GError> error;
         g_subprocess_wait_finish(G_SUBPROCESS(browser), result, &error.outPtr());
@@ -190,10 +196,11 @@ void SessionHost::connectToBrowser(std::unique_ptr<ConnectToBrowserAsyncData>&&
                         return;
                     }
 
-                    data->completionHandler(Succeeded::No);
+                    data->completionHandler(String::fromUTF8(error->message));
                     return;
                 }
-                data->sessionHost->setupConnection(WTFMove(connection), WTFMove(data->completionHandler));
+                data->sessionHost->setupConnection(WTFMove(connection));
+                data->completionHandler(std::nullopt);
         }, data);
     });
 }
@@ -212,7 +219,7 @@ static void dbusConnectionCallAsyncReadyCallback(GObject* source, GAsyncResult*
         WTFLogAlways("RemoteInspectorServer failed to send DBus message: %s", error->message);
 }
 
-void SessionHost::setupConnection(GRefPtr<GDBusConnection>&& connection, Function<void (Succeeded)>&& completionHandler)
+void SessionHost::setupConnection(GRefPtr<GDBusConnection>&& connection)
 {
     ASSERT(!m_dbusConnection);
     ASSERT(connection);
@@ -225,8 +232,6 @@ void SessionHost::setupConnection(GRefPtr<GDBusConnection>&& connection, Functio
         introspectionData = g_dbus_node_info_new_for_xml(introspectionXML, nullptr);
 
     g_dbus_connection_register_object(m_dbusConnection.get(), REMOTE_INSPECTOR_CLIENT_OBJECT_PATH, introspectionData->interfaces[0], &s_interfaceVTable, this, nullptr, nullptr);
-
-    completionHandler(Succeeded::Yes);
 }
 
 std::optional<String> SessionHost::matchCapabilities(GVariant* capabilities)

diff --git a/WebDriverTests/ChangeLog b/WebDriverTests/ChangeLog
@@ -1,3 +1,14 @@
+2018-01-22  Carlos Garcia Campos  <cgarcia@igalia.com>
+
+        [GTK] WebDriver: test imported/w3c/webdriver/tests/sessions/new_session/response.py is crashing in the bots
+        https://bugs.webkit.org/show_bug.cgi?id=181904
+
+        Reviewed by Carlos Alberto Lopez Perez.
+
+        Unskip imported/w3c/webdriver/tests/sessions/new_session/response.py.
+
+        * TestExpectations.json:
+
 2018-01-18  Carlos Garcia Campos  <cgarcia@igalia.com>
 
         Unreviewed. Update Selenium WebDriver imported tests.