Skip to content
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

Merged
merged 1 commit into from Feb 22, 2021

Conversation

chromium-wpt-export-bot
Copy link
Collaborator

@chromium-wpt-export-bot chromium-wpt-export-bot commented Sep 29, 2020


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 <arthursonzogni@chromium.org>
Reviewed-by: Pâris Meuleman <pmeuleman@chromium.org>
Cr-Commit-Position: refs/heads/master@{#855712}

Copy link
Collaborator

@wpt-pr-bot wpt-pr-bot left a 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.

@ArthurSonzogni
Copy link
Member

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 <arthursonzogni@chromium.org>
Reviewed-by: Pâris Meuleman <pmeuleman@chromium.org>
Cr-Commit-Position: refs/heads/master@{#855712}
@KyleJu
Copy link
Contributor

KyleJu commented Feb 19, 2021

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?

Thanks for the headsup Arther! @stephenmcgruer could you admin merge?

@stephenmcgruer stephenmcgruer merged commit 9b505f9 into master Feb 22, 2021
@stephenmcgruer stephenmcgruer deleted the chromium-export-cl-2436742 branch February 22, 2021 14:37
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants