Bug 181958 - Add callbacks to testRunner.statisticsSetShouldPartitionCookiesForHost() and testRunner.statisticsUpdateCookiePartitioning()
Summary: Add callbacks to testRunner.statisticsSetShouldPartitionCookiesForHost() and ...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: New Bugs (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: John Wilander
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2018-01-22 15:01 PST by Ryan Haddad
Modified: 2018-01-31 20:45 PST (History)
7 users (show)

See Also:


Attachments
Patch (42.90 KB, patch)
2018-01-30 11:03 PST, John Wilander
no flags Details | Formatted Diff | Diff
Patch (42.91 KB, patch)
2018-01-30 11:20 PST, John Wilander
no flags Details | Formatted Diff | Diff
Patch (56.25 KB, patch)
2018-01-30 14:20 PST, John Wilander
no flags Details | Formatted Diff | Diff
Patch (57.01 KB, patch)
2018-01-30 18:18 PST, John Wilander
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Ryan Haddad 2018-01-22 15:01:58 PST
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
Comment 1 Ryan Haddad 2018-01-22 15:02:17 PST
--- /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
Comment 2 Matt Lewis 2018-01-23 17:08:36 PST
Marked as flaky https://trac.webkit.org/changeset/227464/webkit
Comment 3 Radar WebKit Bug Importer 2018-01-23 17:10:43 PST
<rdar://problem/36801804>
Comment 4 John Wilander 2018-01-30 11:03:32 PST
Created attachment 332669 [details]
Patch
Comment 5 John Wilander 2018-01-30 11:20:38 PST
Created attachment 332673 [details]
Patch
Comment 6 John Wilander 2018-01-30 12:50:03 PST
Test issues on mac-wk2 are unrelated to this patch.
Comment 7 John Wilander 2018-01-30 13:12:27 PST
Comment on attachment 332673 [details]
Patch

Seeing one failed instance of add-partitioning-to-redirect.html. Thus making an additional change.
Comment 8 John Wilander 2018-01-30 14:20:33 PST
Created attachment 332707 [details]
Patch
Comment 9 Brent Fulgham 2018-01-30 14:27:20 PST
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"
Comment 10 John Wilander 2018-01-30 14:30:11 PST
(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.
Comment 11 John Wilander 2018-01-30 17:45:25 PST
Looks good except for the missing description?
Comment 12 John Wilander 2018-01-30 18:18:08 PST
Created attachment 332734 [details]
Patch
Comment 13 Brent Fulgham 2018-01-30 20:48:16 PST
Comment on attachment 332734 [details]
Patch

A nice improvement!
Comment 14 John Wilander 2018-01-30 21:38:24 PST
Comment on attachment 332734 [details]
Patch

Thanks, Brent!
Comment 15 WebKit Commit Bot 2018-01-30 22:01:59 PST
Comment on attachment 332734 [details]
Patch

Clearing flags on attachment: 332734

Committed r227875: <https://trac.webkit.org/changeset/227875>
Comment 16 WebKit Commit Bot 2018-01-30 22:02:01 PST
All reviewed patches have been landed.  Closing bug.
Comment 17 Michael Catanzaro 2018-01-31 16:10:54 PST
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.
Comment 18 Alex Christensen 2018-01-31 16:50:23 PST
(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.
Comment 19 Alex Christensen 2018-01-31 16:59:41 PST
Relanded in https://trac.webkit.org/changeset/227943/webkit
Comment 20 Michael Catanzaro 2018-01-31 18:25:19 PST
(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.
Comment 21 Michael Catanzaro 2018-01-31 18:44:44 PST
> 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.
Comment 22 Alex Christensen 2018-01-31 18:52:53 PST
(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.
Comment 23 Michael Catanzaro 2018-01-31 20:45:19 PST
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.