Bug 185568 – [Curl] Move cookie header handling into background thread.

• Status: RESOLVED
• Resolution: WONTFIX
• Component: WebCore Misc.

[Basuke Suzuki]

For async (normal) request, current implementation add header for cookie and store back the cookie data on main thread. Move them into background thread to get better responsiveness.

[Basuke Suzuki]

Created attachment 340232 [details]
PATCH

[EWS Watchlist]

Comment on attachment 340232 [details]
PATCH

Attachment 340232 [details] did not pass win-ews (win):
Output: http://webkit-queues.webkit.org/results/7656621

New failing tests:
http/tests/security/contentSecurityPolicy/userAgentShadowDOM/allow-video.html

[EWS Watchlist]

Created attachment 340237 [details]
Archive of layout-test-results from ews204 for win-future

The attached test failures were seen while running run-webkit-tests on the win-ews.
Bot: ews204  Port: win-future  Platform: CYGWIN_NT-6.1-2.9.0-0.318-5-3-x86_64-64bit

[Basuke Suzuki]

Created attachment 340241 [details]
PATCH

Fix method name and add WEBCORE_EXPORT.

[Basuke Suzuki]

Created attachment 340244 [details]
PATCH

[youenn fablet]

Is it thread-safe to read/write cookies from different concurrent threads?

[Basuke Suzuki]

Yes, our library embed the sqlite build with thread safe. The library itself can open a database as single thread mode, but WebKit interface doesn’t have an option  to specify that flag so that there’s no worry about that mode on.

[youenn fablet]

Comment on attachment 340244 [details]
PATCH

Some additional comments or questions.

View in context: https://bugs.webkit.org/attachment.cgi?id=340244&action=review

> Source/WebCore/platform/network/curl/CurlRequest.cpp:489
> +void CurlRequest::appendCookieHeader(HTTPHeaderMap& header)

s/header/headers/

> Source/WebCore/platform/network/curl/CurlRequest.cpp:497
> +        header.add(HTTPHeaderName::Cookie, appendedCookieField);

From what I see, we are creating appendedCookieField in a background thread and we are adding it to a HTTPHeaderMap.
Since the HTTPHeaderMap will be destroyed in the same background thread, this is probably fine.

I wonder though whether this is not a bit fragile.
For instance, adding some string value caching to cookieJar.cookieRequestHeaderFieldValue could potentially break this by keeping a ref to appendedCookieField in two different threads.
Or if we were changing m_request http header fields, we might also end up being in trouble.

> Source/WebCore/platform/network/curl/ResourceHandleCurl.cpp:145
> +        curlRequest->setAppendCookieHeader(true);

Shouldn't cookie header handling be based on whether to use credentials or not?
Shouldn't we use the storage session of the NetworkingContext instead of the default one?
Not to say this should be handled in this patch.

[Basuke Suzuki]

Youenn, besides the things you reviewed, we figured out the usage of SQLite in entire code base of WebKit is not 100% sure thread-safe. When I sent a patch 185616, mac-debug-build start failing with crashes. I guess it hit the assertion of libsqlite compiled with thread-support. We temporally give up this patch for a while.