Bug 174845

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

Created attachment 316408 [details] patch

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

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

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.

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.

(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.

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

> 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 on attachment 316408 [details] patch Clearing flags on attachment: 316408 Committed r219948: <http://trac.webkit.org/changeset/219948>

All reviewed patches have been landed. Closing bug.

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.

I think you're right.

(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.

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.

(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.