Bug 181401 – ASSERTION FAILED: m_ptr under WebKit::CacheStorage::Caches::writeRecord

• Component: Service Workers
• Status: RESOLVED FIXED

[Matt Lewis]

Created attachment 330732 [details]
Crash Log

imported/w3c/web-platform-tests/service-workers/service-worker/fetch-event-within-sw.https.html is Crashing on iOS Simulator WK2 Debug.

http://webkit-test-results.webkit.org/dashboards/flakiness_dashboard.html#showAllRuns=true&tests=imported%2Fw3c%2Fweb-platform-tests%2Fservice-workers%2Fservice-worker%2Ffetch-event-within-sw.https.html

build:
https://build-safari.apple.com/results/Trunk-Tigris-iOS-Simulator-Debug-WK2-Tests/r226511_30f11dba226cf241d62ceb606dd9ece8733d1aba%20(4413)/results.html

STDERR

ASSERTION FAILED: m_ptr
/Volumes/Data/slave/trunk-tigris-ios-sim-debug-archive/build/OpenSource/WebKitBuild/Debug-iphonesimulator/usr/local/include/wtf/RefPtr.h(79) : T &WTF::RefPtr<WebKit::NetworkCache::Storage, WTF::DumbPtrTraits<WebKit::NetworkCache::Storage> >::operator*() const [T = WebKit::NetworkCache::Storage, PtrTraits = WTF::DumbPtrTraits<WebKit::NetworkCache::Storage>]
1   0x1176cffad WTFCrash
2   0x10abcb016 WTF::RefPtr<WebKit::NetworkCache::Storage, WTF::DumbPtrTraits<WebKit::NetworkCache::Storage> >::operator*() const
3   0x10abcd194 WebKit::CacheStorage::Caches::writeRecord(WebKit::CacheStorage::Cache const&, WebKit::CacheStorage::RecordInformation const&, WebCore::DOMCacheEngine::Record&&, unsigned long long, WTF::Function<void (std::optional<WebCore::DOMCacheEngine::Error>&&)>&&)
4   0x10abad335 WebKit::CacheStorage::Cache::writeRecordToDisk(WebKit::CacheStorage::RecordInformation const&, WebCore::DOMCacheEngine::Record&&, WTF::Ref<WebKit::CacheStorage::AsynchronousPutTaskCounter, WTF::DumbPtrTraits<WebKit::CacheStorage::AsynchronousPutTaskCounter> >&&, unsigned long long)
5   0x10abace81 WebKit::CacheStorage::Cache::storeRecords(WTF::Vector<WebCore::DOMCacheEngine::Record, 0ul, WTF::CrashOnOverflow, 16ul, WTF::FastMalloc>&&, WTF::Function<void (std::experimental::fundamentals_v3::expected<WTF::Vector<unsigned long long, 0ul, WTF::CrashOnOverflow, 16ul, WTF::FastMalloc>, WebCore::DOMCacheEngine::Error>&&)>&&)
6   0x10abad794 WebKit::CacheStorage::Cache::put(WTF::Vector<WebCore::DOMCacheEngine::Record, 0ul, WTF::CrashOnOverflow, 16ul, WTF::FastMalloc>&&, WTF::Function<void (std::experimental::fundamentals_v3::expected<WTF::Vector<unsigned long long, 0ul, WTF::CrashOnOverflow, 16ul, WTF::FastMalloc>, WebCore::DOMCacheEngine::Error>&&)>&&)
7   0x10ab9a844 WebKit::CacheStorage::Engine::putRecords(unsigned long long, WTF::Vector<WebCore::DOMCacheEngine::Record, 0ul, WTF::CrashOnOverflow, 16ul, WTF::FastMalloc>&&, WTF::Function<void (std::experimental::fundamentals_v3::expected<WTF::Vector<unsigned long long, 0ul, WTF::CrashOnOverflow, 16ul, WTF::FastMalloc>, WebCore::DOMCacheEngine::Error>&&)>&&)::$_4::operator()(std::experimental::fundamentals_v3::expected<std::__1::reference_wrapper<WebKit::CacheStorage::Cache>, WebCore::DOMCacheEngine::Error>&&)
8   0x10ab9a6d4 WTF::Function<void (std::experimental::fundamentals_v3::expected<std::__1::reference_wrapper<WebKit::CacheStorage::Cache>, WebCore::DOMCacheEngine::Error>&&)>::CallableWrapper<WebKit::CacheStorage::Engine::putRecords(unsigned long long, WTF::Vector<WebCore::DOMCacheEngine::Record, 0ul, WTF::CrashOnOverflow, 16ul, WTF::FastMalloc>&&, WTF::Function<void (std::experimental::fundamentals_v3::expected<WTF::Vector<unsigned long long, 0ul, WTF::CrashOnOverflow, 16ul, WTF::FastMalloc>, WebCore::DOMCacheEngine::Error>&&)>&&)::$_4>::call(std::experimental::fundamentals_v3::expected<std::__1::reference_wrapper<WebKit::CacheStorage::Cache>, WebCore::DOMCacheEngine::Error>&&)
9   0x10ab8d6fe WTF::Function<void (std::experimental::fundamentals_v3::expected<std::__1::reference_wrapper<WebKit::CacheStorage::Cache>, WebCore::DOMCacheEngine::Error>&&)>::operator()(std::experimental::fundamentals_v3::expected<std::__1::reference_wrapper<WebKit::CacheStorage::Cache>, WebCore::DOMCacheEngine::Error>&&) const
10  0x10ab8cc7c WebKit::CacheStorage::Engine::readCache(unsigned long long, WTF::Function<void (std::experimental::fundamentals_v3::expected<std::__1::reference_wrapper<WebKit::CacheStorage::Cache>, WebCore::DOMCacheEngine::Error>&&)>&&)
11  0x10ab8cdd7 WebKit::CacheStorage::Engine::putRecords(unsigned long long, WTF::Vector<WebCore::DOMCacheEngine::Record, 0ul, WTF::CrashOnOverflow, 16ul, WTF::FastMalloc>&&, WTF::Function<void (std::experimental::fundamentals_v3::expected<WTF::Vector<unsigned long long, 0ul, WTF::CrashOnOverflow, 16ul, WTF::FastMalloc>, WebCore::DOMCacheEngine::Error>&&)>&&)
12  0x10abdd5e7 WebKit::CacheStorageEngineConnection::putRecords(PAL::SessionID, unsigned long long, unsigned long long, WTF::Vector<WebCore::DOMCacheEngine::Record, 0ul, WTF::CrashOnOverflow, 16ul, WTF::FastMalloc>&&)
13  0x10abefe2a void IPC::callMemberFunctionImpl<WebKit::CacheStorageEngineConnection, void (WebKit::CacheStorageEngineConnection::*)(PAL::SessionID, unsigned long long, unsigned long long, WTF::Vector<WebCore::DOMCacheEngine::Record, 0ul, WTF::CrashOnOverflow, 16ul, WTF::FastMalloc>&&), std::__1::tuple<PAL::SessionID, unsigned long long, unsigned long long, WTF::Vector<WebCore::DOMCacheEngine::Record, 0ul, WTF::CrashOnOverflow, 16ul, WTF::FastMalloc> >, 0ul, 1ul, 2ul, 3ul>(WebKit::CacheStorageEngineConnection*, void (WebKit::CacheStorageEngineConnection::*)(PAL::SessionID, unsigned long long, unsigned long long, WTF::Vector<WebCore::DOMCacheEngine::Record, 0ul, WTF::CrashOnOverflow, 16ul, WTF::FastMalloc>&&), std::__1::tuple<PAL::SessionID, unsigned long long, unsigned long long, WTF::Vector<WebCore::DOMCacheEngine::Record, 0ul, WTF::CrashOnOverflow, 16ul, WTF::FastMalloc> >&&, std::__1::integer_sequence<unsigned long, 0ul, 1ul, 2ul, 3ul>)
14  0x10abeeba0 void IPC::callMemberFunction<WebKit::CacheStorageEngineConnection, void (WebKit::CacheStorageEngineConnection::*)(PAL::SessionID, unsigned long long, unsigned long long, WTF::Vector<WebCore::DOMCacheEngine::Record, 0ul, WTF::CrashOnOverflow, 16ul, WTF::FastMalloc>&&), std::__1::tuple<PAL::SessionID, unsigned long long, unsigned long long, WTF::Vector<WebCore::DOMCacheEngine::Record, 0ul, WTF::CrashOnOverflow, 16ul, WTF::FastMalloc> >, std::__1::integer_sequence<unsigned long, 0ul, 1ul, 2ul, 3ul> >(std::__1::tuple<PAL::SessionID, unsigned long long, unsigned long long, WTF::Vector<WebCore::DOMCacheEngine::Record, 0ul, WTF::CrashOnOverflow, 16ul, WTF::FastMalloc> >&&, WebKit::CacheStorageEngineConnection*, void (WebKit::CacheStorageEngineConnection::*)(PAL::SessionID, unsigned long long, unsigned long long, WTF::Vector<WebCore::DOMCacheEngine::Record, 0ul, WTF::CrashOnOverflow, 16ul, WTF::FastMalloc>&&))
15  0x10abeb73c void IPC::handleMessage<Messages::CacheStorageEngineConnection::PutRecords, WebKit::CacheStorageEngineConnection, void (WebKit::CacheStorageEngineConnection::*)(PAL::SessionID, unsigned long long, unsigned long long, WTF::Vector<WebCore::DOMCacheEngine::Record, 0ul, WTF::CrashOnOverflow, 16ul, WTF::FastMalloc>&&)>(IPC::Decoder&, WebKit::CacheStorageEngineConnection*, void (WebKit::CacheStorageEngineConnection::*)(PAL::SessionID, unsigned long long, unsigned long long, WTF::Vector<WebCore::DOMCacheEngine::Record, 0ul, WTF::CrashOnOverflow, 16ul, WTF::FastMalloc>&&))
16  0x10abea69a WebKit::CacheStorageEngineConnection::didReceiveMessage(IPC::Connection&, IPC::Decoder&)
17  0x10ade4e56 WebKit::NetworkConnectionToWebProcess::didReceiveMessage(IPC::Connection&, IPC::Decoder&)
18  0x10ac02bc3 IPC::Connection::dispatchMessage(IPC::Decoder&)
19  0x10abf8718 IPC::Connection::dispatchMessage(std::__1::unique_ptr<IPC::Decoder, std::__1::default_delete<IPC::Decoder> >)
20  0x10ac031ca IPC::Connection::dispatchOneMessage()
21  0x10ac1b00d IPC::Connection::enqueueIncomingMessage(std::__1::unique_ptr<IPC::Decoder, std::__1::default_delete<IPC::Decoder> >)::$_14::operator()()
22  0x10ac1af69 WTF::Function<void ()>::CallableWrapper<IPC::Connection::enqueueIncomingMessage(std::__1::unique_ptr<IPC::Decoder, std::__1::default_delete<IPC::Decoder> >)::$_14>::call()
23  0x1176ec24b WTF::Function<void ()>::operator()() const
24  0x1177316a3 WTF::RunLoop::performWork()
25  0x117731f44 WTF::RunLoop::performWork(void*)
26  0x10f0d02b1 __CFRUNLOOP_IS_CALLING_OUT_TO_A_SOURCE0_PERFORM_FUNCTION__
27  0x10f16fd31 __CFRunLoopDoSource0
28  0x10f0b4c19 __CFRunLoopDoSources0
29  0x10f0b41ff __CFRunLoopRun
30  0x10f0b3a89 CFRunLoopRunSpecific
31  0x10a4c6e5e -[NSRunLoop(NSRunLoop) runMode:beforeDate:]
LEAK: 1 WebPageProxy

[Radar WebKit Bug Importer]

<rdar://problem/36379022>

[youenn fablet]

Created attachment 331113 [details]
Patch

[Chris Dumez]

Comment on attachment 331113 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=331113&action=review

> Source/WebKit/NetworkProcess/cache/CacheStorageEngineCaches.cpp:182
> +            m_storage = WTFMove(protectedStorage);

If somebody cleared m_storage while we were traversing, it seems wrong to reset m_storage to its original value once the traversal is done.
Wouldn't it make more sense to have the lambda be a no-op if m_storage is null?

[youenn fablet]

(In reply to Chris Dumez from comment #3)
> Comment on attachment 331113 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=331113&action=review
> 
> > Source/WebKit/NetworkProcess/cache/CacheStorageEngineCaches.cpp:182
> > +            m_storage = WTFMove(protectedStorage);
> 
> If somebody cleared m_storage while we were traversing, it seems wrong to
> reset m_storage to its original value once the traversal is done.
> Wouldn't it make more sense to have the lambda be a no-op if m_storage is
> null?

I initially thought that we could return an error in that case.
But this would probably make some tests flaky, hence why I ended up choosing this silent approach.

We are clearing m_storage in clearMemoryRepresentation and are setting m_isInitizalized to false, so that we will be creating m_storage again whenever needed.

Ideally, we should retraverse m_storage to compute m_size correctly in the case of this patch. But this seems complex to the issue of having a wrong m_size value.
m_size should probably be set to zero while it will probably not be.
But it seems better to have a bigger m_size than a smaller m_size.

[youenn fablet]

An alternative might be to update clearMemoryRepresentation and not nullify m_storage if not initialized yet.

[youenn fablet]

Created attachment 331253 [details]
Patch

[Darin Adler]

Comment on attachment 331253 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=331253&action=review

> Source/WebKit/ChangeLog:16
> +        clearMemoryRepresentation is about cleaning the in-memory information of the caches, and
> +        nullifying m_storage is a memory consumption optimization.

The comment doesn’t make this clear and I thin it needs to. I think there are three points we want to make clear in the code, not just the change log, although I hope we can still do it with a fairly short comment:

    1) this "m_storage" part of cleaning is optional and is about memory use optimization, not required for correct behavior

    2) this "m_storage" part of cleaning must not be done during initialization -- this is what the comment currently tries to say, but saying "let's keep it around" is not right; we need to say "we must keep it around"

    3) this "m_storage" part of cleaning is safe to do any time *other* than initialization

So I suggest you improve the comment -- ideally we cleverly come up with a short comment that makes all three of these things clear. Also possibly might want to add a blank line to make this code a separate paragraph so it’s not grouped as closely with code implementing things that are required because of point (1).

[youenn fablet]

Created attachment 331323 [details]
Patch for landing

[youenn fablet]

(In reply to youenn fablet from comment #8)
> Created attachment 331323 [details]
> Patch for landing

I made it clearer that clearMemoryRepresentation is a no-op when a caches is not initialized by returning early in such a case.

[WebKit Commit Bot]

Comment on attachment 331323 [details]
Patch for landing

Clearing flags on attachment: 331323

Committed r226946: <https://trac.webkit.org/changeset/226946>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.