RESOLVED FIXED 175599
[Cache API] Implement Worker connection to the Cache storage engine
https://bugs.webkit.org/show_bug.cgi?id=175599
Summary [Cache API] Implement Worker connection to the Cache storage engine
youenn fablet
Reported 2017-08-15 14:33:08 PDT
[Cache API] Implement Worker connection to the Cache storage engine
Attachments
Patch (40.29 KB, patch)
2017-08-15 15:03 PDT, youenn fablet
no flags
Patch (41.07 KB, patch)
2017-08-15 16:15 PDT, youenn fablet
no flags
Patch (41.07 KB, patch)
2017-08-15 16:39 PDT, youenn fablet
no flags
Patch (36.68 KB, patch)
2017-08-16 13:52 PDT, youenn fablet
no flags
Patch for landing (36.67 KB, patch)
2017-08-16 14:23 PDT, youenn fablet
no flags
Patch for landing (37.70 KB, patch)
2017-08-16 14:27 PDT, youenn fablet
no flags
youenn fablet
Comment 1 2017-08-15 15:03:34 PDT
youenn fablet
Comment 2 2017-08-15 16:15:31 PDT
youenn fablet
Comment 3 2017-08-15 16:39:26 PDT
Chris Dumez
Comment 4 2017-08-16 10:57:17 PDT
Comment on attachment 318197 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=318197&action=review > Source/WebCore/Modules/cache/CacheQueryOptions.h:43 > + CacheQueryOptions options; return { ignoreSearch, ignoreMethod, ignoreVary, cacheName.isolatedCopy() }; It is about the same amount of lines and avoids some unnecessary refcounting churn for the String. > Source/WebCore/Modules/cache/CacheStorageConnection.cpp:137 > + iterator->value(cacheIdentifier, error); It is risky to call a callback while you're holding on to a HashMap iterator. The callback may do things that modify the m_openAndRemoveCachePendingRequests HashMap and would invalidate your iterator, causing a crash on the next line. Please fix. > Source/WebCore/Modules/cache/CacheStorageConnection.cpp:149 > + iterator->value(WTFMove(caches)); It is risky to call a callback while you're holding on to a HashMap iterator. The callback may do things that modify the m_retrieveCachesPendingRequests HashMap and would invalidate your iterator, causing a crash on the next line. Please fix. > Source/WebCore/Modules/cache/CacheStorageConnection.cpp:161 > + iterator->value(WTFMove(records)); It is risky to call a callback while you're holding on to a HashMap iterator. The callback may do things that modify the m_retrieveRecordsPendingRequests HashMap and would invalidate your iterator, causing a crash on the next line. Please fix. > Source/WebCore/Modules/cache/CacheStorageConnection.cpp:173 > + iterator->value(WTFMove(records), error); ditto > Source/WebCore/Modules/cache/CacheStorageConnection.cpp:174 > + m_batchDeleteAndPutPendingRequests.remove(iterator); Method is called remove, member is called delete, please choose one and stick to it. > Source/WebCore/Modules/cache/CacheStorageConnection.cpp:185 > + iterator->value(WTFMove(records), error); ditto. > Source/WebCore/Modules/cache/CacheStorageConnection.h:107 > + uint64_t m_openAndRemoveCachePendingRequestsCounter { 0 }; Why do we need for counters? Can't we just have one to generate identifiers for all kinds of requests? > Source/WebCore/Modules/cache/WorkerCacheStorageConnection.cpp:141 > + isolatedCaches.uncheckedAppend(CacheInfo { cache.identifier, cache.name.isolatedCopy() }); May be nice (i.e. more reusable) to have this as an isolatedCopy() method on the CacheInfo struct instead. > Source/WebCore/Modules/cache/WorkerCacheStorageConnection.cpp:197 > + for (const auto& record : records) This logic is duplicated, may be nice to move it to a utility function. > Source/WebCore/Modules/cache/WorkerCacheStorageConnection.cpp:206 > + for (auto& recordData : recordsData) This logic is duplicated, may be nice to move it to a utility function. > Source/WebCore/Modules/cache/WorkerCacheStorageConnection.h:36 > +class WorkerCacheStorageConnection : public CacheStorageConnection { final? > Source/WebCore/Modules/cache/WorkerCacheStorageConnection.h:56 > + RefPtr<CacheStorageConnection> m_mainThreadConnection; What makes sure this gets destroyed on the main thread? > Source/WebCore/loader/FetchOptions.h:66 > + FetchOptions options = *this; I don't like this pattern because if somebody adds a new data member that is not safely copyable cross-thread, this code will still build but crash. I'd rather it fails building when something adds a new data member and forgets to update this isolatedCopy() method. I.e. I'd rather we use { } initializer with all members explicitly initialized.
youenn fablet
Comment 5 2017-08-16 13:52:05 PDT
youenn fablet
Comment 6 2017-08-16 13:52:56 PDT
Thanks for the review. (In reply to youenn fablet from comment #5) > Created attachment 318287 [details] > Patch Review items should be fixed in this patch
Chris Dumez
Comment 7 2017-08-16 14:00:39 PDT
Comment on attachment 318287 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=318287&action=review r=me > Source/WebCore/Modules/cache/CacheStorageConnection.cpp:83 > + uint64_t requestIdentifier = ++m_requestCounter; nit: m_requestCounter is a bit confusing as it is used as a way to generate identifiers, not *count* requests. Maybe m_lastRequestIdentifier ? > Source/WebCore/Modules/cache/CacheStorageConnection.cpp:131 > + auto callback = m_openAndRemoveCachePendingRequests.take(requestIdentifier); Could be inside the if condition > Source/WebCore/Modules/cache/CacheStorageConnection.cpp:138 > + auto callback = m_retrieveCachesPendingRequests.take(requestIdentifier); ditto > Source/WebCore/Modules/cache/CacheStorageConnection.cpp:145 > + auto callback = m_retrieveRecordsPendingRequests.take(requestIdentifier); ditto > Source/WebCore/Modules/cache/CacheStorageConnection.cpp:152 > + auto callback = m_batchDeleteAndPutPendingRequests.take(requestIdentifier); ditto > Source/WebCore/Modules/cache/CacheStorageConnection.cpp:159 > + auto callback = m_batchDeleteAndPutPendingRequests.take(requestIdentifier); ditto
youenn fablet
Comment 8 2017-08-16 14:23:13 PDT
Created attachment 318288 [details] Patch for landing
youenn fablet
Comment 9 2017-08-16 14:27:04 PDT
Created attachment 318289 [details] Patch for landing
WebKit Commit Bot
Comment 10 2017-08-16 15:09:38 PDT
Comment on attachment 318289 [details] Patch for landing Clearing flags on attachment: 318289 Committed r220810: <http://trac.webkit.org/changeset/220810>
WebKit Commit Bot
Comment 11 2017-08-16 15:09:40 PDT
All reviewed patches have been landed. Closing bug.
Radar WebKit Bug Importer
Comment 12 2017-08-16 15:11:44 PDT
Note You need to log in before you can comment on or make changes to this bug.