Build: B2G 18 12/26/2012 Device: Unagi Steps: 1. Install a hosted app that preloads a small appcache 2. Update the appcache on the server to have different appcache contents 3. Either launch the app or try to manually sync Expected: An update available notification should fire that should indicate updates are available. If I click that notification, I should be able to apply an update to the appcache on my device. Actual: No update notification ever gets fired. From an end-user perspective, I basically do not have a clue if the update was available or was applied as a result of this bug. Nominating to block - hosted apps preloading the appcache (preloaded or non-preloaded) are fully dependent on this feature working.

Blocking nom - original bug for implementing this functionality was a blocker. And the implementation there isn't working for me.

Seems this should block. Marshall, are you the right owner for this? If not, please reassign or give back to nobody for re-triage of owner...

(In reply to Johnny Stenback (:jst, jst@mozilla.com) from comment #2) > Seems this should block. Marshall, are you the right owner for this? If not, > please reassign or give back to nobody for re-triage of owner... I was actually going to suggest Fabrice would be the right owner here.

(In reply to Jason Smith [:jsmith] from comment #3) > (In reply to Johnny Stenback (:jst, jst@mozilla.com) from comment #2) > > Seems this should block. Marshall, are you the right owner for this? If not, > > please reassign or give back to nobody for re-triage of owner... > > I was actually going to suggest Fabrice would be the right owner here. Or Fernando.

Fernando, Fabrice or Gregor are likely good assignees, but they are both on vacation. Someone from the necko team can probably look into the appcache aspect here.

I'm writing tests for this over in bug 826058.

I'll get to this after I do bug 826846. Perhaps Honza has an idea in the meantime.

I presume the offline cache update check is made using nsIOfflineCacheUpdateService.checkForUpdate() ? (introduced in bug 751754 and bug 796045 and bug 809947) Since there is no update notification, it looks like the offline cache manifest has not been first time served with Cache-control: no-cache. That is my first theory. Is the old fashioned Error Console (Shift-Ctrl-J on desktop) present also on FxOS that you could check for what happens? See http://www.janbambas.cz/offline-cache-update-console-logs-introduced-in-firefox-19/ If you give me tools I can reproduce with (I don't have the device) then I'm happy to investigate. Best would be to do this with a desktop build of Fx or FxOS (b2g), but I to this day have zero experience with hosted apps installation and updates.

See URL for test files. See "Simple Appcache Update" in http://mozqa.com/webapi-permissions-tests/ for what I did to test this.

I'm back, so I can work on this now, if that is ok with Jason.

fine with me--let me or Honza know if you need help.

At https://mxr.mozilla.org/mozilla-central/source/dom/apps/src/Webapps.jsm#1233 we check for app cache changes only for non removable apps, since this is the only way we can "update" them. For hosted apps using an appcache and installed from a store, if the manifest has not changed we don't check the app cache. If the web page references the app cache (it should), it will update like any other webpage, but that's a different codepath and will not be seen as an app update. I honestly don't remember why we coded this "only update the appcache with no webapps manifest change" only for preloaded apps.

(In reply to Fabrice Desré [:fabrice] from comment #12) > At > https://mxr.mozilla.org/mozilla-central/source/dom/apps/src/Webapps.jsm#1233 > we check for app cache changes only for non removable apps, since this is > the only way we can "update" them. > > For hosted apps using an appcache and installed from a store, if the > manifest has not changed we don't check the app cache. If the web page > references the app cache (it should), it will update like any other webpage, > but that's a different codepath and will not be seen as an app update. > > I honestly don't remember why we coded this "only update the appcache with > no webapps manifest change" only for preloaded apps. Talked with Jonas about this. We should be showing UI in both cases and give indication of the size of bytes downloaded during the progress of the update.

Jason I would appreciate if you could clarify what is the exact issue here. First of all, comment 0 describes what I think is the normal behavior for web content using appcache. I mean, when the *appcache manifest* is modified in the server, the client updates its cache copy without notifying the user *when the app is launched*. If I am not wrong, there is no call to |mozIDOMApplication.checkForUpdate| in this case. Jason S., correct me if I am wrong, but this is the behavior that you are seeing and that you suggested that it is not correct. In that case, do we actually want to notify the user about an appcache update in the server (with a modified manifest.appcache) before updating its local copy on the client during the app launch? The other issue is the one that Fabrice mentioned in comment 12: "For hosted apps using an appcache and installed from a store, if the manifest has not changed we don't check the app cache". Fabrice (and I think Jonas too) suggested that we should check the app cache even if the manifest.appcache didn't change also for removable apps, which I agree that we should do but it doesn't fit well with the description that you gave for this bug (at least the way I understand it). Note that even if we do that, if I am not wrong, the update won't be notified until there is a call to |mozIDOMApplication.checkForUpdate|, which is not the case during app launch.

(In reply to Fernando Jiménez Moreno [:ferjm] from comment #14) > Jason I would appreciate if you could clarify what is the exact issue here. > > First of all, comment 0 describes what I think is the normal behavior for > web content using appcache. I mean, when the *appcache manifest* is modified > in the server, the client updates its cache copy without notifying the user > *when the app is launched*. If I am not wrong, there is no call to > |mozIDOMApplication.checkForUpdate| in this case. I can confirm this. When you normally browse to an html page with <html manifest="some-manifest"> then the updates is made automatically, according the spec. Only window.applicationCache content object is notified. (onchecking, ondownloading, onprogress, oncached, onerror, onnoupdate, onupdateready). PS, also for others: Please ALWAYS specify which manifest you are talking about (offline cache OR web app).

And, the updated version of the cache will be used on next load of the web app.

(In reply to Fernando Jiménez Moreno [:ferjm] from comment #14) > Jason I would appreciate if you could clarify what is the exact issue here. > > First of all, comment 0 describes what I think is the normal behavior for > web content using appcache. I mean, when the *appcache manifest* is modified > in the server, the client updates its cache copy without notifying the user > *when the app is launched*. If I am not wrong, there is no call to > |mozIDOMApplication.checkForUpdate| in this case. > > Jason S., correct me if I am wrong, but this is the behavior that you are > seeing and that you suggested that it is not correct. In that case, do we > actually want to notify the user about an appcache update in the server > (with a modified manifest.appcache) before updating its local copy on the > client during the app launch? To my understanding what was originally speced, we were supposed to notify the user, show the downloading UI, and apply the appcache update for a hosted app update that preloads appcache. One issue we have right now is the following: *Installed* hosted web apps follow the exact behavior you describe - the "typical" appcache rules with silent updating. *Preinstalled" hosted web apps do *not* follow the behavior you've specified - we show the UI flow I'm describing. There's an inconsistency here - a preloaded hosted app preloading appcache is showing the UI flow, but an installed hosted app preloading appcache does not show the UI flow. I actually don't really have a strong opinion here on the appcache piece for updating on whether we follow the UI or not as long as: 1) We have a consistent UX across preloads vs. installed 2) We are successfully updating the appcache upon launch of the app [1] seems to be the problem in this bug. (In reply to Honza Bambas (:mayhemer) from comment #15) > (In reply to Fernando Jiménez Moreno [:ferjm] from comment #14) > > Jason I would appreciate if you could clarify what is the exact issue here. > > > > First of all, comment 0 describes what I think is the normal behavior for > > web content using appcache. I mean, when the *appcache manifest* is modified > > in the server, the client updates its cache copy without notifying the user > > *when the app is launched*. If I am not wrong, there is no call to > > |mozIDOMApplication.checkForUpdate| in this case. > > I can confirm this. When you normally browse to an html page with <html > manifest="some-manifest"> then the updates is made automatically, according > the spec. Only window.applicationCache content object is notified. > (onchecking, ondownloading, onprogress, oncached, onerror, onnoupdate, > onupdateready). That's true, but not all web app preloading appcache will use <html manifest="some-manifest"> if they say, choose to use the appcache_path. However, we probably could easily evangelize to a web app developer to do both flows to get updates to happen. So I need input here from UX and Jonas. Why is the UX inconsistent here? Are we supposed to be showing the download flow see in preloads with installed web apps as well that preload appcache? Should we remove the UX from both? What should we do as a mitigation?

One piece of confusion for me (as a newcomer to OWA stuff, writing tests) is what the relationship is between listing the appcache in the manifest and placing it in <html manifest="">. When I asked about it, sicking said he wasn't sure, and that it was "probably best to do both". I think we should clarify exactly what effect we want each declaration to do, and document it very explicitly.

(In reply to Bobby Holley (:bholley) from comment #18) > One piece of confusion for me (as a newcomer to OWA stuff, writing tests) is > what the relationship is between listing the appcache in the manifest and > placing it in <html manifest="">. When I asked about it, sicking said he > wasn't sure, and that it was "probably best to do both". > > I think we should clarify exactly what effect we want each declaration to > do, and document it very explicitly. Hmm. appcache_path I believe was originally designed to get appcache resources preloaded on the install of the app so that if I access the app with my network down on first launch, the app should launch with those appcache resources available for that app. If we didn't do this, then the app would fail to launch. <html manifest=""> won't be able to handle that requirement on first launch for two reasons: 1. Data jars - each app gets it's own cookie jar, so visiting the web content in one app to get appcache resources won't preload them in a different app 2. I believe the appcache would preloaded with <html manifest=""> after the app is first launched So my hunch is that the original design of appcache_path was to be able to preload appcache resources without needing to launch the app to update the resources. Although I could be wrong.

Yes it must be that, you're right.

Do we agree on what needs to be done here? I don't think we can do more than ensuring we check for appcache updates even when the app manifest has not changed.

(In reply to Fabrice Desré [:fabrice] from comment #21) > Do we agree on what needs to be done here? I don't think we can do more than > ensuring we check for appcache updates even when the app manifest has not > changed. Maybe? Can't tell if there's consensus or not. Whatever UI flow we are doing for preloaded hosted apps that preload appcache we should also do for installed hosted apps that preload appcache. Is that what we're going for?

Yes, that's my proposal.

(In reply to Fabrice Desré [:fabrice] from comment #23) > Yes, that's my proposal. Sounds good to me. Anyone disagree?

ok with that.

Sounds like we've got the info we need. Let's do it.

What I'm proposing is this: When a hosted app is installed, we install the app manifest (of course), and if the app manifest links to an appcache using appcache_path we also download that app manifest and prime that appcache, i.e. download the resources listen in the appcache manifest. Progress events should be fired as the resources are being downloaded. I believe all of this is already happening. When we later check for updates for this app using the app.checkForUpdate() API, we check for updates for the app manifest, *and*, if the new (and likely the old) manifest contains an appcache_path property, check if there are updates available for that appcache. If *either* the app manifest *or* the appcache needs updating, we fire the updateavailable event and set the downloadAvailable flag to true. When the download() function is called, if we had determined that the appcache needs to be updated, we update the appcache and fire progress events as appropriate. If the appcache does not need to be updated and just the manifest is changed, we immediately fire the downloadsuccess event. Otherwise we wait for the appcache to be downloaded and then fire the downloadsuccess event. Note that in all of these case, when checking the appcache for updates or when downloading a new appcache, we do that by calling into the appcache code. It should now have all the APIs needed to do this. There should be no need to download appcache-related resources manually using XHR. One situation which is somewhat tricky is if the appcache_path property changes between the old and the new manifest. In this case, if the new manifest has a new appcache_path, we should always consider it as if the appcache needs to be updated and so it'll always get downloaded when download() is called. If the new manifest does not have a appcache_path then we do not need to download an appcache when download() is called. Also, I don't think we should have any special handling for the case when any of the HTML pages in the app links to an appcache. I.e. we shouldn't be firing progress events or updateavailable events or anything like that on the app object if the appcache is updated due to a page inside the app uses the appcache. Including if the start-page uses the appcache. The goals here are IMHO: * Always keep the appcache up-to-date for installed application. * The download/install API on the app object should behave consistently for packaged apps, hosted apps with appcache and hosted apps without appcache.

Amen.

What happens if the appcache manifests in "appcache_path" and the manifest in "<html manifest=" are different ?

(In reply to Jonas Sicking (:sicking) from comment #27) > Also, I don't think we should have any special handling for the case when > any of the HTML pages in the app links to an appcache. I.e. we shouldn't be > firing progress events or updateavailable events or anything like that on > the app object if the appcache is updated due to a page inside the app uses > the appcache. Including if the start-page uses the appcache. This needs some new code. If you mean prevent any content notification (via window.applicationCache object) then we have to either completely disable doing updates for app origins triggered by hitting manifest= attribute or disable only the notifications. For consistency I vote for the former. This may also lead to better perfomance since there is significant main thread I/O when checking for app cache updates based on hitting the manifest= attribute. It would also mean that appcache of any app (I'm a bit lost in what all kinds of apps we can have) can only be updated via appcache_path referenced by the app manifest. (In reply to Julien Wajsberg [:julienw] from comment #29) > What happens if the appcache manifests in "appcache_path" and the manifest > in "<html manifest=" are different ? That means that the main page will be marked as foreign in the app cache referenced by appcache_path in the app manifest (it prevents to use that app cache for loading this html page any more), the page will be reloaded, and will try to load from app cache that is referenced by the html manifest attribute. If it is not cached and we are offline, the app will not load. Hence, it may be quit bad. This is a good concern maybe leading to a complete disabling of support of the html manifest attribute for apps and suggesting OWA developers to not include it in their html.

(In reply to Honza Bambas (:mayhemer) from comment #30) > (In reply to Jonas Sicking (:sicking) from comment #27) > > Also, I don't think we should have any special handling for the case when > > any of the HTML pages in the app links to an appcache. I.e. we shouldn't be > > firing progress events or updateavailable events or anything like that on > > the app object if the appcache is updated due to a page inside the app uses > > the appcache. Including if the start-page uses the appcache. > > This needs some new code. If you mean prevent any content notification (via > window.applicationCache object) then we have to either completely disable > doing updates for app origins triggered by hitting manifest= attribute or > disable only the notifications. For consistency I vote for the former. > This may also lead to better perfomance since there is significant main > thread I/O when checking for app cache updates based on hitting the > manifest= attribute. > Correct me if I am wrong, but I think Jonas was referring to notifications via the WebApps API (like https://mxr.mozilla.org/mozilla-central/source/dom/interfaces/apps/nsIDOMApplicationRegistry.idl#41). While you are referring to notifications via window.applicationCache (which might be for example events like https://mxr.mozilla.org/mozilla-central/source/dom/interfaces/offline/nsIDOMOfflineResourceList.idl#99 , right?).

(In reply to Fernando Jiménez Moreno [:ferjm] from comment #31) > Correct me if I am wrong, but I think Jonas was referring to notifications > via the WebApps API (like > https://mxr.mozilla.org/mozilla-central/source/dom/interfaces/apps/ > nsIDOMApplicationRegistry.idl#41). While you are referring to notifications > via window.applicationCache (which might be for example events like > https://mxr.mozilla.org/mozilla-central/source/dom/interfaces/offline/ > nsIDOMOfflineResourceList.idl#99 , right?). Yes, exactly, I am *not* talking about WebApps API. I don't know OWA stuff that well, so I'm sorry for confusion here.

I couldn't properly test yet, but in the mean time any feedback is highly appreciated.

(In reply to Honza Bambas (:mayhemer) from comment #30) > This is a good concern maybe leading to a complete disabling of support of > the html manifest attribute for apps and suggesting OWA developers to not > include it in their html. From an app developer standpoint, this makes the most sense IMO. Having two frobs to twiddle is always error-prone.

Comment on attachment 699989 [details] [diff] [review] v1 Review of attachment 699989 [details] [diff] [review]: ----------------------------------------------------------------- That looks like a correct approach. But did you forget to update your patch with a |let id = this._appId(app.origin);| in the top level function?

(In reply to Honza Bambas (:mayhemer) from comment #30) > (In reply to Jonas Sicking (:sicking) from comment #27) > > Also, I don't think we should have any special handling for the case when > > any of the HTML pages in the app links to an appcache. I.e. we shouldn't be > > firing progress events or updateavailable events or anything like that on > > the app object if the appcache is updated due to a page inside the app uses > > the appcache. Including if the start-page uses the appcache. > > This needs some new code. If you mean prevent any content notification (via > window.applicationCache object) What I meant was that we only disable the notifications that are sent from the mozIDOMApplication object. I.e. the notifications exposed to web pages. So we should still send notifications through window.applicationCache as we always have. I absolutely think we should follow our normal behavior there. And we should definitely update the appcache just like normal. > (In reply to Julien Wajsberg [:julienw] from comment #29) > > What happens if the appcache manifests in "appcache_path" and the manifest > > in "<html manifest=" are different ? > > That means that the main page will be marked as foreign in the app cache > referenced by appcache_path in the app manifest (it prevents to use that app > cache for loading this html page any more), the page will be reloaded, and > will try to load from app cache that is referenced by the html manifest > attribute. If it is not cached and we are offline, the app will not load. > Hence, it may be quit bad. Yeah, this is indeed quite bad. We should definitely look at handling this gracefully somehow, but I don't think it's something we should try to handle for v1. I think it's quite acceptable to simply say "don't do that" in v1.

(In reply to Jonas Sicking (:sicking) from comment #36) > We should definitely look at handling this gracefully somehow, but I don't > think it's something we should try to handle for v1. We could bypass foreign marking and reload when load happens from OWA. > I think it's quite > acceptable to simply say "don't do that" in v1. don't do what? developers shouldn't add the manifest attrib or should ensure it is the same?

(In reply to Honza Bambas (:mayhemer) from comment #37) > don't do what? developers shouldn't add the manifest attrib or should > ensure it is the same? We should document that linking to different manifests from the appcache_path property (in the app-manifest) manifest attribute (in the html markup) is a bad thing and developers should not do it. For v2 we can try to solve it in a better way, but let's discuss that elsewhere since it's unrelated to this bug.

Comment on attachment 700383 [details] [diff] [review] v2 Review of attachment 700383 [details] [diff] [review]: ----------------------------------------------------------------- ::: dom/apps/src/Webapps.jsm @@ +1162,5 @@ > delete aData.app.updateManifest; > }); > } > > + function updateHostedApp(aOldManifest, aNewManifest) { Nit: add a comment explaining why we need both manifests. @@ +1199,5 @@ > app.readyToApplyDownload = false; > app.downloadAvailable = !!manifest.appcache_path; > > + app.name = aNewManifest ? aNewManifest.name : aOldManifest.name; > + app.csp = aNewManifest ? aNewManifest.csp : aOldManifest.csp; Nit: why not use manifest directly? @@ +1344,5 @@ > + } > + aMm.sendAsyncMessage("Webapps:CheckForUpdate:Return:OK", aData); > + this._saveApps(); > + } else { > + // For hosted apps,even if the manifest has not changed, we check nit: apps, even...

Verified on 1/21 build.