Removing website data for a domain should delete corresponding ITP entry

• Component: WebKit Misc.
• Status: RESOLVED FIXED

[Kate Cheney]

Individual deletion of Website Data should selectively delete ITP entries

[Kate Cheney]

<rdar://problem/59473193>

[Kate Cheney]

Created attachment 397402 [details]
Patch

[John Wilander]

Comment on attachment 397402 [details]
Patch

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

r=me with comments.

> Source/WebKit/ChangeLog:18
> +        in the database due to cascading deletions.

Does it also delete for the plist-based store? If so, the terms differ there. No domainID.

> Source/WebKit/NetworkProcess/Classifier/ResourceLoadStatisticsDatabaseStore.cpp:2252
> +    SQLiteStatement removeAllData(m_database, "DELETE FROM ObservedDomains WHERE domainID = ?");

Do we typically inline these static strings? Or should it be declared somewhere else? If here, should we use _s?

> Source/WebKit/NetworkProcess/Classifier/ResourceLoadStatisticsDatabaseStore.cpp:2909
> +    SQLiteStatement linkDecorationStatement(m_database, "SELECT EXISTS (SELECT * FROM TopFrameLinkDecorationsFrom WHERE toDomainID = ? OR fromDomainID = ?)");

Ditto for these strings.

> Source/WebKit/NetworkProcess/NetworkProcess.h:403
> +    void deleteWebsiteDataForOrigins(PAL::SessionID, OptionSet<WebsiteDataType>, const Vector<WebCore::SecurityOriginData>& origins, const Vector<String>& cookieHostNames, const Vector<String>& HSTSCacheHostnames, const Vector<RegistrableDomain>& registrableDomains, CallbackID);

Doesn't need the parameter name registrableDomains.

> Source/WebKit/UIProcess/API/C/WKWebsiteDataStoreRef.cpp:69
> +void WKWebsiteDataStoreRemoveITPDataForDomain(WKWebsiteDataStoreRef dataStoreRef, WKStringRef host, void* context, WKWebsiteDataStoreRemoveITPDataForDomainFunction callback)

We should check if this API should be made SPI since clients could call this to exempt specific domains from ITP treatment.

> Source/WebKit/UIProcess/Network/NetworkProcessProxy.h:110
> +    void deleteWebsiteDataForOrigins(PAL::SessionID, OptionSet<WebKit::WebsiteDataType>, const Vector<WebCore::SecurityOriginData>& origins, const Vector<String>& cookieHostNames, const Vector<String>& HSTSCacheHostNames, const Vector<RegistrableDomain>& registrableDomains, CompletionHandler<void()>&&);

Doesn't need the parameter name registrableDomains.

> Tools/ChangeLog:9
> +        Created 2 new SPIs for testing. One to mimic clearing website data

I think they are C APIs.

> Tools/WebKitTestRunner/InjectedBundle/Bindings/TestRunner.idl:296
> +    boolean domainIDExistsInDatabase(unsigned long domainID);

The "statistics" term was added to these function names as a weak namespace since it's all a flat set of functions under testRunner. We should probably stick to it but then rename it to ITP once we rename ResourceLoadStatistics in source code. Or we could figure out a way to have testRunner.itp.* functions.

[Kate Cheney]

Comment on attachment 397402 [details]
Patch

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

Thanks! I'll address these before landing.

>> Source/WebKit/ChangeLog:18
>> +        in the database due to cascading deletions.
> 
> Does it also delete for the plist-based store? If so, the terms differ there. No domainID.

Good catch! plist handling was so much simpler I forgot to even include it in here.

>> Source/WebKit/NetworkProcess/Classifier/ResourceLoadStatisticsDatabaseStore.cpp:2252
>> +    SQLiteStatement removeAllData(m_database, "DELETE FROM ObservedDomains WHERE domainID = ?");
> 
> Do we typically inline these static strings? Or should it be declared somewhere else? If here, should we use _s?

Good point, the statement could be a constexpr value.

>> Source/WebKit/NetworkProcess/Classifier/ResourceLoadStatisticsDatabaseStore.cpp:2909
>> +    SQLiteStatement linkDecorationStatement(m_database, "SELECT EXISTS (SELECT * FROM TopFrameLinkDecorationsFrom WHERE toDomainID = ? OR fromDomainID = ?)");
> 
> Ditto for these strings.

I'll predeclare these as well.

>> Source/WebKit/UIProcess/API/C/WKWebsiteDataStoreRef.cpp:69
>> +void WKWebsiteDataStoreRemoveITPDataForDomain(WKWebsiteDataStoreRef dataStoreRef, WKStringRef host, void* context, WKWebsiteDataStoreRemoveITPDataForDomainFunction callback)
> 
> We should check if this API should be made SPI since clients could call this to exempt specific domains from ITP treatment.

Good point, I've never had to do that for a WKWebsiteDataStoreRef function before. Maybe I can do this in WKWebsiteDataStore instead.

>> Tools/ChangeLog:9
>> +        Created 2 new SPIs for testing. One to mimic clearing website data
> 
> I think they are C APIs.

Ah, good catch. But one should probably be an SPI. I'll fix this.

>> Tools/WebKitTestRunner/InjectedBundle/Bindings/TestRunner.idl:296
>> +    boolean domainIDExistsInDatabase(unsigned long domainID);
> 
> The "statistics" term was added to these function names as a weak namespace since it's all a flat set of functions under testRunner. We should probably stick to it but then rename it to ITP once we rename ResourceLoadStatistics in source code. Or we could figure out a way to have testRunner.itp.* functions.

Will do!

[Kate Cheney]

Created attachment 397500 [details]
Patch for landing

[Kate Cheney]

(In reply to katherine_cheney from comment #4)
> Comment on attachment 397402 [details]
> Patch

> >> Source/WebKit/UIProcess/API/C/WKWebsiteDataStoreRef.cpp:69
> >> +void WKWebsiteDataStoreRemoveITPDataForDomain(WKWebsiteDataStoreRef dataStoreRef, WKStringRef host, void* context, WKWebsiteDataStoreRemoveITPDataForDomainFunction callback)
> > 
> > We should check if this API should be made SPI since clients could call this to exempt specific domains from ITP treatment.
> 
> Good point, I've never had to do that for a WKWebsiteDataStoreRef function
> before. Maybe I can do this in WKWebsiteDataStore instead.

Landing the patch with an API but tracking the potential change to SPI in a separate radar <rdar://problem/62335968>. I think there may be other APIs for ITP that could/should also be made SPIs.

[EWS]

Committed r260668: <https://trac.webkit.org/changeset/260668>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 397500 [details].