Bug 231379 – [ BigSur wk2 Release ] http/tests/resourceLoadStatistics/sandboxed-nesting-iframe-with-non-sandboxed-iframe-redirect-ip-to-localhost-to-ip.html is a flaky failure

• Component: New Bugs
• Status: RESOLVED FIXED

[Eric Hutchison]

http/tests/resourceLoadStatistics/sandboxed-nesting-iframe-with-non-sandboxed-iframe-redirect-ip-to-localhost-to-ip.html

is a flaky failure on BigSur wk2 Release.


History: https://results.webkit.org/?suite=layout-tests&test=http%2Ftests%2FresourceLoadStatistics%2Fsandboxed-nesting-iframe-with-non-sandboxed-iframe-redirect-ip-to-localhost-to-ip.html

Build: https://build.webkit.org/#/builders/70/builds/5699

Results: https://build.webkit.org/results/Apple-BigSur-Release-WK2-Tests/r283709%20(5699)/results.html

Diff:
--- /Volumes/Data/worker/bigsur-release-tests-wk2/build/layout-test-results/http/tests/resourceLoadStatistics/sandboxed-nesting-iframe-with-non-sandboxed-iframe-redirect-ip-to-localhost-to-ip-expected.txt
+++ /Volumes/Data/worker/bigsur-release-tests-wk2/build/layout-test-results/http/tests/resourceLoadStatistics/sandboxed-nesting-iframe-with-non-sandboxed-iframe-redirect-ip-to-localhost-to-ip-actual.txt
@@ -4,9 +4,10 @@
 
 
 PASS testRunner.isStatisticsRegisteredAsSubFrameUnder("http://localhost", "http://127.0.0.1") is true
-PASS testRunner.isStatisticsRegisteredAsRedirectingTo("http://localhost", "http://127.0.0.1") is true
+FAIL testRunner.isStatisticsRegisteredAsRedirectingTo("http://localhost", "http://127.0.0.1") should be true. Was false.
 PASS testRunner.isStatisticsRegisteredAsRedirectingTo("http://127.0.0.1", "http://localhost") is true
 PASS successfullyParsed is true
+Some tests failed.
 
 TEST COMPLETE

Unable to reproduce text failure locally using r283709 and r283714.

[Radar WebKit Bug Importer]

<rdar://problem/83991245>

[Eric Hutchison]

Updated test expectations at http://trac.webkit.org/changeset/283727/webkit

[John Wilander]

Created attachment 441657 [details]
Patch

[Chris Dumez]

Comment on attachment 441657 [details]
Patch

Can't we add `Cache-Control: no-cache, no-store` header to resourceLoadStatistics/resources/redirect.py ? Do we ever want to cache those redirects?

[John Wilander]

(In reply to Chris Dumez from comment #4)
> Comment on attachment 441657 [details]
> Patch
> 
> Can't we add `Cache-Control: no-cache, no-store` header to
> resourceLoadStatistics/resources/redirect.py ? Do we ever want to cache
> those redirects?

We could. I do recall testing WebKit behavior at some point and redirects were cached regardless. Maybe that was treating temporary redirects as permanent though.

[John Wilander]

(In reply to John Wilander from comment #5)
> (In reply to Chris Dumez from comment #4)
> > Comment on attachment 441657 [details]
> > Patch
> > 
> > Can't we add `Cache-Control: no-cache, no-store` header to
> > resourceLoadStatistics/resources/redirect.py ? Do we ever want to cache
> > those redirects?
> 
> We could. I do recall testing WebKit behavior at some point and redirects
> were cached regardless. Maybe that was treating temporary redirects as
> permanent though.

I thought this through over last night and the only obstacle I came up with is the explicit cache testing in resourceLoadStatistics/cap-cache-max-age-for-prevalent-resource.html. But it uses the dedicated resourceLoadStatistics/resources/cached-permanent-redirect.py so we're good.

Would you prefer only the no-cache response header or a combo?

[Chris Dumez]

(In reply to John Wilander from comment #6)
> (In reply to John Wilander from comment #5)
> > (In reply to Chris Dumez from comment #4)
> > > Comment on attachment 441657 [details]
> > > Patch
> > > 
> > > Can't we add `Cache-Control: no-cache, no-store` header to
> > > resourceLoadStatistics/resources/redirect.py ? Do we ever want to cache
> > > those redirects?
> > 
> > We could. I do recall testing WebKit behavior at some point and redirects
> > were cached regardless. Maybe that was treating temporary redirects as
> > permanent though.
> 
> I thought this through over last night and the only obstacle I came up with
> is the explicit cache testing in
> resourceLoadStatistics/cap-cache-max-age-for-prevalent-resource.html. But it
> uses the dedicated
> resourceLoadStatistics/resources/cached-permanent-redirect.py so we're good.
> 
> Would you prefer only the no-cache response header or a combo?

I think I would prefer "Cache-Control: no-cache, no-store" if it works here. Generating unique URLs is a bit annoying and you have to keep doing it in new tests.

[John Wilander]

(In reply to Chris Dumez from comment #7)
> (In reply to John Wilander from comment #6)
> > (In reply to John Wilander from comment #5)
> > > (In reply to Chris Dumez from comment #4)
> > > > Comment on attachment 441657 [details]
> > > > Patch
> > > > 
> > > > Can't we add `Cache-Control: no-cache, no-store` header to
> > > > resourceLoadStatistics/resources/redirect.py ? Do we ever want to cache
> > > > those redirects?
> > > 
> > > We could. I do recall testing WebKit behavior at some point and redirects
> > > were cached regardless. Maybe that was treating temporary redirects as
> > > permanent though.
> > 
> > I thought this through over last night and the only obstacle I came up with
> > is the explicit cache testing in
> > resourceLoadStatistics/cap-cache-max-age-for-prevalent-resource.html. But it
> > uses the dedicated
> > resourceLoadStatistics/resources/cached-permanent-redirect.py so we're good.
> > 
> > Would you prefer only the no-cache response header or a combo?
> 
> I think I would prefer "Cache-Control: no-cache, no-store" if it works here.
> Generating unique URLs is a bit annoying and you have to keep doing it in
> new tests.

I have not been able to repro the issue locally and neither have the bot watchers. So this is a speculative fix based on prior experience. In this case, I at least did part of the work in the Python redirecter which will be available without extra work in new tests.

[Chris Dumez]

(In reply to John Wilander from comment #8)
> (In reply to Chris Dumez from comment #7)
> > (In reply to John Wilander from comment #6)
> > > (In reply to John Wilander from comment #5)
> > > > (In reply to Chris Dumez from comment #4)
> > > > > Comment on attachment 441657 [details]
> > > > > Patch
> > > > > 
> > > > > Can't we add `Cache-Control: no-cache, no-store` header to
> > > > > resourceLoadStatistics/resources/redirect.py ? Do we ever want to cache
> > > > > those redirects?
> > > > 
> > > > We could. I do recall testing WebKit behavior at some point and redirects
> > > > were cached regardless. Maybe that was treating temporary redirects as
> > > > permanent though.
> > > 
> > > I thought this through over last night and the only obstacle I came up with
> > > is the explicit cache testing in
> > > resourceLoadStatistics/cap-cache-max-age-for-prevalent-resource.html. But it
> > > uses the dedicated
> > > resourceLoadStatistics/resources/cached-permanent-redirect.py so we're good.
> > > 
> > > Would you prefer only the no-cache response header or a combo?
> > 
> > I think I would prefer "Cache-Control: no-cache, no-store" if it works here.
> > Generating unique URLs is a bit annoying and you have to keep doing it in
> > new tests.
> 
> I have not been able to repro the issue locally and neither have the bot
> watchers. So this is a speculative fix based on prior experience. In this
> case, I at least did part of the work in the Python redirecter which will be
> available without extra work in new tests.

It seems silly to me to generate unique URLs to work around a resource being cached when you could simply mark this resource as non-cacheable.

[John Wilander]

Created attachment 441744 [details]
Patch

[Chris Dumez]

Comment on attachment 441744 [details]
Patch

r=me as long as the bots are happy.

[John Wilander]

Thanks, Chris! I'll wait for green.

[John Wilander]

Comment on attachment 441744 [details]
Patch

iOS wk2 queue severely backed up. Marking cq+ now that Mac wk2 is green.

[EWS]

Committed r284473 (243232@main): <https://commits.webkit.org/243232@main>

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