Bug 181958 – Add callbacks to testRunner.statisticsSetShouldPartitionCookiesForHost() and testRunner.statisticsUpdateCookiePartitioning()

• Component: New Bugs
• Status: RESOLVED FIXED

[Ryan Haddad]

The following layout test is flaky on High Sierra WK2

http/tests/resourceLoadStatistics/non-prevalent-resources-can-access-cookies-in-a-third-party-context.html

Probable cause:

This test seems to have been flaky since it was added in https://trac.webkit.org/changeset/227103

Flakiness Dashboard:

https://webkit-test-results.webkit.org/dashboards/flakiness_dashboard.html#showAllRuns=true&tests=http%2Ftests%2FresourceLoadStatistics%2Fnon-prevalent-resources-can-access-cookies-in-a-third-party-context.html

[Ryan Haddad]

--- /Volumes/Data/slave/highsierra-release-tests-wk2/build/layout-test-results/http/tests/resourceLoadStatistics/non-prevalent-resources-can-access-cookies-in-a-third-party-context-expected.txt
+++ /Volumes/Data/slave/highsierra-release-tests-wk2/build/layout-test-results/http/tests/resourceLoadStatistics/non-prevalent-resources-can-access-cookies-in-a-third-party-context-actual.txt
@@ -22,13 +22,13 @@
 --------
 Should receive one partitioned, third party cookie.
 Did not receive cookie named 'firstPartyCookie'.
-Received cookie named 'partitionedCookie'.
-Client-side document.cookie: partitionedCookie=value
+Did not receive cookie named 'partitionedCookie'.
+Client-side document.cookie:
 
 --------
 Frame: '<!--framePath //<!--frame3-->-->'
 --------
 After user interaction, should receive one non-partitioned, first party cookie.
 Received cookie named 'firstPartyCookie'.
-Did not receive cookie named 'partitionedCookie'.
-Client-side document.cookie: firstPartyCookie=value
+Received cookie named 'partitionedCookie'.
+Client-side document.cookie: firstPartyCookie=value,partitionedCookie=value

[Matt Lewis]

Marked as flaky https://trac.webkit.org/changeset/227464/webkit

[Radar WebKit Bug Importer]

<rdar://problem/36801804>

[John Wilander]

Created attachment 332669 [details]
Patch

[John Wilander]

Created attachment 332673 [details]
Patch

[John Wilander]

Test issues on mac-wk2 are unrelated to this patch.

[John Wilander]

Comment on attachment 332673 [details]
Patch

Seeing one failed instance of add-partitioning-to-redirect.html. Thus making an additional change.

[John Wilander]

Created attachment 332707 [details]
Patch

[Brent Fulgham]

Comment on attachment 332707 [details]
Patch

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

> Source/WebKit/ChangeLog:10
> +

Need some descriptive language here. It's hard to know what's going on in this patch!

"Because of the asynchronous nature of cookie handling, test runs can generate false failures because of timing differences. Switch to a completion handler model that prevents post-cookie change logic from running until the processing has actually finished"

[John Wilander]

(In reply to Brent Fulgham from comment #9)
> Comment on attachment 332707 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=332707&action=review
> 
> > Source/WebKit/ChangeLog:10
> > +
> 
> Need some descriptive language here. It's hard to know what's going on in
> this patch!
> 
> "Because of the asynchronous nature of cookie handling, test runs can
> generate false failures because of timing differences. Switch to a
> completion handler model that prevents post-cookie change logic from running
> until the processing has actually finished"

True. Will fix.

[John Wilander]

Looks good except for the missing description?

[John Wilander]

Created attachment 332734 [details]
Patch

[Brent Fulgham]

Comment on attachment 332734 [details]
Patch

A nice improvement!

[John Wilander]

Comment on attachment 332734 [details]
Patch

Thanks, Brent!

[WebKit Commit Bot]

Comment on attachment 332734 [details]
Patch

Clearing flags on attachment: 332734

Committed r227875: <https://trac.webkit.org/changeset/227875>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Michael Catanzaro]

I rolled this out in bug #182357. Reopening.

John, I've landed fixups in bug #182222 and bug #181231 for the last times you modified resource load statistics code that affected the TestController, but don't want to keep playing catch-up here, so please be sure to update the C API and cross-platform TestController before re-landing.

I've also filed bug #182355 because it looks like the Cocoa-specific API may not be needed here at all; that seems to be the root cause of this problem.

[Alex Christensen]

(In reply to Michael Catanzaro from comment #17)
> John, I've landed fixups in bug #182222 and bug #181231 for the last times
> you modified resource load statistics code that affected the TestController,
> but don't want to keep playing catch-up here, so please be sure to update
> the C API and cross-platform TestController before re-landing.
EWS was green.  We did not break the build.  We do not have a linux environment.  We are not under obligation to make all new features completely cross platform.  We have had little support from the linux community in developing this feature.  Fixing things like this is doing the necessary work to port new features to new platforms.  I hope you've been satisfied in my quick reviewing of your patches to do so.  If nobody from the linux community is willing to do the work to maintain this on linux, we can disable the feature on linux.

> I've also filed bug #182355 because it looks like the Cocoa-specific API may
> not be needed here at all; that seems to be the root cause of this problem.
We are preferring the ObjC API more and more.  It is what we have exposed to our third party developers.

[Alex Christensen]

Relanded in https://trac.webkit.org/changeset/227943/webkit

[Michael Catanzaro]

(In reply to Alex Christensen from comment #18)
> EWS was green.  We did not break the build. We do not have a linux environment.

But normally, when you add unimplemented functionality, you give us a heads-up, either asking us to implement the missing code before landing the patch, or just landing and filing a bug for us to implement it later. (Either way, we really appreciate it. :) And normally, when you leave unimplemented code, it's because there is actual platform-specific code that needs to be written, which is often hard to do when flying blind without access to the platform in question. I'm actually very grateful for how well this normally works nowadays; we used to have a lot more trouble a couple years ago.

In contrast, in the specific case of resource load statistics, the C API and TestController breakage has been purely gratuitous: it requires very little effort to add a couple callbacks and hook them up. This is purely mechanical work that can easily be tested on EWS, or by locally changing a couple #ifdefs to switch the TestController to using the C API, if local testing is desired. Alternatively, at least please add FIXMEs to the code, and a bug report to implement the missing functionality, instead of hiding the problem by adding empty callbacks. (A bug report would probably be harder than just implementing it, but whatever.)

I think a rollout was appropriate here, because it's the third time this has happened in resource load statistics recently, and it's really slowing me down. The prior two cases both broke layout tests; this time, we wouldn't have noticed until whenever we try to implement cookie storage partitioning, then it would be quite hard to find the unimplemented callbacks.

> We are not under obligation to make all new features completely cross platform.

Of course; I'd never expect that.

> We have had little support from the linux
> community in developing this feature.  Fixing things like this is doing the
> necessary work to port new features to new platforms.  I hope you've been
> satisfied in my quick reviewing of your patches to do so.  If nobody from
> the linux community is willing to do the work to maintain this on linux, we
> can disable the feature on linux.

I do wish we had more time to help with intelligent tracking prevention development.... Cookie storage partitioning is actually already disabled on all ports except for Cocoa. The rest of the resource load statistics code seems to be working fine already, except for the failing grandfathering test.

> We are preferring the ObjC API more and more.  It is what we have exposed to
> our third party developers.

It's great that you're exposing the nice Objective C API rather than the WK2 C API. But using it in the TestController is going to make development harder for everyone. It turns out resource load statistics accounts for 95% of the Cocoa API use in TestController. We probably should discuss this on webkit-dev eventually, but I've filed bug #182355 to explore it for now. I've left a comment there with some brainstorming.

[Michael Catanzaro]

> The prior two cases both broke layout tests; this time, we wouldn't
> have noticed until whenever we try to implement cookie storage partitioning,
> then it would be quite hard to find the unimplemented callbacks.

I'm just going to leave it broken, because I don't want to keep landing follow-ups. It's going to make it unnecessarily-difficult to get this feature working. FWIW, I only noticed this at all due to luck, because I was testing out other changes and noticed a merge conflict.

[Alex Christensen]

(In reply to Michael Catanzaro from comment #20)
> In contrast, in the specific case of resource load statistics, the C API and
> TestController breakage has been purely gratuitous: it requires very little
> effort to add a couple callbacks and hook them up.
We are not trying to deter resource load statistics development on non-Cocoa platforms.  Actually we designed it so that it could be ported to other platforms.  Resource load statistics was initially implemented on Cocoa platforms only, though.  We are simply following an existing pattern when we add new things to WKTR.  If you go through WKTR's use of Cocoa API and replace it with C API use, then we will be less likely to continue adding test-only Cocoa API just for WKTR and implementing it on Cocoa platforms only.

[Michael Catanzaro]

Can we at least get FIXMEs and bug reports to fill in the missing bits? Otherwise, we don't really have any chance to ever get it right....

(In reply to Alex Christensen from comment #22)
> We are simply following an existing pattern when we
> add new things to WKTR. 

TestController is currently almost entirely cross-platform, except for (a) a couple things that are actually platform-specific, (b) a couple exceptions that we should investigate, and (c) resource load statistics. Resource load statistics is at least 95% of the problematic code.

> If you go through WKTR's use of Cocoa API and
> replace it with C API use, then we will be less likely to continue adding
> test-only Cocoa API just for WKTR and implementing it on Cocoa platforms
> only.

I'm trying to understand this; I know you must have a reason, but I don't see it. We should continue this discussion in bug #182355, or take it to webkit-dev.