If you think a bug might affect users in the 57 release, please set the correct tracking and status flags for Release Management.

AppCache update notification for Slithering appears at every startup of Gaia

VERIFIED FIXED in Firefox 21

Status

()

Core
DOM: Apps
VERIFIED FIXED
5 years ago
5 years ago

People

(Reporter: MattN, Assigned: fabrice)

Tracking

Trunk
mozilla21
ARM
Gonk (Firefox OS)
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(blocking-b2g:tef+, firefox19 wontfix, firefox20 wontfix, firefox21 fixed, b2g18 fixed, b2g18-v1.0.0 fixed)

Details

(Whiteboard: [triage:1/16], URL)

+++ 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.
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.
Depends on: 823648
Assignee: nobody → felash
(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.
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.
I just filed Bug 829934 in addition to the existing Bug 823648.
(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?
Flags: needinfo?(jonas)
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.
I'd be happy to be wrong ;)
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 ;-)
(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?
Flags: needinfo?(jonas)
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
I give this to Fabrice as well, because I think it may be related to bug 829522.
Assignee: felash → fabrice
Depends on: 829522
No longer depends on: 823648

Updated

5 years ago
blocking-b2g: --- → tef?
blocking-b2g: tef? → tef+

Updated

5 years ago
Blocks: 728081
blocking-b2g: tef+ → tef?
Opps. Apparently I renomed by accident.
blocking-b2g: tef? → tef+
(Assignee)

Comment 13

5 years ago
Marking QA wanted, as I can't reproduce anymore now that bug 829522 landed.
(Assignee)

Updated

5 years ago
Keywords: qawanted

Updated

5 years ago
QA Contact: jsmith
I take it I'll have to wait until tomorrow to test this, cause this reproduces on today's 1/15 build.
(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
  }
Keywords: qawanted
Whiteboard: [triage:1/16]
tracking-b2g18: ? → ---
(Assignee)

Comment 16

5 years ago
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
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.
(Assignee)

Comment 18

5 years ago
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
Fixing bug 829934 *or* bug 823648 would fix this bug.
Depends on: 823648, 829934
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.
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.
(Assignee)

Comment 22

5 years ago
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.
Fabrice, exactly. So I don't understand why we'd have a downloadavailable here... Appcache should be up to date so...
(Assignee)

Comment 24

5 years ago
I don't know either. I can't reproduce this anymore :(
(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?
(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.
No longer depends on: 823648
Dependency is fixed now. I'm going to test this now to see what happens now...
Keywords: qawanted
Verified that the dependencies fixed this bug on 1/20 build. Closing.
Status: NEW → RESOLVED
Last Resolved: 5 years ago
Keywords: qawanted
Resolution: --- → FIXED

Updated

5 years ago
Status: RESOLVED → VERIFIED
Component: Networking: Cache → DOM: Apps
status-b2g18: --- → fixed
status-firefox19: --- → wontfix
status-firefox20: --- → wontfix
status-firefox21: --- → fixed
Target Milestone: --- → mozilla21
status-b2g18-v1.0.0: --- → fixed
You need to log in before you can comment on or make changes to this bug.