[SOUP] Synchronous XMLHttpRequests can time out when we reach the max connections...
authorcarlosgc@webkit.org <carlosgc@webkit.org@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Tue, 3 Mar 2015 09:17:24 +0000 (09:17 +0000)
committercarlosgc@webkit.org <carlosgc@webkit.org@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Tue, 3 Mar 2015 09:17:24 +0000 (09:17 +0000)
https://bugs.webkit.org/show_bug.cgi?id=141508

Reviewed by Sergio Villar Senin.

Source/WebCore:

Use SOUP_MESSAGE_IGNORE_CONNECTION_LIMITS flag when loading a
synchronous message instead of increasing the maximum number of
connections allowed if the soup version is recent enough.
The current solution of increasing/decreasing the limits doesn't
always work, because connections are not marked as IDLE in libsoup
until the message is unqueued, but we don't wait for the message
to be unqueued to finish our loads in WebKit, we finish them as
soon as we have finished reading the stream. This causes that
synchronous loads keep blocked in the nested main loop until the
timeout of 10 seconds is fired and the load fails.
Also marked WebCoreSynchronousLoader class as final, the virtual
methods as override and removed the unsused method isSynchronousClient.

* platform/network/soup/ResourceHandleSoup.cpp:
(WebCore::createSoupMessageForHandleAndRequest):
(WebCore::WebCoreSynchronousLoader::WebCoreSynchronousLoader):
(WebCore::WebCoreSynchronousLoader::isSynchronousClient): Deleted.
(WebCore::WebCoreSynchronousLoader::didReceiveResponse):
(WebCore::WebCoreSynchronousLoader::didReceiveData):
(WebCore::WebCoreSynchronousLoader::didReceiveBuffer):
(WebCore::WebCoreSynchronousLoader::didFinishLoading):
(WebCore::WebCoreSynchronousLoader::didFail):
(WebCore::WebCoreSynchronousLoader::didReceiveAuthenticationChallenge):
(WebCore::WebCoreSynchronousLoader::shouldUseCredentialStorage):

Tools:

Add a unit test to check that synchronous XHRs load even if the
maximum connection limits are reached.

* TestWebKitAPI/Tests/WebKit2Gtk/TestResources.cpp:
(testWebViewSyncRequestOnMaxConns):
(serverCallback):
(beforeAll):
* gtk/jhbuild.modules: Bump libsoup version to 2.49.91.

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

Source/WebCore/ChangeLog
Source/WebCore/platform/network/soup/ResourceHandleSoup.cpp
Tools/ChangeLog
Tools/TestWebKitAPI/Tests/WebKit2Gtk/TestResources.cpp
Tools/gtk/jhbuild.modules

index 755b069..e2809ac 100644 (file)
@@ -1,3 +1,35 @@
+2015-03-03  Carlos Garcia Campos  <cgarcia@igalia.com>
+
+        [SOUP] Synchronous XMLHttpRequests can time out when we reach the max connections limit
+        https://bugs.webkit.org/show_bug.cgi?id=141508
+
+        Reviewed by Sergio Villar Senin.
+
+        Use SOUP_MESSAGE_IGNORE_CONNECTION_LIMITS flag when loading a
+        synchronous message instead of increasing the maximum number of
+        connections allowed if the soup version is recent enough.
+        The current solution of increasing/decreasing the limits doesn't
+        always work, because connections are not marked as IDLE in libsoup
+        until the message is unqueued, but we don't wait for the message
+        to be unqueued to finish our loads in WebKit, we finish them as
+        soon as we have finished reading the stream. This causes that
+        synchronous loads keep blocked in the nested main loop until the
+        timeout of 10 seconds is fired and the load fails.
+        Also marked WebCoreSynchronousLoader class as final, the virtual
+        methods as override and removed the unsused method isSynchronousClient.
+
+        * platform/network/soup/ResourceHandleSoup.cpp:
+        (WebCore::createSoupMessageForHandleAndRequest):
+        (WebCore::WebCoreSynchronousLoader::WebCoreSynchronousLoader):
+        (WebCore::WebCoreSynchronousLoader::isSynchronousClient): Deleted.
+        (WebCore::WebCoreSynchronousLoader::didReceiveResponse):
+        (WebCore::WebCoreSynchronousLoader::didReceiveData):
+        (WebCore::WebCoreSynchronousLoader::didReceiveBuffer):
+        (WebCore::WebCoreSynchronousLoader::didFinishLoading):
+        (WebCore::WebCoreSynchronousLoader::didFail):
+        (WebCore::WebCoreSynchronousLoader::didReceiveAuthenticationChallenge):
+        (WebCore::WebCoreSynchronousLoader::shouldUseCredentialStorage):
+
 2015-03-02  David Kilzer  <ddkilzer@apple.com>
 
         [iOS] Disable -Wdeprecated-declaration warnings in WebVideoFullscreenInterfaceAVKit.mm
index fa219fc..d599109 100644 (file)
@@ -75,7 +75,7 @@ namespace WebCore {
 static bool loadingSynchronousRequest = false;
 static const size_t gDefaultReadBufferSize = 8192;
 
-class WebCoreSynchronousLoader : public ResourceHandleClient {
+class WebCoreSynchronousLoader final : public ResourceHandleClient {
     WTF_MAKE_NONCOPYABLE(WebCoreSynchronousLoader);
 public:
 
@@ -86,7 +86,6 @@ public:
         , m_data(data)
         , m_finished(false)
         , m_storedCredentials(storedCredentials)
-        
     {
         // We don't want any timers to fire while we are doing our synchronous load
         // so we replace the thread default main context. The main loop iterations
@@ -96,12 +95,16 @@ public:
         g_main_context_push_thread_default(innerMainContext.get());
         m_mainLoop = adoptGRef(g_main_loop_new(innerMainContext.get(), false));
 
+#if !SOUP_CHECK_VERSION(2, 49, 91)
         adjustMaxConnections(1);
+#endif
     }
 
     ~WebCoreSynchronousLoader()
     {
+#if !SOUP_CHECK_VERSION(2, 49, 91)
         adjustMaxConnections(-1);
+#endif
 
         GMainContext* context = g_main_context_get_thread_default();
         while (g_main_context_pending(context))
@@ -111,6 +114,7 @@ public:
         loadingSynchronousRequest = false;
     }
 
+#if !SOUP_CHECK_VERSION(2, 49, 91)
     void adjustMaxConnections(int adjustment)
     {
         int maxConnections, maxConnectionsPerHost;
@@ -126,23 +130,19 @@ public:
                      NULL);
 
     }
+#endif // SOUP_CHECK_VERSION(2, 49, 91)
 
-    virtual bool isSynchronousClient()
-    {
-        return true;
-    }
-
-    virtual void didReceiveResponse(ResourceHandle*, const ResourceResponse& response)
+    virtual void didReceiveResponse(ResourceHandle*, const ResourceResponse& response) override
     {
         m_response = response;
     }
 
-    virtual void didReceiveData(ResourceHandle*, const char* /* data */, unsigned /* length */, int)
+    virtual void didReceiveData(ResourceHandle*, const char* /* data */, unsigned /* length */, int) override
     {
         ASSERT_NOT_REACHED();
     }
 
-    virtual void didReceiveBuffer(ResourceHandle*, PassRefPtr<SharedBuffer> buffer, int /* encodedLength */)
+    virtual void didReceiveBuffer(ResourceHandle*, PassRefPtr<SharedBuffer> buffer, int /* encodedLength */) override
     {
         // This pattern is suggested by SharedBuffer.h.
         const char* segment;
@@ -153,26 +153,26 @@ public:
         }
     }
 
-    virtual void didFinishLoading(ResourceHandle*, double)
+    virtual void didFinishLoading(ResourceHandle*, double) override
     {
         if (g_main_loop_is_running(m_mainLoop.get()))
             g_main_loop_quit(m_mainLoop.get());
         m_finished = true;
     }
 
-    virtual void didFail(ResourceHandle* handle, const ResourceError& error)
+    virtual void didFail(ResourceHandle* handle, const ResourceError& error) override
     {
         m_error = error;
         didFinishLoading(handle, 0);
     }
 
-    virtual void didReceiveAuthenticationChallenge(ResourceHandle*, const AuthenticationChallenge& challenge)
+    virtual void didReceiveAuthenticationChallenge(ResourceHandle*, const AuthenticationChallenge& challenge) override
     {
         // We do not handle authentication for synchronous XMLHttpRequests.
         challenge.authenticationClient()->receivedRequestToContinueWithoutCredential(challenge);
     }
 
-    virtual bool shouldUseCredentialStorage(ResourceHandle*)
+    virtual bool shouldUseCredentialStorage(ResourceHandle*) override
     {
         return m_storedCredentials == AllowStoredCredentials;
     }
@@ -930,7 +930,14 @@ static bool createSoupMessageForHandleAndRequest(ResourceHandle* handle, const R
     g_signal_connect(d->m_soupMessage.get(), "got-headers", G_CALLBACK(gotHeadersCallback), handle);
     g_signal_connect(d->m_soupMessage.get(), "wrote-body-data", G_CALLBACK(wroteBodyDataCallback), handle);
 
-    soup_message_set_flags(d->m_soupMessage.get(), static_cast<SoupMessageFlags>(soup_message_get_flags(d->m_soupMessage.get()) | SOUP_MESSAGE_NO_REDIRECT));
+    unsigned flags = SOUP_MESSAGE_NO_REDIRECT;
+#if SOUP_CHECK_VERSION(2, 49, 91)
+    // Ignore the connection limits in synchronous loads to avoid freezing the networking process.
+    // See https://bugs.webkit.org/show_bug.cgi?id=141508.
+    if (loadingSynchronousRequest)
+        flags |= SOUP_MESSAGE_IGNORE_CONNECTION_LIMITS;
+#endif
+    soup_message_set_flags(d->m_soupMessage.get(), static_cast<SoupMessageFlags>(soup_message_get_flags(d->m_soupMessage.get()) | flags));
 
 #if ENABLE(WEB_TIMING)
     g_signal_connect(d->m_soupMessage.get(), "network-event", G_CALLBACK(networkEventCallback), handle);
index afea5f3..b4d2d9f 100644 (file)
@@ -1,3 +1,19 @@
+2015-03-03  Carlos Garcia Campos  <cgarcia@igalia.com>
+
+        [SOUP] Synchronous XMLHttpRequests can time out when we reach the max connections limit
+        https://bugs.webkit.org/show_bug.cgi?id=141508
+
+        Reviewed by Sergio Villar Senin.
+
+        Add a unit test to check that synchronous XHRs load even if the
+        maximum connection limits are reached.
+
+        * TestWebKitAPI/Tests/WebKit2Gtk/TestResources.cpp:
+        (testWebViewSyncRequestOnMaxConns):
+        (serverCallback):
+        (beforeAll):
+        * gtk/jhbuild.modules: Bump libsoup version to 2.49.91.
+
 2015-03-02  Alexey Proskuryakov  <ap@apple.com>
 
         Update the name of ASan build step.
index 39a7dd0..dbb88d6 100644 (file)
@@ -22,6 +22,8 @@
 #include "WebKitTestServer.h"
 #include "WebViewTest.h"
 #include <wtf/Vector.h>
+#include <wtf/gobject/GMainLoopSource.h>
+#include <wtf/gobject/GMutexLocker.h>
 #include <wtf/gobject/GRefPtr.h>
 
 static WebKitTestServer* kServer;
@@ -667,6 +669,49 @@ static void testWebResourceSendRequest(SendRequestTest* test, gconstpointer)
     events.clear();
 }
 
+static GMutex s_serverMutex;
+static const unsigned s_maxConnectionsPerHost = 6;
+
+class SyncRequestOnMaxConnsTest: public ResourcesTest {
+public:
+    MAKE_GLIB_TEST_FIXTURE(SyncRequestOnMaxConnsTest);
+
+    void resourceLoadStarted(WebKitWebResource*, WebKitURIRequest*) override
+    {
+        if (!m_resourcesToStartPending)
+            return;
+
+        if (!--m_resourcesToStartPending)
+            g_main_loop_quit(m_mainLoop);
+    }
+
+    void waitUntilResourcesStarted(unsigned requestCount)
+    {
+        m_resourcesToStartPending = requestCount;
+        g_main_loop_run(m_mainLoop);
+    }
+
+    unsigned m_resourcesToStartPending;
+};
+
+static void testWebViewSyncRequestOnMaxConns(SyncRequestOnMaxConnsTest* test, gconstpointer)
+{
+    WTF::GMutexLocker<GMutex> lock(s_serverMutex);
+    test->loadURI(kServer->getURIForPath("/sync-request-on-max-conns-0").data());
+    test->waitUntilResourcesStarted(s_maxConnectionsPerHost + 1); // s_maxConnectionsPerHost resource + main resource.
+
+    for (unsigned i = 0; i < 2; ++i) {
+        GUniquePtr<char> xhr(g_strdup_printf("xhr = new XMLHttpRequest; xhr.open('GET', '/sync-request-on-max-conns-xhr%u', false); xhr.send();", i));
+        webkit_web_view_run_javascript(test->m_webView, xhr.get(), nullptr, nullptr, nullptr);
+    }
+
+    // By default sync XHRs have a 10 seconds timeout, we don't want to wait all that so use our own timeout.
+    GMainLoopSource::scheduleAfterDelayAndDeleteOnDestroy("Timeout", [] { g_assert_not_reached(); }, std::chrono::seconds(1));
+
+    GMainLoopSource::scheduleAndDeleteOnDestroy("Unlock Server Idle", [&lock] { lock.unlock(); });
+    test->waitUntilResourcesLoaded(s_maxConnectionsPerHost + 3); // s_maxConnectionsPerHost resource + main resource + 2 XHR.
+}
+
 static void addCacheHTTPHeadersToResponse(SoupMessage* message)
 {
     // The actual date doesn't really matter.
@@ -760,7 +805,28 @@ static void serverCallback(SoupServer* server, SoupMessage* message, const char*
         soup_message_headers_append(message->response_headers, "Location", "/cancel-this.js");
     } else if (g_str_equal(path, "/invalid.css"))
         soup_message_set_status(message, SOUP_STATUS_CANT_CONNECT);
-    else
+    else if (g_str_has_prefix(path, "/sync-request-on-max-conns-")) {
+        char* contents;
+        gsize contentsLength;
+        if (g_str_equal(path, "/sync-request-on-max-conns-0")) {
+            GString* imagesHTML = g_string_new("<html><body>");
+            for (unsigned i = 1; i <= s_maxConnectionsPerHost; ++i)
+                g_string_append_printf(imagesHTML, "<img src='/sync-request-on-max-conns-%u'>", i);
+            g_string_append(imagesHTML, "</body></html>");
+
+            contentsLength = imagesHTML->len;
+            contents = g_string_free(imagesHTML, FALSE);
+        } else {
+            {
+                // We don't actually need to keep the mutex, so we release it as soon as we get it.
+                WTF::GMutexLocker<GMutex> lock(s_serverMutex);
+            }
+
+            GUniquePtr<char> filePath(g_build_filename(Test::getResourcesDir().data(), "blank.ico", nullptr));
+            g_file_get_contents(filePath.get(), &contents, &contentsLength, 0);
+        }
+        soup_message_body_append(message->response_body, SOUP_MEMORY_TAKE, contents, contentsLength);
+    } else
         soup_message_set_status(message, SOUP_STATUS_NOT_FOUND);
     soup_message_body_complete(message->response_body);
 }
@@ -779,6 +845,9 @@ void beforeAll()
     ResourcesTest::add("WebKitWebResource", "get-data", testWebResourceGetData);
     SingleResourceLoadTest::add("WebKitWebView", "history-cache", testWebViewResourcesHistoryCache);
     SendRequestTest::add("WebKitWebPage", "send-request", testWebResourceSendRequest);
+#if SOUP_CHECK_VERSION(2, 49, 91)
+    SyncRequestOnMaxConnsTest::add("WebKitWebView", "sync-request-on-max-conns", testWebViewSyncRequestOnMaxConns);
+#endif
 }
 
 void afterAll()
index 9380ca3..0bad37a 100644 (file)
     <dependencies>
       <dep package="glib-networking"/>
     </dependencies>
-    <branch module="libsoup" version="2.43.90"
+    <branch module="libsoup" version="2.49.91"
             repo="git.gnome.org"
-            tag="0ea86f566b7d526c8328c7c602ae1be8cda8dd68"/>
+            tag="933de304ffde0257b18f04bf34a653fcc356833c"/>
   </autotools>
 
   <autotools id="fontconfig"