Cache API should make sure to resolve caches.open promises in the same order as called

• Component: Service Workers
• Status: RESOLVED FIXED

[youenn fablet]

Cache API should make sure to resolve caches.open promises in the same order as called.

[youenn fablet]

Created attachment 332429 [details]
Patch

[youenn fablet]

Created attachment 332436 [details]
Patch

[Radar WebKit Bug Importer]

<rdar://problem/36930363>

[youenn fablet]

Created attachment 332470 [details]
Patch

[Chris Dumez]

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]

Created attachment 332577 [details]
Patch

[Chris Dumez]

GTK build is red.

[youenn fablet]

(In reply to Chris Dumez from comment #7)
> GTK build is red.

It seems GTK would like this->clear() instead of clear()...

[youenn fablet]

Created attachment 332580 [details]
Patch

[Chris Dumez]

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]

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

Created attachment 332594 [details]
Patch for landing

[WebKit Commit Bot]

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: [e-mail])
The commit-queue is continuing to process your patch.

[WebKit Commit Bot]

The commit-queue encountered the following flaky tests while processing attachment 332594 [details]:

js/dom/JSON-stringify.html bug 144784 (author: [e-mail])
The commit-queue is continuing to process your patch.

[WebKit Commit Bot]

Comment on attachment 332594 [details]
Patch for landing

Clearing flags on attachment: 332594

Committed r227768: <https://trac.webkit.org/changeset/227768>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.