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

workers/modules/shared-worker-import-failure.html is failing asynchronously for file:// URLS #41745

Open
youennf opened this issue Aug 31, 2023 · 20 comments
Labels

Comments

@youennf
Copy link
Contributor

youennf commented Aug 31, 2023

Looking at WPT results, Chrome and Safari are failing synchronously and Firefox is failing asynchronously.
From looking at the spec, both are allowed.

It would be good to get consensus on what we collectively want there.

@annevk
Copy link
Member

annevk commented Aug 31, 2023

@domenic @allstarschh @smaug---- @hiroshige-g thoughts?

@allstarschh
Copy link
Contributor

According to the spec, it should fail asynchronously (synchronously was the legacy behavior, but is no longer valid)
See the summary from https://phabricator.services.mozilla.com/D179544
I have explained the current behavior and the legacy behavior.

@youennf
Copy link
Contributor Author

youennf commented Aug 31, 2023

Looking at https://wpt.fyi/results/workers/constructors/Worker/same-origin.html?label=experimental&label=master&aligned, it seems that the main issue is that:

  • Chrome and Safari are checking whether the URL is cross origin and fail synchronously in this case.
  • Firefox is doing the same check but fails asynchronously.
  • Firefox is sometimes failing synchronously (at least unsupported:).

@allstarschh
Copy link
Contributor

Looking at https://wpt.fyi/results/workers/constructors/Worker/same-origin.html?label=experimental&label=master&aligned, it seems that the main issue is that:
* Firefox is sometimes failing synchronously (at least unsupported:).

unsupported: should fail synchrounsly according to step 4 in https://html.spec.whatwg.org/#dom-worker

  1. Parse the scriptURL argument relative to outside settings
  2. If this fails, throw a "SyntaxError" DOMException.

@youennf
Copy link
Contributor Author

youennf commented Aug 31, 2023

unsupported: should fail synchrounsly

According the test, it apparently throws a SecurityError, similarly to step 1.

@annevk
Copy link
Member

annevk commented Sep 1, 2023

Why would that URL fail to parse? I think that test has the wrong expectations. Unsupported schemes end up failing in Fetch and as such should result in an asynchronous failure.

I wonder if we should remove step 1 from new Worker(). Are they really different enough from just not allowing script to run at all?

@annevk
Copy link
Member

annevk commented Sep 1, 2023

Created #41758.

I'm also gonna PR the HTML Standard to remove step 1: whatwg/html#9668.

@youennf
Copy link
Contributor Author

youennf commented Sep 1, 2023

@hiroshige-g, is there a plan to align Chromium's implementation to the spec, for instance as part of Interop 2023?

@allstarschh
Copy link
Contributor

Why would that URL fail to parse? I think that test has the wrong expectations. Unsupported schemes end up failing in Fetch and as such should result in an asynchronous failure.

Thanks for the correction, after checking https://url.spec.whatwg.org/#concept-basic-url-parser
"unsupported:" should be parsable.

annevk added a commit that referenced this issue Sep 5, 2023
Also check that a non-parsable URL throws the correct exception.

Identified in #41745.
@annevk
Copy link
Member

annevk commented Sep 5, 2023

The tests have been fixed and the HTML standard no longer contains the security exception step.

I'll leave this open until @hiroshige-g has had a chance to respond to Youenn.

moz-v2v-gh pushed a commit to mozilla/gecko-dev that referenced this issue Sep 13, 2023
…eme, a=testonly

Automatic update from web-platform-tests
Correct worker test for unsupported: scheme

Also check that a non-parsable URL throws the correct exception.

Identified in web-platform-tests/wpt#41745.
--

wpt-commits: 2d504ed2b20e9325243ef22fb95b9f645ae7d87c
wpt-pr: 41758
vinnydiehl pushed a commit to vinnydiehl/mozilla-unified that referenced this issue Sep 14, 2023
…eme, a=testonly

Automatic update from web-platform-tests
Correct worker test for unsupported: scheme

Also check that a non-parsable URL throws the correct exception.

Identified in web-platform-tests/wpt#41745.
--

wpt-commits: 2d504ed2b20e9325243ef22fb95b9f645ae7d87c
wpt-pr: 41758
@annevk annevk added the workers label Sep 26, 2023
@hiroshige-g
Copy link
Contributor

Chromium doesn't have an immediate plan for fixing the implementation and currently have any estimated time frame in which we will fix this bug.

Lightning00Blade pushed a commit to Lightning00Blade/wpt that referenced this issue Dec 11, 2023
Also check that a non-parsable URL throws the correct exception.

Identified in web-platform-tests#41745.
moz-v2v-gh pushed a commit to mozilla/gecko-dev that referenced this issue May 29, 2024
…eme, a=testonly

Automatic update from web-platform-tests
Correct worker test for unsupported: scheme

Also check that a non-parsable URL throws the correct exception.

Identified in web-platform-tests/wpt#41745.
--

wpt-commits: 2d504ed2b20e9325243ef22fb95b9f645ae7d87c
wpt-pr: 41758
github-actions bot pushed a commit to Floorp-Projects/Floorp that referenced this issue Jun 11, 2024
…eme, a=testonly

Automatic update from web-platform-tests
Correct worker test for unsupported: scheme

Also check that a non-parsable URL throws the correct exception.

Identified in web-platform-tests/wpt#41745.
--

wpt-commits: 2d504ed2b20e9325243ef22fb95b9f645ae7d87c
wpt-pr: 41758
@asutherland
Copy link
Contributor

@hiroshige-g Is there likely to be any change in the Chromium implementation on this? @youennf @annevk same question for Webkit.

Firefox is currently experiencing notable breakage on support.apple.com with video playback (BMO bug 1919592) where a control flow path involving Workers diverges because Firefox is not throwing synchronously on new Worker that's failing for CSP on a Blob but other browsers are.

@annevk
Copy link
Member

annevk commented Oct 7, 2024

Maybe we can put these tests back in for Interop 2025? cc @nt1m @karlcow

@asutherland
Copy link
Contributor

It looks like we're going to have Firefox converge to what other browsers are doing. We should probably revert the changes in whatwg/html#9668 and upgrade "may" in the dedicated worker case to "should" or "must". Then the test changes in https://phabricator.services.mozilla.com/D179544 would be ~reverted with passing the test requiring that the exception is thrown synchronously.

@annevk
Copy link
Member

annevk commented Oct 9, 2024

Reverting that HTML change would not handle the CSP case that is causing issues. That was actually removing something that was never implemented so I don't see why we'd restore that.

It also seems quite bad to forever enshrine numerous Fetch checks to have to be done synchronously. If we can limit the damage somehow I think we should.

@asutherland
Copy link
Contributor

Reverting that HTML change would not handle the CSP case that is causing issues. That was actually removing something that was never implemented so I don't see why we'd restore that.

It also seems quite bad to forever enshrine numerous Fetch checks to have to be done synchronously. If we can limit the damage somehow I think we should.

I hear you, but Apple's site is still broken and so we're planning to move to match other browsers on this, so I think the spec will have to change. Tentatively this will be changed as part of https://bugzilla.mozilla.org/show_bug.cgi?id=1471805

@annevk
Copy link
Member

annevk commented Nov 26, 2024

cc @karlcow

@karlcow
Copy link
Contributor

karlcow commented Nov 27, 2024

Thanks @annevk

@asutherland
In https://bugzilla.mozilla.org/show_bug.cgi?id=1919592#c12
I mentioned this was tracked at Apple by rdar://137744381
(this week will be quiet at Apple because of Thanks Giving, so I don't think I will be able to move the needle that much.) I added a comment on the radar to reinforce the need to fix it.

but fwiw, there are positive signs that it could be fixed on the server side at Apple. Let's hope we can have a soonish resolution so Firefox users have a good user experience on Apple servers.

@smaug----
Copy link
Contributor

@miketaylr

@LiangTheDev
Copy link

Looks like the Apple support page doesn't have the how to update video currently and the issue in https://bugzilla.mozilla.org/show_bug.cgi?id=1912873 is fixed.

For aligning the behavior of implementations, I feel that we should align with current spec. I've posted a comment at web-platform-tests/interop#855, which is more focused on the scenario of CSP blocked worker.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

8 participants