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 add a “script type” check in the register algorithm. #1359

Closed
d0iasm opened this issue Oct 4, 2018 · 6 comments
Closed

Should add a “script type” check in the register algorithm. #1359

d0iasm opened this issue Oct 4, 2018 · 6 comments
Assignees

Comments

@d0iasm
Copy link

d0iasm commented Oct 4, 2018

In the current spec, we finish a new register job when it equals newestWorker. Spec says (Register 5. 3. ):

  1. If newestWorker is not null, job’s script url equals newestWorker’s script url, and job’s update via cache mode's value equals registration’s update via cache mode, then:
    1. Invoke Resolve Job Promise with job and registration.
    2. Invoke Finish Job with job and abort these steps.

However, a new register job should be alive when its script url/update via cache equal newestWorker’s ones but its script type does not equal newestWorker’s one. I think script type should also be checked.

@mfalken
Copy link
Member

mfalken commented Oct 5, 2018

Yes, this looks like just an oversight. It should be easy to patch the spec. Did you have WPT ready to test this?

@d0iasm
Copy link
Author

d0iasm commented Oct 24, 2018

Did you have WPT ready to test this?

Yes. I just added a new test for updating the registration with different script type.
web-platform-tests/wpt#13692

@d0iasm
Copy link
Author

d0iasm commented Oct 24, 2018

Just concern:
If in the case that

  1. A user register SW as classic script type.
  2. A user update it to module script type and the main script using module specific syntax(like “import ‘sw2.js’;”).
  3. Only run Update Job without running Register Job (It would happen if a user accesses a page in scope but not a top-level page).
  4. Fail update because of an evaluation error.

Update Job keeps to fail until a user accesses a top-level page having navigator.serviceWorker.register(‘sw2.js’, {type: ‘module’});

Is this behavior intentional?

@mfalken
Copy link
Member

mfalken commented Oct 24, 2018

Is this behavior intentional?

If I'm understanding the concern, it's that the server has changed the script from a classic script to a module script and so Soft Update would fail because the registration is still set to classic script.

I think that's fine. To change the script type you should probably change the script URL to avoid this problem. I'm assuming there is no plan to do something like add info to HTTP response headers to let the browser know whether it's a module or classic script? @domenic

@annevk
Copy link
Member

annevk commented Oct 29, 2018

There's whatwg/html#3205, but that's really about confirming the request and hasn't gotten support (though maybe it could be used for this as well).

@mfalken mfalken self-assigned this Jun 3, 2019
mfalken added a commit to mfalken/ServiceWorker that referenced this issue Jun 3, 2019
This makes the following changes:
* updateViaCache is a live property. This is covered by the WPT
registration-updateviacache.https.html. However we are missing tests for a
register() that rejects with a new updateViaCache property. The desired
behavior is to only update the property if register() resolves.
* Changing worker type bypasses the byte-for-byte update check. This is covered
by WPT update-registration-with-type.https.html.
* Adds Worker type and updateViaCache to the job equivalence check. This
probably can have a WPT for successive register() calls that vary these
properties don’t get coalesced into a single job.

Addresses w3c#1189, w3c#1408, w3c#1359, and w3c#1358.
mfalken added a commit that referenced this issue Jun 3, 2019
…1411)

This makes the following changes:
* updateViaCache is a live property. This is mostly covered by the WPT
registration-updateviacache.https.html. A test that the updateViaCache
change is only committed if the register() call resolves is added in
web-platform-tests/wpt#17131.
* Changing worker type bypasses the byte-for-byte update check. This is covered
by WPT update-registration-with-type.https.html.
* Adds Worker type and updateViaCache to the job equivalence check. A
test for this is added in web-platform-tests/wpt#17131.

Addresses #1189, #1408, #1359, and #1358.
@mfalken
Copy link
Member

mfalken commented Jun 3, 2019

Addressed in #1411.

@mfalken mfalken closed this as completed Jun 3, 2019
pull bot pushed a commit to luojiguicai/chromium that referenced this issue Mar 5, 2021
…gorithm

This CL updates the spec comment since the spec issue was resolved.
w3c/ServiceWorker#1359

Also, 1) updates a TODO comment from using a specific person's name to
the crbug link. 2) Rename the method from
"ContinueWithRegistrationForSameScriptUrl" to
"ContinueWithRegistrationWithSameRegistrationOptions" because it
requires the equality of the script type and the update-via-cache option
too.

No behavior change.

Bug: 824647
Change-Id: I44e146554010e77b9833abc1d2df55e0b1bb78a4
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2732746
Commit-Queue: Asami Doi <asamidoi@chromium.org>
Reviewed-by: Hiroki Nakagawa <nhiroki@chromium.org>
Cr-Commit-Position: refs/heads/master@{#860113}
mjfroman pushed a commit to mjfroman/moz-libwebrtc-third-party that referenced this issue Oct 14, 2022
…gorithm

This CL updates the spec comment since the spec issue was resolved.
w3c/ServiceWorker#1359

Also, 1) updates a TODO comment from using a specific person's name to
the crbug link. 2) Rename the method from
"ContinueWithRegistrationForSameScriptUrl" to
"ContinueWithRegistrationWithSameRegistrationOptions" because it
requires the equality of the script type and the update-via-cache option
too.

No behavior change.

Bug: 824647
Change-Id: I44e146554010e77b9833abc1d2df55e0b1bb78a4
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2732746
Commit-Queue: Asami Doi <asamidoi@chromium.org>
Reviewed-by: Hiroki Nakagawa <nhiroki@chromium.org>
Cr-Commit-Position: refs/heads/master@{#860113}
GitOrigin-RevId: 0fc784bcca55e485ca26e3194b64ca372c0057a1
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

No branches or pull requests

3 participants