Bug 173172 - Use Function rather than std::function for thread safety in WebsiteDataStore
Summary: Use Function rather than std::function for thread safety in WebsiteDataStore
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebKit2 (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Brent Fulgham
URL:
Keywords: InRadar
Depends on: 173168
Blocks:
  Show dependency treegraph
 
Reported: 2017-06-09 11:35 PDT by Brent Fulgham
Modified: 2017-06-09 12:40 PDT (History)
3 users (show)

See Also:


Attachments
Patch (23.37 KB, patch)
2017-06-09 11:47 PDT, Brent Fulgham
cdumez: review+
cdumez: commit-queue-
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Brent Fulgham 2017-06-09 11:35:36 PDT
Audit the WebsiteDataStore code for use of std::function, and switch to our safer WTF::Function version.

Also attempt to reduce copies and ref churn by using WTFMove in more places.
Comment 1 Radar WebKit Bug Importer 2017-06-09 11:39:11 PDT
<rdar://problem/32679311>
Comment 2 Brent Fulgham 2017-06-09 11:47:58 PDT
Created attachment 312467 [details]
Patch
Comment 3 Chris Dumez 2017-06-09 11:59:53 PDT
Comment on attachment 312467 [details]
Patch

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

Will wait for debug bot to be happy before r+ given all the new assertions :)

> Source/WebKit2/UIProcess/WebProcessProxy.cpp:267
> +            callbackAggregator->addDomainsWithDeletedWebsiteData(domainsWithDeletedWebsiteData);

Wouldn't hurt to WTFMove(domainsWithDeletedWebsiteData) here.

> Source/WebKit2/UIProcess/WebsiteData/WebsiteDataStore.cpp:533
> +            String&& domain = dataRecord.topPrivatelyControlledDomain();

This is weird. I think this should stay a String.

> Source/WebKit2/UIProcess/WebsiteData/WebsiteDataStore.cpp:583
> +        explicit CallbackAggregator(Function<void()> completionHandler)

Missing &&.

> Source/WebKit2/UIProcess/WebsiteData/WebsiteDataStore.cpp:820
> +        explicit CallbackAggregator(Function<void()> completionHandler)

Missing &&
Comment 4 Chris Dumez 2017-06-09 12:05:54 PDT
Comment on attachment 312467 [details]
Patch

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

> Source/WebKit2/UIProcess/WebProcessProxy.cpp:214
> +    // We expect this to be called on the main thread so we get the default website data store

Missing period at the end.

> Source/WebKit2/UIProcess/WebProcessProxy.cpp:275
> +    // We expect this to be called on the main thread so we get the default website data store

Missing period at the end.

> Source/WebKit2/UIProcess/WebsiteData/WebsiteDataStore.cpp:188
> +        explicit CallbackAggregator(OptionSet<WebsiteDataFetchOption> fetchOptions, Function<void(Vector<WebsiteDataRecord>)> completionHandler)

Missing &&.
Comment 5 Brent Fulgham 2017-06-09 12:13:51 PDT
Comment on attachment 312467 [details]
Patch

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

>> Source/WebKit2/UIProcess/WebProcessProxy.cpp:267
>> +            callbackAggregator->addDomainsWithDeletedWebsiteData(domainsWithDeletedWebsiteData);
> 
> Wouldn't hurt to WTFMove(domainsWithDeletedWebsiteData) here.

OK!

>> Source/WebKit2/UIProcess/WebProcessProxy.cpp:275
>> +    // We expect this to be called on the main thread so we get the default website data store
> 
> Missing period at the end.

Fixed (and the other one).

>> Source/WebKit2/UIProcess/WebsiteData/WebsiteDataStore.cpp:188
>> +        explicit CallbackAggregator(OptionSet<WebsiteDataFetchOption> fetchOptions, Function<void(Vector<WebsiteDataRecord>)> completionHandler)
> 
> Missing &&.

Whoops!

>> Source/WebKit2/UIProcess/WebsiteData/WebsiteDataStore.cpp:533
>> +            String&& domain = dataRecord.topPrivatelyControlledDomain();
> 
> This is weird. I think this should stay a String.

OK!

>> Source/WebKit2/UIProcess/WebsiteData/WebsiteDataStore.cpp:583
>> +        explicit CallbackAggregator(Function<void()> completionHandler)
> 
> Missing &&.

Good catch!

>> Source/WebKit2/UIProcess/WebsiteData/WebsiteDataStore.cpp:820
>> +        explicit CallbackAggregator(Function<void()> completionHandler)
> 
> Missing &&

+1
Comment 6 Chris Dumez 2017-06-09 12:39:44 PDT
Comment on attachment 312467 [details]
Patch

r=me with nit fixes I suggested.
Comment 7 Brent Fulgham 2017-06-09 12:40:38 PDT
Committed r218011: <http://trac.webkit.org/changeset/218011>