1290944 - load service worker scripts no-cache by default

Status: RESOLVED FIXED

(Core :: DOM: Service Workers, defect, P2)

Description

[Ben Kelly [:bkelly, not reviewing]]

We've decided to load service workers no-cache by default:

https://github.com/slightlyoff/ServiceWorker/issues/893#issuecomment-236016162

There is still some spec work to do to provide an API to opt-in to the http cached behavior.

We will always still bypass after 24 hours.

Comment 1

[Hsin-Yi Tsai (she/her) [:hsinyi]]

Ben is gong to work on this!

Comment 2

[Ben Hsu [:HoPang]]

Attachment: Part 1: Add useCache to RegistrationOptions

Comment 3

[Ben Hsu [:HoPang]]

Attachment: Part 2: Passing useCache to ServiceWorkerRegisterJob

Comment 4

[Ben Hsu [:HoPang]]

Attachment: Part 3: Introduce a new testcase (WIP)

Comment 5

[Ben Hsu [:HoPang]]

Hi Tom,

Since I am new to this field, I am not sure whether I understand this issue correctly. I think we are making service worker registering fetching new script by default. Thus, I place a flag in  ServiceWorkerRegisterJob via which we can skip a condition check to proceed to fetch new script. In addition to the implementation code, I steal an existing testcase to prove my concept and prototype. Do you mind giving me some advice?

Comment 6

[Ben Kelly [:bkelly, not reviewing]]

This bug is more about setting VALIDATE_ALWAYS on the nsIChannel load flags:

https://dxr.mozilla.org/mozilla-central/source/dom/workers/ServiceWorkerScriptCache.cpp#625

Note, we need to verify that this affects importScripts() as well.  I have my doubts that it propagates properly through ScriptLoader, but haven't looked.  I wrote bug 1290939 about that previously.

Comment 7

[Ben Hsu [:HoPang]]

Thanks for the information! 

Per an helpful offline discussion with TOM, I found out that I was terribly wrong thinking the 24hr criteria[0] is implementing elsewhere. but still I have some questions for the settings of nsIRequest after reading comments in nsIRequest.idl[0].

1) Do LOAD_BYPASS_CACHE send out a normal request and still update the Http cache?
2) Should we do align the settings for 24hr expiration updating and script loading when registration?

[0] https://dxr.mozilla.org/mozilla-central/source/dom/workers/ServiceWorkerScriptCache.cpp#628
[1] https://dxr.mozilla.org/mozilla-central/source/netwerk/base/nsIRequest.idl#150

Comment 8

[Ben Kelly [:bkelly, not reviewing]]

I find it helpful to look at this method which maps the fetch spec cache values to our internal flags:

  https://dxr.mozilla.org/mozilla-central/source/netwerk/protocol/http/HttpBaseChannel.cpp#2442

Here you can see that "LOAD_BYPASS_CACHE" corresponds to the fetch spec "reload" value.  So it should update the http cache.

So what we want is:

* By default use 'no-cache' fetch cache mode which corresponds to VALIDATE_ALWAYS load flag.
* If the code sets 'useCache:true' during the register then we should use 'default' fetch cache mode which corresponds to no load flag.
* No matter what, if 24 hours have passed since last check we set LOAD_BYPASS_CACHE.

And these values should apply to both the top level SW script and any importScripts().

Note, we will also need to persist the 'useCache:true' value.  So this will involve updating the on-disk ServiceWorkerRegistrar file format, etc.

Does that help?  Not sure if I completely understood your question (2).

Comment 9

[Ben Hsu [:HoPang]]

Greatly appreciate your responses, they indeed help a lot :)

Comment 10

[Ben Hsu [:HoPang]]

Attachment: Part 1: Add useCache to RegistrationOptions(WebIDL)

Comment 11

[Ben Hsu [:HoPang]]

Attachment: Part 2: Load the top level SW script with http cache bypass settings

Comment 12

[Ben Hsu [:HoPang]]

Attachment: Part 3: ImportScripts() in SW should use the same http cache bypass settings as its top level SW script

Comment 13

[Ben Hsu [:HoPang]]

Attachment: Part 4: Update an existing testcase

Comment 14

[Ben Hsu [:HoPang]]

Attachment: Part 3: ImportScripts() in SW should use the same http cache bypass settings as its top level SW script

Comment 15

[Ben Hsu [:HoPang]]

Attachment: Part 2: Load the top level SW script with http cache bypass settings

Comment 16

[Ben Hsu [:HoPang]]

Hi Ben, 

Do you mind reviewing these patches? Hope you don't mind that I try to solve both of them in this bug since their implementations are related to each other in some levels, where Part 1 and Part 2 patches are mainly for this bug, while part 3 patch is mainly for 1290939.

Part 1:
Extend the WebIDL file.

Part 2:
Force download the top-level script by default and make it able to opt out with a WebAPI parameter.

Part 3:
Since the http cache setting is not saved, I save it in ServiceWorkerInfo, and later pass it to the WorkerLoadInfo which controls how the script files are loaded in the worker. 

Part 4:
Testcase!

Besides, I have a question about imported scripts.
Do we have to update imported scripts when the top-level script remains the same?

Comment 17

[Ben Kelly [:bkelly, not reviewing]]

Sorry, I ran out of time today.  I'll review this tomorrow.

Comment 18

[Ben Kelly [:bkelly, not reviewing]]

Comment on attachment 8809289 [details] [diff] [review]
Part 2: Load the top level SW script with http cache bypass settings

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

This is a good start, but I think it would be better to pass the default load flags instead of a boolean value.  Also, I'm not sure removing the register() short-circuit step is correct.

Please try with the load flags approach and flag again.  Thanks!

::: dom/workers/ServiceWorkerContainer.cpp
@@ +208,5 @@
>    }
>  
> +  // TODO: Add spec link here
> +  bool bypassHttpCache =
> +    aOptions.mUseCache.WasPassed() ? !aOptions.mUseCache.Value() : true;

So, I mention in other comments that "bypass http cache" is not a great name for the flag because:

1. Its confusing that it does not actually imply LOAD_BYPASS_CACHE, but something different.
2. Its a boolean flag, but the opposite of the public API and what the spec will presumably use.

Also, using booleans in APIs is generally discouraged because it makes reading call sites harder.

I think you could solve all of these problems by passing a nsIRequest load flags value instead of a boolean.

So here you would do:

  uint32_t loadFlags = aOptions.mUseCache.WasPassed() && !aOptions.mUseCache.Value()
                     ? nsIRequest::VALIDATE_ALWAYS
                     : nsIRequest::LOAD_DEFAULT;

Then if we exceed the 24 hour threshold you can do:

  aLoadFlags |= nsIRequest::LOAD_BYPASS_CACHE;

::: dom/workers/ServiceWorkerRegisterJob.cpp
@@ +45,5 @@
>        swm->StoreRegistration(mPrincipal, registration);
>      }
>      registration->mPendingUninstall = false;
>      RefPtr<ServiceWorkerInfo> newest = registration->Newest();
> +    if (!mBypassHttpCache && newest && mScriptSpec.Equals(newest->ScriptSpec())) {

I don't see anything in the spec issue about this change.  This check basically prevents an update from being attempted, so I don't think the `useCache` flag should influence it at all.

::: dom/workers/ServiceWorkerScriptCache.cpp
@@ +633,5 @@
>      flags |= nsIRequest::LOAD_BYPASS_CACHE;
>    }
>  
> +  if (aBypassHttpCache) {
> +    flags |= nsIRequest::VALIDATE_ALWAYS;

Another reason to pass a "UseCache" value is the name "bypass cache" is confusing when mixed with out nsIRequest flags.  Here we have a "bypass cache" flag that does not set LOAD_BYPASS_CACHE.

::: dom/workers/ServiceWorkerScriptCache.h
@@ +48,5 @@
>  nsresult
>  Compare(ServiceWorkerRegistrationInfo* aRegistration,
>          nsIPrincipal* aPrincipal, const nsAString& aCacheName,
> +        const nsAString& aURL, CompareCallback* aCallback,
> +        nsILoadGroup* aLoadGroup, bool mBypassHttpCache);

nit: aBypassHttpCache

::: dom/workers/ServiceWorkerUpdateJob.cpp
@@ +180,5 @@
>    RefPtr<ServiceWorkerRegistrationInfo> ref = mRegistration;
>    return ref.forget();
>  }
>  
>  ServiceWorkerUpdateJob::ServiceWorkerUpdateJob(Type aType,

You do not initialize mBypassHttpCache in the other constructor.  Its getting a random value.

::: dom/workers/ServiceWorkerUpdateJob.h
@@ +36,5 @@
>                           nsIPrincipal* aPrincipal,
>                           const nsACString& aScope,
>                           const nsACString& aScriptSpec,
> +                         nsILoadGroup* aLoadGroup,
> +                         bool mBypassHttpCache);

nit: Args should start with "a" and member start with "m".  So this should be `aBypassHttpCache`.

@@ +66,5 @@
>    // Finish().  This method corresponds to the spec Update algorithm.
>    void
>    Update();
>  
> +  bool mBypassHttpCache;

Please make this const.

Passing around "bypass cache" is a bit confusing when the spec and API is the logical opposite.  Could we store and pass "UseCache" instead?

nit: And I'd prefer to keep members private and expose a protected BypassHttpCache() getter method instead.

Comment 19

[Ben Kelly [:bkelly, not reviewing]]

Comment on attachment 8809287 [details] [diff] [review]
Part 3: ImportScripts() in SW should use the same http cache bypass settings as its top level SW script

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

I'm sorry, but there are a few issues with this patch.  The biggest:

1. The `useCache` value is not persisted with the service worker registration on disk.  So restarting the browser just falls back to a bogus default value.
2. I don't think the cache settings are passed correctly.  It seems update jobs always use the equivalent if LOAD_BYPASS_CACHE which is not correct.

::: dom/workers/ScriptLoader.cpp
@@ +2263,5 @@
>    info->mURL = aScriptURL;
> +  // For a service worker, before trying to load the main script here, the new
> +  // script has already been loaded into http cache when comparing with the
> +  // stale copy in the DOM cache. Thus, we don't have to actually download it
> +  // again here.

I thought the comparison code stored the script in the CacheStorage API for offline use.  Shouldn't we hit that case for the main script instead of performing a new nsIChannel request?  I'm honestly not sure, but I think we need to know the answer here in order to determine if this is the correct change.  Can you add some debug and test?  If its not stored in CacheStorage I think we should understand why.

Note that bug 1290951 will make all the importScripts() effectively hit this same case as well.

::: dom/workers/ServiceWorkerManager.cpp
@@ +1696,5 @@
>    const nsCString& currentWorkerURL = aRegistration.currentWorkerURL();
>    if (!currentWorkerURL.IsEmpty()) {
>      registration->SetActive(
>        new ServiceWorkerInfo(registration->mPrincipal, registration->mScope,
> +                            currentWorkerURL, aRegistration.cacheName(), WorkerLoadInfo::Restore));

This is wrong.  You must save the `useCache` value in the registration file on disk and reload it.

::: dom/workers/ServiceWorkerUpdateJob.cpp
@@ +415,5 @@
>    // Begin step 7 of the Update algorithm to evaluate the new script.
> +  WorkerLoadInfo::HttpCacheSetting httpCacheSetting = WorkerLoadInfo::Normal;
> +  switch (mType) {
> +    case Type::Update:
> +      httpCacheSetting = WorkerLoadInfo::Restore;

Why does an update job always use LOAD_BYPASS_CACHE?  That is wrong.  It should use the LOAD_DEFAULT or VALIDATE_ALWAYS depending on if `useCache` was passed in the `register()` call.  We only should use LOAD_BYPASS_CACHE if the 24 hour threshold has been passed.

::: dom/workers/Workers.h
@@ +271,5 @@
>    bool mStorageAllowed;
>    bool mServiceWorkersTestingInWindow;
>    PrincipalOriginAttributes mOriginAttributes;
>  
> +  enum HttpCacheSetting {

Similar to the last patch, I think it would be better to store the nsIRequest load flags instead of a new type definition.

Comment 20

[Ben Kelly [:bkelly, not reviewing]]

Comment on attachment 8809228 [details] [diff] [review]
Part 4: Update an existing testcase

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

A good start, but I think my comments in the previous patches will make these tests not work.  For example, I think your `register()` calls should not produce the results you expect here.

Also, it would be nice if you could verify that `useCache: false` means only revalidation.  For example, create a server python script that returns a 304 response if it sees an `If-Modified-Since` or `If-None-Match` header.

::: testing/web-platform/tests/service-workers/service-worker/registration.https.html
@@ +384,5 @@
> +        .then(worker => getMessageFromServiceWorker(worker))
> +        .then(value => saved = value)
> +
> +        // Register for the second time.
> +        .then(() => navigator.serviceWorker.register(script, {scope: scope}))

I don't think using `register()` here makes sense.  This should short-circuit since the script is the same as the installed service worker.  Please use `registration.update()` instead.

@@ +425,5 @@
> +
> +        // Register for the second time.
> +        .then(() => navigator.serviceWorker.register(script, {scope: scope, useCache: true}))
> +        .then(registration => {
> +            let p1 = new Promise(resolve => window.setTimeout(resolve, 1000));

Instead of using a setTimeout() please register a separate dummy service worker on a different scope.  By the time that service worker installs and activates you should be reasonably assured this update should have fired.

Comment 21

[Ben Kelly [:bkelly, not reviewing]]

Note, I also wrote this question/comment on the spec issue:

https://github.com/w3c/ServiceWorker/issues/893#issuecomment-260442769

Comment 22

[Ben Hsu [:HoPang]]

Hi Ben, and greatly thanks for your careful review.

----

I spent some time digging deeper into how actually service worker scripts are loaded, and I must apology for that I was wrong about the usage of the DOM cache of a service worker. Even with the APIs[0][1] loading the script are called with the URL of a script, the copy in the DOM cache of a service worker is visited[2] and used[3] when loading its main script and the imported scripts. However, I am not quite sure whether the imported scripts are saved, since I failed to find `put()` is called after loading the script from the network. If you don't mind, I'd like to take 1290951 and do a better test for this at that time.

[0] http://searchfox.org/mozilla-central/source/dom/workers/ScriptLoader.cpp#2242
[1] http://searchfox.org/mozilla-central/source/dom/workers/ScriptLoader.cpp#2256
[2] http://searchfox.org/mozilla-central/source/dom/workers/ScriptLoader.cpp#1528
[3] http://searchfox.org/mozilla-central/source/dom/workers/ScriptLoader.cpp#1683

In fact, I have been struggled how to revising these patches. There are two major questions I'd like to clear, (1) where to save the LoadFlags and (2) what to do when registering a new service worker with a different LoadFlags while there is already an active service worker with the same URL.

For the first question, according to the review, I think ServiceWorkerRegistrationInfo is more preferred to merely ServiceWorkerInfo, since maybe we'd like to expose this attribute and we have to save LoadFlags for restoring service workers in Gecko. Thus, the new patches mainly save LoadFlags in a ServiceWorkerRegistration and propagate down to ServiceWorkerInfo and components needed to load the script files. Now, I make the LoadFlags a constant in ServiceWorkerInfo, which makes the LoadFlags service worker doesn't affected by the registration. 

As to the second question, though the new patches now short-circuit the registration without doing anything, I still have some questions during this circumstance...

(1) Do what want to update the LoadFlags before short-circuiting the registration?
(2) If we don't, are we not doing anything for the invocation even with a different LoadFlags?
    and it seems that the LoadFlags of a ServiceWorkerRegistrationInfo can never be modified.
(3) If we do want to update the LoadFlags, should the new LoadFlags affect the currently active service worker?
(4) Do we accept that the active service worker still running the old scripts after registering a service worker with `useCache: false`?

----

Given that the new patches for Part 2 and Part 3 are very different from the previous patches, I split them to make them easier to review, and each patch is self compilable. Below is the concise overview of the patch, while the more detailed description stays with each patch if there is any.

Patch 2-1:
Save the LoadFlags in ServiceWorkerRegistrationInfo, make is an essential parameter when creating a ServiceWorkerRegistrationInfo, and update related IPC files.

Patch 2-2: 
Fetch the main service worker script according to the LoadFlags in its ServiceWorkerRegistrationInfo. (Affect both registering and updating)

Patch 2-3:
Set the LoadFlags when invoking `register()`

Patch 3-1:
Propagate the LoadFlags of a ServiceWorkerRegistrationInfo to ServiceWorkerInfo when creating a ServiceWorkerInfo.

Patch 3-2:
Save the result of the script expiration check and pass it when creating a new ServiceWorkerRegistrationInfo.

Patch 3-3:
Propagate the LoadFlags to related components.

Patch 4:
Extend an existing testcase.

Comment 23

[Ben Hsu [:HoPang]]

Attachment: (V2) Part 2.1: Save load flags in ServiceWorkerRegistration

Comment 24

[Ben Hsu [:HoPang]]

Attachment: (V2) Part 2.2: Load the main script with the load flags of the ServiceWorkerRegistration

Comment 25

[Ben Hsu [:HoPang]]

Attachment: (V2) Part 2.3: Set the load flags when registering a service worker

Comment 26

[Ben Hsu [:HoPang]]

Comment on attachment 8814814 [details] [diff] [review]
(V2) Part 2.3: Set the load flags when registering a service worker

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

::: dom/workers/ServiceWorkerRegisterJob.cpp
@@ +50,5 @@
>      if (newest && mScriptSpec.Equals(newest->ScriptSpec())) {
>        SetRegistration(registration);
>        Finish(NS_OK);
>        return;
>      }

As comment 22 mentioned, though the spec doesn't say what we have to here, should we start thinking doing anything in this branch[0] such as updating the LoadFlags or creating a new service worker?

[0] http://searchfox.org/mozilla-central/source/dom/workers/ServiceWorkerRegisterJob.cpp#38

Comment 27

[Ben Hsu [:HoPang]]

Attachment: (V2) Part 3.1: Propogate load flags to ServiceWorkerInfo

Comment 28

[Ben Hsu [:HoPang]]

Attachment: (V2) Part 3.2: Pass LOAD_BYPASS_CACHE caused by script expiration timer to ServiceWorkerInfo

Comment 29

[Ben Hsu [:HoPang]]

Attachment: (V2) Part 3.3: Propagate load flags to ScriptLoader when loading scripts

Comment 30

[Ben Hsu [:HoPang]]

Attachment: (V2) Part 4: Update an existing testcase

Comment 31

[Ben Kelly [:bkelly, not reviewing]]

I asked Jake about your concerns regarding changing useCache values.  He suggests we should not perform the register() short circuit if the useCache value changes:

9:31 AM <wanderview> JakeA: so we are implementing the "use no-cache by default on updates" thing in gecko and a question arose
9:32 AM <wanderview> the issue defines an API like register(scriptURL, { useCache: true })
9:32 AM <wanderview> the spec also short circuits registration if the scriptURL and scope are the same as an existing registration
9:33 AM <wanderview> our question is, should that short circuit still happen even if the "useCache" value has changed?
9:33 AM <wanderview> or should there be another way for a service worker to change its "useCache" value?
9:33 AM <wanderview> JakeA: ^^^
9:34 AM <wanderview> I'm inclined to implement without any ability to change useCache to start, but thought I would get your opinion
9:34 AM <JakeA> wanderview: changing the value of useCache in .register feels like the right way to update this value
9:35 AM <JakeA> wanderview: whether it causes an immediate refetch of the SW (like changing the url does) is another question
9:35 AM <wanderview> JakeA: so the bypass check should be "if script URL, scope, and useCache value are identical, then bypass" ?
9:35 AM <JakeA> wanderview: yeah
9:36 AM <wanderview> ok
9:36 AM <wanderview> JakeA: it would be easiest if it just acted the same as if a script URL changed
9:36 AM <JakeA> wanderview: that works for me then

So we should indeed change this line:

https://dxr.mozilla.org/mozilla-central/source/dom/workers/ServiceWorkerRegisterJob.cpp#48

To something like:

  if (newest && mScriptSpec.Equals(newest->ScriptSpec()) && mUseCache == newest->mUseCache) {
    // short circuit
  }

Comment 32

[Ben Kelly [:bkelly, not reviewing]]

Comment on attachment 8814812 [details] [diff] [review]
(V2) Part 2.1: Save load flags in ServiceWorkerRegistration

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

::: dom/workers/ServiceWorkerRegistrarTypes.ipdlh
@@ +17,5 @@
>    nsString cacheName;
>  
>    PrincipalInfo principal;
> +
> +  uint32_t loadFlags;

This is just the first step here.  In order to persist the value to disk you need to modify ServiceWorkerRegistrar::WriteData():

https://dxr.mozilla.org/mozilla-central/source/dom/workers/ServiceWorkerRegistrar.cpp#636

And then ServiceWorkerRegistrar::ReadData() to get the value back off disk:

https://dxr.mozilla.org/mozilla-central/source/dom/workers/ServiceWorkerRegistrar.cpp#270

Note that the file on disk is versioned, etc.  You'll need to create a new version number for the schema including this additional field.

Comment 33

[Ben Kelly [:bkelly, not reviewing]]

Comment on attachment 8814814 [details] [diff] [review]
(V2) Part 2.3: Set the load flags when registering a service worker

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

::: dom/workers/ServiceWorkerRegisterJob.cpp
@@ +48,3 @@
>      registration->mPendingUninstall = false;
>      RefPtr<ServiceWorkerInfo> newest = registration->Newest();
>      if (newest && mScriptSpec.Equals(newest->ScriptSpec())) {

As I mentioned in comment 31, please make this check take into account useCache value.  I think its probably something like:

  if (newest && mScriptSpec.Equals(newest->ScriptSpec()) &&
      registration->LoadFlags() == mLoadFlags) {

::: dom/workers/ServiceWorkerRegisterJob.h
@@ +17,5 @@
>  // but then uses ServiceWorkerUpdateJob to implement the Update and Install
>  // spec algorithms.
>  class ServiceWorkerRegisterJob final : public ServiceWorkerUpdateJob
>  {
> +  nsLoadFlags mLoadFlags;

nit: This can be const.

Comment 34

[Ben Kelly [:bkelly, not reviewing]]

Comment on attachment 8814816 [details] [diff] [review]
(V2) Part 3.2: Pass LOAD_BYPASS_CACHE caused by script expiration timer to ServiceWorkerInfo

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

r=me with comments addressed

::: dom/workers/ServiceWorkerScriptCache.h
@@ +41,5 @@
>                     const nsAString& aNewCacheName,
>                     const nsACString& aMaxScope) = 0;
>  
> +  /*
> +   * Right before fecthing the main script from the network, we checks whether

s/fecthing/fetching/
s/checks/check/

::: dom/workers/ServiceWorkerUpdateJob.cpp
@@ +100,5 @@
>                     const nsAString& aNewCacheName,
>                     const nsACString& aMaxScope) override
>    {
> +    mJob->ComparisonResult(aStatus, aInCacheAndEqual, aNewCacheName, aMaxScope,
> +                           mJob->LoadFlags());

If its sets on the job in SaveLoadFlags(), why is it passed as an arg here?

@@ +333,5 @@
>  ServiceWorkerUpdateJob::ComparisonResult(nsresult aStatus,
>                                           bool aInCacheAndEqual,
>                                           const nsAString& aNewCacheName,
> +                                         const nsACString& aMaxScope,
> +                                         nsLoadFlags aLoadFlags)

Why does this need to be an argument?  Can you just use mLoadFlags in this method?

@@ +428,5 @@
>      new ServiceWorkerInfo(mRegistration->mPrincipal,
>                            mRegistration->mScope,
> +                          mScriptSpec,
> +                          aNewCacheName,
> +                          aLoadFlags);

Can't this be mLoadFlags?

::: dom/workers/ServiceWorkerUpdateJob.h
@@ +69,5 @@
>    Update();
>  
> +  nsLoadFlags&
> +  LoadFlags() {
> +    return mLoadFlags;

Please put the implementation in the .cpp file unless there is a measured perf impact requiring it to be in the header.

Also, please make it just return by value and use a separate SetLoadFlags() to change the value.  We only do this sort return-a-reference-for-assignment thing for classes like strings that need to do magical memory management things.

Comment 35

[Ben Kelly [:bkelly, not reviewing]]

Comment on attachment 8814817 [details] [diff] [review]
(V2) Part 3.3: Propagate load flags to ScriptLoader when loading scripts

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

Thanks.  This makes the changes to ScriptLoader much easier to read.  r=me with comments addressed.

Please make sure to add the extra patch I requested for moving the docshell load flags to use the new WorkerLoadInfo.mLoadFlags mechanism.

::: dom/workers/ScriptLoader.cpp
@@ +926,5 @@
>            nsresult rv = docShell->GetDefaultLoadFlags(&loadFlags);
>            NS_ENSURE_SUCCESS(rv, rv);
>          }
>        }
>      }

I think line 910 to 927 should be removed from here.  Instead we should set the WorkerLoadInfo.mLoadFlags in WorkerPrivate::GetLoadInfo().  For the nested worker case we can get the value from the parent worker.  For the main thread case we can get the value from the docshell like here.

I just think its important we have only one path for setting load flags in the script loader.

Can you do this as a separate patch in this bug?

@@ +935,5 @@
>        bool useDefaultEncoding = !(!parentWorker && IsMainWorkerScript());
>        rv = ChannelFromScriptURL(principal, baseURI, parentDoc, loadGroup, ios,
>                                  secMan, loadInfo.mURL, IsMainWorkerScript(),
>                                  mWorkerScriptType,
>                                  mWorkerPrivate->ContentPolicyType(), loadFlags,

Then you can get rid of the `loadFlags` variable here and just pass loadInfo.mLoadFlags.

::: dom/workers/WorkerPrivate.h
@@ +485,5 @@
> +      // try to intercept it.  If the interception matches the current
> +      // ServiceWorker's scope then we could deadlock the load.
> +      return nsIChannel::LOAD_BYPASS_SERVICE_WORKER |
> +             mLoadInfo.mServiceWorkerLoadFlags;
> +    }

Lets just move this logic into ServiceWorkerPrivate.cpp where it sets the value.  We can then just have an mLoadFlags value and do:

  nsLoadFlags
  GetLoadFlags() const
  {
    return mLoadFlags;
  }

::: dom/workers/Workers.h
@@ +255,5 @@
>    nsAutoPtr<mozilla::ipc::PrincipalInfo> mPrincipalInfo;
>    nsCString mDomain;
>  
>    nsString mServiceWorkerCacheName;
> +  nsLoadFlags mServiceWorkerLoadFlags;

I think this should just be called mLoadFlags.  We could in theory set load flags for other workers as well.  The fact that ServiceWorkerPrivate set the value in this case should not matter to the ScriptLoader.

Comment 36

[Ben Kelly [:bkelly, not reviewing]]

Comment on attachment 8814818 [details] [diff] [review]
(V2) Part 4: Update an existing testcase

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

r=me with comments addressed

::: testing/web-platform/tests/service-workers/service-worker/registration.https.html
@@ +368,5 @@
> +// NOTE: "Test with useCache: true" should be placed before the other two tests,
> +// "Test with useCache: false" and "Test with useCache: default", since we don't
> +// want the targeted scripts having any cached copy in http cache when testing
> +// with "Test with useCache: true", while we do want cached copies when testing
> +// with "Test with useCache: false" and "Test with useCache: default".

I think you can avoid these kinds of issues if you append a cache busting param to the script names in use.  Something like:

  var script = 'resources/update-max-aged-worker.py?usecache=true'

@@ +437,5 @@
> +        .then(() => navigator.serviceWorker.register(script, {scope: scope,
> +                                                              useCache: false}))
> +        .then(registration => swr = registration)
> +        .then(() => wait_for_update(t, swr))
> +        .then(worker => getMessageFromServiceWorker(worker, 'conditional'))

Can you change "conditional" to "revalidate"?  I think that would be make it a bit easier to understand since thats the terminology used in the network stack.

Comment 37

[Ben Kelly [:bkelly, not reviewing]]

FYI, the issue with double-loading scripts came up in the spec discussion.  My proposal to fix:

https://github.com/w3c/ServiceWorker/pull/1023#issuecomment-267043753

I can't remember, how did we address it here?

Comment 38

[Ben Hsu [:HoPang]]

IIRC, we didn't spend too much time on the double-loading scripts issue. Instead, we talked more on what to compare. For double-loading scripts, I fully agree that we should download every imported script only once during the comparison and use the script copy for later execution as your proposal.

Alone with what to compare, when to do the byte-check is unclear to me. Current we do the byte check before eval the main script, and it works great. However, it becomes a problem when extending the byte-check, IMHO, we should do the byte-check after the evaluation, and decide whether to send an install event to the newly created SW according to the result of the byte-check.

As to what to compare, I think it's about how to populate the "script resource map". IMHO, the "script resource map" of a SW should contain every script imported during every phase (eval, install, active). When doing the byte-check, once we find there is a single script different from the one in the "script resource map" of the newest SW, then there should be a new SW.

Comment 39

[Ben Kelly [:bkelly, not reviewing]]

Well, the "what to compare" is the list of things imported by the current service worker.  This is a known quantity when performing an update.  Its true that a new script may choose to import different dependencies, but that does not affect the byte-for-byte check.

I think pseudocode would look something like:

  let updateNeeded = false;
  let cache = await caches.open(sw-special-cache);
  let resources = await cache.keys();
  for (let request of resources) {
    let responses = await Promise.all([
      cache.match(request).text(),
      fetch(request).text()
    ]);
    if (responses[0] !== responses[1]) {
      updateNeeded = true;
      break;
    }
  }

Does that make sense?

Comment 40

[Ben Hsu [:HoPang]]

(In reply to Ben Kelly [:bkelly] from comment #39)
>   let updateNeeded = false;
>   let cache = await caches.open(sw-special-cache);
>   let resources = await cache.keys();
>   for (let request of resources) {
>     let responses = await Promise.all([
>       cache.match(request).text(),
>       fetch(request).text()
>     ]);
>     if (responses[0] !== responses[1]) {
>       updateNeeded = true;
>       break;
>     }
>   }

Given a script composed of main script(MAIN), imported script during evaluation(ISE), imported script during installation(ISI), and imported script during(ISA), say two scripts composed of exactly the same MAIN and ISE but ISI and ISA with different content, are we still considering that the two script are the same? If so, the pseudocode looks good to me.

(In reply to Ben Kelly [:bkelly] from comment #39)
> Its true that a new script may choose to import different dependencies,
> but that does not affect the byte-for-byte check.

I believe the difference between the dependencies fails the byte-check, and thus I totally agree with you that this doesn't affect anything.

Comment 41

[Ben Hsu [:HoPang]]

Typo: imported script during activation (ISA)

Comment 42

[Ben Kelly [:bkelly, not reviewing]]

I believe the update algorithm should only be performing the byte-for-byte check against installed service workers.  So the pseudocode definitely includes the "ISI" script.  I don't think we want to allow imports after the installation is complete, though.  So ISA should not be possible.

Comment 43

[Ben Hsu [:HoPang]]

Attachment: (V3) Part 2.1: Save load flags in ServiceWorkerRegistration

This patch updates both WriteData() and ReadData(), and the modification of the related gtest is in Patch 4.2.

Comment 44

[Ben Hsu [:HoPang]]

Attachment: (V3) Part 2.2: Load the main script with the load flags of the ServiceWorkerRegistration

Comment 45

[Ben Hsu [:HoPang]]

Attachment: (V3) Part 2.3: Set the load flags when registering a service worker

Comment 46

[Ben Hsu [:HoPang]]

Attachment: (V3) Part 3.1: Propogate load flags to ServiceWorkerInfo

Comment 47

[Ben Hsu [:HoPang]]

Attachment: (V3) Part 3.2: Pass LOAD_BYPASS_CACHE caused by script expiration timer to ServiceWorkerInfo

Comment 48

[Ben Hsu [:HoPang]]

Attachment: (V3) Part 3.3: Propagate load flags to ScriptLoader when loading scripts

Hi Ben, and sorry for your disappointment.

I'd like to centralize the generation of mLoadFlags in WorkerPrivate::GetLoadInfo(), but I cannot find a easy way doing this, since WorkerPrivateParent::GetWindow()[0] is designed to be accessed only on the main thread, which fails nested worker cases. I do have a quick fix by loosening the assertion and use raw pointers instead of nsCOMPtrs, but I don't think it's a good patch... Maybe having a bug for centralizing the setting of mLoadFlags is more appropriate?

[0] http://searchfox.org/mozilla-central/source/dom/workers/WorkerPrivate.h#630

Comment 49

[Ben Hsu [:HoPang]]

Attachment: (V3) Part 4.1: Update a related wpt testcase

New testcases for registering a SW with a different useCache are added.

Comment 50

[Ben Hsu [:HoPang]]

Attachment: Part 4.2: Update a related gtest

Comment 51

[Ben Kelly [:bkelly, not reviewing]]

(In reply to Ben Hsu [:HoPang] from comment #48)
> I'd like to centralize the generation of mLoadFlags in
> WorkerPrivate::GetLoadInfo(), but I cannot find a easy way doing this, since
> WorkerPrivateParent::GetWindow()[0] is designed to be accessed only on the
> main thread, which fails nested worker cases. I do have a quick fix by
> loosening the assertion and use raw pointers instead of nsCOMPtrs, but I
> don't think it's a good patch... Maybe having a bug for centralizing the
> setting of mLoadFlags is more appropriate?

Can you get the LoadFlags from the parent worker when not on the main thread?  We do that sort of thing in a lot of this code.  Get something from the window/document when on main thread or get it from the worker parent when on a worker thread.

Comment 52

[Ben Kelly [:bkelly, not reviewing]]

Comment on attachment 8820308 [details] [diff] [review]
(V3) Part 2.1: Save load flags in ServiceWorkerRegistration

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

r=me with comments addressed.

::: dom/workers/ServiceWorkerRegistrar.cpp
@@ +367,5 @@
>        CopyUTF8toUTF16(cacheName, entry->cacheName());
> +
> +      nsAutoCString loadFlags;
> +      GET_LINE(loadFlags);
> +      entry->loadFlags() = loadFlags.ToInteger(&rv, 16);

We should also validate that this value is either LOAD_NORMAL or VALIDATE_ALWAYS.  If its not then return NS_ERROR_INVALID_ARG.

@@ +398,5 @@
> +        return NS_ERROR_INVALID_ARG;
> +      }
> +      entry->currentWorkerHandlesFetch() =
> +        fetchFlag.EqualsLiteral(SERVICEWORKERREGISTRAR_TRUE);
> +

You must initialize entry->loadFlags() here and in the other legacy versions.  The IPC generated structure types do not provide a default value.

I think the right default here is probably VALIDATE_ALWAYS to reflect our new default behavior.

@@ +785,5 @@
>  
>      buffer.Append(NS_ConvertUTF16toUTF8(data[i].cacheName()));
>      buffer.Append('\n');
>  
> +    buffer.AppendInt(data[i].loadFlags(), 16);

Please add an assertion like:

MOZ_DIAGNOSTIC_ASSERT(data[i].loadFlags() == nsIRequest::LOAD_NORMAL ||
                      data[i].loadFlags() == nsIRequest::VALIDATE_ALWAYS);

Also, we should make it hard to accidentally change the value of these constants breaking our serialized format.  So add:

static_assert(nsIRequest::LOAD_NORMAL == 0, "LOAD_NORMAL matches serialized value.");
static_assert(nsIRequest::VALIDATE_ALWAYS == (1 << 11), "VALIDATE_ALWAYS matches serialized value");

Comment 53

[Ben Kelly [:bkelly, not reviewing]]

Comment on attachment 8820326 [details] [diff] [review]
Part 4.2: Update a related gtest

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

r=me with comments addressed

::: dom/workers/test/gtest/TestReadWrite.cpp
@@ +158,5 @@
>    buffer.Append("\n");
>    buffer.Append("scope 1\ncurrentWorkerURL 1\n");
>    buffer.Append(SERVICEWORKERREGISTRAR_FALSE "\n");
>    buffer.Append("cacheName 1\n");
> +  buffer.AppendInt(0xFF, 16);

Please use nsIRequest::VALIDATE_ALWAYS here.

@@ +489,5 @@
> +  ASSERT_STREQ("scope 0", data[0].scope().get());
> +  ASSERT_STREQ("currentWorkerURL 0", data[0].currentWorkerURL().get());
> +  ASSERT_TRUE(data[0].currentWorkerHandlesFetch());
> +  ASSERT_STREQ("cacheName 0", NS_ConvertUTF16toUTF8(data[0].cacheName()).get());
> +  ASSERT_EQ(nsIRequest::LOAD_NORMAL, data[0].loadFlags());

I think this is luck.  And I also think we should default to VALIDATE_ALWAYS since its our new default behavior.  We shouldn't grant the `useCache:true` behavior unless that is set on the registration.

Comment 54

[Ben Kelly [:bkelly, not reviewing]]

Comment on attachment 8820324 [details] [diff] [review]
(V3) Part 4.1: Update a related wpt testcase

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

::: testing/web-platform/tests/service-workers/service-worker/registration-useCache.https.html
@@ +163,5 @@
> +        .then(r => {
> +            var action = navigator.serviceWorker.register.bind(
> +                navigator.serviceWorker, script, {scope: scope,
> +                                                  useCache: true});
> +            return testActionDoesNotCreateSW(r.registration, action);

Can you update this test case to show that:
 
1) An update() action before the new registration *does* create a new SW.
2) An update() action after the new registration does *not* create a new SW.

Basically show that the useCache flag was really flipped in practice.

@@ +181,5 @@
> +        .then(r => {
> +            var action = navigator.serviceWorker.register.bind(
> +                navigator.serviceWorker, script, {scope: scope,
> +                                                  useCache: false});
> +            return testActionDoesCreateSW(r.registration, action, t, r.values);

And steps here to show:

1) An update() action before the new registration does *not* create a new SW.
2) An update() action after the new registration *does( create a new SW.

Comment 55

[Ben Kelly [:bkelly, not reviewing]]

Please file a follow-up bug to fix the double-download when we detect an update is needed.

Comment 56

[Ben Hsu [:HoPang]]

Attachment: (V4) Part 2.1: Save load flags in ServiceWorkerRegistration

Comment 57

[Ben Hsu [:HoPang]]

Attachment: (V4) Part 4.1: Update a related wpt testcase

Comment 58

[Ben Hsu [:HoPang]]

Attachment: (V2) Part 4.2: Update a related gtest

Comment 59

[Ben Hsu [:HoPang]]

(In reply to Ben Kelly [Food Coma / PTO / back Jan 3 :bkelly] from comment #55)
> Please file a follow-up bug to fix the double-download when we detect an
> update is needed.

Since the main scripts are now using the cached scripts used for byte-check, I think that we can take care of this issue in Bug 1290951 where we are going to extend the byte-check to imported scripts.

Comment 60

[Ben Hsu [:HoPang]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=48051cb0183198e99773e13bb07f2d77b1df217f

Comment 61

[Carsten Book [:Tomcat]]

needs rebasing

Comment 62

[Tom Tung [:tt, :ttung]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=683c9e119f73

Comment 63

[Ben Hsu [:HoPang]]

Hello Carsten,

I've tried the patches this week again, and they can merge without conflicts. The try result is in comment 62. Do you mind checking them in again?

Comment 64

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/79a87a518349
Part 1: Add useCache to RegistrationOptions(WebIDL). r=bkelly
https://hg.mozilla.org/integration/mozilla-inbound/rev/5942c0eb4063
Part 2.1: Save load flags in ServiceWorkerRegistration. r=bkelly
https://hg.mozilla.org/integration/mozilla-inbound/rev/ec97d27c2c21
Part 2.2: Load the main script with the load flags of the ServiceWorkerRegistration. r=bkelly
https://hg.mozilla.org/integration/mozilla-inbound/rev/ce2a3a922fdb
Part 2.3: Set the load flags when registering a service worker. r=bkelly
https://hg.mozilla.org/integration/mozilla-inbound/rev/0f2ad0e45944
Part 3.1: Propogate load flags to ServiceWorkerInfo. r=bkelly
https://hg.mozilla.org/integration/mozilla-inbound/rev/0b743f4d588d
Part 3.2: Pass LOAD_BYPASS_CACHE caused by script expiration timer to ServiceWorkerInfo. r=bkelly
https://hg.mozilla.org/integration/mozilla-inbound/rev/46dadd7d159e
Part 3.3: Propagate load flags to ScriptLoader when loading scripts. r=bkelly
https://hg.mozilla.org/integration/mozilla-inbound/rev/834e40db5d88
Part 4.1: Update a related wpt testcase. r=bkelly
https://hg.mozilla.org/integration/mozilla-inbound/rev/2cee79166dd0
Part 4.2: Update a related gtest. r=bkelly

Comment 65

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/79a87a518349
https://hg.mozilla.org/mozilla-central/rev/5942c0eb4063
https://hg.mozilla.org/mozilla-central/rev/ec97d27c2c21
https://hg.mozilla.org/mozilla-central/rev/ce2a3a922fdb
https://hg.mozilla.org/mozilla-central/rev/0f2ad0e45944
https://hg.mozilla.org/mozilla-central/rev/0b743f4d588d
https://hg.mozilla.org/mozilla-central/rev/46dadd7d159e
https://hg.mozilla.org/mozilla-central/rev/834e40db5d88
https://hg.mozilla.org/mozilla-central/rev/2cee79166dd0