Bug 222828 – Update ApplicationCacheStorage::originsWithCache() to return a HashSet<SecurityOriginData> instead of a Vector<Ref<SecurityOrigin>>

• Component: WebKit2
• Status: RESOLVED FIXED

[Chris Dumez]

Update ApplicationCacheStorage::originsWithCache() to return a HashSet<SecurityOriginData> instead of a Vector<Ref<SecurityOrigin>>.

[Chris Dumez]

Created attachment 422419 [details]
Patch

[Chris Dumez]

Created attachment 422443 [details]
Patch

[Chris Dumez]

Comment on attachment 422443 [details]
Patch

Patch is ready for review.

[Darin Adler]

Comment on attachment 422443 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=422443&action=review

> Source/WebCore/ChangeLog:11
> +        Update ApplicationCacheStorage::originsWithCache() to return a HashSet<SecurityOriginData> instead of a Vector<Ref<SecurityOrigin>>.
> +        Using a HashSet makes sure we do not return duplicates (which do occur as per ApplicationCacheStorage implementation) and makes typing
> +        more consistent with other similar storage functions. Using SecurityOriginData is more lightweight and sufficient for our use cases.
> +        It also takes care of a FIXME comment in WebsiteDataStore::fetchDataAndApply about switching to SecurityOriginData.

The downside of switching to HashSet is that any algorithms that involve iterating through the origins now iterate in an unpredictable order. As long as that’s not an issue, this will be great.

> Source/WebKit/UIProcess/WebsiteData/WebsiteDataStore.cpp:536
> +            for (auto& origin : storage->originsWithCache()) {

This is a situation where we might want to consider some kind of sorting so we have predictable behavior. I worry about creating a vector that preserves the randomness of hash table order and doing anything with that vector.

In the early days of Safari some of our most difficult to diagnose performance and correctness bugs were hard to reproduce because they depended on hash ordering. Although the biggest one had a property that this does not, which is that the hash was almost global (I think it was used by NSNotificationCenter) so seemingly unrelated code to add something was having an effect on something seemingly unrelated.

[Chris Dumez]

Comment on attachment 422443 [details]
Patch

Clearing flags on attachment: 422443

Committed r274022 (234958@main): <https://commits.webkit.org/234958@main>

[Chris Dumez]

All reviewed patches have been landed.  Closing bug.

[EWS]

Tools/Scripts/svn-apply failed to apply attachment 422443 [details] to trunk.
Please resolve the conflicts and upload a new patch.

[Radar WebKit Bug Importer]

<rdar://problem/75120436>