Closed Bug 1181389 Opened 9 years ago Closed 8 years ago

check for full package update when doing service worker update

Categories

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

Product:
Core
Component:
DOM: Service Workers
Type:
defect

Tracking

()

Status:
RESOLVED WONTFIX
Milestone:
FxOS-S8 (02Oct)

People

(Reporter: pauljt, Assigned: dimi)

References

Details

In order to make service workers work with the package update logic we should couple package update with service worker update. When the ServiceWorker spec require the browser to check for updates of, or download updates of, the ServiceWorker script, we instead update the full signed package.

This means that both when we do an "automatic" ServiceWorker update check, such as when the user visit a page which uses the ServiceWorker, and when the ServiceWorkerRegistration.update() function is called, that we update the full package rather than just the ServiceWorker script.
I will take it first because I am working on NSec. Since I am a freshman of SW, please feel free to give me any advise/suggestion about how to handle this issue, thanks!
Assignee: nobody → dlee
Status: NEW → ASSIGNED
Do you have a rough idea how you plan to implement this?

For the record, I think some of us on the service worker team specifically said we didn't want a lot of special case logic for updating app packages in ServiceWorkerManager.

Is this still an app:// url or is it an https:// url now with some kind of extra signing thing?

What we discussed previously was SWM could set a load flag on the nsIChannel for the service worker script which the nsIChannel implementation could detect to load the entire app.

Are we still planning something like that?
Flags: needinfo?(ptheriault)
(In reply to Ben Kelly [:bkelly] from comment #2)
> Do you have a rough idea how you plan to implement this?

I'm looking for the advice of the SW team here. The text above comes straight from https://wiki.mozilla.org/FirefoxOS/New_security_model, I'm just breaking it into bugs so we can get an implementation plan.

> For the record, I think some of us on the service worker team specifically
> said we didn't want a lot of special case logic for updating app packages in
> ServiceWorkerManager.

I remember that discussion, happy to solve this 

> Is this still an app:// url or is it an https:// url now with some kind of
> extra signing thing?

https:// served packaged (via bug 1036275) which can also be signed. Signing is orthogonal though, and only required for heightened permissions.

> What we discussed previously was SWM could set a load flag on the nsIChannel
> for the service worker script which the nsIChannel implementation could
> detect to load the entire app.
> 
> Are we still planning something like that?

I'm not sure, I don't have an implementation plan here yet. We should look at what is currently implemented for http packages (bug 1036275). IIRC our whistler discussion, the point of this approach you suggest is be that the package loading is basically transparent to SWM?
Flags: needinfo?(ptheriault)
Flags: needinfo?(bkelly)
Paul, I think Ehsan said he discussed some of this with your team at Whistler.  Unfortunately I was not in those meetings (my fault).  He said there were some options floated that were less intrusive.

I think the best thing to do here is to get both teams in a vidyo room to hash out the plan.  Lets see if tonight's meeting works for everyone.

My main concern is that we're already struggling with the complexity of the service worker code.  I fear to add a lot of additional complexity in the same code for a custom firefox feature.  If we can do anything to separate the code here I think it will help us in the long run.
Flags: needinfo?(bkelly)
Looking at the wiki page you linked, it seems the URL for the SW script would look something like:

  https://website.com/RSSReader2000/package.pak!//sw.js

It seems to me the nsIChannel protocol handler must be knowledgeable enough to look inside the package for sw.js here.  I think this means we can also create a flag that the nsIChannel implementation can look at to check for an updated package before reading the script.

This sounds reasonable to me and like what I was hoping to see.  Sorry for my concern earlier.  I think having this filed on DOM::ServiceWorkers confused me since the bulk of the work will be in the protocol handler that deals with these package URL things.
Actually, I think this should just work with the code we have today.  The wiki says packages are "installed" using the http cache.  The ServiceWorker already bypasses the http cache when doing an update:

  https://dxr.mozilla.org/mozilla-central/source/dom/workers/ServiceWorkerScriptCache.cpp#134
Priority: -- → P2
Target Milestone: --- → FxOS-S8 (02Oct)
If SW is checking update for the following sw script:
 https://website.com/RSSReader2000/package.pak!//sw.js
SW will set LOAD_BYPASS_CACHE flag and necko will do the rest of things. What service worker will do is get the result and if sw script is different it will go through service worker's life cycle.

Since there is no different for necko while handling update of
https://website.com/RSSReader2000/package.pak!//sw.js and
https://website.com/RSSReader2000/package.pak!//index.html
I would assume if we have implemented the update logic of package app in necko(or other place ?), then update should just work fine without modifying service worker.

But we should definitely need to think about the logic of firing sw life cycle events(bug 1181390).

Hi Jonas, Ben, does this make sense to you?
Flags: needinfo?(jonas)
Flags: needinfo?(bkelly)
What needs to happen when we update the SW of a *pinned* package is:

1. Download *whole* new package. Since necko supports streaming packages, even once
   https://website.com/RSSReader2000/package.pak!//sw.js has been loaded, parts of the package might not
   yet have been loaded. So simply waiting for sw.js to download is not enough.
2. Wait until the user is no longer running the application.
3. Commit the new package to the cache. I.e. update the cache to contain the new version of the package.
4. Run the SW update steps.

I don't think that the existing code will cause all of this to happen automatically.
Flags: needinfo?(jonas)
(In reply to Jonas Sicking (:sicking) from comment #8)
> What needs to happen when we update the SW of a *pinned* package is:
> 
> 1. Download *whole* new package. Since necko supports streaming packages,
> even once
>    https://website.com/RSSReader2000/package.pak!//sw.js has been loaded,
> parts of the package might not
>    yet have been loaded. So simply waiting for sw.js to download is not
> enough.
> 2. Wait until the user is no longer running the application.
> 3. Commit the new package to the cache. I.e. update the cache to contain the
> new version of the package.
> 4. Run the SW update steps.
> 
> I don't think that the existing code will cause all of this to happen
> automatically.

Yes, Current code won't work.
As mentioned in Comment 8, there are 4 steps for SW update. Bug 1181390 is talking about step 2~4, and this one is about the first step(download package).

I am thinking that the first step may not have to modify service worker code.
Note, the SW spec changed how updates work a bit and we need to catch up.  See bug 1207727.

In particular, I believe this may mean we do not bypass the http cache at all here.  Instead I think the spec limits the max age of the sw script to 24 hours and just lets the http cache do its thing.

So the LOAD_BYPASS_CACHE flag may go away once bug 1207727 is implemented.

Dimi, any chance you want to take bug 1207727?  You could then think about how to integrate the package updates while fixing our normal SW updates.
Flags: needinfo?(bkelly)
(In reply to Ben Kelly [:bkelly] from comment #10)
> Note, the SW spec changed how updates work a bit and we need to catch up. 
> See bug 1207727.
> 
> In particular, I believe this may mean we do not bypass the http cache at
> all here.  Instead I think the spec limits the max age of the sw script to
> 24 hours and just lets the http cache do its thing.
> 
> So the LOAD_BYPASS_CACHE flag may go away once bug 1207727 is implemented.
> 
> Dimi, any chance you want to take bug 1207727?  You could then think about
> how to integrate the package updates while fixing our normal SW updates.

I would like to, but i will work on bug 1179064 first.
If no one is working on bug 1207727 when i finished, i will take it :)
(In reply to Ben Kelly [:bkelly] from comment #10)
> Note, the SW spec changed how updates work a bit and we need to catch up. 
> See bug 1207727.
> 
> In particular, I believe this may mean we do not bypass the http cache at
> all here.  Instead I think the spec limits the max age of the sw script to
> 24 hours and just lets the http cache do its thing.
> 
> So the LOAD_BYPASS_CACHE flag may go away once bug 1207727 is implemented.
> 
> Dimi, any chance you want to take bug 1207727?  You could then think about
> how to integrate the package updates while fixing our normal SW updates.

Hi Ben,
One question, just check the spec and there are four cases we will do the soft update:
1.[9.8 Handle Fetch]20.3.1
2.[9.8 Handle Fetch]21.2.1
3.[9.8 Handle Fetch]22.2.1
4.[9.9 Handle Functional Event]7 (24 hour timer)

And from SoftUpdate spec
https://slightlyoff.github.io/ServiceWorker/spec/service_worker/index.html#soft-update-algorithm
It still says "force bypass cache flag", so why do you mentioned that LOAD_BYPASS_CACHE flag may go away ?
Flags: needinfo?(bkelly)
Well, I don't see where it actually sets "force bypass cache flag" any more.  Nothing seems to set it as far as I can tell.
Flags: needinfo?(bkelly)
So, there is still a case where we would use LOAD_BYPASS_CACHE in the new update algorithm.  Step 10 says:

> 10. If newestWorker is not null and registration's last update check time is not null, then:
>   1. If the time difference in seconds calculated by the current time minus registration's
>      last update check time is greater than 86400, or force bypass cache flag is set, set r's
>      cache mode to "reload".
> Note: Even if the cache mode is not set to "reload", the user agents obey Cache-Control header's
>       max-age value in the network layer to determine if it should bypass the browser cache.

So when we are over the 24 hours (or the force bypass cache flag gets set somehow), then we would use LOAD_BYPASS_CACHE.

Otherwise, however, we do obey the Cache-Control max-age.  So the script can still be updated without LOAD_BYPASS_CACHE in that case.

How does Cache-Control max-age work with a pinned package?  Is the max-age of the whole package consulted?  If that is the case, then maybe this is all fine still.
(In reply to Ben Kelly [:bkelly] from comment #14)
> So, there is still a case where we would use LOAD_BYPASS_CACHE in the new
> update algorithm.  Step 10 says:
> 
> > 10. If newestWorker is not null and registration's last update check time is not null, then:
> >   1. If the time difference in seconds calculated by the current time minus registration's
> >      last update check time is greater than 86400, or force bypass cache flag is set, set r's
> >      cache mode to "reload".
> > Note: Even if the cache mode is not set to "reload", the user agents obey Cache-Control header's
> >       max-age value in the network layer to determine if it should bypass the browser cache.
> 
> So when we are over the 24 hours (or the force bypass cache flag gets set
> somehow), then we would use LOAD_BYPASS_CACHE.
> 
> Otherwise, however, we do obey the Cache-Control max-age.  So the script can
> still be updated without LOAD_BYPASS_CACHE in that case.
> 
> How does Cache-Control max-age work with a pinned package?  Is the max-age
> of the whole package consulted?  If that is the case, then maybe this is all
> fine still.
For my understanding is that we won't check the max-age of whole package all the time, only when a url inside the pinned package is visited and it is expired. Then whole package will be downloaded to a temporarily file. And until the previous package is no longer used(opened) by user, then the new one will replace the old one.

Hi paul, could you confirm this?
Flags: needinfo?(ptheriault)
Comment from the meeting: when we have a request which sets LOAD_BYPASS_CACHE flag, it must NOT bypass the pinned cache.
Flags: needinfo?(ptheriault)
...which might mean that we need to add some additional flag in addition to LOAD_BYPASS_CACHE. Or some such.
(In reply to Jonas Sicking (:sicking) from comment #17)
> ...which might mean that we need to add some additional flag in addition to
> LOAD_BYPASS_CACHE. Or some such.

If some does the equivalent of shift+reload, do you expect it to bypass the package?
I left it to honza to decide how shift-reload should behave. Check with him.
Honza, how are you planning to handle shift+reload for pinned packages?  Jonas has expressed a desire to not bypass the package when service worker uses LOAD_BYPASS_CACHE.  Does this match your current plans for that flag or do we need to come up with some new package-specific flag?
Flags: needinfo?(honzab.moz)
(In reply to Ben Kelly [:bkelly] from comment #20)
> Honza, how are you planning to handle shift+reload for pinned packages? 
> Jonas has expressed a desire to not bypass the package when service worker
> uses LOAD_BYPASS_CACHE.  Does this match your current plans for that flag or
> do we need to come up with some new package-specific flag?

Jonas agreed it'd be my call how Ctrl-F5'd behave.  

There is nothing special done at the moment.  It will bypass the cache on ctrl-F5 (means pinning is bypassed as well, since it's implemented inside the HTTP cache).  

If there is a strong desire not to, then file bugs.  It will need some special code to drop the cache flag somehow.  Not easy to do!  And IMO going against what users may expect.
Flags: needinfo?(honzab.moz)
I don't think we should change shift-F5 behaviour. Instead we should set some other flag here to indicate that we just want to circumvent the normal cache, not pinned resources.
Right.  I was just trying to find out if these two cases would be aligned or not.  I guess they are not and we need a different thing.
Depends on: 1207727
The new security model project was suspended.
We had discussed the idea of NSEC v2, which seems we are unlikely to use signed package format.
So there is no reason to keep working on this bug.

p.s. We don't have plan on NSEC v2 either.
Status: ASSIGNED → RESOLVED
Closed: 8 years ago
Resolution: --- → WONTFIX
You need to log in before you can comment on or make changes to this bug.