194588 2019-02-13 03:04:12 -0800 Crash in WebKit::CacheStorage::Engine::cachesRootPath 2019-02-13 11:25:01 -0800 1 1 1 Unclassified WebKit Service Workers WebKit Nightly Build Unspecified Unspecified RESOLVED FIXED InRadar P2 Normal --- 1 koivisto webkit-unassigned cdumez cgarcia commit-queue ews-watchlist ggaren rniwa youennf oldest_to_newest 1505683 0 koivisto 2019-02-13 03:04:12 -0800 0 WebKit 0x000000019f27d268 WebKit::CacheStorage::Engine::cachesRootPath(WebCore::ClientOrigin const&) + 536 (Optional.h:537) 1 WebKit 0x000000019f27d0a0 WebKit::CacheStorage::Engine::cachesRootPath(WebCore::ClientOrigin const&) + 80 (CacheStorageEngine.cpp:61) 2 WebKit 0x000000019f28dca0 WTF::Function<void (WTF::Optional<WebCore::DOMCacheEngine::Error>&&)>::CallableWrapper<WebKit::CacheStorage::Engine::readCachesFromDisk(WebCore::ClientOrigin const&, WTF::Function<void (std::experimental::fundamentals_v3::expected<std::__1::reference_wrapper<WebKit::CacheStorage::Caches>, WebCore::DOMCacheEngine::Error>&&)>&&)::$_22>::call(WTF::Optional<WebCore::DOMCacheEngine::Error>&&) + 616 (CacheStorageEngine.cpp:324) 3 WebKit 0x000000019f27d34c WebKit::CacheStorage::Engine::~Engine() + 224 (Function.h:56) 4 WebKit 0x000000019f288408 WTF::RefCounted<WebKit::CacheStorage::Engine>::deref() const + 32 (CacheStorageEngine.cpp:66) 5 WebKit 0x000000019f28c35c WTF::HashTable<PAL::SessionID, WTF::KeyValuePair<PAL::SessionID, WTF::RefPtr<WebKit::CacheStorage::Engine, WTF::DumbPtrTraits<WebKit::CacheStorage::Engine> > >, WTF::KeyValuePairKeyExtractor<WTF::KeyValuePair<PAL::SessionID, WTF::RefPtr<WebKit::CacheStorage::Engine, WTF::DumbPtrTraits<WebKit::CacheStorage::Engine> > > >, WTF::SessionIDHash, WTF::HashMap<PAL::SessionID, WTF::RefPtr<WebKit::CacheStorage::Engine, WTF::DumbPtrTraits<WebKit::CacheStorage::Engine> >, WTF::SessionIDHash, WTF::HashTraits<PAL::SessionID>, WTF::HashTraits<WTF::RefPtr<WebKit::CacheStorage::Engine, WTF::DumbPtrTraits<WebKit::CacheStorage::Engine> > > >::KeyValuePairTraits, WTF::HashTraits<PAL::SessionID> >::remove(WTF::KeyValuePair<PAL::SessionID, WTF::RefPtr<WebKit::CacheStorage::Engine, WTF::DumbPtrTraits<WebKit::CacheStorage::Engine> > >*) + 36 (RefPtr.h:44) 6 WebKit 0x000000019f27d804 WTF::HashMap<PAL::SessionID, WTF::RefPtr<WebKit::CacheStorage::Engine, WTF::DumbPtrTraits<WebKit::CacheStorage::Engine> >, WTF::SessionIDHash, WTF::HashTraits<PAL::SessionID>, WTF::HashTraits<WTF::RefPtr<WebKit::CacheStorage::Engine, WTF::DumbPtrTraits<WebKit::CacheStorage::Engine> > > >::remove(PAL::SessionID const&) + 52 (HashTable.h:1060) 7 WebKit 0x000000019f27d7a8 WebKit::CacheStorage::Engine::destroyEngine(PAL::SessionID) + 44 (CacheStorageEngine.cpp:105) 8 WebKit 0x000000019f24ca94 WebKit::NetworkProcess::destroySession(PAL::SessionID) + 52 (NetworkProcess.cpp:456) 9 WebKit 0x000000019f218e28 WebKit::NetworkProcess::didReceiveNetworkProcessMessage(IPC::Connection&, IPC::Decoder&) + 2480 (HandleMessage.h:41) 1505684 1 koivisto 2019-02-13 03:08:51 -0800 <rdar://problem/46363997> 1505687 2 361907 koivisto 2019-02-13 03:14:38 -0800 Created attachment 361907 patch 1505717 3 youennf 2019-02-13 07:44:39 -0800 cachesRootPath is called from readCachesFromDisk lambda. Shouldn't we change the readCachesFromDisk lbamda to early return if the lambda receives an error of value Error::Internal. Error::Internal is not clear in that case, maybe we could introduce an enum for initialize callback, something like { OK, WriteError, BeingDeleted }. 1505746 4 koivisto 2019-02-13 09:47:57 -0800 Possibly, but this seems like the simplest, safest fix for the branch. Not checking the existence of the salt is dangerous in any case, 1505774 5 youennf 2019-02-13 10:56:30 -0800 (In reply to Antti Koivisto from comment #4) > Possibly, but this seems like the simplest, safest fix for the branch. Not > checking the existence of the salt is dangerous in any case, OK, let's go with this patch and I'll improve it with a follow-up. 1505776 6 youennf 2019-02-13 10:59:51 -0800 (In reply to youenn fablet from comment #5) > (In reply to Antti Koivisto from comment #4) > > Possibly, but this seems like the simplest, safest fix for the branch. Not > > checking the existence of the salt is dangerous in any case, > > OK, let's go with this patch and I'll improve it with a follow-up. Hum, the potential risk with the current patch is that we proceed with the current flow, execute some callbacks and end up refine the engine even though we are in its destructor. 1505777 7 youennf 2019-02-13 10:59:57 -0800 s/refine/refing 1505779 8 youennf 2019-02-13 11:03:50 -0800 (In reply to youenn fablet from comment #6) > (In reply to youenn fablet from comment #5) > > (In reply to Antti Koivisto from comment #4) > > > Possibly, but this seems like the simplest, safest fix for the branch. Not > > > checking the existence of the salt is dangerous in any case, > > > > OK, let's go with this patch and I'll improve it with a follow-up. > > Hum, the potential risk with the current patch is that we proceed with the > current flow, execute some callbacks and end up refine the engine even > though we are in its destructor. Theoretical issue currently. 1505791 9 361907 commit-queue 2019-02-13 11:24:59 -0800 Comment on attachment 361907 patch Clearing flags on attachment: 361907 Committed r241448: <https://trac.webkit.org/changeset/241448> 1505792 10 commit-queue 2019-02-13 11:25:01 -0800 All reviewed patches have been landed. Closing bug. 361907 2019-02-13 03:14:38 -0800 2019-02-13 11:24:59 -0800 patch sw-cache-salt.patch text/plain 1348 koivisto SW5kZXg6IFNvdXJjZS9XZWJLaXQvQ2hhbmdlTG9nCj09PT09PT09PT09PT09PT09PT09PT09PT09 PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT0KLS0tIFNvdXJjZS9XZWJL aXQvQ2hhbmdlTG9nCShyZXZpc2lvbiAyNDE0MzApCisrKyBTb3VyY2UvV2ViS2l0L0NoYW5nZUxv Zwkod29ya2luZyBjb3B5KQpAQCAtMSwzICsxLDE2IEBACisyMDE5LTAyLTEzICBBbnR0aSBLb2l2 aXN0byAgPGFudHRpQGFwcGxlLmNvbT4KKworICAgICAgICBDcmFzaCBpbiBXZWJLaXQ6OkNhY2hl U3RvcmFnZTo6RW5naW5lOjpjYWNoZXNSb290UGF0aAorICAgICAgICBodHRwczovL2J1Z3Mud2Vi a2l0Lm9yZy9zaG93X2J1Zy5jZ2k/aWQ9MTk0NTg4CisgICAgICAgIDxyZGFyOi8vcHJvYmxlbS80 NjM2Mzk5Nz4KKworICAgICAgICBSZXZpZXdlZCBieSBOT0JPRFkgKE9PUFMhKS4KKworICAgICAg ICAqIE5ldHdvcmtQcm9jZXNzL2NhY2hlL0NhY2hlU3RvcmFnZUVuZ2luZS5jcHA6CisgICAgICAg IChXZWJLaXQ6OkNhY2hlU3RvcmFnZTo6RW5naW5lOjpjYWNoZXNSb290UGF0aCk6CisKKyAgICAg ICAgU2FsdCBtYXkgaGF2ZSBub3QgYmVlbiBpbml0aWFsaXplZCB5ZXQgd2hlbiB0aGUgRW5naW5l IGlzIGRlc3Ryb3llZC4KKwogMjAxOS0wMi0xMyAgUnlvc3VrZSBOaXdhICA8cm5pd2FAd2Via2l0 Lm9yZz4KIAogICAgICAgICBDcmFzaCBpbiBQYWdlOjpzZXRBY3Rpdml0eVN0YXRlIGJlY2F1c2Ug bV9wYWdlIGlzIG51bGwKSW5kZXg6IFNvdXJjZS9XZWJLaXQvTmV0d29ya1Byb2Nlc3MvY2FjaGUv Q2FjaGVTdG9yYWdlRW5naW5lLmNwcAo9PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09 PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09Ci0tLSBTb3VyY2UvV2ViS2l0L05ldHdv cmtQcm9jZXNzL2NhY2hlL0NhY2hlU3RvcmFnZUVuZ2luZS5jcHAJKHJldmlzaW9uIDI0MTI5MykK KysrIFNvdXJjZS9XZWJLaXQvTmV0d29ya1Byb2Nlc3MvY2FjaGUvQ2FjaGVTdG9yYWdlRW5naW5l LmNwcAkod29ya2luZyBjb3B5KQpAQCAtNDgsNyArNDgsNyBAQCB1c2luZyBuYW1lc3BhY2UgTmV0 d29ya0NhY2hlOwogCiBTdHJpbmcgRW5naW5lOjpjYWNoZXNSb290UGF0aChjb25zdCBXZWJDb3Jl OjpDbGllbnRPcmlnaW4mIG9yaWdpbikKIHsKLSAgICBpZiAoIXNob3VsZFBlcnNpc3QoKSkKKyAg ICBpZiAoIXNob3VsZFBlcnNpc3QoKSB8fCAhbV9zYWx0KQogICAgICAgICByZXR1cm4geyB9Owog CiAgICAgS2V5IGtleShvcmlnaW4udG9wT3JpZ2luLnRvU3RyaW5nKCksIG9yaWdpbi5jbGllbnRP cmlnaW4udG9TdHJpbmcoKSwgeyB9LCB7IH0sIHNhbHQoKSk7Cg==