Bug 182193

Summary: Cache API should make sure to resolve caches.open promises in the same order as called
Product: WebKit Reporter: youenn fablet <youennf>
Component: Service WorkersAssignee: youenn fablet <youennf>
Status: RESOLVED FIXED    
Severity: Normal CC: achristensen, cdumez, cgarcia, commit-queue, ews-watchlist, webkit-bug-importer
Priority: P2 Keywords: InRadar
Version: WebKit Nightly Build   
Hardware: Unspecified   
OS: Unspecified   
Bug Depends on:    
Bug Blocks: 182134    
Attachments:
Description Flags
Patch
none
Patch
none
Patch
none
Patch
none
Patch
none
Patch for landing none

youenn fablet
Reported 2018-01-26 16:06:18 PST
Cache API should make sure to resolve caches.open promises in the same order as called.
Attachments
Patch (8.26 KB, patch)
2018-01-26 16:44 PST, youenn fablet 		no flags
DetailsFormatted DiffDiff
Patch (8.18 KB, patch)
2018-01-26 17:14 PST, youenn fablet 		no flags
DetailsFormatted DiffDiff
Patch (13.19 KB, patch)
2018-01-27 07:34 PST, youenn fablet 		no flags
DetailsFormatted DiffDiff
Patch (14.89 KB, patch)
2018-01-29 13:58 PST, youenn fablet 		no flags
DetailsFormatted DiffDiff
Patch (14.92 KB, patch)
2018-01-29 14:28 PST, youenn fablet 		no flags
DetailsFormatted DiffDiff
Patch for landing (14.78 KB, patch)
2018-01-29 15:58 PST, youenn fablet 		no flags
DetailsFormatted DiffDiff
Show Obsolete (5) View All Add attachment proposed patch, testcase, etc.
youenn fablet
Comment 1 2018-01-26 16:44:43 PST
Created attachment 332429 [details] Patch
youenn fablet
Comment 2 2018-01-26 17:14:44 PST
Created attachment 332436 [details] Patch
Radar WebKit Bug Importer
Comment 3 2018-01-26 17:15:08 PST
<rdar://problem/36930363>
youenn fablet
Comment 4 2018-01-27 07:34:55 PST
Created attachment 332470 [details] Patch
Chris Dumez
Comment 5 2018-01-29 09:42:37 PST
Comment on attachment 332470 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=332470&action=review > Source/WebKit/NetworkProcess/cache/CacheStorageEngineCaches.cpp:554 > + m_pendingWritingCachesToDiskCallbacks.append([this, weakThis = m_weakFactory.createWeakPtr(*this), updateCounter, callback = WTFMove(callback)] () mutable { Why do we need this weakThis? m_pendingWritingCachesToDiskCallbacks is a data member of |this| and therefore would be cleared when |this| is destroyed. Same comment for the other lambdas. > Source/WebKit/NetworkProcess/cache/CacheStorageEngineCaches.h:94 > + WeakPtr<Caches> createWeakPtr() { return m_weakFactory.createWeakPtr(*this); } If we need to use a weakPtr somewhere for some reason, I think you should rely on this getter more instead of using m_weakFactory.createWeakPtr(*this) in your lambda captures. > Source/WebKit/NetworkProcess/cache/CacheStorageEngineCaches.h:110 > + Deque<CompletionHandler<void()>> m_pendingWritingCachesToDiskCallbacks; Assuming the Caches object can be deleted, how can this be CompletionHandlers? When the Caches object gets destroyed, what makes sure the completion handlers have been called?
youenn fablet
Comment 6 2018-01-29 13:58:25 PST
Created attachment 332577 [details] Patch
Chris Dumez
Comment 7 2018-01-29 14:04:48 PST
GTK build is red.
youenn fablet
Comment 8 2018-01-29 14:28:29 PST
(In reply to Chris Dumez from comment #7) > GTK build is red. It seems GTK would like this->clear() instead of clear()...
youenn fablet
Comment 9 2018-01-29 14:28:55 PST
Created attachment 332580 [details] Patch
Chris Dumez
Comment 10 2018-01-29 15:35:20 PST
Comment on attachment 332580 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=332580&action=review > Source/WebKit/NetworkProcess/cache/CacheStorageEngineCaches.cpp:211 > + ASSERT(!error); Why this assertion? If I have 2 clear requests in m_pendingWritingCachesToDiskCallbacks, then presumably, the second one will be resolved with an error once the first request is processed, right?
youenn fablet
Comment 11 2018-01-29 15:41:23 PST
(In reply to Chris Dumez from comment #10) > Comment on attachment 332580 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=332580&action=review > > > Source/WebKit/NetworkProcess/cache/CacheStorageEngineCaches.cpp:211 > > + ASSERT(!error); > > Why this assertion? If I have 2 clear requests in > m_pendingWritingCachesToDiskCallbacks, then presumably, the second one will > be resolved with an error once the first request is processed, right? In fact, even if a previous write disk operation failed or if there is a clear request, we should anyway try clearing things, especially since we have no ways currently to expose a potential failure in clearing stuff. I'll remove the ASSERT and the if statement.
youenn fablet
Comment 12 2018-01-29 15:58:05 PST
Created attachment 332594 [details] Patch for landing
WebKit Commit Bot
Comment 13 2018-01-29 17:21:40 PST
The commit-queue encountered the following flaky tests while processing attachment 332594 [details]: imported/w3c/web-platform-tests/fetch/api/redirect/redirect-to-dataurl-worker.html bug 182270 (author: youennf@gmail.com) The commit-queue is continuing to process your patch.
WebKit Commit Bot
Comment 14 2018-01-29 17:21:49 PST
The commit-queue encountered the following flaky tests while processing attachment 332594 [details]: js/dom/JSON-stringify.html bug 144784 (author: fpizlo@apple.com) The commit-queue is continuing to process your patch.
WebKit Commit Bot
Comment 15 2018-01-29 17:41:27 PST
Comment on attachment 332594 [details] Patch for landing Clearing flags on attachment: 332594 Committed r227768: <https://trac.webkit.org/changeset/227768>
WebKit Commit Bot
Comment 16 2018-01-29 17:41:28 PST
All reviewed patches have been landed. Closing bug.
Note You need to log in before you can comment on or make changes to this bug.