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] Introduce an "owner document" for each WorkletGlobalScope. #389

Closed
wants to merge 2 commits into from

Conversation

bfgeek
Copy link
Contributor

@bfgeek bfgeek commented Apr 14, 2017

Fixes #385.

@bfgeek
Copy link
Contributor Author

bfgeek commented Apr 14, 2017

@annevk does this work?

@annevk
Copy link
Member

annevk commented Apr 14, 2017

I guess I was not specific enough. The problem is mostly with WorkletGlobalScope. I see that you defer to host specifications for them lifetime, but I don't think that's good enough nor necessary. We know that all worklet globals will be owned by a Window/document so we might as well write that down.

@bfgeek bfgeek changed the title [worklets] Add [[Worklets]] internal slot to window. [worklets] Introduce an "owner document" for each WorkletGlobalScope. Apr 17, 2017
@bfgeek
Copy link
Contributor Author

bfgeek commented Apr 17, 2017

@annevk PTAL, I think this is enough? Each WorkletGlobalScope has an "owner document".

This also introduces a "destroy a WorkletGlobalScope" algorithm - may want to change the event loop in html spec for this instead...

@annevk
Copy link
Member

annevk commented Apr 17, 2017

Do you have tests for this? In particular, if an initial about:blank document gets replaced, does the WorkletGlobalScope go away or stay? I'm asking since @wanderview wants to change ownership of workers to be global object based and maybe we want worklets to work like that too?

@bfgeek
Copy link
Contributor Author

bfgeek commented Apr 17, 2017

We don't currently have tests for this; (@nhiroki we should add tests for this :) )

But we currently do kill the globals when the document get's destroyed, @nhiroki please correct me if I'm wrong:
https://cs.chromium.org/webrtc/src/third_party/WebKit/Source/core/workers/Worklet.cpp?q=worklet.cpp&l=65

@nhiroki
Copy link
Contributor

nhiroki commented Apr 17, 2017

@bfgeek You're right.

I'm not sure how we can confirm it in web-platform-tests. For workers we could test it by creating a worker in a frame, closing the frame and postMessage+setTimeout like [1], but worklets don't have such a communication path.

[1] https://github.com/w3c/web-platform-tests/blob/master/workers/WorkerGlobalScope_close.htm

@nhiroki
Copy link
Contributor

nhiroki commented Apr 17, 2017

Filed a chromium bug: https://crbug.com/712133

@annevk
Copy link
Member

annevk commented Apr 17, 2017

@nhiroki with audio worklets it should be easy to test, but they are still rather speculative at this point. Can a paint worklet draw anything that can then be imported by canvas or some such? Or does it intentionally not leak anything from its insides? (Sorry, I used to know this, but I forgot.)

@nhiroki
Copy link
Contributor

nhiroki commented Apr 19, 2017

@annevk Thank you for the suggestion. I'll try the approach.

@annevk
Copy link
Member

annevk commented Apr 23, 2017

It seems weird btw for the owner to not be the global object as you do expose the accessor to the worklet on the global object.

@tabatkins tabatkins removed the ready label Jun 13, 2019
domenic added a commit to whatwg/html that referenced this pull request Oct 16, 2020
Closes w3c/css-houdini-drafts#1000.

This provides a baseline by porting over all the existing text from
https://drafts.css-houdini.org/worklets/, modernizing and restructuring
it along the way. It does not yet fix many of the open logged issues
(although it does fix some; see below).

Notable changes from that document:

* Rearranged sections to better match workers, and my sense of flow.

* Moved worklet script fetching to be siblings with all the other script
  fetching algorithms.

* Improved clarity and guidance on what specifications that define
  worklets should do, including fleshing out the fake worklet example.

* Changed "create a WorkletGlobalScope", which took one set of
  arguments, to "create a worklet global scope", which just takes a
  Worklet instance. This appears to match better how the algorithm is
  used, e.g. in
  https://drafts.css-houdini.org/css-paint-api/#draw-a-paint-image step
  10.

* Updated "report an error" to bail out for non-EventTarget globals,
  like WorkletGlobalScope. Closes #2611.

* Updated worklets to only be exposed in secure contexts. Closes
  w3c/css-houdini-drafts#505.

* Makes the lifetime of creating and terminating WorkletGlobalScopes
  more explicit. Closes
  w3c/css-houdini-drafts#224. Closes
  w3c/css-houdini-drafts#389.

* Explicitly start and stop the event loop for a given
  WorkletGlobalScope upon creation/termination. Closes
  w3c/css-houdini-drafts#843.
  Closes w3c/css-houdini-drafts#318 for real.

* Fixes creation of new worklet global scopes to only run the top-level
  module scripts added via addModule(), which will automatically run
  their dependencies. Previously it would run all module scripts loaded
  into the worklet, so dependencies would be run in the order they were
  fetched, not as part of the top-down module evaluation process. Closes
  w3c/css-houdini-drafts#264.
@domenic
Copy link
Contributor

domenic commented Oct 16, 2020

With whatwg/html#6056, we can close this. The lifetime of WorkletGlobalScopes is now very explicit.

@tabatkins tabatkins closed this Oct 16, 2020
@annevk annevk deleted the worklets-document branch October 19, 2020 08:16
@syncbot syncbot restored the worklets-document branch December 23, 2020 06:18
@tabatkins tabatkins deleted the worklets-document branch May 13, 2021 15:58
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[worklets] Owner of a worklet is not a formally defined thing
5 participants