Bug 210864 - Removing website data for a domain should delete corresponding ITP entry
Summary: Removing website data for a domain should delete corresponding ITP entry
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebKit Misc. (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: katherine_cheney
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2020-04-22 10:19 PDT by katherine_cheney
Modified: 2020-04-24 14:08 PDT (History)
2 users (show)

See Also:


Attachments
Patch (68.24 KB, patch)
2020-04-23 17:20 PDT, katherine_cheney
no flags Details | Formatted Diff | Diff
Patch for landing (71.57 KB, patch)
2020-04-24 13:35 PDT, katherine_cheney
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description katherine_cheney 2020-04-22 10:19:01 PDT
Individual deletion of Website Data should selectively delete ITP entries
Comment 1 katherine_cheney 2020-04-22 10:20:13 PDT
<rdar://problem/59473193>
Comment 2 katherine_cheney 2020-04-23 17:20:54 PDT
Created attachment 397402 [details]
Patch
Comment 3 John Wilander 2020-04-24 10:37:30 PDT
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.
Comment 4 katherine_cheney 2020-04-24 11:08:29 PDT
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!
Comment 5 katherine_cheney 2020-04-24 13:35:51 PDT
Created attachment 397500 [details]
Patch for landing
Comment 6 katherine_cheney 2020-04-24 13:40:57 PDT
(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.
Comment 7 EWS 2020-04-24 14:08:14 PDT
Committed r260668: <https://trac.webkit.org/changeset/260668>

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