751754 - Allow separation between the update-available and start-download states in appcache

Status: RESOLVED FIXED

(Core :: Networking: Cache, defect, P2)

Description

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

The use case here is the following:
 - an app manager (or app) has checked for an update
 - there's an update available
 - however, the phone is on a billed pipe (e.g. 3g)
 - the user prefers to defer the download until on wifi

Another use case here is allowing "user agents" to show UI for users to manually apply updates, instead of them being automatically applied.  This matches android's default behavior.

We need new API here.  Might fit in with bug 751750 and bug 751751.

Comment 1

[Dietrich Ayala (:dietrich)]

Required for impl of current UX flows, per cjones.

Comment 2

[Dietrich Ayala (:dietrich)]

Andrew why'd you switch back to nom?

Jason, can you explain what input from Josh is actually needed? He's not even cc'd on the bug (until now).

Comment 3

[Josh Carpenter [:jcarpenter]]

I think I know what's needed here, and will follow up (hopefully tonight) with App update flows that depend on this bug. This is a P1 blocker, IMO. We're launching into a market where bandwidth is limited, expensive, and slow. We can't just automatically begin downloads every time an update is detected or users will be rightfully enraged. 

cc clee for input here.

Comment 4

[Andreas Gal :gal]

I don't think I agree with comment 3. If you go to amazon.com and parts of that page are offline cached, you don't get the option to use the old version for an extended period. I think this is an excellent feature to drop. Note that all packed apps are not affected by this. Only appcache-d apps which are more web-y.

Comment 5

[Andreas Gal :gal]

Also, I think it would be a spec violation to defer this update. We can change the spec, but if we decide to take on this feature, we should actually also do that.

Comment 6

[Andrew Overholt [:overholt]]

I'm not sure why I took off basecamp-blocker+.  It could have been a mid-air collision or something.  Re-setting to +.  If anyone disagrees, please reset it to ?.

Comment 7

[Andrew Overholt [:overholt]]

Jason, is this something you can do?  If not, can you think of a better owner?  Thanks.

Comment 8

[Jason Duell]

Honza should be back any day now and knows this code much better than me.

Comment 9

[Chris Lee [:clee]]

Josh, comment 4 I do understand (and mostly agree with), but I believe impacts your current spec.

Can you provide an update here?

Comment 10

[Josh Carpenter [:jcarpenter]]

Re: comment 4:

> I don't think I agree with comment 3. If you go to amazon.com and parts of that page are offline cached, you don't get the option to use the old version for an extended period. I think this is an excellent feature to drop. Note that all packed apps are not affected by this. Only appcache-d apps which are more web-y.

Perhaps I need to learn more about App Cache. My understanding has been that a 1k change to a 100 MB app cache will require re-downloading the whole cache. Tim clarified for me over IRC that delta updates are possible, but require best practices on part of third party devs and HTTP servers. "...the learning curve to master optimizing app cache is hard." So I infer that not many people are going to do The Right Thing.

In which case, user opens HorsePinball 2, and immediately the app begins to pull down a 6mb unoptimized full app cache refresh. Over their Brazilian data connection.

I believe we need to give the user _some_ degree of control here.

If we cannot do what I'd proposed in my specs (defer download, allow use of current version in interim, and only update once connection condition is met or user manually initiates), then perhaps the following:

* User opens app. 
* App automatically checks for update (Please confirm? Do we know the approx bandwidth transaction size?).
* App finds available update.
* App prompts user: "Update available. HorsePinball 3, 6.5 MB. Download: Later | Now." With option for "Download over WiFi only."
* If user selects "Now", app goes into "Downloading" state (as per new Install specs, here: https://www.dropbox.com/sh/b0kyykhzcfkpm8b/ReFTX_E72X). It cannot be used until update is complete (I presume).
* If user selects "Later", app goes into "Not yet downloaded" state. App downloads once:
** Manually: User manually initiates (taps on icon)
** Automatically: WiFi connection is established (if user selected "Download over WiFi only")
** Automatically: User is reminded at scheduled interval, eg: once daily (if user did not select "Download over WiFi only").

Essentially we allow the user to defer download, but lock them out of the app in the interim.

Comment 11

[Jason Duell]

Honza hoping for LOE:S but hasn't looked into it enough yet to be sure.

Comment 12

[Honza Bambas (:mayhemer)]

(In reply to Josh Carpenter [:jcarpenter] from comment #10)
> Re: comment 4:
> 
> > I don't think I agree with comment 3. If you go to amazon.com and parts of that page are offline cached, you don't get the option to use the old version for an extended period. I think this is an excellent feature to drop. Note that all packed apps are not affected by this. Only appcache-d apps which are more web-y.
> 
> Perhaps I need to learn more about App Cache. My understanding has been that
> a 1k change to a 100 MB app cache will require re-downloading the whole
> cache. Tim clarified for me over IRC that delta updates are possible, but
> require best practices on part of third party devs and HTTP servers. "...the
> learning curve to master optimizing app cache is hard." So I infer that not
> many people are going to do The Right Thing.

As you say, it depends on how well the server is configured.  To shortly sync on knowledge how the update works:
- we download the manifest
- we check whether its content is different from the manifest we have now cached
- if so, we immediately start the Update Process
- Update Process means:
  - download all resources listed in the new manifest
  - each http request for each resource is using the current version of the app cache as HTTP cache to satisfy cache requests
  - so, you may (and probably usually get) 304 Not Modified response to most of resource requests, thus, we may reuse the content w/o downloading it again


> 
> In which case, user opens HorsePinball 2, and immediately the app begins to
> pull down a 6mb unoptimized full app cache refresh. Over their Brazilian
> data connection.
> 
> I believe we need to give the user _some_ degree of control here.

Depends...  We want to start using the a version of a cache ASAP, also because of potential security issues with the current version that the new version is fixing.

> 
> If we cannot do what I'd proposed in my specs (defer download, allow use of
> current version in interim, and only update once connection condition is met
> or user manually initiates), then perhaps the following:
> 
> * User opens app. 
> * App automatically checks for update (Please confirm? Do we know the approx
> bandwidth transaction size?).

A note: it is hard to estimate the size prior to download, but there is a bug that tries to solve it (bug 751758)

> * App finds available update.
> * App prompts user: "Update available. HorsePinball 3, 6.5 MB. Download:
> Later | Now." With option for "Download over WiFi only."
> * If user selects "Now", app goes into "Downloading" state (as per new
> Install specs, here: https://www.dropbox.com/sh/b0kyykhzcfkpm8b/ReFTX_E72X).
> It cannot be used until update is complete (I presume).

You presume well.

> * If user selects "Later", app goes into "Not yet downloaded" state. App
> downloads once:
> ** Manually: User manually initiates (taps on icon)
> ** Automatically: WiFi connection is established (if user selected "Download
> over WiFi only")
> ** Automatically: User is reminded at scheduled interval, eg: once daily (if
> user did not select "Download over WiFi only").
> 
> Essentially we allow the user to defer download, but lock them out of the
> app in the interim.

So, as I understand, under some global conditions (like the connection type) you want to let the user decide whether to actually download or not, right?


Based on that, the plan for a patch would be:
- I need some service that tells me we are under conditions to don't allow the download w/o a consent
- when the condition is met, the update process is stopped before starting the all-resource download
- instead, a new event will be fired (should probably be a content one)
- state of the cache will not change, just the ui has to remember the prompt has already been shown in context of, say, single session
- when user clicks the [Download now] button, then we schedule the update with some "force" flag using the offline cache update service

Comment 13

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Actually, I don't think that we should disable appcaches from updating while running the app. That's something that we should look into for later, but I suspect that such a big change might break apps in important scenarios right now.

I think for now we should focus on making it possible to implement the update API in bug 790558 for apps backed by an appcache. All we *really* need for that is:

* Ability to check if a new version is available without automatically downloading
  the new version if one is found.
* Ability to download the new version.

I take it we already have the second capability. I.e. at any point we can ask the appcache code to update a given manifest within a given appid, right?

So all we really need is the former. I.e. the ability to ask the appcache code if there's a new manifest available without also starting to automatically download that new version. Note that it's likely ok if we just check for a new manifest. Even if that technically can result in false positives where a new manifest is available, but none of the resources actually has changed.

I don't think anything else needs to happen in this bug.


What would also be *nice* to have, but definitely not a requirement, is if we can get notified whenever the appcache checks for an update and discovering that a new version is not available. That way we can update the "lastUpdateCheck" date. But that's not a blocker and should probably be done separately.

Also in the nice-to-have category is getting notifications whenever we start to download an update, and when we're done downloading that update. But I think that might already exist? With that we could fire appropriate events when we start to download a new version. I don't know if those notifications exist which provide the appid for which we're downloading data.

These two things would likely go into separate bugs.

Comment 14

[Honza Bambas (:mayhemer)]

(In reply to Jonas Sicking (:sicking) from comment #13)
> * Ability to check if a new version is available without automatically
> downloading
>   the new version if one is found.

That should be relatively easy, just expose a new API like nsIOfflineCacheService.checkForUpdate(manifestURL, callback).  Instead of a callback we could use observer service as well.  Depends on what is the most convenient way for you.  Just let me know.

> * Ability to download the new version.

Use the current scheduleUpdate API.  The same thing you use to start the first update when user accepts clicks the allow offline caching button.

> 
> I take it we already have the second capability. I.e. at any point we can
> ask the appcache code to update a given manifest within a given appid, right?

Yeah, use that API and provide a window belonging to the app.  If you don't have a window, then we need some new API to provide the appid and inbrowser flag.

> 
> So all we really need is the former. I.e. the ability to ask the appcache
> code if there's a new manifest available without also starting to
> automatically download that new version. Note that it's likely ok if we just
> check for a new manifest. Even if that technically can result in false
> positives where a new manifest is available, but none of the resources
> actually has changed.

Not sure what you mean by false positives.  I don't expect this to happen often in production world.

> 
> I don't think anything else needs to happen in this bug.
> 
> 
> What would also be *nice* to have, but definitely not a requirement, is if
> we can get notified whenever the appcache checks for an update and
> discovering that a new version is not available. That way we can update the
> "lastUpdateCheck" date. But that's not a blocker and should probably be done
> separately.

Simple to do.  We can just add a new content event to window.applicationCache if that fits your needs.

> 
> Also in the nice-to-have category is getting notifications whenever we start
> to download an update, and when we're done downloading that update. But I
> think that might already exist? With that we could fire appropriate events
> when we start to download a new version. I don't know if those notifications
> exist which provide the appid for which we're downloading data.

It is already there.  We have window.applicationCache.ondownloading and oncached.

> 
> These two things would likely go into separate bugs.

Comment 15

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

(In reply to Honza Bambas (:mayhemer) from comment #14)
> (In reply to Jonas Sicking (:sicking) from comment #13)
> > * Ability to check if a new version is available without automatically
> > downloading
> >   the new version if one is found.
> 
> That should be relatively easy, just expose a new API like
> nsIOfflineCacheService.checkForUpdate(manifestURL, callback).  Instead of a
> callback we could use observer service as well.  Depends on what is the most
> convenient way for you.  Just let me know.

A callback would definitely be the best. But we need to pass in the AppID and the browserflag as well.

> > * Ability to download the new version.
> 
> Use the current scheduleUpdate API.  The same thing you use to start the
> first update when user accepts clicks the allow offline caching button.
> 
> > 
> > I take it we already have the second capability. I.e. at any point we can
> > ask the appcache code to update a given manifest within a given appid, right?
> 
> Yeah, use that API and provide a window belonging to the app.  If you don't
> have a window, then we need some new API to provide the appid and inbrowser
> flag.

For none of this we have a window. So we need the ability to pass in manifesturl+appid+browserflag.

> > So all we really need is the former. I.e. the ability to ask the appcache
> > code if there's a new manifest available without also starting to
> > automatically download that new version. Note that it's likely ok if we just
> > check for a new manifest. Even if that technically can result in false
> > positives where a new manifest is available, but none of the resources
> > actually has changed.
> 
> Not sure what you mean by false positives.  I don't expect this to happen
> often in production world.

Someone might update the manifest file itself, but not add, remove or change any resources. So when we just look at the manifest to see if there's an update available, it'll look like we need to update and so we'll return "update available" to various APIs and UI.

But when we go to update no new data ends up actually being downloaded. That's what I meant by "false positive".

Like you, I don't expect this to happen often in production world. So I'm not worried about it. I don't think it's something we should try to address for now.

> > I don't think anything else needs to happen in this bug.
> > 
> > What would also be *nice* to have, but definitely not a requirement, is if
> > we can get notified whenever the appcache checks for an update and
> > discovering that a new version is not available. That way we can update the
> > "lastUpdateCheck" date. But that's not a blocker and should probably be done
> > separately.
> 
> Simple to do.  We can just add a new content event to
> window.applicationCache if that fits your needs.
> 
> > 
> > Also in the nice-to-have category is getting notifications whenever we start
> > to download an update, and when we're done downloading that update. But I
> > think that might already exist? With that we could fire appropriate events
> > when we start to download a new version. I don't know if those notifications
> > exist which provide the appid for which we're downloading data.
> 
> It is already there.  We have window.applicationCache.ondownloading and
> oncached.

I'd have to look if these events are enough. But I'd say lets worry about that in separate bugs once the basic stuff has been done.

Comment 16

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

So to sum up, the work that I think we should do in this bug is:

Add two functions like:
nsIOfflineCacheUpdateService.checkForUpdate(manifestURL, appid, inbrowser, callback)

This would use the callback to indicate if an update is available, but would never cause an update to actually start.

nsIOfflineCacheUpdateService.scheduleUpdate(manifestURL, appid, inbrowser)

This is similar to the existing scheduleUpdate, but doesn't require passing in a window object.

Potentially scheduleUpdate should also take a documentURL for consistency with the existing scheduleUpdate? Should checkForUpdate as well?

Comment 17

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Honza: If you need help on this bug, please let us know.

Comment 18

[Honza Bambas (:mayhemer)]

Jonas, I started the work already.  I have written the code, now making it to build.

One important info: with API like scheduleUpdate(manifestURL, appid, inbrowser) the update cannot be invoked on the child process.  To make it available, the containing window (tab) must be provided.  See code at and bellow [1].

The question here is, do you have window to provide (also with the appid and inbrowser available in its load context) ?

[1] http://hg.mozilla.org/mozilla-central/annotate/a1e7318fabc5/uriloader/prefetch/OfflineCacheUpdateChild.cpp#l365

Comment 19

[Honza Bambas (:mayhemer)]

Attachment: v1

- so far, introduced only nsIOfflineCacheUpdateService.checkForUpdate
- needed small tweak in nsHttpChannel: we didn't respect INHIBIT_CACHING for offline cache
- during the update I'm using app cache for write identical to app cache for read since, that is the only way to actually reach the server w/o large and dangerous modifications to nsHttpChannel cache opening and checking
- has a test

Comment 20

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Sweet!

Honza: I realized that we really need the
nsIOfflineCacheUpdateService.scheduleUpdate(manifestURL, appid, inbrowser)

function. Right now I think we end up using the wrong appid/browserflag when doing the original download for B2G apps. This is because we don't have a window with the appropriate appid/browserflag when we are doing the original app download. This didn't matter before bug 756717 was fixed, but will matter now that it is.

Do you want me to file a separate bug on this?

Comment 21

[Honza Bambas (:mayhemer)]

(In reply to Jonas Sicking (:sicking) from comment #20)
> Do you want me to file a separate bug on this?

Ok, I would definitely want this in a separate patch, so a whole bug is even better ;)  Yes please.  It should be blocked by this one, since some of the code changes will be needed by both parts.

Comment 22

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Sorry, I had missed the below comment.

(In reply to Honza Bambas (:mayhemer) from comment #18)
> Jonas, I started the work already.  I have written the code, now making it
> to build.
> 
> One important info: with API like scheduleUpdate(manifestURL, appid,
> inbrowser) the update cannot be invoked on the child process. To make it
> available, the containing window (tab) must be provided.  See code at and
> bellow [1].
> 
> The question here is, do you have window to provide (also with the appid and
> inbrowser available in its load context) ?

There are two ways that we interact with the appcache:

The implementation of the install/update API is what needs these new APIs. This code is actually already only calling the appcache code in the parent process. In this scenario we never have a window. This is the only case where we need this new API.


The other case is when we're actually loading an app which has been cached by the appcache. In this case the app is running in a child process. This case uses the "normal" flow so here we do have a window. And here we do just want to follow the normal update mechanisms where we check for an update when the app is loaded and automatically download it if an update is available. No changes needed here, and we have no need to call these new APIs here.


Is that ok?

Comment 23

[Honza Bambas (:mayhemer)]

(In reply to Jonas Sicking (:sicking) from comment #22)
> Is that ok?

OK.  In that case I think I can make the patch a bit simpler.  There is no need to support this on the client side then.  It will be just instant NOT_IMPLEMENTED.

Comment 24

[Andrew Overholt [:overholt]]

This should probably never have had the feature keyword.  Sorry for the noise.

Comment 25

[Jason Duell]

Comment on attachment 665002 [details] [diff] [review]
v1

my reading of comment 23 is "don't review this yet" :)

Comment 26

[Honza Bambas (:mayhemer)]

Attachment: v1.1

The same as v1 except:
- removed e10s support completely (huge simplification)
- filled comments in IDLs
- some tiny tweaks with white spaces and so

Comment 27

[Honza Bambas (:mayhemer)]

Attachment: v1.1 merged after bug 786299 landing

Jason, please check this patch as well, there might be some issue introduced during merge, but local tests are passing well.

Comment 28

[Honza Bambas (:mayhemer)]

There is one issue in v1.1 of the patch: FindUpdate also includes "Check- Update-Only" updates and coalesces with them.  Such updates have to be ignored in FindUpdate.

Comment 29

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Jason: review ping?

Comment 30

[Jason Duell]

Attachment: changes to fix patch v1.1-merged -> v1.2

Needed a couple lines changed to compile.

Comment 31

[Jason Duell]

Attachment: v1.2

Comment 32

[Jason Duell]

Comment on attachment 674109 [details] [diff] [review]
v1.2

Ah, meh, not so fast.  I just noticed that while the mochitest succeeds with 4 tests, when you look at the command line output you see

TEST-PASS | unknown test url | No update avail
TEST-PASS | unknown test url | Update avail (1)
TEST-PASS | unknown test url | Update avail (2)
TEST-PASS | unknown test url | No update avail (2)
TEST-UNEXPECTED-FAIL | unknown test url | No update avail (2) - got offline-cache-update-available, expected offline-cache-update-unavailable

I.e it looks like the updateCheck observer is getting called multiple times, but since we teardown the test in the first Observe call, mochitest isn't catching it.

I stuck in a counter that only calls teardown after 100 notifications, and we get 100 notifications.  We don't actually get the failure, though, so perhaps that's a shutdown race or something.

Comment 33

[Jason Duell]

Looks like this was just a mistaken infinite loop in the test code.  +r to an updated patch that contains this provided honza agrees with me that this is indeed the fix (it works for me and makes sense given a debug stack trace):

 function manifestUpdated()
 {
+  // Avoid loop :)
+  applicationCache.onupdateready = null;
+

Comment 34

[Honza Bambas (:mayhemer)]

Jason, thanks for your reviews and comments.  The unexpected log worries me about something a bit more serious.  We shouldn't get this notification at all and also not offline-cache-update-AVAILABLE (where from would it come?).

I'll take a look.

Comment 35

[Honza Bambas (:mayhemer)]

Attachment: v1.3

The problem really was something more serious then just a test recursion.  "only update check" updates must not notify any app cache state changes.  These state change notifications (oncached and onupdateready) happen in the applicationCache JS object implementation.

To successfully let applicationCache (nsDOMOfflineResourceList) bypass notification for "only update check" updates, such updates have to claim they are partial.  

Partial updates are opposite of "full" updates and must not produce any notifications.  They are only configured for dynamic items or document updates.  "only update check" updates fall to this category as well.


Interdiff (one line) for actual review is coming.

Comment 36

[Honza Bambas (:mayhemer)]

Attachment: v1.2 -> v1.3 idiff

- changing the partial attribute implementation to also return true for update only checks

Comment 37

[Honza Bambas (:mayhemer)]

And forgot to note: about the redundant test *failure* - that is quit strange!  I locally reproduced that just ones, but unfortunately my NSPR logging was not setup correctly at that moment.  That is sad, I really would like to catch this, since there is probably something rotten inside here...  I don't even have enough info to report a bug on it right now.

Comment 38

[Jason Duell]

Comment on attachment 674705 [details] [diff] [review]
v1.3

> about the redundant test *failure* - that is quite strange!

I can reproduce it quite easily with the 1.2 patch.  I assume it's due to something that happens as the browser is getting shut down (from the test's  OfflineTest.teardown/finish calls).  I can get an NSPR log and/or run the debugger on it if you want more info on what's going on.  Let me know.

Comment 39

[Honza Bambas (:mayhemer)]

(In reply to Jason Duell (:jduell) from comment #38)
> I can get an NSPR log and/or run the
> debugger on it if you want more info on what's going on.  Let me know.

Oh, yes please!  Include nsOfflineCacheUpdate:5 please.

Comment 40

[Honza Bambas (:mayhemer)]

Comment on attachment 674705 [details] [diff] [review]
v1.3

https://hg.mozilla.org/integration/mozilla-inbound/rev/17a6bcffcbeb

Comment 41

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/17a6bcffcbeb

Comment 42

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Could someone elaborate on hoe risky this would be to take on mozilla-aurora?

Comment 43

[Honza Bambas (:mayhemer)]

(In reply to Jonas Sicking (:sicking) from comment #42)
> Could someone elaborate on hoe risky this would be to take on mozilla-aurora?

If there were something broken on m-c we would know soon, let it just few days bake.  Since the patch only actually adds a new API, I think it is safe to uplift.

Comment 44

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/862bc295a7bb