web-platform-tests / results-collection

Other
41 stars 46 forks source link

`cookies/prefix/__secure.header.https.html` results seem off. #599

Closed mikewest closed 6 years ago

mikewest commented 6 years ago

https://wpt.fyi/results/cookies/prefix/__secure.header.https.html shows as failing on Firefox and Chrome. It seems to pass on both Chromium's bots and in Chrome when run directly from https://w3c-test.org/cookies/prefix/__secure.header.https.html. I'm happy to dig in a bit to try to figure out why it's failing in the harness, but it's not at all clear to me where I could look for logs or anything else.

Halp? :)

jugglinmike commented 6 years ago

I'm on the case!

jugglinmike commented 6 years ago

This looks like a problem with test interaction.

To reproduce with the WPT CLI, run the following commands:

$ ./wpt run firefox cookies/prefix/__secure.document-cookie.https.html
$ ./wpt run firefox cookies/prefix/__secure.header.https.html
$ ./wpt run firefox cookies/prefix/__secure.document-cookie.https.html cookies/prefix/__secure.header.https.html

The first two will pass, but the third will fail, ostensibly due to __secure.header.https.html. This is also reproducible using chrome in place of firefox.

To reproduce with w3c-test.org Visit pages in the following sequence:

  1. https://w3c-test.org/cookies/prefix/__secure.header.https.html (all sub-tests pass)
  2. https://w3c-test.org/cookies/prefix/__secure.document-cookie.https.html (all sub-tests pass)
  3. https://w3c-test.org/cookies/prefix/__secure.header.https.html (all sub-tests fail)

Both tests have been written to defensively remove cookies prior to testing via the erase_cookie_from_js utility function. That function attempts to remove cookies by overwriting with an expired entry that has no domain attribute, but the final sub-test in __secure.document-cookie.https.html sets a cookie with an explicit domain attribute.

This is where I get a little lost. In HTML, document.cookie delegates to rfc6265, where the Storage Model describes how the domain is interpreted. According to steps 4 through 6, these strings should describe the same cookie in the store.

However, Chrome, Edge, Firefox, IE, and Safari all store these cookies independently:

<script>
document.cookie = 'foo=1';
document.cookie = 'foo=2';
document.cookie = 'foo=3; domain=' + document.location.hostname;
document.cookie = 'foo=4; domain=' + document.location.hostname;
console.log(document.cookie); // 'foo=2; foo=4'
</script>

That would explain the failure we're seeing: __secure.document-cookie.https.html finishes by setting a cookie which __secure.header.https.html fails to delete, causing an error in its "sanity check." It would even account for the fact that Safari Technology Preview passes the reported test. STP is the only browser that fails __secure.document-cookie.https.html, so it never sets the domain-specifying cookie.

This is a strong indication that I've misinterpreted the spec. Given that we work on different time zones, verifying this could delay a fix for another day at least. To avoid that, I've filed a patch based on the assumption that the mistake is in my reading (and not all the major browsers): https://github.com/web-platform-tests/wpt/pull/12817

jugglinmike commented 6 years ago

Separately, here's my take on how an unstable test like this could make its way into WPT's master branch. The project's continuous integration system rejects tests that are unstable, so the patch that introduced it should have been blocked in the review process. Instead, CI labeled the patch as stable.

This seems to be a case of unfortunate scheduling. The following command executes each test 10 times:

./wpt check-stability chrome:dev cookies/prefix/__secure.{document-cookie,header}.https.html

It reports __secure.document-cookie.https.html as "stable" (consistently passing), and it reports __secure.header.https.html as "stable" (consistently failing). It exits with a code of 0 because it does not identify inconsistent behavior. That's because for each iteration, wpt check-stability executes the two tests in series and then restarts the browser.

WPT offers a second command for verifying test stability:

./wpt run --verify chrome submissions/cookies/prefix/__secure.{document-cookie,header}.https.html

That command reports __secure.document-cookie.https.html as unstable (failing 50% of the time), and it reports __secure.header.https.html as "stable" (consistently passing). It exits with a non-zero exit code. It does this because it runs the first test 10 times in series and then runs the second test 10 times in series. (It reports 5 passes and 5 failures because it restarts the browser after each failure.)

We've recently been discussing relevant changes to the CI setup, so I'll raise this issue there.

mikewest commented 6 years ago

This is where I get a little lost. In HTML, document.cookie delegates to rfc6265, where the Storage Model describes how the domain is interpreted. According to steps 4 through 6, these strings should describe the same cookie in the store.

However, Chrome, Edge, Firefox, IE, and Safari all store these cookies independently:

Amusingly, we just fixed this in the cookie spec: https://github.com/httpwg/http-extensions/commit/b415d03dbf4f009d298c69a50bde9eb6248e2a99

Thank you very much for digging in, and apologies that it was my own fault. :(

jugglinmike commented 6 years ago

Thanks for the link (it's good to know my reading was accurate), and no worries about the mistake