1353636 - Change ServiceWorker useCache to an enum and default to "imports"

Status: RESOLVED FIXED

(Core :: DOM: Service Workers, enhancement)

Description

[Ben Kelly [:bkelly, not reviewing]]

In bug 1290944 we changed the service worker update check to use no-cache by default and provided a `useCache` boolean script could use to opt-in to http cache behavior.

At the recent spec meeting, however, we decided that this is not a good idea when combined with checking all importScripts() during update.  It seems that some 3rd party scripts being imported via importScripts() could be melted when this is turned on.

The new solution is to use an enumeration instead of a boolean:

https://github.com/w3c/ServiceWorker/issues/1104

The default will be no-cache for the top level script, but will use caching for importScripts().

This bug is to implement this new behavior.  I will write a separate bug to prevent us from shipping the boolean `useCache` in FF53.

Comment 1

[Ben Hsu [:HoPang]]

Attachment: P1: Hide useCache registrationOptions

Comment 2

[Ben Kelly [:bkelly, not reviewing]]

Note, Jake has written a wpt test for the updated spec:

https://github.com/w3c/web-platform-tests/pull/5515

Ho-Pang, can you review that pull request and give any feedback to Jake?  Thanks!

Comment 3

[Ben Hsu [:HoPang]]

Sorry for the late response, I've updated it just now :)

Comment 4

[Ben Kelly [:bkelly, not reviewing]]

Looks like the spec changes landed:

https://github.com/w3c/ServiceWorker/pull/1107

Comment 5

[Ben Hsu [:HoPang]]

Hi Ben,

After working on it for a while, I'd like to separate this bug into sub-bugs, where,

1). Pass the testcase provided by Jake (only include the ones testing via ServiceWorkerContainer.register() api)
2). Expose ServiceWorkerRegistration::updateViaCache (WebAPI).
3). Update SERVICEWORKERREGISTRAR_VERSION and corresponding implementations.
4). Other stuff in the testcase provided by Jake.

Since ServiceWorkerRegistration doesn't have a reference to its corresponding ServiceWorkerRegistrationInfo, we should acquire it via ServiceWorkerManager, and thus I'd like to do it in a separated bug.

Do you think this plan acceptable?

Comment 6

[Ben Kelly [:bkelly, not reviewing]]

> Do you think this plan acceptable?

Sounds good to me.

Comment 7

[Ben Hsu [:HoPang]]

Just a simple update here.

At this moment, I think I've what to pass phase I. However, there are something needed to be updated in the testcase provided by Jake, which I've commented on the github thread, so I am moving to the following phases.

Comment 8

[Ben Hsu [:HoPang]]

Attachment: P1: Implementation (WIP)

In this patch, I use ServivceWorkerUpdateViaCache to replace the original
LoadFlags which used for setting whether HttpCache should be bypassed when
updating ServiceWorkers.

One interesting thing about this patch is that we still save LoadFlags in
ServiceWorkerInfo, since the LoadFlags only affect imported scripts. Thus,
we can simply translate ServiceWorkerUpdateViaCache into the desired LoadFlags
via the following table, and are able keep the implementation after byte-check
remaining the same.

ServiceWorkerUpdateViaCache | LoadFlags
----------------------------+----------------
Imports                     | LOAD_NORMAL
All                         | LOAD_NORMAL
None                        | VALIDATE_ALWAYS

Comment 9

[Ben Hsu [:HoPang]]

This is another update, I've finished implementing phase 3. Now, I am waiting for the landing of new wpt-testcacse.

Comment 10

[Ben Kelly [:bkelly, not reviewing]]

The WPT test cases have landed in upstream and James Graham tell me they are in inbound now.  Hopefully they will be merged to m-c by the time you wake up tomorrow.

Comment 11

[Ben Hsu [:HoPang]]

Attachment: P1: Move from nsLoadFlags to UpdateViaCache

Comment 12

[Ben Hsu [:HoPang]]

Attachment: P2: Expose ServieworkerRegistration.updateViaCache

Comment 13

[Ben Hsu [:HoPang]]

Attachment: P3: Update SERVICEWORKERREGISTRAR_VERSION and a related gtest

Comment 14

[Ben Hsu [:HoPang]]

Attachment: P4: Enable a related web-platform-test. r=bkelly

Comment 15

[Ben Hsu [:HoPang]]

Comment on attachment 8894420 [details] [diff] [review]
P1: Move from nsLoadFlags to UpdateViaCache

Hi Ben, and thanks for your help

This part is the phase 1 in Comment 5, where its details are written in comment 8. May I ask for your review?

Comment 16

[Ben Hsu [:HoPang]]

Comment on attachment 8894421 [details] [diff] [review]
P2: Expose ServieworkerRegistration.updateViaCache

This part is for the phase 2 of comment 5.

Comment 17

[Ben Hsu [:HoPang]]

Comment on attachment 8894422 [details] [diff] [review]
P3: Update SERVICEWORKERREGISTRAR_VERSION and a related gtest

This part is for phase 3 in comment 5.

Comment 18

[Ben Hsu [:HoPang]]

Comment on attachment 8894426 [details] [diff] [review]
P4: Enable a related web-platform-test. r=bkelly

This is the test case for both phase 1 and phase 2 in comment 5.

Comment 19

[Ben Kelly [:bkelly, not reviewing]]

Comment on attachment 8894420 [details] [diff] [review]
P1: Move from nsLoadFlags to UpdateViaCache

Review of attachment 8894420 [details] [diff] [review]:
-----------------------------------------------------------------

r=me with comments addressed.

::: dom/webidl/ServiceWorkerContainer.webidl
@@ +42,4 @@
>  
>  dictionary RegistrationOptions {
>    USVString scope;
> +  ServiceWorkerUpdateViaCache updateViaCache;

Please use the default `= "imports"` like the spec does.  Boris tells me this should be supported.

::: dom/workers/ServiceWorkerContainer.cpp
@@ +200,5 @@
>    }
>  
> +  ServiceWorkerUpdateViaCache updateViaCache =
> +    aOptions.mUpdateViaCache.WasPassed() ? aOptions.mUpdateViaCache.Value()
> +                                         : ServiceWorkerUpdateViaCache::Imports;

If we use the default value in the webidl, this should not be necessary.

::: dom/workers/ServiceWorkerInfo.cpp
@@ +235,5 @@
> +
> +  // Scripts of a service worker should always be loaded bypass service workers.
> +  // Otherwise, we might not be able to update a service worker correctly, if
> +  // there is a service worker generating the script.
> +  MOZ_ASSERT(mLoadFlags & nsIChannel::LOAD_BYPASS_SERVICE_WORKER);

MOZ_DIAGNOSTIC_ASSERT() for something cheap like this.

::: dom/workers/ServiceWorkerInfo.h
@@ +34,4 @@
>    ServiceWorkerState mState;
>    OriginAttributes mOriginAttributes;
>  
> +  // This LoadFlags is only applied to imorted scripts, since the main script

s/imorted/imported/

@@ +34,5 @@
>    ServiceWorkerState mState;
>    OriginAttributes mOriginAttributes;
>  
> +  // This LoadFlags is only applied to imorted scripts, since the main script
> +  // has alreday been downloaded when practicing bytecheck. This LoadFlag is

s/alreday/already/
s/practicing bytecheck/performing the bytecheck/

@@ +41,5 @@
> +  //   2. (Optional) nsIRequest::VALIDATE_ALWAYS
> +  //      depends on ServiceWorkerUpdateViaCache of its registration.
> +  //   3. (optional) nsIRequest::LOAD_BYPASS_CACHE
> +  //      depends on whether the update timer is expired.
> +  const nsLoadFlags mLoadFlags;

Maybe rename this to "mImportsLoadFlags" or something in a follow-on patch?

::: dom/workers/ServiceWorkerManager.cpp
@@ +210,5 @@
>    }
>  
> +  // This is a workaround before we update SERVICEWORKERREGISTRAR_VERSION and
> +  // the related implementation.
> +  ServiceWorkerUpdateViaCache UVC = aRegistration->GetUpdateViaCache();

nit: Lower case chars for stack variables.

@@ +2055,5 @@
>  
> +  // This is a workaround before we update SERVICEWORKERREGISTRAR_VERSION and
> +  // the related implementation.
> +  nsLoadFlags flags = aRegistration.loadFlags();
> +  ServiceWorkerUpdateViaCache UVC =

nit: uvc

::: dom/workers/ServiceWorkerUpdateJob.cpp
@@ +430,5 @@
>  
>    // Begin step 7 of the Update algorithm to evaluate the new script.
> +  nsLoadFlags flags = aLoadFlags;
> +  if (GetUpdateViaCache() == ServiceWorkerUpdateViaCache::None) {
> +    aLoadFlags |= nsIRequest::VALIDATE_ALWAYS;

This needs to |= flags, not aLoadFlags, right?  It seems this value is being lost.

Also, can we rename this to importFlags or something else to help explain why we are only skipping cache for "none", but not "imports"?

Comment 20

[Ben Kelly [:bkelly, not reviewing]]

I'll try to pick this up again later tonight after the kids are in bed.

Comment 21

[Ben Kelly [:bkelly, not reviewing]]

Comment on attachment 8894422 [details] [diff] [review]
P3: Update SERVICEWORKERREGISTRAR_VERSION and a related gtest

Review of attachment 8894422 [details] [diff] [review]:
-----------------------------------------------------------------

r=me with comments addressed.  I only skimmed the test.

::: dom/workers/ServiceWorkerManager.cpp
@@ +2075,5 @@
>  
> +  nsLoadFlags flags =
> +    aRegistration.updateViaCache() == static_cast<uint16_t>(ServiceWorkerUpdateViaCache::All)
> +      ? nsIRequest::LOAD_NORMAL
> +      : nsIRequest::VALIDATE_ALWAYS;

Maybe clarify this is for the top level script only?

::: dom/workers/ServiceWorkerRegistrar.cpp
@@ +378,5 @@
> +      entry->updateViaCache() = updateViaCache.ToInteger(&rv, 16);
> +      if (NS_WARN_IF(NS_FAILED(rv))) {
> +        return rv;
> +      } else if (entry->updateViaCache() < nsIServiceWorkerRegistrationInfo::UPDATE_VIA_CACHE_IMPORTS &&
> +                 entry->updateViaCache() > nsIServiceWorkerRegistrationInfo::UPDATE_VIA_CACHE_NONE) {

I think this will always evaluate to false.  Since its an unsigned int you probably just want the check against the max value.

::: dom/workers/ServiceWorkerRegistrarTypes.ipdlh
@@ +19,4 @@
>  
>    PrincipalInfo principal;
>  
> +  uint16_t updateViaCache;

Our packing sucks here, but moving the fields around would require fixing any constructors as well.  Maybe file a follow-up bug?  Ideally the 2-byte int and the 1-byte bool would be at the end of the list.

Comment 22

[Ben Kelly [:bkelly, not reviewing]]

Comment on attachment 8894426 [details] [diff] [review]
P4: Enable a related web-platform-test. r=bkelly

Review of attachment 8894426 [details] [diff] [review]:
-----------------------------------------------------------------

Thanks!

Comment 23

[Ben Hsu [:HoPang]]

Attachment: (V2) P1: Move from nsLoadFlags to UpdateViaCache. r=bkelly

Comment addressed.

Comment 24

[Ben Hsu [:HoPang]]

Attachment: (V2) P2: Expose ServieworkerRegistration.updateViaCache. r=bkelly

Fixed an unused variable in an assertion.

Comment 25

[Ben Hsu [:HoPang]]

Attachment: (V2) P3: Update SERVICEWORKERREGISTRAR_VERSION and a related gtest. r=bkelly

Comment addressed.

Comment 26

[Ben Hsu [:HoPang]]

Attachment: (V2) P3: Update SERVICEWORKERREGISTRAR_VERSION and a related gtest. r=bkelly

Comment addressed

Comment 27

[Ben Hsu [:HoPang]]

Attachment: (V3) P3: Update SERVICEWORKERREGISTRAR_VERSION and a related gtest. r=bkelly

Fix a typo!

Comment 28

[Ben Hsu [:HoPang]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=699af3dced7b61be257cae076579fea65e338d65

Comment 29

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/9865180c7e59
Part 1: Move from nsLoadFlags to UpdateViaCache. r=bkelly
https://hg.mozilla.org/integration/mozilla-inbound/rev/128194809ab3
Part 2: Expose ServieworkerRegistration.updateViaCache. r=bkelly
https://hg.mozilla.org/integration/mozilla-inbound/rev/cbba741c3061
Part 3: Update SERVICEWORKERREGISTRAR_VERSION and a related gtest. r=bkelly
https://hg.mozilla.org/integration/mozilla-inbound/rev/8cda11dd3e3f
Part 4: Enable a related web-platform-test. r=bkelly

Comment 30

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/9865180c7e59
https://hg.mozilla.org/mozilla-central/rev/128194809ab3
https://hg.mozilla.org/mozilla-central/rev/cbba741c3061
https://hg.mozilla.org/mozilla-central/rev/8cda11dd3e3f

Comment 31

[Andrew Sutherland [:asuth] (he/him)]

We currently have 9 different intermittents on the WPT registration-updateviacache.https.html[1] updated in part 4, :HoPang, can you take a look at them?  (I hit them a few different times on an unrelated try run; I'm not volunteering for ServiceWorker intermittent triage duty ;)

1: https://bugzilla.mozilla.org/buglist.cgi?quicksearch=updateviacache

Comment 32

[Ben Kelly [:bkelly, not reviewing]]

I believe we landed a fix to the test in the last 24 hours or so.

Comment 33

[Andrew Sutherland [:asuth] (he/him)]

(In reply to Ben Kelly [:bkelly] from comment #32)
> I believe we landed a fix to the test in the last 24 hours or so.

To mozilla repos, or the github repo?  This is the tip of mozilla-inbound and mozilla-central for the file which still shows only 1 revision:
https://hg.mozilla.org/integration/mozilla-inbound/log/tip/testing/web-platform/tests/service-workers/service-worker/registration-updateviacache.https.html
https://hg.mozilla.org/mozilla-central/log/tip/testing/web-platform/tests/service-workers/service-worker/registration-updateviacache.https.html

Comment 34

[Ben Kelly [:bkelly, not reviewing]]

It was in a resources file. See bug 1391532.

Comment 35

[Andrew Sutherland [:asuth] (he/him)]

Awesome!  Sorry about the noise!

(Also, clearly I need to improve how I do my bugzilla quicksearches, because if mine had included FIXED with a recent transition, I would have seen that more obviously.  Also, I probably should also just abandon thunderbird in favor of gmail; if I weren't afraid of the 15-second lockups on its quicksearch/quickfilter, I would have found the recent activity as well.)