Bug 190321 – Cache API tests are flaky due to file writing failing from time to time

• Component: Service Workers
• Status: RESOLVED FIXED

[youenn fablet]

As can be seen from the flakiness dashboard, http/wpt/cache-storage/cache-put-keys.https.any.worker.html is still failing.
From logging gathered on my machine, this is due to errors when writing files through NetworkStorageCache.

[youenn fablet]

Created attachment 351695 [details]
Patch

[Alex Christensen]

Comment on attachment 351695 [details]
Patch

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

> Source/WebKit/NetworkProcess/cache/CacheStorageEngine.cpp:386
> +void Engine::writeFile(const String& folderPath, const String& filename, NetworkCache::Data&& data, WebCore::DOMCacheEngine::CompletionCallback&& callback)

Shouldn't folderPath be derivable from filename?

> Source/WebKit/NetworkProcess/cache/NetworkCacheStorage.cpp:1029
> +        RunLoop::main().dispatch([protectedThis = WTFMove(protectedThis), completionHandler = WTFMove(completionHandler)]() mutable {
> +            completionHandler();

RunLoop::main().dispatch(WTFMove(completionHandler));

[youenn fablet]

(In reply to Alex Christensen from comment #2)
> Comment on attachment 351695 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=351695&action=review
> 
> > Source/WebKit/NetworkProcess/cache/CacheStorageEngine.cpp:386
> > +void Engine::writeFile(const String& folderPath, const String& filename, NetworkCache::Data&& data, WebCore::DOMCacheEngine::CompletionCallback&& callback)
> 
> Shouldn't folderPath be derivable from filename?

Sure, I can change that to separate it.

> 
> > Source/WebKit/NetworkProcess/cache/NetworkCacheStorage.cpp:1029
> > +        RunLoop::main().dispatch([protectedThis = WTFMove(protectedThis), completionHandler = WTFMove(completionHandler)]() mutable {
> > +            completionHandler();
> 
> RunLoop::main().dispatch(WTFMove(completionHandler));

We cannot do that right now.
We should probably change to use DestructionThread::Main and stop passing protectedThis back to the main thread.
I'll do that in a separate patch though since it is touching more code in NetworkCacheStorage.cpp

[youenn fablet]

Created attachment 351704 [details]
Patch

[Alex Christensen]

Comment on attachment 351704 [details]
Patch

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

This is even worse.  What I meant is we could only pass one string, like /path/to/file then inside the lambda do a substring operation on it.  But I preferred your other approach to this.

> Source/WebKit/NetworkProcess/cache/CacheStorageEngineCaches.cpp:43
> +#define CACHESLIST_FILENAME "cacheslist"_s

oh please no.

[youenn fablet]

Created attachment 351831 [details]
Patch

[Chris Dumez]

Comment on attachment 351831 [details]
Patch

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

> Source/WebKit/NetworkProcess/cache/CacheStorageEngineCaches.cpp:508
> +            callback(Error::WriteDisk);

DiskWrite would look better IMO.

[youenn fablet]

Thanks for the review.

> DiskWrite would look better IMO.

And ReadDisk -> DiskRead I guess.

[Chris Dumez]

(In reply to youenn fablet from comment #8)
> Thanks for the review.
> 
> > DiskWrite would look better IMO.
> 
> And ReadDisk -> DiskRead I guess.

Yes.

[WebKit Commit Bot]

Comment on attachment 351831 [details]
Patch

Clearing flags on attachment: 351831

Committed r237071: <https://trac.webkit.org/changeset/237071>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Radar WebKit Bug Importer]

<rdar://problem/45230934>