Bug 175599 - [Cache API] Implement Worker connection to the Cache storage engine
Summary: [Cache API] Implement Worker connection to the Cache storage engine
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebCore Misc. (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: youenn fablet
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2017-08-15 14:33 PDT by youenn fablet
Modified: 2017-08-16 15:11 PDT (History)
8 users (show)

See Also:


Attachments
Patch (40.29 KB, patch)
2017-08-15 15:03 PDT, youenn fablet
no flags Details | Formatted Diff | Diff
Patch (41.07 KB, patch)
2017-08-15 16:15 PDT, youenn fablet
no flags Details | Formatted Diff | Diff
Patch (41.07 KB, patch)
2017-08-15 16:39 PDT, youenn fablet
no flags Details | Formatted Diff | Diff
Patch (36.68 KB, patch)
2017-08-16 13:52 PDT, youenn fablet
no flags Details | Formatted Diff | Diff
Patch for landing (36.67 KB, patch)
2017-08-16 14:23 PDT, youenn fablet
no flags Details | Formatted Diff | Diff
Patch for landing (37.70 KB, patch)
2017-08-16 14: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 2017-08-15 14:33:08 PDT
[Cache API] Implement Worker connection to the Cache storage engine
Comment 1 youenn fablet 2017-08-15 15:03:34 PDT
Created attachment 318179 [details]
Patch
Comment 2 youenn fablet 2017-08-15 16:15:31 PDT
Created attachment 318194 [details]
Patch
Comment 3 youenn fablet 2017-08-15 16:39:26 PDT
Created attachment 318197 [details]
Patch
Comment 4 Chris Dumez 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.
Comment 5 youenn fablet 2017-08-16 13:52:05 PDT
Created attachment 318287 [details]
Patch
Comment 6 youenn fablet 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
Comment 7 Chris Dumez 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
Comment 8 youenn fablet 2017-08-16 14:23:13 PDT
Created attachment 318288 [details]
Patch for landing
Comment 9 youenn fablet 2017-08-16 14:27:04 PDT
Created attachment 318289 [details]
Patch for landing
Comment 10 WebKit Commit Bot 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>
Comment 11 WebKit Commit Bot 2017-08-16 15:09:40 PDT
All reviewed patches have been landed.  Closing bug.
Comment 12 Radar WebKit Bug Importer 2017-08-16 15:11:44 PDT
<rdar://problem/33927819>