WebDriver: service hangs after a browser crash
authorcarlosgc@webkit.org <carlosgc@webkit.org@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Fri, 26 Jan 2018 13:20:11 +0000 (13:20 +0000)
committercarlosgc@webkit.org <carlosgc@webkit.org@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Fri, 26 Jan 2018 13:20:11 +0000 (13:20 +0000)
https://bugs.webkit.org/show_bug.cgi?id=182170

Reviewed by Carlos Alberto Lopez Perez.

This is currently happening in the GTK+ debug bot. There's a test that makes the browser crash due to an assert,
hanging the whole process and preventing the rest of the tests from running. When the browser crashes, we
correctly handle the pending requests, by completing them with an error. However, if the client tries to send
another command we fail to send the message to the browser and the reply is never sent to the client. In the
case of the tests, delete session command is sent, but never gets a reply.

* Session.cpp:
(WebDriver::Session::isConnected const): Return whether the session is connected to the browser.
* Session.h:
* SessionHost.cpp:
(WebDriver::SessionHost::sendCommandToBackend): Pass the message ID to SessionHost::sendMessageToBackend().
* SessionHost.h:
* WebDriverService.cpp:
(WebDriver::WebDriverService::deleteSession): Ignore unknown errors if the session is no longer connected.
* glib/SessionHostGlib.cpp:
(WebDriver::SessionHost::sendMessageToBackend): Handle errors when sending the command by completing the request
with an error.

git-svn-id: https://svn.webkit.org/repository/webkit/trunk@227674 268f45cc-cd09-0410-ab3c-d52691b4dbfc

Source/WebDriver/ChangeLog
Source/WebDriver/Session.cpp
Source/WebDriver/Session.h
Source/WebDriver/SessionHost.cpp
Source/WebDriver/SessionHost.h
Source/WebDriver/WebDriverService.cpp
Source/WebDriver/glib/SessionHostGlib.cpp

index 07587d9..058f7ec 100644 (file)
@@ -1,5 +1,30 @@
 2018-01-26  Carlos Garcia Campos  <cgarcia@igalia.com>
 
+        WebDriver: service hangs after a browser crash
+        https://bugs.webkit.org/show_bug.cgi?id=182170
+
+        Reviewed by Carlos Alberto Lopez Perez.
+
+        This is currently happening in the GTK+ debug bot. There's a test that makes the browser crash due to an assert,
+        hanging the whole process and preventing the rest of the tests from running. When the browser crashes, we
+        correctly handle the pending requests, by completing them with an error. However, if the client tries to send
+        another command we fail to send the message to the browser and the reply is never sent to the client. In the
+        case of the tests, delete session command is sent, but never gets a reply.
+
+        * Session.cpp:
+        (WebDriver::Session::isConnected const): Return whether the session is connected to the browser.
+        * Session.h:
+        * SessionHost.cpp:
+        (WebDriver::SessionHost::sendCommandToBackend): Pass the message ID to SessionHost::sendMessageToBackend().
+        * SessionHost.h:
+        * WebDriverService.cpp:
+        (WebDriver::WebDriverService::deleteSession): Ignore unknown errors if the session is no longer connected.
+        * glib/SessionHostGlib.cpp:
+        (WebDriver::SessionHost::sendMessageToBackend): Handle errors when sending the command by completing the request
+        with an error.
+
+2018-01-26  Carlos Garcia Campos  <cgarcia@igalia.com>
+
         WebDriver: timeouts value and cookie expiry should be limited to max safe integer
         https://bugs.webkit.org/show_bug.cgi?id=182167
 
index 48adfe8..5822174 100644 (file)
@@ -69,6 +69,11 @@ const Capabilities& Session::capabilities() const
     return m_host->capabilities();
 }
 
+bool Session::isConnected() const
+{
+    return m_host->isConnected();
+}
+
 static std::optional<String> firstWindowHandleInResult(JSON::Value& result)
 {
     RefPtr<JSON::Array> handles;
index 65d3ffb..8e89da7 100644 (file)
@@ -48,6 +48,7 @@ public:
 
     const String& id() const;
     const Capabilities& capabilities() const;
+    bool isConnected() const;
     Seconds scriptTimeout() const  { return m_scriptTimeout; }
     Seconds pageLoadTimeout() const { return m_pageLoadTimeout; }
     Seconds implicitWaitTimeout() const { return m_implicitWaitTimeout; }
index 18d50c1..f164929 100644 (file)
@@ -55,7 +55,7 @@ long SessionHost::sendCommandToBackend(const String& command, RefPtr<JSON::Objec
         messageBuilder.append(parameters->toJSONString());
     }
     messageBuilder.append('}');
-    sendMessageToBackend(messageBuilder.toString());
+    sendMessageToBackend(sequenceID, messageBuilder.toString());
 
     return sequenceID;
 }
index efaf127..79ce9c1 100644 (file)
@@ -71,7 +71,7 @@ private:
     };
 
     void inspectorDisconnected();
-    void sendMessageToBackend(const String&);
+    void sendMessageToBackend(long, const String&);
     void dispatchMessage(const String&);
 
 #if USE(GLIB)
index 3934070..73ebd57 100644 (file)
@@ -754,7 +754,11 @@ void WebDriverService::deleteSession(RefPtr<JSON::Object>&& parameters, Function
 
     auto session = std::exchange(m_session, nullptr);
     session->close([this, session, completionHandler = WTFMove(completionHandler)](CommandResult&& result) mutable {
-        completionHandler(WTFMove(result));
+        // Ignore unknown errors when closing the session if the browser is closed.
+        if (result.isError() && result.errorCode() == CommandResult::ErrorCode::UnknownError && !session->isConnected())
+            completionHandler(CommandResult::success());
+        else
+            completionHandler(WTFMove(result));
     });
 }
 
index 795b092..12c9c1e 100644 (file)
@@ -340,19 +340,38 @@ void SessionHost::sendMessageToFrontend(uint64_t connectionID, uint64_t targetID
     dispatchMessage(String::fromUTF8(message));
 }
 
-void SessionHost::sendMessageToBackend(const String& message)
+struct MessageContext {
+    long messageID;
+    SessionHost* host;
+};
+
+void SessionHost::sendMessageToBackend(long messageID, const String& message)
 {
     ASSERT(m_dbusConnection);
     ASSERT(m_connectionID);
     ASSERT(m_target.id);
 
+    auto messageContext = std::make_unique<MessageContext>(MessageContext { messageID, this });
     g_dbus_connection_call(m_dbusConnection.get(), nullptr,
         INSPECTOR_DBUS_OBJECT_PATH,
         INSPECTOR_DBUS_INTERFACE,
         "SendMessageToBackend",
         g_variant_new("(tts)", m_connectionID, m_target.id, message.utf8().data()),
         nullptr, G_DBUS_CALL_FLAGS_NO_AUTO_START,
-        -1, m_cancellable.get(), dbusConnectionCallAsyncReadyCallback, nullptr);
+        -1, m_cancellable.get(), [](GObject* source, GAsyncResult* result, gpointer userData) {
+            auto messageContext = std::unique_ptr<MessageContext>(static_cast<MessageContext*>(userData));
+            GUniqueOutPtr<GError> error;
+            GRefPtr<GVariant> resultVariant = adoptGRef(g_dbus_connection_call_finish(G_DBUS_CONNECTION(source), result, &error.outPtr()));
+            if (!resultVariant && !g_error_matches(error.get(), G_IO_ERROR, G_IO_ERROR_CANCELLED)) {
+                auto responseHandler = messageContext->host->m_commandRequests.take(messageContext->messageID);
+                if (responseHandler) {
+                    auto errorObject = JSON::Object::create();
+                    errorObject->setInteger(ASCIILiteral("code"), -32603);
+                    errorObject->setString(ASCIILiteral("message"), String::fromUTF8(error->message));
+                    responseHandler({ WTFMove(errorObject), true });
+                }
+            }
+        }, messageContext.release());
 }
 
 } // namespace WebDriver