Skip to content

Commit

Permalink
Merge r180927 - [SOUP] Synchronous XMLHttpRequests can time out when …
Browse files Browse the repository at this point in the history 
…we reach the max connections limit

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.
  • Loading branch information
@carlosgcampos
carlosgcampos committed Mar 3, 2015
1 parent 9fbeeb0 commit 67e6ac0
Show file tree
Hide file tree
Showing 5 changed files with 142 additions and 18 deletions.
32 changes: 32 additions & 0 deletions Source/WebCore/ChangeLog
View 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-02-28 Simon Fraser <simon.fraser@apple.com>

FrameView::layoutTimerFired() should update style if needed before doing layout
Expand Down
37 changes: 22 additions & 15 deletions Source/WebCore/platform/network/soup/ResourceHandleSoup.cpp
View file
Expand Up @@ -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:

Expand All @@ -86,7 +86,6 @@ class WebCoreSynchronousLoader : public ResourceHandleClient {
, 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
Expand All @@ -96,12 +95,16 @@ class WebCoreSynchronousLoader : public ResourceHandleClient {
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))
Expand All @@ -111,6 +114,7 @@ class WebCoreSynchronousLoader : public ResourceHandleClient {
loadingSynchronousRequest = false;
}

#if !SOUP_CHECK_VERSION(2, 49, 91)
void adjustMaxConnections(int adjustment)
{
int maxConnections, maxConnectionsPerHost;
Expand All @@ -126,23 +130,19 @@ class WebCoreSynchronousLoader : public ResourceHandleClient {
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;
Expand All @@ -153,26 +153,26 @@ class WebCoreSynchronousLoader : public ResourceHandleClient {
}
}

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;
}
Expand Down Expand Up @@ -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);
Expand Down
16 changes: 16 additions & 0 deletions Tools/ChangeLog
View 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-02-25 Benjamin Poulain <bpoulain@apple.com>

CodeBlock crashes when dumping op_push_name_scope
Expand Down
71 changes: 70 additions & 1 deletion Tools/TestWebKitAPI/Tests/WebKit2Gtk/TestResources.cpp
View file
Expand Up @@ -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;
Expand Down Expand Up @@ -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.
Expand Down Expand Up @@ -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);
}
Expand All @@ -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()
Expand Down
4 changes: 2 additions & 2 deletions Tools/gtk/jhbuild.modules
View file
Expand Up @@ -182,9 +182,9 @@
<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"
Expand Down

0 comments on commit 67e6ac0

Please sign in to comment.