Bug 175599 – [Cache API] Implement Worker connection to the Cache storage engine

• Status: RESOLVED
• Resolution: FIXED
• Component: WebCore Misc.

[youenn fablet]

[Cache API] Implement Worker connection to the Cache storage engine

[youenn fablet]

Created attachment 318179 [details]
Patch

[youenn fablet]

Created attachment 318194 [details]
Patch

[youenn fablet]

Created attachment 318197 [details]
Patch

[Chris Dumez]

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]

Created attachment 318287 [details]
Patch

[youenn fablet]

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 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]

Created attachment 318288 [details]
Patch for landing

[youenn fablet]

Created attachment 318289 [details]
Patch for landing

[WebKit Commit Bot]

Comment on attachment 318289 [details]
Patch for landing

Clearing flags on attachment: 318289

Committed r220810: <http://trac.webkit.org/changeset/220810>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Radar WebKit Bug Importer]

<rdar://problem/33927819>