Bug 209634

Summary: Add SPI to configure WebsiteDataStores with a URL for standalone web applications and use it to disable first-party website data removal in ITP
Product: WebKit Reporter: David Quesada <david_quesada>
Component: WebKit APIAssignee: John Wilander <wilander>
Status: RESOLVED FIXED    
Severity: Normal CC: achristensen, bfulgham, mjs, sam, tsavell, webkit-bug-importer, wilander
Priority: P2 Keywords: InRadar
Version: WebKit Nightly Build   
Hardware: Unspecified   
OS: Unspecified   
See Also: https://bugs.webkit.org/show_bug.cgi?id=211190
Attachments:
Description Flags
Patch
none
Patch
none
Patch
none
Patch for landing
none
Patch none

Description David Quesada 2020-03-26 17:26:52 PDT 
rdar://problem/60943970
Comment 1 David Quesada 2020-03-26 17:50:49 PDT 
Created attachment 394683 [details]
Patch
Comment 2 Sam Weinig 2020-03-27 08:11:04 PDT 
Comment on attachment 394683 [details]
Patch

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

> Source/WebKit/ChangeLog:10
> +        Add the ability to configure a website data store so that its network session operates
> +        using WebCore::FirstPartyWebsiteDataRemovalMode::None.

This needs tests.
Comment 3 Alex Christensen 2020-03-31 09:40:30 PDT 
Comment on attachment 394683 [details]
Patch

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

> Source/WebKit/UIProcess/WebsiteData/WebsiteDataStoreConfiguration.h:203
> +    bool m_resourceLoadStatisticsFirstPartyWebsiteDataRemovalEnabled { true };

This should not be inside PLATFORM(COCOA)
Comment 4 John Wilander 2020-04-02 12:00:14 PDT 
Created attachment 395284 [details]
Patch
Comment 5 David Quesada 2020-04-02 12:14:29 PDT 
Comment on attachment 395284 [details]
Patch

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

> Source/WebKit/UIProcess/API/Cocoa/_WKWebsiteDataStoreConfiguration.h:77
> +@property (nonatomic, nullable, copy, setter=setStandaloneApplication:) NSURL *standaloneApplicationURL WK_API_AVAILABLE(macos(WK_MAC_TBA), ios(WK_IOS_TBA));

Why explicitly specify the setter -setStandaloneApplication: and not just use the default -setStandaloneApplicationURL: ?
Comment 6 John Wilander 2020-04-02 12:17:05 PDT 
(In reply to David Quesada from comment #5)
> Comment on attachment 395284 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=395284&action=review
> 
> > Source/WebKit/UIProcess/API/Cocoa/_WKWebsiteDataStoreConfiguration.h:77
> > +@property (nonatomic, nullable, copy, setter=setStandaloneApplication:) NSURL *standaloneApplicationURL WK_API_AVAILABLE(macos(WK_MAC_TBA), ios(WK_IOS_TBA));
> 
> Why explicitly specify the setter -setStandaloneApplication: and not just
> use the default -setStandaloneApplicationURL: ?

I got build errors without the explicit setter but Alex made some suggestions so I'll remove it before landing.
Comment 7 Alex Christensen 2020-04-02 12:23:44 PDT 
Comment on attachment 395284 [details]
Patch

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

> Tools/WebKitTestRunner/cocoa/TestControllerCocoa.mm:156
> +    if (options.useEphemeralSession && options.standaloneWebApplicationURL.length()) {

This could be made a bit more concise and expandable:

if (options.useEphemeralSession 
    || options.standaloneWebApplicationURL.length()) {
    auto config = options.useEphemeralSession ? [[[_WKWebsiteDataStoreConfiguration alloc] initNonPersistentConfiguration] autorelease] : [[[_WKWebsiteDataStoreConfiguration alloc] init] autorelease];
    if (options.standaloneWebApplicationURL.length())
        config.standaloneApplication = ...;
   // we will probably want to add more things here soon.
    ... // set websiteDataStore using config and enable resource load statistics.
}

> Tools/WebKitTestRunner/cocoa/TestControllerCocoa.mm:157
> +        auto ephemeralWebsiteDataStoreConfig = [[_WKWebsiteDataStoreConfiguration alloc] initNonPersistentConfiguration];

This is a memory leak as it now stands.
Comment 8 John Wilander 2020-04-02 12:26:55 PDT 
(In reply to Alex Christensen from comment #7)
> Comment on attachment 395284 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=395284&action=review
> 
> > Tools/WebKitTestRunner/cocoa/TestControllerCocoa.mm:156
> > +    if (options.useEphemeralSession && options.standaloneWebApplicationURL.length()) {
> 
> This could be made a bit more concise and expandable:
> 
> if (options.useEphemeralSession 
>     || options.standaloneWebApplicationURL.length()) {
>     auto config = options.useEphemeralSession ?
> [[[_WKWebsiteDataStoreConfiguration alloc] initNonPersistentConfiguration]
> autorelease] : [[[_WKWebsiteDataStoreConfiguration alloc] init] autorelease];
>     if (options.standaloneWebApplicationURL.length())
>         config.standaloneApplication = ...;
>    // we will probably want to add more things here soon.
>     ... // set websiteDataStore using config and enable resource load
> statistics.
> }

Much better. Will adopt.

> > Tools/WebKitTestRunner/cocoa/TestControllerCocoa.mm:157
> > +        auto ephemeralWebsiteDataStoreConfig = [[_WKWebsiteDataStoreConfiguration alloc] initNonPersistentConfiguration];
> 
> This is a memory leak as it now stands.

Darn it. I thought I had gone through all of them. Will fix.
Comment 9 John Wilander 2020-04-02 16:40:57 PDT 
Created attachment 395321 [details]
Patch
Comment 10 Alex Christensen 2020-04-02 16:45:01 PDT 
Comment on attachment 395321 [details]
Patch

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

> Tools/TestWebKitAPI/Tests/WebKitCocoa/WKWebsiteDatastore.mm:312
> +TEST(WKWebsiteDataStore, StandaloneApplicationURL)

This test isn't really useful.  It just verifies that properties are copied correctly.
Comment 11 John Wilander 2020-04-02 16:51:09 PDT 
(In reply to Alex Christensen from comment #10)
> Comment on attachment 395321 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=395321&action=review
> 
> > Tools/TestWebKitAPI/Tests/WebKitCocoa/WKWebsiteDatastore.mm:312
> > +TEST(WKWebsiteDataStore, StandaloneApplicationURL)
> 
> This test isn't really useful.  It just verifies that properties are copied
> correctly.

Right, it was added to make sure the setting is maintained through copies of the config. Do you think I should remove it?
Comment 12 Alex Christensen 2020-04-02 17:14:17 PDT 
yes
Comment 13 John Wilander 2020-04-02 17:18:14 PDT 
(In reply to Alex Christensen from comment #12)
> yes

Will do before landing. Waiting for the wk2 bots now. Thanks for the review!
Comment 14 John Wilander 2020-04-02 18:54:09 PDT 
Created attachment 395332 [details]
Patch for landing
Comment 15 EWS 2020-04-02 20:56:49 PDT 
Committed r259440: <https://trac.webkit.org/changeset/259440>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 395332 [details].
Comment 16 Truitt Savell 2020-04-03 09:43:37 PDT 
It looks like these two new tests:

http/tests/resourceLoadStatistics/standalone-web-application-exempt-from-website-data-deletion-database.html
http/tests/resourceLoadStatistics/standalone-web-application-exempt-from-website-data-deletion.html

added in https://trac.webkit.org/changeset/259440/webkit

are failing or timing out. History:
https://results.webkit.org/?suite=layout-tests&suite=layout-tests&test=http%2Ftests%2FresourceLoadStatistics%2Fstandalone-web-application-exempt-from-website-data-deletion.html&test=http%2Ftests%2FresourceLoadStatistics%2Fstandalone-web-application-exempt-from-website-data-deletion-database.html

Results:
https://build.webkit.org/results/Apple-Catalina-Release-WK2-Tests/r259460%20(4334)/results.html
Comment 17 John Wilander 2020-04-03 10:17:18 PDT 
(In reply to Truitt Savell from comment #16)
> It looks like these two new tests:
> 
> http/tests/resourceLoadStatistics/standalone-web-application-exempt-from-
> website-data-deletion-database.html
> http/tests/resourceLoadStatistics/standalone-web-application-exempt-from-
> website-data-deletion.html
> 
> added in https://trac.webkit.org/changeset/259440/webkit
> 
> are failing or timing out. History:
> https://results.webkit.org/?suite=layout-tests&suite=layout-
> tests&test=http%2Ftests%2FresourceLoadStatistics%2Fstandalone-web-
> application-exempt-from-website-data-deletion.
> html&test=http%2Ftests%2FresourceLoadStatistics%2Fstandalone-web-application-
> exempt-from-website-data-deletion-database.html
> 
> Results:
> https://build.webkit.org/results/Apple-Catalina-Release-WK2-Tests/
> r259460%20(4334)/results.html

Thanks for reporting, Truitt. Will look into this.
Comment 18 John Wilander 2020-04-03 10:20:44 PDT 
(In reply to John Wilander from comment #17)
> (In reply to Truitt Savell from comment #16)
> > It looks like these two new tests:
> > 
> > http/tests/resourceLoadStatistics/standalone-web-application-exempt-from-
> > website-data-deletion-database.html
> > http/tests/resourceLoadStatistics/standalone-web-application-exempt-from-
> > website-data-deletion.html
> > 
> > added in https://trac.webkit.org/changeset/259440/webkit
> > 
> > are failing or timing out. History:
> > https://results.webkit.org/?suite=layout-tests&suite=layout-
> > tests&test=http%2Ftests%2FresourceLoadStatistics%2Fstandalone-web-
> > application-exempt-from-website-data-deletion.
> > html&test=http%2Ftests%2FresourceLoadStatistics%2Fstandalone-web-application-
> > exempt-from-website-data-deletion-database.html
> > 
> > Results:
> > https://build.webkit.org/results/Apple-Catalina-Release-WK2-Tests/
> > r259460%20(4334)/results.html
> 
> Thanks for reporting, Truitt. Will look into this.

The linked failure seems to be one missing line in the output, namely the HttpOnly cookie. The weird thing is that it's not saying the cookie has been deleted. There just isn't a result for that cookie.

Will try to reproduce locally and do a manual audit of the code.
Comment 19 John Wilander 2020-04-03 11:08:01 PDT 
(In reply to John Wilander from comment #18)
> (In reply to John Wilander from comment #17)
> > (In reply to Truitt Savell from comment #16)
> > > It looks like these two new tests:
> > > 
> > > http/tests/resourceLoadStatistics/standalone-web-application-exempt-from-
> > > website-data-deletion-database.html
> > > http/tests/resourceLoadStatistics/standalone-web-application-exempt-from-
> > > website-data-deletion.html
> > > 
> > > added in https://trac.webkit.org/changeset/259440/webkit
> > > 
> > > are failing or timing out. History:
> > > https://results.webkit.org/?suite=layout-tests&suite=layout-
> > > tests&test=http%2Ftests%2FresourceLoadStatistics%2Fstandalone-web-
> > > application-exempt-from-website-data-deletion.
> > > html&test=http%2Ftests%2FresourceLoadStatistics%2Fstandalone-web-application-
> > > exempt-from-website-data-deletion-database.html
> > > 
> > > Results:
> > > https://build.webkit.org/results/Apple-Catalina-Release-WK2-Tests/
> > > r259460%20(4334)/results.html
> > 
> > Thanks for reporting, Truitt. Will look into this.
> 
> The linked failure seems to be one missing line in the output, namely the
> HttpOnly cookie. The weird thing is that it's not saying the cookie has been
> deleted. There just isn't a result for that cookie.
> 
> Will try to reproduce locally and do a manual audit of the code.

I've now run 50 iterations with a release build without a repro of the missing output line.
Comment 20 Truitt Savell 2020-04-03 15:04:32 PDT 
Reverted r259440 for reason:

Introduced 2 failing tests on Mac and iOS

Committed r259516: <https://trac.webkit.org/changeset/259516>
Comment 21 John Wilander 2020-04-15 17:22:56 PDT 
Created attachment 396595 [details]
Patch
Comment 22 John Wilander 2020-04-15 20:12:34 PDT 
Comment on attachment 396595 [details]
Patch

mac-debug-wk1 failures are unrelated.
Comment 23 EWS 2020-04-15 20:38:14 PDT 
Committed r260169: <https://trac.webkit.org/changeset/260169>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 396595 [details].
Comment 24 John Wilander 2020-04-16 10:09:49 PDT 
<rdar://problem/61891463> tracks the re-landing.