Bug 173172

Summary: Use Function rather than std::function for thread safety in WebsiteDataStore
Product: WebKit Reporter: Brent Fulgham <bfulgham>
Component: WebKit2Assignee: Brent Fulgham <bfulgham>
Status: RESOLVED FIXED    
Severity: Normal CC: bfulgham, cdumez, webkit-bug-importer
Priority: P2 Keywords: InRadar
Version: WebKit Nightly Build   
Hardware: Unspecified   
OS: Unspecified   
Bug Depends on: 173168    
Bug Blocks:    
Attachments:
Description Flags
Patch cdumez: review+, cdumez: commit-queue-

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>