183261 – IOChannel::read and IOChannel::write can destroy the completion handler in the thread used to manipulate files

RESOLVED FIXED183261

IOChannel::read and IOChannel::write can destroy the completion handler in the thread used to manipulate files

https://bugs.webkit.org/show_bug.cgi?id=183261

Summary IOChannel::read and IOChannel::write can destroy the completion handler in th...

youenn fablet

Reported 2018-03-01 14:10:08 PST

It is probably expected that the completion handler should be destroyed on the thread it is called. This is a problem in CacheStorage::Engine::readFile/writeFile since the completion handlers own references.

Attachments
Patch (2.66 KB, patch) 2018-03-01 14:12 PST, youenn fablet	no flags	Details Formatted Diff Diff
View All Add attachment proposed patch, testcase, etc.

youenn fablet

Comment 1 2018-03-01 14:10:38 PST

Thread 8 Crashed:: Dispatch queue: com.apple.libdispatch-io.deviceq.1 0 com.apple.JavaScriptCore 0x000000011856a834 WTFCrash + 36 (Assertions.cpp:271) 1 com.apple.WebKit 0x0000000102590d61 WebKit::NetworkCache::Storage::~Storage() + 81 (NetworkCacheStorage.cpp:236) 2 com.apple.WebKit 0x0000000102591375 WebKit::NetworkCache::Storage::~Storage() + 21 (NetworkCacheStorage.cpp:242) 3 com.apple.WebKit 0x00000001023a87b7 WTF::ThreadSafeRefCounted<WebKit::NetworkCache::Storage>::deref() const + 71 (ThreadSafeRefCounted.h:71) 4 com.apple.WebKit 0x00000001023a8711 void WTF::derefIfNotNull<WebKit::NetworkCache::Storage>(WebKit::NetworkCache::Storage*) + 49 (RefPtr.h:46) 5 com.apple.WebKit 0x00000001023a86d9 WTF::RefPtr<WebKit::NetworkCache::Storage, WTF::DumbPtrTraits<WebKit::NetworkCache::Storage> >::~RefPtr() + 41 (RefPtr.h:70) 6 com.apple.WebKit 0x00000001023a1035 WTF::RefPtr<WebKit::NetworkCache::Storage, WTF::DumbPtrTraits<WebKit::NetworkCache::Storage> >::~RefPtr() + 21 (RefPtr.h:70) 7 com.apple.WebKit 0x00000001023a0f64 WebKit::CacheStorage::Caches::~Caches() + 164 (CacheStorageEngineCaches.cpp:56) 8 com.apple.WebKit 0x00000001023a1095 WebKit::CacheStorage::Caches::~Caches() + 21 (CacheStorageEngineCaches.cpp:56) 9 com.apple.WebKit 0x0000000102365fe7 WTF::RefCounted<WebKit::CacheStorage::Caches>::deref() const + 71 (RefCounted.h:145) 10 com.apple.WebKit 0x00000001023744cf WTF::Ref<WebKit::CacheStorage::Caches, WTF::DumbPtrTraits<WebKit::CacheStorage::Caches> >::~Ref() + 47 (Ref.h:62) 11 com.apple.WebKit 0x0000000102373ee5 WTF::Ref<WebKit::CacheStorage::Caches, WTF::DumbPtrTraits<WebKit::CacheStorage::Caches> >::~Ref() + 21 (Ref.h:62) 12 com.apple.WebKit 0x00000001023a656c WebKit::CacheStorage::Caches::storeOrigin(WTF::Function<void (std::optional<WebCore::DOMCacheEngine::Error>&&)>&&)::$_2::~$_2() + 44 (CacheStorageEngineCaches.cpp:97) 13 com.apple.WebKit 0x00000001023a16d5 WebKit::CacheStorage::Caches::storeOrigin(WTF::Function<void (std::optional<WebCore::DOMCacheEngine::Error>&&)>&&)::$_2::~$_2() + 21 (CacheStorageEngineCaches.cpp:97) 14 com.apple.WebKit 0x00000001023ab021 WTF::Function<void (std::optional<WebCore::DOMCacheEngine::Error>&&)>::CallableWrapper<WebKit::CacheStorage::Caches::storeOrigin(WTF::Function<void (std::optional<WebCore::DOMCacheEngine::Error>&&)>&&)::$_2>::~CallableWrapper() + 49 (Function.h:91) 15 com.apple.WebKit 0x00000001023aaf25 WTF::Function<void (std::optional<WebCore::DOMCacheEngine::Error>&&)>::CallableWrapper<WebKit::CacheStorage::Caches::storeOrigin(WTF::Function<void (std::optional<WebCore::DOMCacheEngine::Error>&&)>&&)::$_2>::~CallableWrapper() + 21 (Function.h:91) 16 com.apple.WebKit 0x00000001023aaf49 WTF::Function<void (std::optional<WebCore::DOMCacheEngine::Error>&&)>::CallableWrapper<WebKit::CacheStorage::Caches::storeOrigin(WTF::Function<void (std::optional<WebCore::DOMCacheEngine::Error>&&)>&&)::$_2>::~CallableWrapper() + 25 (Function.h:91) 17 com.apple.WebKit 0x000000010236811f WTF::Function<void (std::optional<WebCore::DOMCacheEngine::Error>&&)>::~Function() + 175 (memory:2603) 18 com.apple.WebKit 0x0000000102363815 WTF::Function<void (std::optional<WebCore::DOMCacheEngine::Error>&&)>::~Function() + 21 (Forward.h:56) 19 com.apple.WebKit 0x0000000102376825 WebKit::CacheStorage::Engine::writeFile(WTF::String const&, WebKit::NetworkCache::Data&&, WTF::Function<void (std::optional<WebCore::DOMCacheEngine::Error>&&)>&&)::$_9::operator()()::'lambda'(int)::~'lambda'(int)() + 21 (CacheStorageEngine.cpp:282) 20 com.apple.WebKit 0x0000000102375ea5 WebKit::CacheStorage::Engine::writeFile(WTF::String const&, WebKit::NetworkCache::Data&&, WTF::Function<void (std::optional<WebCore::DOMCacheEngine::Error>&&)>&&)::$_9::operator()()::'lambda'(int)::~'lambda'(int)() + 21 (CacheStorageEngine.cpp:282) 21 com.apple.WebKit 0x00000001023765d1 WTF::Function<void (int)>::CallableWrapper<WebKit::CacheStorage::Engine::writeFile(WTF::String const&, WebKit::NetworkCache::Data&&, WTF::Function<void (std::optional<WebCore::DOMCacheEngine::Error>&&)>&&)::$_9::operator()()::'lambda'(int)>::~CallableWrapper() + 49 (Function.h:91) 22 com.apple.WebKit 0x0000000102376485 WTF::Function<void (int)>::CallableWrapper<WebKit::CacheStorage::Engine::writeFile(WTF::String const&, WebKit::NetworkCache::Data&&, WTF::Function<void (std::optional<WebCore::DOMCacheEngine::Error>&&)>&&)::$_9::operator()()::'lambda'(int)>::~CallableWrapper() + 21 (Function.h:91) 23 com.apple.WebKit 0x00000001023764a9 WTF::Function<void (int)>::CallableWrapper<WebKit::CacheStorage::Engine::writeFile(WTF::String const&, WebKit::NetworkCache::Data&&, WTF::Function<void (std::optional<WebCore::DOMCacheEngine::Error>&&)>&&)::$_9::operator()()::'lambda'(int)>::~CallableWrapper() + 25 (Function.h:91) 24 com.apple.WebKit 0x00000001023767ff WTF::Function<void (int)>::~Function() + 175 (memory:2603) 25 com.apple.WebKit 0x0000000102375e85 WTF::Function<void (int)>::~Function() + 21 (Forward.h:56) 26 com.apple.WebKit 0x00000001025589c3 WebKit::NetworkCache::IOChannel::write(unsigned long, WebKit::NetworkCache::Data const&, WTF::WorkQueue*, WTF::Function<void (int)>&&)::$_2::~$_2() + 35 (NetworkCacheIOChannelCocoa.mm:115) 27 com.apple.WebKit 0x00000001025586d5 WebKit::NetworkCache::IOChannel::write(unsigned long, WebKit::NetworkCache::Data const&, WTF::WorkQueue*, WTF::Function<void (int)>&&)::$_2::~$_2() + 21 (NetworkCacheIOChannelCocoa.mm:115) 28 com.apple.WebKit 0x0000000102559230 WTF::BlockPtr<void (bool, NSObject<OS_dispatch_data>*, int)> WTF::BlockPtr<void (bool, NSObject<OS_dispatch_data>*, int)>::fromCallable<WebKit::NetworkCache::IOChannel::write(unsigned long, WebKit::NetworkCache::Data const&, WTF::WorkQueue*, WTF::Function<void (int)>&&)::$_2>(WebKit::NetworkCache::IOChannel::write(unsigned long, WebKit::NetworkCache::Data const&, WTF::WorkQueue*, WTF::Function<void (int)>&&)::$_2)::'lambda'(void const*)::operator()(void const*) const + 32 (BlockPtr.h:79) 29 com.apple.WebKit 0x0000000102559208 WTF::BlockPtr<void (bool, NSObject<OS_dispatch_data>*, int)> WTF::BlockPtr<void (bool, NSObject<OS_dispatch_data>*, int)>::fromCallable<WebKit::NetworkCache::IOChannel::write(unsigned long, WebKit::NetworkCache::Data const&, WTF::WorkQueue*, WTF::Function<void (int)>&&)::$_2>(WebKit::NetworkCache::IOChannel::write(unsigned long, WebKit::NetworkCache::Data const&, WTF::WorkQueue*, WTF::Function<void (int)>&&)::$_2)::'lambda'(void const*)::__invoke(void const*) + 24 (BlockPtr.h:77) 30 libsystem_blocks.dylib 0x00007fff57d1199d _Block_release + 111 31 libdispatch.dylib 0x00007fff57c8acd0 _dispatch_dispose + 61 32 libdispatch.dylib 0x00007fff57c915ca _dispatch_call_block_and_release + 12 33 libdispatch.dylib 0x00007fff57c89d88 _dispatch_client_callout + 8 34 libdispatch.dylib 0x00007fff57c9e209 _dispatch_queue_serial_drain + 635 35 libdispatch.dylib 0x00007fff57c91136 _dispatch_queue_invoke + 373 36 libdispatch.dylib 0x00007fff57c9eeff _dispatch_root_queue_drain_deferred_wlh + 332 37 libdispatch.dylib 0x00007fff57ca2d13 _dispatch_workloop_worker_thread + 880 38 libsystem_pthread.dylib 0x00007fff57fce033 _pthread_wqthread + 980 39 libsystem_pthread.dylib 0x00007fff57fcdc4d start_wqthread + 13

youenn fablet

Comment 2 2018-03-01 14:12:12 PST

Created attachment 334852 [details] Patch

Michael Catanzaro

Comment 3 2018-03-01 16:31:38 PST

Comment on attachment 334852 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=334852&action=review > Source/WebKit/NetworkProcess/cache/NetworkCacheIOChannelCocoa.mm:106 > + auto callback = WTFMove(completionHandler); > + callback(data, error); ...this seems like quite a footgun. Are you sure that the problem is really that the callback is being destroyed on the wrong thread, and not that the callback is being *executed* on the wrong thread? (Reasoning about threaded code is hard, so I reserve the right to be wrong. ;) It looks like dispatch_io_write and dispatch_io_read execute their callbacks on an internal secondary thread, correct? That seems weird and risky to be, but maybe that's a convention for macOS APIs? Wouldn't the expected behavior be for completionHandler to be called on the same thread that called IOChannel::read (or IOChannel::write)? That's the convention we follow for threadsafe objects in GLib. (More typically, our objects that use secondary threads are not themselves intended to be threadsafe, and we instead ensure the callback is executed on the thread where the object was created. But NetworkCacheIOChannel is ThreadSafeRefCounted, so that wouldn't be appropriate here.)

youenn fablet

Comment 4 2018-03-01 16:47:43 PST

(In reply to Michael Catanzaro from comment #3) > Comment on attachment 334852 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=334852&action=review > > > Source/WebKit/NetworkProcess/cache/NetworkCacheIOChannelCocoa.mm:106 > > + auto callback = WTFMove(completionHandler); > > + callback(data, error); > > ...this seems like quite a footgun. Are you sure that the problem is really > that the callback is being destroyed on the wrong thread, and not that the > callback is being *executed* on the wrong thread? (Reasoning about threaded > code is hard, so I reserve the right to be wrong. ;) In CacheStorage code path, there is an ASSERT(RunLoop::isMain()) so either the callback is not executed at all or it is executed in the right thread. > > It looks like dispatch_io_write and dispatch_io_read execute their callbacks > on an internal secondary thread, correct? That seems weird and risky to be, > but maybe that's a convention for macOS APIs? I am not familiar with dispatch_io_write and dispatch_io_read, hopefully Antti will know more. It seems we are passing the thread on which the callback should be called as fourth parameter. Since this callback may be called several times, it might be that it is supposed to be destroyed on the thread that is calling dispatch_io_write and dispatch_io_read. But IOChannel::read/write expose a one-time callback so it makes sense to me to execute and destroy the callback on the same thread.

WebKit Commit Bot

Comment 5 2018-03-02 08:52:47 PST

Comment on attachment 334852 [details] Patch Clearing flags on attachment: 334852 Committed r229172: <https://trac.webkit.org/changeset/229172>

WebKit Commit Bot

Comment 6 2018-03-02 08:52:49 PST

All reviewed patches have been landed. Closing bug.

Radar WebKit Bug Importer

Comment 7 2018-03-02 08:53:23 PST

<rdar://problem/38069783>

Note You need to log in before you can comment on or make changes to this bug.

Status RESOLVED

Resolution FIXED

Priority P2

Severity Normal

Classification Unclassified

Version WebKit Nightly Build

Hardware Unspecified

OS Unspecified

Product WebKit

Component Service Workers

Assignee

youenn fablet

Reported

2018-03-01 14:10 PST

Modified

2018-03-02 08:53 PST History

CC List

7 users Show

URL

Keywords InRadar

Depends on

Blocks