183261 2018-03-01 14:10:08 -0800 IOChannel::read and IOChannel::write can destroy the completion handler in the thread used to manipulate files 2018-03-02 08:53:23 -0800 1 1 1 Unclassified WebKit Service Workers WebKit Nightly Build Unspecified Unspecified RESOLVED FIXED InRadar P2 Normal --- 1 youennf youennf cdumez cgarcia commit-queue ews-watchlist koivisto mcatanzaro webkit-bug-importer oldest_to_newest 1403036 0 youennf 2018-03-01 14:10:08 -0800 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. 1403037 1 youennf 2018-03-01 14:10:38 -0800 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 1403038 2 334852 youennf 2018-03-01 14:12:12 -0800 Created attachment 334852 Patch 1403097 3 334852 mcatanzaro 2018-03-01 16:31:38 -0800 Comment on attachment 334852 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.) 1403108 4 youennf 2018-03-01 16:47:43 -0800 (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. 1403233 5 334852 commit-queue 2018-03-02 08:52:47 -0800 Comment on attachment 334852 Patch Clearing flags on attachment: 334852 Committed r229172: <https://trac.webkit.org/changeset/229172> 1403234 6 commit-queue 2018-03-02 08:52:49 -0800 All reviewed patches have been landed. Closing bug. 1403235 7 webkit-bug-importer 2018-03-02 08:53:23 -0800 <rdar://problem/38069783> 334852 2018-03-01 14:12:12 -0800 2018-03-02 08:52:47 -0800 Patch bug-183261-20180301141211.patch text/plain 2722 youennf U3VidmVyc2lvbiBSZXZpc2lvbjogMjI5MDQ5CmRpZmYgLS1naXQgYS9Tb3VyY2UvV2ViS2l0L0No YW5nZUxvZyBiL1NvdXJjZS9XZWJLaXQvQ2hhbmdlTG9nCmluZGV4IDI2NjY2OGM3NTU3YzM4OWMy YWVkYmQ0MDk0N2RiZWUxYzYzYjQwYTQuLmRhZWUyMWRkYzhhMTFjM2I2Nzc3NWQwYTdiNjVjMzVj M2FiMmEzOGUgMTAwNjQ0Ci0tLSBhL1NvdXJjZS9XZWJLaXQvQ2hhbmdlTG9nCisrKyBiL1NvdXJj ZS9XZWJLaXQvQ2hhbmdlTG9nCkBAIC0xLDMgKzEsMTYgQEAKKzIwMTgtMDMtMDEgIFlvdWVubiBG YWJsZXQgIDx5b3Vlbm5AYXBwbGUuY29tPgorCisgICAgICAgIElPQ2hhbm5lbDo6cmVhZCBhbmQg SU9DaGFubmVsOjp3cml0ZSBjYW4gZGVzdHJveSB0aGUgY29tcGxldGlvbiBoYW5kbGVyIGluIHRo ZSB0aHJlYWQgdXNlZCB0byBtYW5pcHVsYXRlIHRocmVhZAorICAgICAgICBodHRwczovL2J1Z3Mu d2Via2l0Lm9yZy9zaG93X2J1Zy5jZ2k/aWQ9MTgzMjYxCisKKyAgICAgICAgUmV2aWV3ZWQgYnkg Tk9CT0RZIChPT1BTISkuCisKKyAgICAgICAgTW92aW5nIHRoZSBjb21wbGV0aW9uIGhhbmRsZXIg d2hlbiBiZWluZyBjYWxsZWQgc28gdGhhdCBpdCBnZXRzIGRlc3R0cm95ZWQgaW4gdGhlIHRocmVh ZCBpdCBpcyBjYWxsZWQuCisKKyAgICAgICAgKiBOZXR3b3JrUHJvY2Vzcy9jYWNoZS9OZXR3b3Jr Q2FjaGVJT0NoYW5uZWxDb2NvYS5tbToKKyAgICAgICAgKFdlYktpdDo6TmV0d29ya0NhY2hlOjpJ T0NoYW5uZWw6OnJlYWQpOgorICAgICAgICAoV2ViS2l0OjpOZXR3b3JrQ2FjaGU6OklPQ2hhbm5l bDo6d3JpdGUpOgorCiAyMDE4LTAzLTAxICBZb3Vlbm4gRmFibGV0ICA8eW91ZW5uQGFwcGxlLmNv bT4KIAogICAgICAgICBMYXlvdXRUZXN0IGltcG9ydGVkL3czYy93ZWItcGxhdGZvcm0tdGVzdHMv c2VydmljZS13b3JrZXJzL3NlcnZpY2Utd29ya2VyL2ZldGNoLWV2ZW50LXdpdGhpbi1zdy5odHRw cy5odG1sIGlzIGEgZmxha3kgZmFpbHVyZQpkaWZmIC0tZ2l0IGEvU291cmNlL1dlYktpdC9OZXR3 b3JrUHJvY2Vzcy9jYWNoZS9OZXR3b3JrQ2FjaGVJT0NoYW5uZWxDb2NvYS5tbSBiL1NvdXJjZS9X ZWJLaXQvTmV0d29ya1Byb2Nlc3MvY2FjaGUvTmV0d29ya0NhY2hlSU9DaGFubmVsQ29jb2EubW0K aW5kZXggNzZiMTRlMzM0OGVjNWM2ZmZjZjIyYjhlZGZjOWYzY2Y4NjRhN2ExOS4uOWRjZDY5YjJl ZWQzMWZjMTE0ZjcxMWFkNGVjM2YxNGIxNTUyMmM0MiAxMDA2NDQKLS0tIGEvU291cmNlL1dlYktp dC9OZXR3b3JrUHJvY2Vzcy9jYWNoZS9OZXR3b3JrQ2FjaGVJT0NoYW5uZWxDb2NvYS5tbQorKysg Yi9Tb3VyY2UvV2ViS2l0L05ldHdvcmtQcm9jZXNzL2NhY2hlL05ldHdvcmtDYWNoZUlPQ2hhbm5l bENvY29hLm1tCkBAIC0xMDIsNyArMTAyLDggQEAgdm9pZCBJT0NoYW5uZWw6OnJlYWQoc2l6ZV90 IG9mZnNldCwgc2l6ZV90IHNpemUsIFdvcmtRdWV1ZSogcXVldWUsIEZ1bmN0aW9uPHZvaWQKICAg ICAgICAgICAgIHJldHVybjsKICAgICAgICAgRGlzcGF0Y2hQdHI8ZGlzcGF0Y2hfZGF0YV90PiBm aWxlRGF0YVB0cihmaWxlRGF0YSk7CiAgICAgICAgIERhdGEgZGF0YShmaWxlRGF0YVB0cik7Ci0g ICAgICAgIGNvbXBsZXRpb25IYW5kbGVyKGRhdGEsIGVycm9yKTsKKyAgICAgICAgYXV0byBjYWxs YmFjayA9IFdURk1vdmUoY29tcGxldGlvbkhhbmRsZXIpOworICAgICAgICBjYWxsYmFjayhkYXRh LCBlcnJvcik7CiAgICAgICAgIGRpZENhbGxDb21wbGV0aW9uSGFuZGxlciA9IHRydWU7CiAgICAg fSkuZ2V0KCkpOwogfQpAQCAtMTEyLDkgKzExMywxMCBAQCB2b2lkIElPQ2hhbm5lbDo6d3JpdGUo c2l6ZV90IG9mZnNldCwgY29uc3QgRGF0YSYgZGF0YSwgV29ya1F1ZXVlKiBxdWV1ZSwgRnVuY3Rp bwogICAgIFJlZlB0cjxJT0NoYW5uZWw+IGNoYW5uZWwodGhpcyk7CiAgICAgYXV0byBkaXNwYXRj aERhdGEgPSBkYXRhLmRpc3BhdGNoRGF0YSgpOwogICAgIGF1dG8gZGlzcGF0Y2hRdWV1ZSA9IHF1 ZXVlID8gcXVldWUtPmRpc3BhdGNoUXVldWUoKSA6IGRpc3BhdGNoX2dldF9tYWluX3F1ZXVlKCk7 Ci0gICAgZGlzcGF0Y2hfaW9fd3JpdGUobV9kaXNwYXRjaElPLmdldCgpLCBvZmZzZXQsIGRpc3Bh dGNoRGF0YSwgZGlzcGF0Y2hRdWV1ZSwgQmxvY2tQdHI8dm9pZChib29sLCBkaXNwYXRjaF9kYXRh X3QsIGludCk+Ojpmcm9tQ2FsbGFibGUoW2NoYW5uZWwsIGNvbXBsZXRpb25IYW5kbGVyID0gV1RG TW92ZShjb21wbGV0aW9uSGFuZGxlcildKGJvb2wgZG9uZSwgZGlzcGF0Y2hfZGF0YV90IGZpbGVE YXRhLCBpbnQgZXJyb3IpIHsKKyAgICBkaXNwYXRjaF9pb193cml0ZShtX2Rpc3BhdGNoSU8uZ2V0 KCksIG9mZnNldCwgZGlzcGF0Y2hEYXRhLCBkaXNwYXRjaFF1ZXVlLCBCbG9ja1B0cjx2b2lkKGJv b2wsIGRpc3BhdGNoX2RhdGFfdCwgaW50KT46OmZyb21DYWxsYWJsZShbY2hhbm5lbCwgY29tcGxl dGlvbkhhbmRsZXIgPSBXVEZNb3ZlKGNvbXBsZXRpb25IYW5kbGVyKV0oYm9vbCBkb25lLCBkaXNw YXRjaF9kYXRhX3QgZmlsZURhdGEsIGludCBlcnJvcikgbXV0YWJsZSB7CiAgICAgICAgIEFTU0VS VF9VTlVTRUQoZG9uZSwgZG9uZSk7Ci0gICAgICAgIGNvbXBsZXRpb25IYW5kbGVyKGVycm9yKTsK KyAgICAgICAgYXV0byBjYWxsYmFjayA9IFdURk1vdmUoY29tcGxldGlvbkhhbmRsZXIpOworICAg ICAgICBjYWxsYmFjayhlcnJvcik7CiAgICAgfSkuZ2V0KCkpOwogfQogCg==