Bug 179037

Summary: Crash in: com.apple.WebKit: WebKit::CacheStorage::Caches::initializeSize(WTF::Function<void (std::optional<WebCore::DOMCacheEngine::Error>&&)>&&) + 30 (CacheStorageEngineCaches.cpp:163)
Product: WebKit Reporter: Matt Lewis <jlewis3>
Component: New BugsAssignee: youenn fablet <youennf>
Status: RESOLVED FIXED    
Severity: Normal CC: buildbot, cdumez, cgarcia, commit-queue, webkit-bug-importer, youennf
Priority: P2 Keywords: InRadar
Version: WebKit Nightly Build   
Hardware: Unspecified   
OS: Unspecified   
See Also: https://bugs.webkit.org/show_bug.cgi?id=178944
Attachments:
Description Flags
Patch
none
Patch none

Description Matt Lewis 2017-10-30 16:35:09 PDT
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.
Comment 1 Chris Dumez 2017-10-30 16:39:02 PDT

*** This bug has been marked as a duplicate of bug 179035 ***
Comment 2 youenn fablet 2017-10-30 18:56:18 PDT
Reopening to attach new patch.
Comment 3 youenn fablet 2017-10-30 18:56:19 PDT
Created attachment 325408 [details]
Patch
Comment 4 Chris Dumez 2017-10-30 19:05:12 PDT
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?
Comment 5 Chris Dumez 2017-10-30 19:10:16 PDT
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.
Comment 6 youenn fablet 2017-10-30 19:17:12 PDT
(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.
Comment 7 youenn fablet 2017-10-30 19:17:24 PDT
> 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.
Comment 8 youenn fablet 2017-10-31 11:14:30 PDT
Created attachment 325461 [details]
Patch
Comment 9 WebKit Commit Bot 2017-10-31 13:20:15 PDT
Comment on attachment 325461 [details]
Patch

Clearing flags on attachment: 325461

Committed r224240: <https://trac.webkit.org/changeset/224240>
Comment 10 WebKit Commit Bot 2017-10-31 13:20:16 PDT
All reviewed patches have been landed.  Closing bug.
Comment 11 Radar WebKit Bug Importer 2017-11-15 12:35:36 PST
<rdar://problem/35567834>
Comment 12 youenn fablet 2017-11-28 16:18:00 PST
*** Bug 179035 has been marked as a duplicate of this bug. ***