Bug 231971 – http/tests/resourceLoadStatistics/website-data-removal-for-site-navigated-to-with-link-decoration.html is a flaky timeout

• Component: New Bugs
• Status: RESOLVED FIXED

[Kate Cheney]

http/tests/resourceLoadStatistics/website-data-removal-for-site-navigated-to-with-link-decoration.html is a flaky timeout

[Kate Cheney]

Created attachment 441764 [details]
Patch

[Kate Cheney]

rdar://84270108

[John Wilander]

Comment on attachment 441764 [details]
Patch

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

> LayoutTests/http/tests/resourceLoadStatistics/operating-dates-all-but-cookies-not-removed-with-user-interaction-6-days-ago.html:236
> +        await resetCookies();

Shouldn't this have been a JS failure before or is it just a warning in IDEs?

> LayoutTests/http/tests/resourceLoadStatistics/website-data-removal-for-site-navigated-to-with-link-decoration-expected.txt:23
> +        localhost

This is surprising. Do you know what caused this change?

> LayoutTests/http/tests/resourceLoadStatistics/website-data-removal-for-site-navigated-to-with-link-decoration-expected.txt:35
> +        127.0.0.1

Ditto since this mirrors the above.

[Kate Cheney]

Comment on attachment 441764 [details]
Patch

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

>> LayoutTests/http/tests/resourceLoadStatistics/operating-dates-all-but-cookies-not-removed-with-user-interaction-6-days-ago.html:236
>> +        await resetCookies();
> 
> Shouldn't this have been a JS failure before or is it just a warning in IDEs?

Not sure about the warning, but it didn't cause a JS error.

>> LayoutTests/http/tests/resourceLoadStatistics/website-data-removal-for-site-navigated-to-with-link-decoration-expected.txt:23
>> +        localhost
> 
> This is surprising. Do you know what caused this change?

See ChangeLog. Reseting cookies involves a redirect, which changes the prevalence and data records removed as well.

[John Wilander]

Comment on attachment 441764 [details]
Patch

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

>>> LayoutTests/http/tests/resourceLoadStatistics/website-data-removal-for-site-navigated-to-with-link-decoration-expected.txt:23
>>> +        localhost
>> 
>> This is surprising. Do you know what caused this change?
> 
> See ChangeLog. Reseting cookies involves a redirect, which changes the prevalence and data records removed as well.

I see. Does it really cause a redirect though? Or a navigation? Why didn't we see flakiness adding that extra redirect before? Maybe we did? I suspect this is our delayed redirect detection kicking in. Do you think so too?

[Kate Cheney]

(In reply to John Wilander from comment #5)
> Comment on attachment 441764 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=441764&action=review
> 
> >>> LayoutTests/http/tests/resourceLoadStatistics/website-data-removal-for-site-navigated-to-with-link-decoration-expected.txt:23
> >>> +        localhost
> >> 
> >> This is surprising. Do you know what caused this change?
> > 
> > See ChangeLog. Reseting cookies involves a redirect, which changes the prevalence and data records removed as well.
> 
> I see. Does it really cause a redirect though? Or a navigation? Why didn't
> we see flakiness adding that extra redirect before? Maybe we did? I suspect
> this is our delayed redirect detection kicking in. Do you think so too?

It causes a window.open(), which maybe we don't store in ITP. Do we have delayed redirect detection? If so, then that is probably what this is. It probably wasn't kicking in before because resetCookies() never finished - notifyDone was always called immediately after. My guess is the shift from php to python changed the timing up to make this occasionally flaky.

[John Wilander]

Comment on attachment 441764 [details]
Patch

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

>>>>> LayoutTests/http/tests/resourceLoadStatistics/website-data-removal-for-site-navigated-to-with-link-decoration-expected.txt:23
>>>>> +        localhost
>>>> 
>>>> This is surprising. Do you know what caused this change?
>>> 
>>> See ChangeLog. Reseting cookies involves a redirect, which changes the prevalence and data records removed as well.
>> 
>> I see. Does it really cause a redirect though? Or a navigation? Why didn't we see flakiness adding that extra redirect before? Maybe we did? I suspect this is our delayed redirect detection kicking in. Do you think so too?
> 
> It causes a window.open(), which maybe we don't store in ITP. Do we have delayed redirect detection? If so, then that is probably what this is. It probably wasn't kicking in before because resetCookies() never finished - notifyDone was always called immediately after. My guess is the shift from php to python changed the timing up to make this occasionally flaky.

I bet you it opens a popup that in turn redirects to be able to clear cookies for both 127.0.0.1 and localhost.

> LayoutTests/http/tests/resourceLoadStatistics/website-data-removal-for-site-navigated-to-with-link-decoration-expected.txt:29
> +    dataRecordsRemoved: 2

This is strange too. Do you know why an additional data removal run is done?

[Kate Cheney]

(In reply to John Wilander from comment #7)
> Comment on attachment 441764 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=441764&action=review
> 
> >>>>> LayoutTests/http/tests/resourceLoadStatistics/website-data-removal-for-site-navigated-to-with-link-decoration-expected.txt:23
> >>>>> +        localhost
> >>>> 
> >>>> This is surprising. Do you know what caused this change?
> >>> 
> >>> See ChangeLog. Reseting cookies involves a redirect, which changes the prevalence and data records removed as well.
> >> 
> >> I see. Does it really cause a redirect though? Or a navigation? Why didn't we see flakiness adding that extra redirect before? Maybe we did? I suspect this is our delayed redirect detection kicking in. Do you think so too?
> > 
> > It causes a window.open(), which maybe we don't store in ITP. Do we have delayed redirect detection? If so, then that is probably what this is. It probably wasn't kicking in before because resetCookies() never finished - notifyDone was always called immediately after. My guess is the shift from php to python changed the timing up to make this occasionally flaky.
> 
> I bet you it opens a popup that in turn redirects to be able to clear
> cookies for both 127.0.0.1 and localhost.
> 
> > LayoutTests/http/tests/resourceLoadStatistics/website-data-removal-for-site-navigated-to-with-link-decoration-expected.txt:29
> > +    dataRecordsRemoved: 2
> 
> This is strange too. Do you know why an additional data removal run is done?

I think because 127.0.0.1 is newly prevalent (because it is redirected-to by a prevalent domain) so it does another processing and removes that data.

[Kate Cheney]

(In reply to Kate Cheney from comment #8)
> (In reply to John Wilander from comment #7)
> > Comment on attachment 441764 [details]
> > Patch
> > 
> > View in context:
> > https://bugs.webkit.org/attachment.cgi?id=441764&action=review
> > 
> > >>>>> LayoutTests/http/tests/resourceLoadStatistics/website-data-removal-for-site-navigated-to-with-link-decoration-expected.txt:23
> > >>>>> +        localhost
> > >>>> 
> > >>>> This is surprising. Do you know what caused this change?
> > >>> 
> > >>> See ChangeLog. Reseting cookies involves a redirect, which changes the prevalence and data records removed as well.
> > >> 
> > >> I see. Does it really cause a redirect though? Or a navigation? Why didn't we see flakiness adding that extra redirect before? Maybe we did? I suspect this is our delayed redirect detection kicking in. Do you think so too?
> > > 
> > > It causes a window.open(), which maybe we don't store in ITP. Do we have delayed redirect detection? If so, then that is probably what this is. It probably wasn't kicking in before because resetCookies() never finished - notifyDone was always called immediately after. My guess is the shift from php to python changed the timing up to make this occasionally flaky.
> > 
> > I bet you it opens a popup that in turn redirects to be able to clear
> > cookies for both 127.0.0.1 and localhost.
> > 
> > > LayoutTests/http/tests/resourceLoadStatistics/website-data-removal-for-site-navigated-to-with-link-decoration-expected.txt:29
> > > +    dataRecordsRemoved: 2
> > 
> > This is strange too. Do you know why an additional data removal run is done?
> 
> I think because 127.0.0.1 is newly prevalent (because it is redirected-to by
> a prevalent domain) so it does another processing and removes that data.

To clarify, 127.0.0.1 is first not-prevalent, so IDB and LocalStorage are removed (first removal). Then it becomes prevalent, so cookies are removed (second removal).

[John Wilander]

I have to look at the setup here and understand what’s getting classified and not.

[John Wilander]

I think I now have a grasp of this.

This test is about navigations from a prevalent source domain to a non-prevalent destination domain, with link decoration. In such cases, non-cookie website data should be marked for deletion on the non-prevalent destination domain.

In this test, localhost is the prevalent source domain and 127.0.0.1 is the non-prevalent destination domain.

Making a redirect to a prevalent domain marks the redirecting domains as prevalent too. This is called collusion detection.

The change adds a redirect from the non-prevalent 127.0.0.1 to the prevalent localhost, effective making 127.0.0.1 prevalent too. This risks hiding a regression or introducing flakiness since 1) prevalent domains get their website data deleted too and 127.0.0.1 will no be in that category regardless of link decoration (the regression hiding risk), and 2) website data removal for prevalent resources without user interaction as first party includes cookies (the flakiness risk).

I believe the reason why I dump ITP data here is to make sure that 127.0.0.1 is *not* prevalent and gets its non-cookie website data deleted anyway. That's the expected behavior and the intent of the link decoration rule.

We have to come up with a way to reset cookies without adding this extra redirect causing 127.0.0.1 to be marked prevalent.

[John Wilander]

Let's have a look at how resetCookies() works.

[John Wilander]

(In reply to John Wilander from comment #12)
> Let's have a look at how resetCookies() works.

The gist of it (a bunch of code removed):

***

let urls = [
    "http://127.0.0.1:8000",
    "http://localhost:8000",
];

for (let url of urls) {
    g_childWindow = window.open(url + "/cookies/resources/cookie-utility.py?queryfunction=deleteCookiesAndPostMessage", "reset");
}

***

I believe this causes the global child window to navigate from 127.0.0.1 to localhost before unload has fired which has always been considered an effective redirect in WebKit.

The best course of action here is to delete these cookies ourselves. Let's add a resourceLoadStatistics/resources/delete-all-cookies.py file and open it in a same-site 127.0.0.1 iframe. When it posts back to the main frame that it's done, we end the test.

[John Wilander]

(In reply to John Wilander from comment #13)
> (In reply to John Wilander from comment #12)
> > Let's have a look at how resetCookies() works.
> 
> The gist of it (a bunch of code removed):
> 
> ***
> 
> let urls = [
>     "http://127.0.0.1:8000",
>     "http://localhost:8000",
> ];
> 
> for (let url of urls) {
>     g_childWindow = window.open(url +
> "/cookies/resources/cookie-utility.
> py?queryfunction=deleteCookiesAndPostMessage", "reset");
> }
> 
> ***
> 
> I believe this causes the global child window to navigate from 127.0.0.1 to
> localhost before unload

*onload

> has fired which has always been considered an
> effective redirect in WebKit.
> 
> The best course of action here is to delete these cookies ourselves. Let's
> add a resourceLoadStatistics/resources/delete-all-cookies.py file and open
> it in a same-site 127.0.0.1 iframe. When it posts back to the main frame
> that it's done, we end the test.

[Kate Cheney]

Created attachment 441961 [details]
Patch

[John Wilander]

Comment on attachment 441961 [details]
Patch

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

I don't think we need to clear any cookies for localhost because we don't set cookies for localhost. We just need to clear cookies for 127.0.0.1 which is same-site at the final page load so a mere iframe loading a simple "delete all cookies I see incoming" Python file should do it.

> LayoutTests/http/tests/resourceLoadStatistics/resources/util.js:37
> +                testRunner.setAlwaysAcceptCookies(false);

I'm not sure this bypasses ITP's cookie blocking which is not based on CFNetwork's policy mechanism.

[Kate Cheney]

(In reply to John Wilander from comment #16)
> Comment on attachment 441961 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=441961&action=review
> 
> I don't think we need to clear any cookies for localhost because we don't
> set cookies for localhost. We just need to clear cookies for 127.0.0.1 which
> is same-site at the final page load so a mere iframe loading a simple
> "delete all cookies I see incoming" Python file should do it.

I want this to be useful for all ITP tests that set cookies. If there are any that might need to clear cookies for localhost now or in the future I think we should keep it.

> 
> > LayoutTests/http/tests/resourceLoadStatistics/resources/util.js:37
> > +                testRunner.setAlwaysAcceptCookies(false);
> 
> I'm not sure this bypasses ITP's cookie blocking which is not based on
> CFNetwork's policy mechanism.

I don't think that is the point. I think its just a reset to consistent state mechanism.

[John Wilander]

(In reply to Kate Cheney from comment #17)
> (In reply to John Wilander from comment #16)
> > Comment on attachment 441961 [details]
> > Patch
> > 
> > View in context:
> > https://bugs.webkit.org/attachment.cgi?id=441961&action=review
> > 
> > I don't think we need to clear any cookies for localhost because we don't
> > set cookies for localhost. We just need to clear cookies for 127.0.0.1 which
> > is same-site at the final page load so a mere iframe loading a simple
> > "delete all cookies I see incoming" Python file should do it.
> 
> I want this to be useful for all ITP tests that set cookies. If there are
> any that might need to clear cookies for localhost now or in the future I
> think we should keep it.
> 
> > 
> > > LayoutTests/http/tests/resourceLoadStatistics/resources/util.js:37
> > > +                testRunner.setAlwaysAcceptCookies(false);
> > 
> > I'm not sure this bypasses ITP's cookie blocking which is not based on
> > CFNetwork's policy mechanism.
> 
> I don't think that is the point. I think its just a reset to consistent
> state mechanism.

Sorry, I'm saying you can't open a cross-site iframe to delete cookies because ITP blocks all cross-site cookie access and you can't relax ITP's blocking with testRunner.setAlwaysAcceptCookies(). I thought that was what you're doing.

I suggested that we just open a same-site iframe to 127.0.0.1 to clear its cookies since those are the only cookies we set in this test case. If we had set cookies for localhost, we would have had to navigate to a localhost first party context to be able to clear them.

[Kate Cheney]

(In reply to John Wilander from comment #18)
> (In reply to Kate Cheney from comment #17)
> > (In reply to John Wilander from comment #16)
> > > Comment on attachment 441961 [details]
> > > Patch
> > > 
> > > View in context:
> > > https://bugs.webkit.org/attachment.cgi?id=441961&action=review
> > > 
> > > I don't think we need to clear any cookies for localhost because we don't
> > > set cookies for localhost. We just need to clear cookies for 127.0.0.1 which
> > > is same-site at the final page load so a mere iframe loading a simple
> > > "delete all cookies I see incoming" Python file should do it.
> > 
> > I want this to be useful for all ITP tests that set cookies. If there are
> > any that might need to clear cookies for localhost now or in the future I
> > think we should keep it.
> > 
> > > 
> > > > LayoutTests/http/tests/resourceLoadStatistics/resources/util.js:37
> > > > +                testRunner.setAlwaysAcceptCookies(false);
> > > 
> > > I'm not sure this bypasses ITP's cookie blocking which is not based on
> > > CFNetwork's policy mechanism.
> > 
> > I don't think that is the point. I think its just a reset to consistent
> > state mechanism.
> 
> Sorry, I'm saying you can't open a cross-site iframe to delete cookies
> because ITP blocks all cross-site cookie access and you can't relax ITP's
> blocking with testRunner.setAlwaysAcceptCookies(). I thought that was what
> you're doing.
> 
> I suggested that we just open a same-site iframe to 127.0.0.1 to clear its
> cookies since those are the only cookies we set in this test case. If we had
> set cookies for localhost, we would have had to navigate to a localhost
> first party context to be able to clear them.

Ah, I see. Ok so then I will remove localhost. I just checked and none of these tests set cookies for localhost, and we can add that in the future if we need it for a new test.

[Kate Cheney]

Created attachment 442044 [details]
Patch

[John Wilander]

Comment on attachment 442044 [details]
Patch

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

r=me with comment. Also, see if you can get the style checker to be happy.

> LayoutTests/http/tests/resourceLoadStatistics/resources/util.js:19
> +        "http://127.0.0.1:8000",

Could we make this a parameter instead? And throw an error if the current location doesn't match the parameter? That would make it explicit to anyone using the function and also avoid mistakes.

[Kate Cheney]

Created attachment 442046 [details]
Patch for landing

[Kate Cheney]

(In reply to John Wilander from comment #21)
> Comment on attachment 442044 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=442044&action=review
> 
> r=me with comment. Also, see if you can get the style checker to be happy.
> 

Thanks! Moving the module level import to the top of the file causes timeouts, I think it has to be after:

file = __file__.split(':/cygwin')[-1]
http_root = os.path.dirname(os.path.dirname(os.path.abspath(os.path.dirname(file))))
sys.path.insert(0, http_root)

This is how it is in the regular resetCookies() function also.

> > LayoutTests/http/tests/resourceLoadStatistics/resources/util.js:19
> > +        "http://127.0.0.1:8000",
> 
> Could we make this a parameter instead? And throw an error if the current
> location doesn't match the parameter? That would make it explicit to anyone
> using the function and also avoid mistakes.

Fixed.

[EWS]

Committed r284631 (243352@main): <https://commits.webkit.org/243352@main>

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