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

Should update() called on an installing worker reject (as currently does?) #1155

Closed
jungkees opened this issue May 29, 2017 · 9 comments · May be fixed by web-platform-tests/wpt#6107
Closed

Comments

@jungkees
Copy link
Collaborator

registration.update() step 5 has:

If the context object’s relevant settings object’s global object globalObject is a ServiceWorkerGlobalScope object, and globalObject’s associated service worker's state is installing, reject p with an "InvalidStateError" exception and abort these steps.

@mattto, @wanderview, AFAICT, Chrome doesn't have this error checking. Does Firefox check this? Do we need this error checking here?

@wanderview
Copy link
Member

I believe we do. We added it to prevent the service worker spinning on a SW that calls update() before install completes and then has its install fail. If we queue an update job in those circumstances we will continuously run jobs that each fail. So we added this step to prevent self-update on uninstalled SW.

@jungkees
Copy link
Collaborator Author

Right. I remember the discussion. I'll keep it while addressing the issue raised through #1152: ensuring update() returns a promise regardless of the error conditions.

Do we have a wpt test for this already by the way?

@jungkees
Copy link
Collaborator Author

Fixed the control flow of update() method that rejected a non-returned promise and aborted right away: 6281143.

@wanderview
Copy link
Member

There is this test, but it seems to suggest it shouldn't throw:

https://github.com/w3c/web-platform-tests/blob/56ea52b8668ceee63ec196c59dd11ad7797e4f0d/service-workers/service-worker/ServiceWorkerGlobalScope/resources/update-worker.js#L25

It seems we pass the test, so perhaps we don't implement the exception yet.

@jungkees
Copy link
Collaborator Author

Added a check to the test to ensure it throws: web-platform-tests/wpt#6107. Running the test with this change locally, chrome passes it already.

@delapuente
Copy link

Let me a question, please. What happens if I call update() twice in a row on an already installed service worker?

@jungkees
Copy link
Collaborator Author

jungkees commented Jun 9, 2017

They are basically scheduled to the job queue. If the job queue is empty when the first update is scheduled, that job is run right away. So, the second update will be queued and run later again. If more (same) update jobs are scheduled, they are not queued but aggregated (Schedule Job step 2.2) as a duplicate to the update job already pushed in the queue.

EDIT: I was wrong. Please see the following comment.

@jungkees
Copy link
Collaborator Author

jungkees commented Jun 9, 2017

@delapuente, the first job queued and run will be popped from the queue when the job finishes. So, the second job should have been aggregated no matter whether the job queue was empty or not when the first job arrived. That is, the consecutive update() calls will always be aggregated. So, only one update occurs. (Note that, if other type of jobs (register or unregister) are queued in between, the latter update job is queued.)

@mfalken
Copy link
Member

mfalken commented Jun 10, 2019

Closing this one as it's been settled and in the spec, though see related #1429.

@mfalken mfalken closed this as completed Jun 10, 2019
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 a pull request may close this issue.

4 participants