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

[worklets] dynamic import() #506

Closed
annevk opened this issue Nov 6, 2017 · 26 comments · Fixed by whatwg/html#6070
Closed

[worklets] dynamic import() #506

annevk opened this issue Nov 6, 2017 · 26 comments · Fixed by whatwg/html#6070
Assignees

Comments

@annevk
Copy link
Member

annevk commented Nov 6, 2017

I thought we already had an issue for this somewhere. If worklets cannot have fetch(), I don't think it makes sense for them to have dynamic import(). That would cause the same kind of execution problems.

cc @domenic @nhiroki @bfgeek

@annevk
Copy link
Member Author

annevk commented Nov 6, 2017

See whatwg/fetch#527 (comment) for issues if we decide to allow it somehow.

@bfgeek
Copy link
Contributor

bfgeek commented Nov 6, 2017

I agree that it makes sense we shouldn't have import().

@bfgeek
Copy link
Contributor

bfgeek commented Nov 6, 2017

I'm not sure what the behaviour should be, if import() is just hidden, or it throws. :/

@surma
Copy link
Member

surma commented Nov 6, 2017

If we are disallowing it, I’d say hide it. A function whose only purpose is to throw seems odd.

@domenic
Copy link
Contributor

domenic commented Nov 6, 2017

It's not a function; it's syntax. It can't be removed without language changes. I'm not even sure what hiding it would mean.

@bfgeek
Copy link
Contributor

bfgeek commented Nov 6, 2017

Oh didn't realize that, makes sense.

@annevk
Copy link
Member Author

annevk commented Nov 7, 2017

@domenic I assume we can make it reject the promise it returns? (Depending on what data gets passed to Fetch, we could even return a network error there for all fetches made by worklets. Not sure that's ideal however as it's unlikely to be implemented that way.)

@domenic
Copy link
Contributor

domenic commented Nov 7, 2017

Yeah, that's doable.

@nhiroki
Copy link
Contributor

nhiroki commented Nov 8, 2017

Filed a tracking issue in crbug.

@domenic
Copy link
Contributor

domenic commented Nov 8, 2017

To be clear, I don't think we should reject import() in worklets, per my position from the earlier thread on the same topic. I think all forms of importing code should work in worklets, including lazily importing. I was only commenting on the technical feasibility here.

@annevk
Copy link
Member Author

annevk commented Nov 8, 2017

I wonder how worklets work with the opaque origin setup if their modules can have non-dynamic imports. Did you review that @domenic? Or maybe the opaque origin is assigned after all dependencies are fetched?

The main reason to block dynamic import() is the same reason we block fetch(); to make it clear worklets are not supposed to have that kind of code.

@nhiroki
Copy link
Contributor

nhiroki commented Nov 8, 2017

I wonder how worklets work with the opaque origin setup if their modules can have non-dynamic imports. Did you review that @domenic? Or maybe the opaque origin is assigned after all dependencies are fetched?

IIUC, non-dynamic imports are handled as sub-resources of the owner document like addModule(), so the opaque origin is not a problem. Chromium implemented this behavior and there are WPT tests for the case.

@annevk
Copy link
Member Author

annevk commented Nov 8, 2017

Thanks. But the referrer would be the top-level module script's (base) URL? So yeah, if we want dynamic import() to work we'd need some justification as to why that's okay and fetch() is not and then figure out how it should actually work as per the comment following OP.

@nhiroki
Copy link
Contributor

nhiroki commented Nov 8, 2017

But the referrer would be the top-level module script's (base) URL?

Yes. It's the same with plain ES6 modules. I'm now adding WPT tests for the referrer of descendant scripts (crbug).

@bfgeek
Copy link
Contributor

bfgeek commented Nov 9, 2017

Part of the justification that it shouldn't work for me at least, is that worklet global scopes are allowed to be transient, and when they are created again they should contain all the same code as the other WGS. Allowing the dynamic import here makes it a little too easy for them to diverge.

We won't be able to remove all non-deterministic apis, but this one seems easy to reason about?

@css-meeting-bot
Copy link
Member

The Working Group just discussed dynamic import(), and agreed to the following resolutions:

  • RESOLVED: dynamic import() in worklets is a no-op that returns a rejected promise (or equivalent after passing thru JS people)
The full IRC log of that discussion <TabAtkins> Topic: dynamic import()
<TabAtkins> github: https://github.com//issues/506
<TabAtkins> iank_: ES6 modules are a thing
<TabAtkins> iank_: worklets are built on them
<TabAtkins> iank_: WE have a dynamic import() now
<TabAtkins> iank_: you can say import(url), returns a promise
<TabAtkins> iank_: Do we want to allwo in worklets?
<TabAtkins> TabAtkins: And we don't allow fetch(), so...
<TabAtkins> Rossen: Motivation?
<TabAtkins> iank_: Consistency with other global scopes.
<TabAtkins> smfr: Can't do anything async, right? The paint has be to sync.
<TabAtkins> iank_: Right. It would just be a communication channel to the server.
<TabAtkins> dbaron: Woudl this expose whether you're recycling the global?
<TabAtkins> iank_: Potentially. It wouldn't be intercepted by a SW.
<TabAtkins> smfr: And it would mean your context is running code at a weird time.
<TabAtkins> iank_: So room opinion seems to be to throw?
<TabAtkins> TabAtkins: If we don't allow any other network communication...
<TabAtkins> Rossen: Yeah, with all the worklets, just makes more sense.
<TabAtkins> iank_: So the proposed resolution is that dynamic import becomes a no-op that returns a rejected promise.
<TabAtkins> RESOLVED: dynamic import() in worklets is a no-op that returns a rejected promise (or equivalent after passing thru JS people)

@domenic
Copy link
Contributor

domenic commented Nov 10, 2017

I don't think we should be piecemeal disabling parts of the JS language. It should be principled, e.g. adding a switch that disables dynamic import and Math.random() and Date.now()/new Date()/etc. at the same time.

@annevk
Copy link
Member Author

annevk commented Nov 10, 2017

It is principled, no network APIs.

@domenic
Copy link
Contributor

domenic commented Nov 10, 2017

Then static import should be disallowed.

@annevk
Copy link
Member Author

annevk commented Nov 10, 2017

Why? Those are all resolved before execution.

@domenic
Copy link
Contributor

domenic commented Nov 10, 2017

They're still network APIs, so it seems the line is not very principled?

@bfgeek
Copy link
Contributor

bfgeek commented Nov 10, 2017

Anne is right. This keeps inline with "no communication to the outside world" once you've added the scripts, i.e. no networking apis.

@nhiroki
Copy link
Contributor

nhiroki commented Nov 15, 2017

What error code does import() return on WorkletGlobalScope? NotSupportedError?

@nhiroki
Copy link
Contributor

nhiroki commented Nov 28, 2017

If there is no objection, I'm going to make import() on WorkletGlobalScope reject the promise with NotAllowedError (CL).

@annevk
Copy link
Member Author

annevk commented Nov 28, 2017

Apologies for not replying sooner. I think TypeError would be better for a JavaScript language construct.

@nhiroki
Copy link
Contributor

nhiroki commented Nov 28, 2017

@annevk Thank you! TypeError sounds good. I didn't know NotSupportedError is not a native error type defined in the ECMAScript spec. I'll update the CL.

MXEBot pushed a commit to mirror/chromium that referenced this issue Dec 5, 2017
All networking APIs including dynamic import() are disallowed on
WorkletGlobalScope. This CL makes dynamic import() always reject a promise with
TypeError[1] on WorkletGlobalScope.

[1] w3c/css-houdini-drafts#506 (comment)

Bug: 782538
Change-Id: Ieb94a4502b0ba35ff9b56e51c8fc34467ab96d0c
Reviewed-on: https://chromium-review.googlesource.com/790059
Commit-Queue: Hiroki Nakagawa <nhiroki@chromium.org>
Reviewed-by: Ian Kilpatrick <ikilpatrick@chromium.org>
Reviewed-by: Hiroshige Hayashizaki <hiroshige@chromium.org>
Reviewed-by: Kouhei Ueno <kouhei@chromium.org>
Cr-Commit-Position: refs/heads/master@{#521295}
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

8 participants
@surma @domenic @tabatkins @nhiroki @bfgeek @annevk @css-meeting-bot and others