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

ServiceWorker: restructure data structure and algorithms to better match the spec #26108

Closed
gterzian opened this issue Apr 4, 2020 · 8 comments · Fixed by #26317
Closed

ServiceWorker: restructure data structure and algorithms to better match the spec #26108

gterzian opened this issue Apr 4, 2020 · 8 comments · Fixed by #26317
Labels
A-constellation Involves the constellation A-content/script Related to the script thread B-meta This issue tracks the status of multiple, related pieces of work

Comments

@gterzian
Copy link
Member

gterzian commented Apr 4, 2020

EDIT: the second comment is more accurate, still keeping this here for a basic overview of how it currently works. We need to move the Jobqueue to the constellation or service worker manager level.

pub struct JobQueue(pub DomRefCell<HashMap<ServoUrl, Vec<Job>>>);

Spec: https://w3c.github.io/ServiceWorker/#dfn-job-queue

The JobQueue used to run algorithms for service workers seems to currently be tied to having a ScriptThread being available.

See for example how a job is scheduled:

pub fn schedule_job(&self, job: Job, script_thread: &ScriptThread) {

Or how a job is run:

pub fn run_job(&self, scope_url: ServoUrl, script_thread: &ScriptThread) {

So I assume this only works in the context of a window GlobalScope.

Since all the service workers IDL interfaces are also available to workers, I think we should try to make the job queue also work for a worker global.

We could try to do something similar as what is done with the MicrotaskQueue ,

microtask_queue: Rc<MicrotaskQueue>,

which is available on the GlobalScope in addition to being available on the ScriptThread, so that worker event-loops can also use it.

.perform_a_microtask_checkpoint();

Although to prevent a problem like #20908 we could try to just move the job queue entirely onto the global, and remove it from the script-thread, so functions like run_job and schedule_job could be made to take a &GlobalScope instead of a &ScriptThread.

@jdm jdm added the A-content/script Related to the script thread label Apr 4, 2020
@gterzian
Copy link
Member Author

gterzian commented Apr 8, 2020

Ok I've taken a closer look at this, and although I'm still figuring a few things out, it appears to me that the goal of this issue is not so much to use a GlobalScope versus a ScriptThread in the various JobQueue operations, it might rather be breaking up the algorithms and data structure between "what is running in a script context" and "what is not", and moving some parts to the constellation, and the other parts would probably indeed belong best on the GlobalScope versus the ScriptThread(to make it work in all scopes).

For example the concept of a scope-to-registration-map), to me it appears this should be unique across the UA, in other words if different pages running in different ScriptThread(but with the same origin) look something up in it, they should see the same result.

Currently we partition this map per ScriptThread, see

registration_map: DomRefCell<HashMap<ServoUrl, Dom<ServiceWorkerRegistration>>>,

For example:

In Register, we probably want to:

  1. Run steps 1 to 3 " Reject Job Promise" in the global scope calling avigator-service-worker-register.
  2. Then step 4 "Get Registration", that actually should probably run at the constellation level.
  3. Same thing with "Set Registration"(run if there is no existing registration), at step 6.1
  4. Finally, if the algorithm goes to the "Update" step 7, that's when we actually run a new ServiceWorkerGlobalScope, so again that's probably where the constellation would communicate with a ServiceWorkerManager for that origin to actually spin-up a new worker, and then back with the "client" GlobalScope as part of running "Install" with the new worker up and running(this queues some tasks back on an event-loop to fire events related to the update).

So this is I think also about making a difference between DOM concepts like ServiceWorker and ServiceWorkerRegistration, which "point" to the actual thing that is either some data at the constellation level, or something running a ServiceWorkerGlobalScope in a service worker process.

So for example there can be multiple DOM objects pointing to the same "thing", like a running worker or a registration(each environment can only have one DOM ServiceWorker for a given worker, however multiple environment can off-course have their own ServiceWorker which should all point to the same worker when their origin matches).

For example compare a environment-settings-object-service-worker-registration-object-map with the "scope-to-registration-map" mentioned above. The first thing is on a "per environment setting(read GlobalScope I think) basis", wherease the second one is unique for the entire UA.

The first one represents a map of ServiceWorkerRegistration that are in use in a specific environment, and those point to the non-DOM "service worker registration", which probably should be some data at the constellation level, or maybe the service worker manager for the corresponding origin(it's defined as "a tuple of a scope url and a set of service workers, an installing worker, a waiting worker, and an active worker").

So what we currently have on the script-thread, and which should be moved to the global, is actually this "environment-settings-object-service-worker-registration-object-map", and what we're missing is the "scope-to-registration-map", which belongs on the constellation, or perhaps rather the service worker manager for an origin.

This requires some more looking into, and hopefully it can be broken up cleanly in issues or PR, but it could also get a bit messy...

@gterzian gterzian changed the title ServiceWorker: enable JobQueue in worker scopes ServiceWorker: restructure data structure and algorithms to better match the spec Apr 8, 2020
@gterzian gterzian added A-constellation Involves the constellation B-meta This issue tracks the status of multiple, related pieces of work labels Apr 8, 2020
@CYBAI
Copy link
Member

CYBAI commented Apr 11, 2020

@gterzian is there anything I can work on for this meta issue 👀 ?

@gterzian
Copy link
Member Author

gterzian commented Apr 12, 2020

@CYBAI I think there will be lots of things to do but I'm sure what yet, currently still looking at the spec and the code.

I'm pretty sure we need to move the JobQueue operations to run on the constellation or service worker manager for an origin, and callback into script where necessary, and have a bunch of data on the constellation or sw manager that would match various DOM objects(similar to what was done with MessagePort and BroadcastChannel), but its not completely clear to me yet.

I also don't know how to break it up into issues yet, but I'll try to do so. Perhaps I'll start with some initial restructuring that opens the way for similar work on top of it...

@CYBAI
Copy link
Member

CYBAI commented Apr 12, 2020

@gterzian Thanks! I will read the spec and code first!

@gterzian
Copy link
Member Author

gterzian commented Apr 14, 2020

By the way, this sort of confirms that the Job queue should be somewhere at the constellation or service worker manager level(probably the latter) w3c/ServiceWorker#1224

@gterzian
Copy link
Member Author

gterzian commented Apr 15, 2020

Ok here is a bit of an action plan for this:

  1. I'd like to try an initial refactoring that would bring the "job queue" concept in line with the spec. This will require moving it out of the script-thread(and actually remove the DOM object), and into an IPC based workflow involving the constellation and the service worker manager, with steps running back on script in the context of a global scope(as opposed to a script-thread), by way of IPC routes. While it's "clear" to me what needs to be done, it's not clear in the sense that I can write it down clearly in an issue, hence I'd like to simply get my hands dirty and figure out the details while implementing the change. I think that refactoring can close this issue.
  2. Once that initial refactoring is in place, I would hope that the "right" structure would be in place, so that we (cc @CYBAI) can simply start chipping away at the spec and failing tests.

@jdm could you please let me know what you think of this?

@jdm
Copy link
Member

jdm commented Apr 15, 2020

Sounds reasonable!

@gterzian
Copy link
Member Author

This is a good write-up on the ServiceWorker architecture in Chromium: https://chromium.googlesource.com/chromium/src/+/lkgr/content/browser/service_worker/README.md#renderer-process

Where you see written the "Browser process", you can substitute with the "Constellation and the ServiceWorkerManager", while the "Renderer process" is what happens in script(with the exception of serviceworker_manager.rs), and that covers both the actual ServiceWorkerGlobalScope running code "inside" the worker, and also other global scopes which are "clients" of the worker and use various WebIDL interfaces like ServiceWorkerRegistration, ServiceWorkerContainer`, and ServiceWorker`.

bors-servo added a commit that referenced this issue Apr 30, 2020
ServiceWorker: restructure Job Queue, Register flow, to better match spec

<!-- Please describe your changes on the following line: -->

---
<!-- Thank you for contributing to Servo! Please replace each `[ ]` by `[X]` when the step is complete, and replace `___` with appropriate data: -->
- [ ] `./mach build -d` does not report any errors
- [ ] `./mach test-tidy` does not report any errors
- [ ] These changes fix #26108 (GitHub issue number if applicable)

<!-- Either: -->
- [ ] There are tests for these changes OR
- [ ] These changes do not require tests because ___

<!-- Also, please make sure that "Allow edits from maintainers" checkbox is checked, so that we can help you if you get stuck somewhere along the way.-->

<!-- Pull requests that do not address these steps are welcome, but they will require additional verification as part of the review process. -->
bors-servo added a commit that referenced this issue Apr 30, 2020
ServiceWorker: restructure Job Queue, Register flow, to better match spec

<!-- Please describe your changes on the following line: -->

---
<!-- Thank you for contributing to Servo! Please replace each `[ ]` by `[X]` when the step is complete, and replace `___` with appropriate data: -->
- [ ] `./mach build -d` does not report any errors
- [ ] `./mach test-tidy` does not report any errors
- [ ] These changes fix #26108 (GitHub issue number if applicable)

<!-- Either: -->
- [ ] There are tests for these changes OR
- [ ] These changes do not require tests because ___

<!-- Also, please make sure that "Allow edits from maintainers" checkbox is checked, so that we can help you if you get stuck somewhere along the way.-->

<!-- Pull requests that do not address these steps are welcome, but they will require additional verification as part of the review process. -->
bors-servo added a commit that referenced this issue Apr 30, 2020
ServiceWorker: restructure Job Queue, Register flow, to better match spec

<!-- Please describe your changes on the following line: -->

---
<!-- Thank you for contributing to Servo! Please replace each `[ ]` by `[X]` when the step is complete, and replace `___` with appropriate data: -->
- [ ] `./mach build -d` does not report any errors
- [ ] `./mach test-tidy` does not report any errors
- [ ] These changes fix #26108 (GitHub issue number if applicable)

<!-- Either: -->
- [ ] There are tests for these changes OR
- [ ] These changes do not require tests because ___

<!-- Also, please make sure that "Allow edits from maintainers" checkbox is checked, so that we can help you if you get stuck somewhere along the way.-->

<!-- Pull requests that do not address these steps are welcome, but they will require additional verification as part of the review process. -->
bors-servo added a commit that referenced this issue Apr 30, 2020
ServiceWorker: restructure Job Queue, Register flow, to better match spec

<!-- Please describe your changes on the following line: -->

---
<!-- Thank you for contributing to Servo! Please replace each `[ ]` by `[X]` when the step is complete, and replace `___` with appropriate data: -->
- [ ] `./mach build -d` does not report any errors
- [ ] `./mach test-tidy` does not report any errors
- [ ] These changes fix #26108 (GitHub issue number if applicable)

<!-- Either: -->
- [ ] There are tests for these changes OR
- [ ] These changes do not require tests because ___

<!-- Also, please make sure that "Allow edits from maintainers" checkbox is checked, so that we can help you if you get stuck somewhere along the way.-->

<!-- Pull requests that do not address these steps are welcome, but they will require additional verification as part of the review process. -->
bors-servo added a commit that referenced this issue Apr 30, 2020
ServiceWorker: restructure Job Queue, Register flow, to better match spec

<!-- Please describe your changes on the following line: -->

---
<!-- Thank you for contributing to Servo! Please replace each `[ ]` by `[X]` when the step is complete, and replace `___` with appropriate data: -->
- [ ] `./mach build -d` does not report any errors
- [ ] `./mach test-tidy` does not report any errors
- [ ] These changes fix #26108 (GitHub issue number if applicable)

<!-- Either: -->
- [ ] There are tests for these changes OR
- [ ] These changes do not require tests because ___

<!-- Also, please make sure that "Allow edits from maintainers" checkbox is checked, so that we can help you if you get stuck somewhere along the way.-->

<!-- Pull requests that do not address these steps are welcome, but they will require additional verification as part of the review process. -->
bors-servo added a commit that referenced this issue May 1, 2020
ServiceWorker: restructure Job Queue, Register flow, to better match spec

<!-- Please describe your changes on the following line: -->

---
<!-- Thank you for contributing to Servo! Please replace each `[ ]` by `[X]` when the step is complete, and replace `___` with appropriate data: -->
- [ ] `./mach build -d` does not report any errors
- [ ] `./mach test-tidy` does not report any errors
- [ ] These changes fix #26108 (GitHub issue number if applicable)

<!-- Either: -->
- [ ] There are tests for these changes OR
- [ ] These changes do not require tests because ___

<!-- Also, please make sure that "Allow edits from maintainers" checkbox is checked, so that we can help you if you get stuck somewhere along the way.-->

<!-- Pull requests that do not address these steps are welcome, but they will require additional verification as part of the review process. -->
bors-servo added a commit that referenced this issue May 1, 2020
ServiceWorker: restructure Job Queue, Register flow, to better match spec

<!-- Please describe your changes on the following line: -->

---
<!-- Thank you for contributing to Servo! Please replace each `[ ]` by `[X]` when the step is complete, and replace `___` with appropriate data: -->
- [ ] `./mach build -d` does not report any errors
- [ ] `./mach test-tidy` does not report any errors
- [ ] These changes fix #26108 (GitHub issue number if applicable)

<!-- Either: -->
- [ ] There are tests for these changes OR
- [ ] These changes do not require tests because ___

<!-- Also, please make sure that "Allow edits from maintainers" checkbox is checked, so that we can help you if you get stuck somewhere along the way.-->

<!-- Pull requests that do not address these steps are welcome, but they will require additional verification as part of the review process. -->
bors-servo added a commit that referenced this issue May 12, 2020
ServiceWorker: restructure Job Queue, Register flow, to better match spec

<!-- Please describe your changes on the following line: -->

---
<!-- Thank you for contributing to Servo! Please replace each `[ ]` by `[X]` when the step is complete, and replace `___` with appropriate data: -->
- [ ] `./mach build -d` does not report any errors
- [ ] `./mach test-tidy` does not report any errors
- [ ] These changes fix #26108 (GitHub issue number if applicable)

<!-- Either: -->
- [ ] There are tests for these changes OR
- [ ] These changes do not require tests because ___

<!-- Also, please make sure that "Allow edits from maintainers" checkbox is checked, so that we can help you if you get stuck somewhere along the way.-->

<!-- Pull requests that do not address these steps are welcome, but they will require additional verification as part of the review process. -->
bors-servo added a commit that referenced this issue May 21, 2020
ServiceWorker: restructure Job Queue, Register flow, to better match spec

<!-- Please describe your changes on the following line: -->

---
<!-- Thank you for contributing to Servo! Please replace each `[ ]` by `[X]` when the step is complete, and replace `___` with appropriate data: -->
- [ ] `./mach build -d` does not report any errors
- [ ] `./mach test-tidy` does not report any errors
- [ ] These changes fix #26108 (GitHub issue number if applicable)

<!-- Either: -->
- [ ] There are tests for these changes OR
- [ ] These changes do not require tests because ___

<!-- Also, please make sure that "Allow edits from maintainers" checkbox is checked, so that we can help you if you get stuck somewhere along the way.-->

<!-- Pull requests that do not address these steps are welcome, but they will require additional verification as part of the review process. -->
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-constellation Involves the constellation A-content/script Related to the script thread B-meta This issue tracks the status of multiple, related pieces of work
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants