Bug 190321 - Cache API tests are flaky due to file writing failing from time to time
Summary: Cache API tests are flaky due to file writing failing from time to time
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Service Workers (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: youenn fablet
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2018-10-05 13:18 PDT by youenn fablet
Modified: 2018-10-12 10:58 PDT (History)
8 users (show)

See Also:


Attachments
Patch (12.11 KB, patch)
2018-10-05 13:27 PDT, youenn fablet
no flags Details | Formatted Diff | Diff
Patch (13.03 KB, patch)
2018-10-05 16:08 PDT, youenn fablet
no flags Details | Formatted Diff | Diff
Patch (8.37 KB, patch)
2018-10-08 16:27 PDT, youenn fablet
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description youenn fablet 2018-10-05 13:18:23 PDT
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.
Comment 1 youenn fablet 2018-10-05 13:27:52 PDT
Created attachment 351695 [details]
Patch
Comment 2 Alex Christensen 2018-10-05 15:53:17 PDT
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));
Comment 3 youenn fablet 2018-10-05 16:00:33 PDT
(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
Comment 4 youenn fablet 2018-10-05 16:08:31 PDT
Created attachment 351704 [details]
Patch
Comment 5 Alex Christensen 2018-10-05 21:16:48 PDT
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.
Comment 6 youenn fablet 2018-10-08 16:27:42 PDT
Created attachment 351831 [details]
Patch
Comment 7 Chris Dumez 2018-10-12 10:19:27 PDT
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.
Comment 8 youenn fablet 2018-10-12 10:31:57 PDT
Thanks for the review.

> DiskWrite would look better IMO.

And ReadDisk -> DiskRead I guess.
Comment 9 Chris Dumez 2018-10-12 10:32:33 PDT
(In reply to youenn fablet from comment #8)
> Thanks for the review.
> 
> > DiskWrite would look better IMO.
> 
> And ReadDisk -> DiskRead I guess.

Yes.
Comment 10 WebKit Commit Bot 2018-10-12 10:57:27 PDT
Comment on attachment 351831 [details]
Patch

Clearing flags on attachment: 351831

Committed r237071: <https://trac.webkit.org/changeset/237071>
Comment 11 WebKit Commit Bot 2018-10-12 10:57:29 PDT
All reviewed patches have been landed.  Closing bug.
Comment 12 Radar WebKit Bug Importer 2018-10-12 10:58:28 PDT
<rdar://problem/45230934>