RESOLVED INVALID 231472
IOCache::read and IOCache::write should be called with a serial workqueue 
https://bugs.webkit.org/show_bug.cgi?id=231472
Summary IOCache::read and IOCache::write should be called with a serial workqueue
Jean-Yves Avenard [:jya]
Reported 2021-10-09 00:30:20 PDT
Seen while going over bug 231418 IOCache::read and IOCache::write are called with a WorkQueue as parameter [1] In Engine::readFile [2] and Caches::retrieveOriginFromDirectory [3]; it is called with the main WorkQueue which is a serial work queue In Storage::dispatchReadOperation [4] however it is called using the ioQueue which is the a concurrent one, only to be called again using the main thread one in Storage::traverse [5] The use of a concurrent WorkQueue is dangerous as there's no guarantee in the order of execution of the tasks queued, and they could run simultaneously on different thread. The glibc implementation in particular, doesn't guarantee that the completion handler will be called in the right order should there be multiple call to read. For now however, the glibc WorkQueue is always a serial one, but should this change in the future, it could cause undefined behaviour. [1] https://webkit-search.igalia.com/webkit/rev/db21dfc9fcff4b8205577497cc74941727528dfb/Source/WebKit/NetworkProcess/cache/NetworkCacheIOChannel.h#52-53 [2] https://webkit-search.igalia.com/webkit/rev/db21dfc9fcff4b8205577497cc74941727528dfb/Source/WebKit/NetworkProcess/cache/CacheStorageEngine.cpp#505 [3] https://webkit-search.igalia.com/webkit/rev/db21dfc9fcff4b8205577497cc74941727528dfb/Source/WebKit/NetworkProcess/cache/CacheStorageEngineCaches.cpp#87 [4] https://webkit-search.igalia.com/webkit/rev/db21dfc9fcff4b8205577497cc74941727528dfb/Source/WebKit/NetworkProcess/cache/NetworkCacheStorage.cpp#752 [5] https://webkit-search.igalia.com/webkit/rev/db21dfc9fcff4b8205577497cc74941727528dfb/Source/WebKit/NetworkProcess/cache/NetworkCacheStorage.cpp#1000
Attachments
Add attachment proposed patch, testcase, etc.
Chris Dumez
Comment 1 2021-10-09 07:57:35 PDT
I think we want to keep a concurrent work queue for cocoa at least.
Jean-Yves Avenard [:jya]
Comment 2 2021-10-09 15:21:29 PDT
(In reply to Chris Dumez from comment #1) > I think we want to keep a concurrent work queue for cocoa at least. The API used by the cocoa code blocks and serialises the callback. So we could give to read a serial queue where to run the completion handler which itself queue to the concurrent queue. It would make the code identical to now with minimal performance impact (which remained to be measured imho) but safer for other platforms
Jean-Yves Avenard [:jya]
Comment 3 2021-10-09 17:51:23 PDT
In further read; the code and use of the work queue is fine. It is expected for things to run out of order and will continue only once all tasks have run
Note You need to log in before you can comment on or make changes to this bug.