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

data URL dedicated workers and agent clusters #23137

Open
wants to merge 5 commits into
base: master
Choose a base branch
from

Conversation

annevk
Copy link
Member

@annevk annevk commented Apr 21, 2020

@wpt-pr-bot wpt-pr-bot temporarily deployed to wpt-preview-23137 April 21, 2020 13:06 Inactive
Copy link
Member

@domenic domenic left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Interesting, so browsers don't even do this for iframes...


[
new SharedArrayBuffer(),
new WebAssembly.Module(new WasmModuleBuilder().toBuffer())
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should we test WebAssembly.Memory too?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That follows the underlying object (a literal AB or SAB) so I don't think it's needed, but we could. If you also wanted to test it backed by AB though we'd have to invert all the pass conditions for that variant.

t.add_cleanup(() => frame.remove());
frame.onload = t.step_func(() => {
self.addEventListener("message", t.step_func(({ data }) => {
assert_equals(data, "PASS");
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this can never happen since both message and messageerror will cause a "FAIL" to be posted? More reason to change the messages posted in the way I describe above.

@guest271314
Copy link
Contributor

FYI this appears to be "working as intended" at Firefox 77.

@annevk
Copy link
Member Author

annevk commented Apr 21, 2020

Thanks @domenic, hopefully this is a bit better.

@guest271314
Copy link
Contributor

One item this test reveals is that security.block_script_with_wrong_mime and security.block_Worker_with_wrong_mime are not stopping the data URL with no MIME type from loading. Am not sure if that is working as intended.

@wpt-pr-bot wpt-pr-bot temporarily deployed to wpt-preview-23137 April 21, 2020 14:47 Inactive
@annevk
Copy link
Member Author

annevk commented Apr 21, 2020

@guest271314 that sounds like a bug. Can you create a separate test for that? I'll add text/javascript here.

Copy link
Member

@domenic domenic left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM. Glad to see that frames are in a separate agent cluster after all.

@wpt-pr-bot wpt-pr-bot temporarily deployed to wpt-preview-23137 April 21, 2020 15:05 Inactive
@guest271314
Copy link
Contributor

guest271314 commented Apr 21, 2020

@annevk

that sounds like a bug. Can you create a separate test for that?

Not sure if that is currently testable and verifiable at WPT, see #20284 (comment).

Opened console at Nightly and ran

#20284 (comment)

dataURLWorker = `
  onmessage = () => self.postMessage("FAIL");
  onmessageerror = () => self.postMessage("PASS");
`;
[
  new ArrayBuffer()
].forEach(instance => {

    const worker = new Worker(`data:,${dataURLWorker}`);
    worker.onmessage = e => console.log(e.data);
    worker.postMessage(instance);
});

Workers are not prohibited from loading any particular MIME type, correct? What is the expected result of a data URL without MIME type being set as URL to a Worker?

Should a Firefox bug be filed?

@annevk
Copy link
Member Author

annevk commented Apr 22, 2020

@guest271314 I'd make a test for module workers to ensure it's enforced there. Perhaps also with a blob URL and such. If user agents eventually also enforce things for classic workers we could extend those tests.

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