829853 - AppCache update notification for Slithering appears at every startup of Gaia

Status: VERIFIED FIXED

(Core Graveyard :: DOM: Apps, defect)

Description

[Matthew N. [:MattN]]

+++ This bug was initially created as a clone of Bug #823143 +++

A Slithering update is presented when updates are checked even if nothing changed in the GitHub repo.

App name in Marketplace: Slithering
Origin: http://snake-app.github.com/
AppCache Manifest: http://snake-app.github.com/snake.appcache

Repro:
1) install beta unagi nightly build
2) install Slithering app from marketplace
3) Check for app updates

Expected:
- no Slithering update offered since the app hasn't changed

Actual:
- Slithering update is offered

I was seeing the following in logcat the other day but that may be bug 823143:
12-19 10:57:49.197: E/GeckoConsole(110): Content JS INFO at app://system.gaiamobile.org/js/updatable.js:89 in anonymous: downloadError event, error code is APP_CACHE_DOWNLOAD_ERROR
12-19 10:57:49.197: E/GeckoConsole(110): Content JS INFO at app://system.gaiamobile.org/js/app_install_manager.js:191 in ai_handleDownloadError: downloadError event, error code is APP_CACHE_DOWNLOAD_ERROR

The Firefox desktop error console shows:
Offline cache doesn't need to update, URL=http://snake-app.github.com/snake.appcache

Note that I'm having trouble reproducing the problem with today's Gaia build which I think is because of bug 829522.

Comment 1

[Julien Wajsberg [:julienw]]

Hi,

I know this is painful, but currently, manifest files must be served with ETag. Github doesn't do that and I think that is what you're currently seeing.

I already filed Bug 823648 so that we support Last-Modified headers too, it was not considered a priority for v1 but I will definitely push hard to make this happen.

The AppCache errors are still something to be investigated though, I'll take a look on Monday.

Comment 2

[Jason Smith [:jsmith]]

(In reply to Julien Wajsberg [:julienw] from comment #1)
> Hi,
> 
> I know this is painful, but currently, manifest files must be served with
> ETag. Github doesn't do that and I think that is what you're currently
> seeing.
> 
> I already filed Bug 823648 so that we support Last-Modified headers too, it
> was not considered a priority for v1 but I will definitely push hard to make
> this happen.
> 
> The AppCache errors are still something to be investigated though, I'll take
> a look on Monday.

Mini-manifests and zip archives have to be served with an etag. Webapp manifests do *not* need to be served with an etag. So there's a different problem going on here.

Comment 3

[Julien Wajsberg [:julienw]]

Jason, mini-manifests and Webapp manifests are handled in the same way. The ETag header is what is currently used to actually know that there is an update, we have no other mechanism at all.

Comment 4

[Julien Wajsberg [:julienw]]

I just filed Bug 829934 in addition to the existing Bug 823648.

Comment 5

[Jason Smith [:jsmith]]

(In reply to Julien Wajsberg [:julienw] from comment #3)
> Jason, mini-manifests and Webapp manifests are handled in the same way. The
> ETag header is what is currently used to actually know that there is an
> update, we have no other mechanism at all.

Then that's wrong and simply is not going to work for updates. We have hundreds of webapps already up and running on marketplace where they followed the non-etag approach as they were trained that way originally. The etag approach was fine for packaged apps because the marketplace where users are installing from could handle this. But that definitely isn't going to work for hosted apps held on a per domain basis.

Hosted apps simply can't rely on the etag approach. Packaged apps I thought were the only ones that would make that reliance. Jonas - Can you clarify?

Comment 6

[Jason Smith [:jsmith]]

In other words:

The packaged apps approach with etags is sustainable because the marketplace handles doing the serving piece - so there's virtually little impact unless someone wants to host their own store for v1, which is very low priority.

But that's a completely different story for hosted apps. We do *not* have control over those manifests directly - the app developer controls it entirely. And we've already got hundreds of webapps up and running that did not add etag support, because they never knew about this. This is asking for an evangelism battle I'm not sure is worth fighting. Right now, it's likely users will then *not* get updates for hosted apps then.

But something in my brain doesn't align with the statements in comment 3 - I know I tested this about a week ago with a hosted app generally and the update worked fine without etag headers (except that the process had to be restarted in order to see the update). So somehow I think the story is confused here.

Comment 7

[Julien Wajsberg [:julienw]]

I'd be happy to be wrong ;)

Comment 8

[Julien Wajsberg [:julienw]]

I checked deeper in the Webapps.jsm file and I am actually wrong, sorry about that.

If we get a 200 for the manifest (which we are if there is no ETag), then we ask if there is an update in the appCache. My C++ skills stop here ;-)

Comment 9

[Jason Smith [:jsmith]]

(In reply to Julien Wajsberg [:julienw] from comment #8)
> I checked deeper in the Webapps.jsm file and I am actually wrong, sorry
> about that.
> 
> If we get a 200 for the manifest (which we are if there is no ETag), then we
> ask if there is an update in the appCache. My C++ skills stop here ;-)

What happens if the app manifest itself has changed?

Comment 10

[Julien Wajsberg [:julienw]]

We take it into account in [1].

And also, I just saw that we always check for any appcache change even if we get a 304. So please forget everything I said here. I'll work on this on Monday when I'll be rested.

[1] http://mxr.mozilla.org/mozilla-central/source/dom/apps/src/Webapps.jsm#1184

Comment 11

[Julien Wajsberg [:julienw]]

I give this to Fabrice as well, because I think it may be related to bug 829522.

Comment 12

[Jason Smith [:jsmith]]

Opps. Apparently I renomed by accident.

Comment 13

[[:fabrice] Fabrice Desré]

Marking QA wanted, as I can't reproduce anymore now that bug 829522 landed.

Comment 14

[Jason Smith [:jsmith]]

I take it I'll have to wait until tomorrow to test this, cause this reproduces on today's 1/15 build.

Comment 15

[Jason Smith [:jsmith]]

(In reply to Fabrice Desré [:fabrice] from comment #13)
> Marking QA wanted, as I can't reproduce anymore now that bug 829522 landed.

Still reproduces, although you need to install the app, restart the phone, and then check for updates. You'll find an update for the app.

If that's better suited for a different bug, let me know.

  "{36d1b5ae-9721-4b89-b8ac-ef429c8a0e49}": {
    "name": "Slithering",
    "csp": "",
    "installOrigin": "https://marketplace.firefox.com",
    "origin": "http://snake-app.github.com",
    "receipts": [],
    "installTime": 1358353500273,
    "manifestURL": "http://snake-app.github.com/snake.webapp",
    "appStatus": 1,
    "removable": true,
    "id": "{36d1b5ae-9721-4b89-b8ac-ef429c8a0e49}",
    "localId": 1023,
    "basePath": "/data/local/webapps",
    "progress": 485382,
    "installState": "installed",
    "downloadAvailable": true,
    "downloading": false,
    "readyToApplyDownload": false,
    "downloadSize": 0,
    "lastUpdateCheck": 1358353500273,
    "updateTime": 1358353670050,
    "etag": null,
    "installerAppId": 1022,
    "installerIsBrowser": false,
    "lastCheckedUpdate": 1358353670026
  }

Comment 16

[[:fabrice] Fabrice Desré]

In this case (first update check after install), a prompt is expected because of the lack of etag. Stangely I can also reproduce that after a gecko restart, with this entry in webapps.json :

"{f8c2b94b-cc60-45e5-bbe0-ffe01b2eaebf}": {
    "name": "Slithering",
    "csp": "",
    "installOrigin": "https://marketplace.firefox.com",
    "origin": "http://snake-app.github.com",
    "receipts": [],
    "installTime": 1358358810362,
    "manifestURL": "http://snake-app.github.com/snake.webapp",
    "appStatus": 1,
    "removable": true,
    "id": "{f8c2b94b-cc60-45e5-bbe0-ffe01b2eaebf}",
    "localId": 1025,
    "basePath": "/data/local/webapps",
    "progress": 0,
    "installState": "installed",
    "downloadAvailable": false,
    "downloading": false,
    "readyToApplyDownload": false,
    "downloadSize": 0,
    "lastUpdateCheck": 1358358810362,
    "updateTime": 1358365213940,
    "etag": null,
    "installerAppId": 1021,
    "installerIsBrowser": false,
    "lastCheckedUpdate": 1358365213897,
    "retryingDownload": false
  }

Note that here downloadAvailable is false, so something must be wrong on the gaia side (we get "I/Gecko   ( 3612): UpdatePrompt: appsUpdated: 0 apps to update" in logcat)

At this point, if we accept the download, we get a never ending spinner from bug 830661

Comment 17

[Jason Smith [:jsmith]]

I thought hosted apps weren't connected to etags?

If we're always doing the etag check, then I'd expect basically every app to always say it has an app update upon install. Almost every hosted app developer I don't think has used etags for their webapp manifests for hosted apps.

Comment 18

[[:fabrice] Fabrice Desré]

We use etag when checking the webapp manifest. We also need to add support for a hash of the manifest's content. This is bug 829934

Comment 19

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

Fixing bug 829934 *or* bug 823648 would fix this bug.

Comment 20

[Julien Wajsberg [:julienw]]

even if I'd be happy to fix these bugs, I thought that for hosted+appcache apps, if we get a status 200 then we check for appcache before sending downloadavailable event.

Comment 21

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

No, I think we need to send the updateavailable event in order to indicate that the application manifest has changed. Since application manifest changes can be visible to the user (changes icon and in the future changes app name). And so we should trigger the same UI flows as for updates to the application contents.

Comment 22

[[:fabrice] Fabrice Desré]

What we do for hosted app is:

1) we check the manifest, and in all cases run the following steps:

2) if there's no appacache, we send a downloadapplied event.
3) if there's an appcache, we check if it needs to be updated.
  3.1) If yes, we send a downloadavailable event.
  3.2) If no, we send a downloadapplied event.

Comment 23

[Julien Wajsberg [:julienw]]

Fabrice, exactly. So I don't understand why we'd have a downloadavailable here... Appcache should be up to date so...

Comment 24

[[:fabrice] Fabrice Desré]

I don't know either. I can't reproduce this anymore :(

Comment 25

[Andrew Overholt [:overholt]]

(In reply to Fabrice Desré [:fabrice] from comment #24)
> I don't know either. I can't reproduce this anymore :(

Johnny says he can see this with the Maps app on his phone today.  Fabrice, maybe you can take a look at Johnny's phone?

Comment 26

[Jason Smith [:jsmith]]

(In reply to Andrew Overholt [:overholt] from comment #25)
> (In reply to Fabrice Desré [:fabrice] from comment #24)
> > I don't know either. I can't reproduce this anymore :(
> 
> Johnny says he can see this with the Maps app on his phone today.  Fabrice,
> maybe you can take a look at Johnny's phone?

The maps app issue is a different bug - bug 830913.

I did manage to reproduce this issue and demonstrate it to Fabrice. You needed to restart the phone after you installed the Slithering app, check for updates, and you'll find the update, even though nothing changed.

bug 829934 will likely resolve this bug.

Comment 27

[Jason Smith [:jsmith]]

Dependency is fixed now. I'm going to test this now to see what happens now...

Comment 28

[Jason Smith [:jsmith]]

Verified that the dependencies fixed this bug on 1/20 build. Closing.