-
Notifications
You must be signed in to change notification settings - Fork 3.1k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[COOP] access reporting: fix flakes reporting.html #25834
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The review process for this patch is being conducted in the Chromium project.
8a383e8
to
b910212
Compare
b910212
to
89de933
Compare
44f8a45
to
12e875d
Compare
I will proceed with landing this despite not passing wpt-{chrome,firefox}-dev-stability. There was existing problems:
This patches fixes the last two ones. However it remains the first two. This is a strict improvement. Could you approve this patch? |
--- The WPT testharness.js is defining: ``` WindowTestEnvironment.prototype._forEach_windows = function(callback) { // Iterate over the windows [self ... top, opener]. The callback is // passed two objects, the first one is the window object itself, the // second one is a boolean indicating whether or not it's on the same // origin as the (...) ``` This causes some postMessage to be sent cross-window. They are reported and causes failures. For instance in reporting-observer.html test. This patch removes testharness.js from executor.html This fixed the issue for reporting-observer.html More generally, we shouldn't check access in between two windows, when one of them is using testharness.js --- Second discovery: The official web-platform-test runner sometimes drop POST requests when many are requested in parallel. Using a lock fixes the issue. --- Bug: 1090273 Change-Id: I261b9bfece935e3613c250a3a9402a1c8a6ff14e Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2436742 Commit-Queue: Arthur Sonzogni <[email protected]> Reviewed-by: Pâris Meuleman <[email protected]> Cr-Commit-Position: refs/heads/master@{#855712}
12e875d
to
a3f54a8
Compare
Thanks for the headsup Arther! @stephenmcgruer could you admin merge? |
The WPT testharness.js is defining:
This causes some postMessage to be sent cross-window. They are reported
and causes failures. For instance in reporting-observer.html test.
This patch removes testharness.js from executor.html
This fixed the issue for reporting-observer.html
More generally, we shouldn't check access in between two windows, when
one of them is using testharness.js
Second discovery:
The official web-platform-test runner sometimes drop POST requests
when many are requested in parallel. Using a lock fixes the issue.
Bug: 1090273
Change-Id: I261b9bfece935e3613c250a3a9402a1c8a6ff14e
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2436742
Commit-Queue: Arthur Sonzogni <[email protected]>
Reviewed-by: Pâris Meuleman <[email protected]>
Cr-Commit-Position: refs/heads/master@{#855712}