Bug 194588 – Crash in WebKit::CacheStorage::Engine::cachesRootPath

• Status: RESOLVED
• Resolution: FIXED
• Component: Service Workers

[Antti Koivisto]

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)

[Antti Koivisto]

<rdar://problem/46363997>

[Antti Koivisto]

Created attachment 361907 [details]
patch

[youenn fablet]

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

[Antti Koivisto]

Possibly, but this seems like the simplest, safest fix for the branch. Not checking the existence of the salt is dangerous in any case,

[youenn fablet]

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

[youenn fablet]

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

[youenn fablet]

s/refine/refing

[youenn fablet]

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

[WebKit Commit Bot]

Comment on attachment 361907 [details]
patch

Clearing flags on attachment: 361907

Committed r241448: <https://trac.webkit.org/changeset/241448>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.