Bug 180816 – Service worker script fetching currently always uses the network cache

• Status: RESOLVED
• Resolution: FIXED
• Component: Service Workers

[Chris Dumez]

Service worker script fetching currently always uses the network cache. This is incorrect as per:
- https://w3c.github.io/ServiceWorker/#update-algorithm (step 7.2)

[Chris Dumez]

Created attachment 329358 [details]
WIP Patch

Working on writing a test.

[Chris Dumez]

Created attachment 329365 [details]
Patch

[Alex Christensen]

Comment on attachment 329365 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=329365&action=review

> Source/WebCore/workers/service/server/SWServerJobQueue.cpp:315
> +    //   current time minus registration's last update check time is greater than 86400.

Why 86400?  Why do we want to force the cache to refresh every day?  Can't the server say how long it stays valid?

[Alex Christensen]

Comment on attachment 329365 [details]
Patch

As much as I think that's a bad idea, it's what the spec says.

[youenn fablet]

Comment on attachment 329365 [details]
Patch

I wonder whether cachePolicy could be a parameter of ServiceWorkerJobData.
That might help not passing this parameter explicitly everywhere.

Also, the test would be a good candidate for WPT upload. Would need to be testharness.js based and use WPT server.

[Chris Dumez]

(In reply to youenn fablet from comment #5)
> Comment on attachment 329365 [details]
> Patch
> 
> I wonder whether cachePolicy could be a parameter of ServiceWorkerJobData.
> That might help not passing this parameter explicitly everywhere.

The job is passed from the WebProcess to the StorageProcess. Then when we trigger the script fetch, we only pass the job identifier from the StorageProcess to the WebProcess and the WebProcess looks up the job in its own map.

Wether or not we want to bypass the cache is determined by the StorageProcess so it cannot be passed via the Job.

> 
> Also, the test would be a good candidate for WPT upload. Would need to be
> testharness.js based and use WPT server.

[Chris Dumez]

Comment on attachment 329365 [details]
Patch

Clearing flags on attachment: 329365

Committed r225916: <https://trac.webkit.org/changeset/225916>

[Chris Dumez]

All reviewed patches have been landed.  Closing bug.

[Radar WebKit Bug Importer]

<rdar://problem/36054182>

[Geoffrey Garen]

> As much as I think that's a bad idea, it's what the spec says.

I think we should file a PR to remove this policy from the spec. It is stupid.

[Chris Dumez]

(In reply to Geoffrey Garen from comment #10)
> > As much as I think that's a bad idea, it's what the spec says.
> 
> I think we should file a PR to remove this policy from the spec. It is
> stupid.

What exactly do you guys object to? You think updating a service worker should always consult the disk cache?

The way it is currently specified, it lets the developer decide what the behavior should be.

[youenn fablet]

Maybe the issue is the 84600 seconds rule.

Note that it might not have a terrific impact since it will in most cases end up in a conditional request. Nothing too harmful here. Maybe this overriding of HTTP cache headers is a security counter measure, it would be good to know the rationale.

The spec does not say a word about when a user agent is expected to stop a service worker. It may also update it based on its own rules. By having this simple rule, one benefit is some consistency in what web developers can expect across all browsers.

If we are offline though, the update will fail and IIUC, we might retry everytime until we succeed to have a response. If so, maybe there is some room for optimization.

In the same vein, in many cases, we might get the same script resource.
Might not be worth doing the full update thing in that case.

[Geoffrey Garen]

> What exactly do you guys object to? You think updating a service worker
> should always consult the disk cache?
> 
> The way it is currently specified, it lets the developer decide what the
> behavior should be.

I'm objecting to the needless inefficiency of reloading each service worker every day, even if there's a valid unexpired cache entry.

In what sense does the developer decide? Can the developer specify not to perform the daily reload?

[Chris Dumez]

(In reply to Geoffrey Garen from comment #13)
> > What exactly do you guys object to? You think updating a service worker
> > should always consult the disk cache?
> > 
> > The way it is currently specified, it lets the developer decide what the
> > behavior should be.
> 
> I'm objecting to the needless inefficiency of reloading each service worker
> every day, even if there's a valid unexpired cache entry.
> 
> In what sense does the developer decide? Can the developer specify not to
> perform the daily reload?

Ok. The 24-hour force reload is indeed not something the developer can control. As I said, Alex and your comment were so vague, I had no idea what your were objecting to.

I would agree that the 24-hour force reload is excessive, especially in combination with soft update (which we do not implement yet). Without soft update (which happens when a load is intercepted by a service worker), we only fetch the script when:
1. The page registers a service worker
2. The page calls update() explicitly on its service worker

Therefore, without soft update, this happens rarely.

[Chris Dumez]

(In reply to Chris Dumez from comment #14)
> (In reply to Geoffrey Garen from comment #13)
> > > What exactly do you guys object to? You think updating a service worker
> > > should always consult the disk cache?
> > > 
> > > The way it is currently specified, it lets the developer decide what the
> > > behavior should be.
> > 
> > I'm objecting to the needless inefficiency of reloading each service worker
> > every day, even if there's a valid unexpired cache entry.
> > 
> > In what sense does the developer decide? Can the developer specify not to
> > perform the daily reload?
> 
> Ok. The 24-hour force reload is indeed not something the developer can
> control. As I said, Alex and your comment were so vague, I had no idea what
> your were objecting to.
> 
> I would agree that the 24-hour force reload is excessive, especially in
> combination with soft update (which we do not implement yet). Without soft
> update (which happens when a load is intercepted by a service worker), we
> only fetch the script when:
> 1. The page registers a service worker
> 2. The page calls update() explicitly on its service worker
> 
> Therefore, without soft update, this happens rarely.

Soft update on handle fetch is specified at:
- https://w3c.github.io/ServiceWorker/#ref-for-soft-update①

This is the part that I think is too aggressive. But we do not implement this yet.

Without soft update, update only happens when the page's script explicitly asks us to update. Therefore, I think it is fine to go to the network in such case IMHO.

[youenn fablet]

Note also that this might be a protection against buggy service worker scripts.
It is indeed much more difficult to update a buggy service worker than a buggy web page.

[Geoffrey Garen]

> Without soft update, update only happens when the page's script explicitly
> asks us to update. Therefore, I think it is fine to go to the network in
> such case IMHO.

Yes, we agree: An explicit registration or call to update() is a fine time to reload from origin if 24 hours have passed; but soft update goes too far by mandating reload for each service worker every 24 hours.

[youenn fablet]

(In reply to Geoffrey Garen from comment #17)
> > Without soft update, update only happens when the page's script explicitly
> > asks us to update. Therefore, I think it is fine to go to the network in
> > such case IMHO.
> 
> Yes, we agree: An explicit registration or call to update() is a fine time
> to reload from origin if 24 hours have passed; but soft update goes too far
> by mandating reload for each service worker every 24 hours.

Some links:
- https://github.com/w3c/ServiceWorker/issues/514
- https://github.com/w3c/ServiceWorker/issues/715

Other browsers seem to implement this rule.

Buggy/malicious service workers blocking any kind of update seems to be one of the main reason for the current policy.
This policy allows preventing web developers from making errors that they could not easily fix.

I do not see any easy solution other than doing some regular pinging to the server or hoping that web devs will not fall into that very easy trap.
Do we have a better plan here?

The pinging modality could be optimized though, like updating all service worker at the same time for instance.

[Geoffrey Garen]

> Buggy/malicious service workers blocking any kind of update seems to be one
> of the main reason for the current policy.
> This policy allows preventing web developers from making errors that they
> could not easily fix.

I don't see how this situation is different from any other case of serving a bad main resource with a long cache expiry date. Yes, those bugs happen sometimes. But they have not brought down the internet, and there is nothing unique about ServiceWorkers here.

After an author has done appropriate user testing and deployed some software, caching for more than a day should be possible. Developers who only want to cache for a day can send a cache expiry value of 24 hours.

[youenn fablet]

> I don't see how this situation is different from any other case of serving a
> bad main resource with a long cache expiry date. Yes, those bugs happen
> sometimes. But they have not brought down the internet, and there is nothing
> unique about ServiceWorkers here.

I guess one difference is the amplitude of the risk.

If you screw things up on one main resource, only this main resource will be affected.
Chances are users will anyway navigate to some other resources and scripts can just bust the cache with fetch+"no-cache".

If you screw things up with the SW script, all web site main resources might be screwed.
There might be no possibility to inject new code to fix that bug.
One might need to rely on user cleaning the cache or doing some hard reload.