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.
<rdar://problem/83991245>
Updated test expectations at http://trac.webkit.org/changeset/283727/webkit
Created attachment 441657 [details] Patch
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?
(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.
(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?
(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.
(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.
(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.
Created attachment 441744 [details] Patch
Comment on attachment 441744 [details] Patch r=me as long as the bots are happy.
Thanks, Chris! I'll wait for green.
Comment on attachment 441744 [details] Patch iOS wk2 queue severely backed up. Marking cq+ now that Mac wk2 is green.
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].