Bug 174845 – [Curl] Bug fix after r219606

• Component: WebCore Misc.
• Status: RESOLVED FIXED

[Daewoong Jang]

Fixes bugs introduced by r219606. However I believe there are still issues related to multi-threading, which needs to be solved.

[Daewoong Jang]

Related bug: https://bugs.webkit.org/show_bug.cgi?id=173991

[Daewoong Jang]

Created attachment 316408 [details]
patch

[Build Bot]

Comment on attachment 316408 [details]
patch

Attachment 316408 [details] did not pass ios-sim-ews (ios-simulator-wk2):
Output: http://webkit-queues.webkit.org/results/4187484

New failing tests:
imported/w3c/IndexedDB-private-browsing/idbfactory_open12.html

[Build Bot]

Created attachment 316416 [details]
Archive of layout-test-results from ews123 for ios-simulator-wk2

The attached test failures were seen while running run-webkit-tests on the ios-sim-ews.
Bot: ews123  Port: ios-simulator-wk2  Platform: Mac OS X 10.12.5

[Basuke Suzuki]

isEmpty is apparently bug. Thanks.

For header manipulation, they are sequential and no chance to conflict with job thread and main thread because first time it appears on main thread is when all headers are collected. I don't think putting into response header is required to be done in main thread.

[Basuke Suzuki]

We are working on async resource handle client implementation. For that, further refactoring and implementation is needed. One thing we notice about multi-threaded version is that redirect request will fail with assertion that header and contents will be dispatched asynchronously and too late to deny delivering of the body.

[Daewoong Jang]

(In reply to Basuke Suzuki from comment #5)
> isEmpty is apparently bug. Thanks.
> 

You're welcome :)

> For header manipulation, they are sequential and no chance to conflict with
> job thread and main thread because first time it appears on main thread is
> when all headers are collected. I don't think putting into response header
> is required to be done in main thread.

It could be a problem because ResourceResponseBase uses AtomicString objects. AtomicString objects exist within thread scope, and they crash when they're being deleted from another thread without completion of their own thread. This means(I suppose) we should not set strings such as m_httpStatusText from a work thread.

[Fujii Hironori]

*** Bug 174814 has been marked as a duplicate of this bug. ***

[Basuke Suzuki]

> It could be a problem because ResourceResponseBase uses AtomicString
> objects. AtomicString objects exist within thread scope, and they crash when
> they're being deleted from another thread without completion of their own
> thread. This means(I suppose) we should not set strings such as
> m_httpStatusText from a work thread.

Make sense. Now I understand why passing the ResourceResponse either by copying and moving causes crash at the end. Okay, I agree this patch is reasonable.

[WebKit Commit Bot]

Comment on attachment 316408 [details]
patch

Clearing flags on attachment: 316408

Committed r219948: <http://trac.webkit.org/changeset/219948>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Fujii Hironori]

The ref counter of String is not thread-safe.
This patch is passing a String (header) from the curl thread to main thread.
I think isolatedCopy() should be used.

[Alex Christensen]

I think you're right.

[Daewoong Jang]

(In reply to Fujii Hironori from comment #12)
> The ref counter of String is not thread-safe.
> This patch is passing a String (header) from the curl thread to main thread.
> I think isolatedCopy() should be used.

Actually, ResourceHandle is not thread-safe either. It inherits from RefCounted, not ThreadSafeRefCounted. I think RefCounted<ResourceHandle>(job) you see there should be a problem too.

I have been testing multi-threaded curl and it seems unstable for now. Sometimes WebKit crashes when deferencing a ResourceHandle, or inside of callback that called by curl.

I think it would be better to support old single-threaded curl without hurting Basuke's work. We could switch to multi-threaded curl later when it's fully tested. I'll look into it.

[Basuke Suzuki]

We apologize about those unstable state caused by the change. We almost ready to send a next patch to solve HTTP redirection handling in webcore. That introduce the thread safe middle-man object to manage background curl job and ResourceHandle, which makes ResourceHandle free from threading issues. Can you please wait for a days? Supporting old style is so much pain and we want to move forward to concentrate on new model. In the long run, it must save us very much. I really don't want to take a look inside ResourceHandleManager again.

[Daewoong Jang]

(In reply to Basuke Suzuki from comment #15)
> We apologize about those unstable state caused by the change. We almost
> ready to send a next patch to solve HTTP redirection handling in webcore.
> That introduce the thread safe middle-man object to manage background curl
> job and ResourceHandle, which makes ResourceHandle free from threading
> issues. Can you please wait for a days? Supporting old style is so much pain
> and we want to move forward to concentrate on new model. In the long run, it
> must save us very much. I really don't want to take a look inside
> ResourceHandleManager again.

You don't have to apologize. I'm really happy to see that finally curl implementation is being modernized. My thought is that if you are going to introduce a new feature to WebKit, it would be better to leave access to old feature as legacy - at least for some time. I believe WebKit does like this.

I respect your work and I won't damage it fundamentally(like bring ResourceHandleManager back to life). I'm going to patch my working copy and I hope see your patch soon. :) Thank you.