Bug 179037 – Crash in: com.apple.WebKit: WebKit::CacheStorage::Caches::initializeSize(WTF::Function<void (std::optional<WebCore::DOMCacheEngine::Error>&&)>&&) + 30 (CacheStorageEngineCaches.cpp:163)

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[Matt Lewis]

There was a flaky crash introduced after revision r224132 https://trac.webkit.org/changeset/224132/webkit

The first test the flaky crash showed up on was:

imported/w3c/web-platform-tests/service-workers/service-worker/fetch-event-within-sw.https.html

first build:
https://build.webkit.org/results/Apple%20El%20Capitan%20Release%20WK2%20(Tests)/r224136%20(5579)/results.html
https://build.webkit.org/builders/Apple%20El%20Capitan%20Release%20WK2%20(Tests)/builds/5579

The test also is a flaky failure on all platforms.

Attached is the crash log.

[Chris Dumez]

*** This bug has been marked as a duplicate of bug 179035 ***

[youenn fablet]

Reopening to attach new patch.

[youenn fablet]

Created attachment 325408 [details]
Patch

[Chris Dumez]

Comment on attachment 325408 [details]
Patch

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

> Source/WebKit/NetworkProcess/cache/CacheStorageEngineCaches.cpp:136
> +    storeOrigin([protectedThis = makeRef(*this), this, callback = WTFMove(callback)] (std::optional<Error>&& error) mutable {

Why don't you need the same in readCachesFromDisk() below?

[Chris Dumez]

Comment on attachment 325408 [details]
Patch

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

>> Source/WebKit/NetworkProcess/cache/CacheStorageEngineCaches.cpp:136
>> +    storeOrigin([protectedThis = makeRef(*this), this, callback = WTFMove(callback)] (std::optional<Error>&& error) mutable {
> 
> Why don't you need the same in readCachesFromDisk() below?

You initialized m_storage above, but what if somebody calls clearMemoryRepresentation() before your lambda gets called? Your lambda uses m_storage without null checking it and the crash log show it is dereferencing null.

[youenn fablet]

(In reply to Chris Dumez from comment #4)
> Comment on attachment 325408 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=325408&action=review
> 
> > Source/WebKit/NetworkProcess/cache/CacheStorageEngineCaches.cpp:136
> > +    storeOrigin([protectedThis = makeRef(*this), this, callback = WTFMove(callback)] (std::optional<Error>&& error) mutable {
> 
> Why don't you need the same in readCachesFromDisk() below?

I got too fast, storeOrigin lambda does not protect this since it is done within storeOrigin implementation, like readCachesFromDisk.

[youenn fablet]

> You initialized m_storage above, but what if somebody calls
> clearMemoryRepresentation() before your lambda gets called? Your lambda uses
> m_storage without null checking it and the crash log show it is
> dereferencing null.

That is probably the actual issue.

[youenn fablet]

Created attachment 325461 [details]
Patch

[WebKit Commit Bot]

Comment on attachment 325461 [details]
Patch

Clearing flags on attachment: 325461

Committed r224240: <https://trac.webkit.org/changeset/224240>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Radar WebKit Bug Importer]

<rdar://problem/35567834>

[youenn fablet]

*** Bug 179035 has been marked as a duplicate of this bug. ***