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

• Component: Service Workers
• Status: RESOLVED FIXED

[youenn fablet]

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.

[youenn fablet]

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]

Created attachment 334852 [details]
Patch

[Michael Catanzaro]

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]

(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 on attachment 334852 [details]
Patch

Clearing flags on attachment: 334852

Committed r229172: <https://trac.webkit.org/changeset/229172>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Radar WebKit Bug Importer]

<rdar://problem/38069783>