Bug 209634 – Add SPI to configure WebsiteDataStores with a URL for standalone web applications and use it to disable first-party website data removal in ITP

• Component: WebKit API
• Status: RESOLVED FIXED

[David Quesada]

rdar://problem/60943970

[David Quesada]

Created attachment 394683 [details]
Patch

[Sam Weinig]

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.

[Alex Christensen]

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)

[John Wilander]

Created attachment 395284 [details]
Patch

[David Quesada]

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: ?

[John Wilander]

(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.

[Alex Christensen]

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.

[John Wilander]

(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.

[John Wilander]

Created attachment 395321 [details]
Patch

[Alex Christensen]

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.

[John Wilander]

(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?

[Alex Christensen]

yes

[John Wilander]

(In reply to Alex Christensen from comment #12)
> yes

Will do before landing. Waiting for the wk2 bots now. Thanks for the review!

[John Wilander]

Created attachment 395332 [details]
Patch for landing

[EWS]

Committed r259440: <https://trac.webkit.org/changeset/259440>

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

[Truitt Savell]

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

[John Wilander]

(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.

[John Wilander]

(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.

[John Wilander]

(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.

[Truitt Savell]

Reverted r259440 for reason:

Introduced 2 failing tests on Mac and iOS

Committed r259516: <https://trac.webkit.org/changeset/259516>

[John Wilander]

Created attachment 396595 [details]
Patch

[John Wilander]

Comment on attachment 396595 [details]
Patch

mac-debug-wk1 failures are unrelated.

[EWS]

Committed r260169: <https://trac.webkit.org/changeset/260169>

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

[John Wilander]

<rdar://problem/61891463> tracks the re-landing.