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

SharedWorker: Assign unique names to SharedWorkers to avoid unintentional matching #21857

Merged
merged 1 commit into from Mar 16, 2020

Conversation

chromium-wpt-export-bot
Copy link
Collaborator

This CL assigns unique names to SharedWorkers in WPTs for referrer
policy and csp of module shared workers to avoid unintentinal matching.

Before this CL, the flakiness is reported for shared-worker-import-csp.html.
See #21591 for more detail.

Bug: 1051779
Change-Id: Id4f7e02510189320366862499f81a20a01089c76
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2055913
Commit-Queue: Eriko Kurimoto <elkurin@google.com>
Reviewed-by: Hiroki Nakagawa <nhiroki@chromium.org>
Reviewed-by: Hiroshige Hayashizaki <hiroshige@chromium.org>
Cr-Commit-Position: refs/heads/master@{#741849}

…onal matching

This CL assigns unique names to SharedWorkers in WPTs for referrer
policy and csp of module shared workers to avoid unintentinal matching.

Before this CL, the flakiness is reported for shared-worker-import-csp.html.
See #21591 for more detail.

Bug: 1051779
Change-Id: Id4f7e02510189320366862499f81a20a01089c76
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2055913
Commit-Queue: Eriko Kurimoto <elkurin@google.com>
Reviewed-by: Hiroki Nakagawa <nhiroki@chromium.org>
Reviewed-by: Hiroshige Hayashizaki <hiroshige@chromium.org>
Cr-Commit-Position: refs/heads/master@{#741849}
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.

@LukeZielinski
Copy link
Contributor

@elkurin It looks like we still see some Firefox flakiness here, although different from #21591. It looks like we're only failing when running FF with chaos mode with restarts. Would you be able to take a look and see if there's a problem with the tests, or if this a FF bug?

Thanks

Unstable results

Test Subtest Results Messages
/workers/modules/shared-worker-import-csp.html script-src 'self' directive should disallow cross origin static import. FAIL: 1/5, PASS: 4/5 assert_equals: expected (string) "LOADED" but got (object) ["ERROR"]
/workers/modules/shared-worker-import-csp.html worker-src 'self' directive should override script-src * directive and disallow cross origin static import. FAIL: 1/5, PASS: 4/5 assert_equals: expected (string) "LOADED" but got (object) ["ERROR"]
/workers/modules/shared-worker-import-csp.html script-src 'self' directive should disallow cross origin dynamic import. FAIL: 1/5, PASS: 4/5 assert_equals: expected (string) "LOADED" but got (object) ["ERROR"]

::: Running tests in a loop 10 times : PASS
::: Running tests in a loop with restarts 5 times : PASS
::: Running tests in a loop 10 times with flags chaos_mode_flags=3 : PASS
::: Running tests in a loop with restarts 5 times with flags chaos_mode_flags=3 : FAIL

@elkurin
Copy link
Contributor

elkurin commented Feb 25, 2020

Thanks for reporting!

According to your test results, the test is failing with the assertion:
assert_equals(msgEvent.data, 'LOADED');
which is checked in openWindow(). It expects to get string 'LOADED' which is posted soon after window.onmessage handler is registered. But it seems we get object ["ERROR"] instead which can only be sent from the opened window and expected to be sent to the second message event checked in the assertion assert_array_equals(msg_event.data, expectedImportedModules). So, the test result implies that the first message event is mistakenly getting the message which should be sent to the second message event.
In this test, it is guaranteed that openWindow() runs before the second message event occurs since we use 'await'.

I think this kind of error happens only when promise_test() runs in parallel. I'm not familiar with chaos_mode_flags, but is it a mode to run several promise_test() at the same time? If so, failing makes sense.

@stephenmcgruer
Copy link
Contributor

Hi @elkurin . Sorry for the delay on getting back to you. 'Chaos mode' in Firefox is something built into their browser (ChaosMode.h), which alters things like thread scheduling, network scheduling, etc, without (as I understand it) changing the web platform semantics. It sometimes exposes bugs in tests, and sometimes exposes bugs in the Firefox implementation.

It should not cause promise_tests to run in parallel, so if you're confident that the test is correct then we should admin-merge this PR and file a bug with Firefox.

Are you able to own opening a bug with Firefox (https://bugzilla.mozilla.org/)?

@elkurin
Copy link
Contributor

elkurin commented Mar 12, 2020

Thanks @stephenmcgruer . Sorry for the delay.
If Chaos mode doesn't cause promise_tests to run in parallel, then I'm sure that this test should run correctly.
I opened the FireFox bug issue: https://bugzilla.mozilla.org/show_bug.cgi?id=1621874, so if possible please merge this wpt.

@stephenmcgruer
Copy link
Contributor

Certainly, thanks for your efforts looking into this :)

@stephenmcgruer stephenmcgruer merged commit ec2bf5a into master Mar 16, 2020
@stephenmcgruer stephenmcgruer deleted the chromium-export-d5cd6b98a6 branch March 16, 2020 21:17
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

5 participants