Bug 174845

Summary: [Curl] Bug fix after r219606
Product: WebKit Reporter: Daewoong Jang <daewoong.jang>
Component: WebCore Misc.Assignee: Daewoong Jang <daewoong.jang>
Status: RESOLVED FIXED    
Severity: Normal CC: achristensen, Basuke.Suzuki, buildbot, commit-queue, galpeter, Hironori.Fujii
Priority: P2    
Version: WebKit Nightly Build   
Hardware: Unspecified   
OS: Unspecified   
Attachments:
Description Flags
patch
none
Archive of layout-test-results from ews123 for ios-simulator-wk2 none

Description Daewoong Jang 2017-07-25 17:09:30 PDT
Fixes bugs introduced by r219606. However I believe there are still issues related to multi-threading, which needs to be solved.
Comment 1 Daewoong Jang 2017-07-25 17:12:08 PDT
Related bug: https://bugs.webkit.org/show_bug.cgi?id=173991
Comment 2 Daewoong Jang 2017-07-25 17:14:40 PDT
Created attachment 316408 [details]
patch
Comment 3 Build Bot 2017-07-25 18:41:02 PDT
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
Comment 4 Build Bot 2017-07-25 18:41:04 PDT
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
Comment 5 Basuke Suzuki 2017-07-25 21:45:53 PDT
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.
Comment 6 Basuke Suzuki 2017-07-25 21:49:48 PDT
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.
Comment 7 Daewoong Jang 2017-07-25 22:01:13 PDT
(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.
Comment 8 Fujii Hironori 2017-07-26 02:02:45 PDT
*** Bug 174814 has been marked as a duplicate of this bug. ***
Comment 9 Basuke Suzuki 2017-07-26 07:40:11 PDT
> 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.
Comment 10 WebKit Commit Bot 2017-07-26 08:29:33 PDT
Comment on attachment 316408 [details]
patch

Clearing flags on attachment: 316408

Committed r219948: <http://trac.webkit.org/changeset/219948>
Comment 11 WebKit Commit Bot 2017-07-26 08:29:35 PDT
All reviewed patches have been landed.  Closing bug.
Comment 12 Fujii Hironori 2017-07-26 16:41:43 PDT
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.
Comment 13 Alex Christensen 2017-07-26 16:42:28 PDT
I think you're right.
Comment 14 Daewoong Jang 2017-07-27 01:58:42 PDT
(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.
Comment 15 Basuke Suzuki 2017-07-27 11:16:27 PDT
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.
Comment 16 Daewoong Jang 2017-07-27 16:13:18 PDT
(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.