832408 - Notifications stick around after a failed 3rd party app update

Status: RESOLVED FIXED

(Core Graveyard :: DOM: Apps, defect)

Description

[(not reading bugmail) Nick Desaulniers [:\n]]

Testing on 1/18/13 Nightly b2g build.  In a packaged app that fails to update due to invalid signature, a notification appears for an update.  When the user clicks update, the system silently fails (from the user's perspective) and the notification is not removed.  The user would probably believe the click was not registered and keep clicking the notification that is silently failing.  Presumably, the marketplace should not allow apps with invalid signatures to pass the review process for packaged apps, but notifications should be removed for any failed update.

Comment 1

[Jason Smith [:jsmith]]

This seems to be a common pain point - if we fail an update, we ask to update again immediately. Then the failure cycle happens again.

Maybe it's time to get some UX ideas to improve this.

Comment 2

[Jason Smith [:jsmith]]

Dogfooders are yelling all over the place about this bug. And it's giving me a headache. So many dupes!

We need a better mechanism to know when *not* to show update available when we know we're going to fail.

Comment 3

[[:fabrice] Fabrice Desré]

The issue here is as follows:

1- we detect that an update is available. This sets downloadAvailable = true
2- this triggers the notification
3- we fail to install the new package
4- downloadAvailable is still true because nothing prevents the server side to correct the issue.
5- goto 2

So, we could reset the app's state to "nothing available to update" after an update failure.

Comment 4

[Jason Smith [:jsmith]]

(In reply to Fabrice Desré [:fabrice] from comment #3)
> The issue here is as follows:
> 
> 1- we detect that an update is available. This sets downloadAvailable = true
> 2- this triggers the notification
> 3- we fail to install the new package
> 4- downloadAvailable is still true because nothing prevents the server side
> to correct the issue.
> 5- goto 2
> 
> So, we could reset the app's state to "nothing available to update" after an
> update failure.

That's a possibility. The idea I had in this bug was to do common checks for things we know before download that will cause the download to fail and not fire the downloadAvailable notification. That would ease the pain that our dogfooders are experiencing right now.

Comment 5

[Jason Smith [:jsmith]]

Looks like Fabrice is right. I can't reproduce this on today's build. wfm

Comment 6

[[:fabrice] Fabrice Desré]

Jason, I'm reopening because I don't think this is the issue that dogfooders are facing (their problem is the transition from hosted to packaged of the maps app). I'm working on a patch for this one.

Comment 7

[[:fabrice] Fabrice Desré]

Attachment: patch

This patch does address the general issue with failing updates by staging the update manifest and the etags/hash until we actually call applyDownload().

Works fine in my tests here, but definitely needs other eyeballs.

Comment 8

[Fernando Jiménez Moreno [:ferjm]]

Comment on attachment 704364 [details] [diff] [review]
patch

Review of attachment 704364 [details] [diff] [review]:
-----------------------------------------------------------------

The patch looks good, but I am afraid that it is not complete.

Testing shows an issue with the AppDownloadManager:

I/Gecko   (  458): -*-*- Webapps.jsm : startDownload for http://apptester.eu01.aws.af.cm/apps/packagedApp/manifest.webapp
I/Gecko   (  458): -*-*- Webapps.jsm : downloadPackage {"manifestURL":"http://apptester.eu01.aws.af.cm/apps/packagedApp/manifest.webapp","origin":"app://{55361da4-cafb-44ff-96e0-6e63fe7b7acc}","downloadSize":8522}
I/Gecko   (  458): -*-*- Webapps.jsm : Free storage: 2399199232. Download size: 8522
I/Gecko   (  458): -*-*- Webapps.jsm : About to download http://apptester.eu01.aws.af.cm/apps/packagedApp/application.zipj
I/Gecko   (  458): -*-*- Webapps.jsm : Add If-None-Match header: "10004-1358844882000"
I/Gecko   (  458): -*-*- Webapps.jsm : isUpdate true
I/Gecko   (  458): -*-*- AppDownloadManager.jsm : Adding http://apptester.eu01.aws.af.cm/apps/packagedApp/manifest.webapp
I/Gecko   (  458): -*-*- AppDownloadManager.jsm : aDownload [object Object]
I/Gecko   (  458): -*-*- AppDownloadManager.jsm : Disk space is now free
I/Gecko   (  458): -*-*- Webapps.jsm : We found no ETag Header, canceling the request
I/Gecko   (  458): -*-*- AppDownloadManager.jsm : Removing http://apptester.eu01.aws.af.cm/apps/packagedApp/manifest.webapp
I/Gecko   (  458): -*-*- Webapps.jsm : onStopRequest 2152398850
I/Gecko   (  458): -*-*- Webapps.jsm : Cleanup: NETWORK_ERROR
I/Gecko   (  458): -*-*- AppDownloadManager.jsm : Getting http://apptester.eu01.aws.af.cm/apps/packagedApp/manifest.webapp
I/Gecko   (  458): -*-*- AppDownloadManager.jsm : Download undefined
I/Gecko   (  458): -*-*- Webapps.jsm : Got download from AppDownloadManager undefined
I/Gecko   (  458): -*-*- Webapps.jsm : Saving /data/local/webapps/webapps.json

As you can see the download is removed from AppDownloadManager as soon as we get the error when trying to get the etag header (from a manifest package that doesn't exist in the server). That makes that the app install state changes to "pending" (https://mxr.mozilla.org/mozilla-central/source/dom/apps/src/Webapps.jsm#1932) which leaves the icon useless in the homescreen.

::: dom/apps/src/AppsUtils.jsm
@@ +56,5 @@
>        downloadSize: aApp.downloadSize || 0,
>        lastUpdateCheck: aApp.lastUpdateCheck,
>        updateTime: aApp.updateTime,
>        etag: aApp.etag,
> +      stagedEtag: aApp.stagedEtag,

This property is not used anywhere. It seems that you are using aApp.staged.etag instead.

::: dom/apps/src/Webapps.js
@@ +578,5 @@
>              this._fireEvent("downloadsuccess", this._ondownloadsuccess);
>              this._fireEvent("downloadapplied", this._ondownloadapplied);
>              break;
>            case "downloaded":
> +            if (msg.manifest) {

Could you add a comment here explaining that we don't update packaged apps update manifest until the download is applied, please?

::: dom/apps/src/Webapps.jsm
@@ +950,4 @@
>      let file = FileUtils.getFile(DIRECTORY_NAME,
> +                               ["webapps", id, isUpdate ? "staged-update.webapp"
> +                                                        : "update.webapp"],
> +                               true);

nit: indentation

@@ +2224,5 @@
> +                  if (!app.staged) {
> +                    app.staged = { };
> +                  }
> +                  app.staged.packageEtag =
> +                    requestChannel.getResponseHeader("Etag");

Did you remove the try-catch statement on purpose? I am not entirely sure about why it was needed in first place. It seems that |getResponseHeader()| doesn't throw if the header doesn't exist.

Comment 9

[[:fabrice] Fabrice Desré]

Attachment: patch v2

(In reply to Fernando Jiménez Moreno [:ferjm] from comment #8)
> Comment on attachment 704364 [details] [diff] [review]
> patch
> 
> Review of attachment 704364 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> The patch looks good, but I am afraid that it is not complete.
> 
> Testing shows an issue with the AppDownloadManager:
> 
> As you can see the download is removed from AppDownloadManager as soon as we
> get the error when trying to get the etag header (from a manifest package
> that doesn't exist in the server). That makes that the app install state
> changes to "pending"
> (https://mxr.mozilla.org/mozilla-central/source/dom/apps/src/Webapps.
> jsm#1932) which leaves the icon useless in the homescreen.

yep, I removed everything from the onStartRequest() handler since it's useless now that we have hashing support.

> ::: dom/apps/src/AppsUtils.jsm
> @@ +56,5 @@
> >        downloadSize: aApp.downloadSize || 0,
> >        lastUpdateCheck: aApp.lastUpdateCheck,
> >        updateTime: aApp.updateTime,
> >        etag: aApp.etag,
> > +      stagedEtag: aApp.stagedEtag,
> 
> This property is not used anywhere. It seems that you are using
> aApp.staged.etag instead.

Good catch, this was a leftover from a previous version of the patch.

> ::: dom/apps/src/Webapps.js
> @@ +578,5 @@
> >              this._fireEvent("downloadsuccess", this._ondownloadsuccess);
> >              this._fireEvent("downloadapplied", this._ondownloadapplied);
> >              break;
> >            case "downloaded":
> > +            if (msg.manifest) {
> 
> Could you add a comment here explaining that we don't update packaged apps
> update manifest until the download is applied, please?

done.

> ::: dom/apps/src/Webapps.jsm
> @@ +950,4 @@
> >      let file = FileUtils.getFile(DIRECTORY_NAME,
> > +                               ["webapps", id, isUpdate ? "staged-update.webapp"
> > +                                                        : "update.webapp"],
> > +                               true);
> 
> nit: indentation

done.

> @@ +2224,5 @@
> > +                  if (!app.staged) {
> > +                    app.staged = { };
> > +                  }
> > +                  app.staged.packageEtag =
> > +                    requestChannel.getResponseHeader("Etag");
> 
> Did you remove the try-catch statement on purpose? I am not entirely sure
> about why it was needed in first place. It seems that |getResponseHeader()|
> doesn't throw if the header doesn't exist.

Looking at the implementation of getResponseHeader() at https://mxr.mozilla.org/mozilla-central/source/netwerk/protocol/http/HttpBaseChannel.cpp#1030, this can throws in two cases:
- when called before we got the http headers
- when the header we're looking for is not in https://mxr.mozilla.org/mozilla-central/source/netwerk/protocol/http/nsHttpAtomList.h
That means that we will never throw here, since we're looking for ETag in a 200 request.

Comment 10

[Fernando Jiménez Moreno [:ferjm]]

Comment on attachment 705048 [details] [diff] [review]
patch v2

Review of attachment 705048 [details] [diff] [review]:
-----------------------------------------------------------------

Thanks Fabrice! It seems that we also need the fix at Bug 833587 to make this work properly.

r=me

Comment 11

[[:fabrice] Fabrice Desré]

Comment on attachment 705048 [details] [diff] [review]
patch v2

[Approval Request Comment]
This is part of a set of patches that helps hardening against edge cases that may let the phone in a bad state. Risk is low compared to the reward.

Comment 12

[Julien Wajsberg [:julienw]]

I don't know if this is this specific fix that triggers this as I have 3 different patches about updates, but I see the following regressions:

* start install from http://everlong.org/mozilla/bigpackaged/
* trigger a download error (for example: cancel the install)
* restart the install from Homescreen
* wait until the end
=> the icon doesn't change in Homescreen.
However the notification is removed.

This probably means that downloadsuccess was sent but not downloadapplied.

I thought that on app installs we always get downlaodsuccess and downloadapplied in the same time but it seems that changed ?

I see the following error in logcat:

E/GeckoConsole(  489): [JavaScript Error: "[Exception... "Component returned failure code: 0x80520006 (NS_ERROR_FILE_TARGET_DOES_NOT_EXIST) [nsIFile.moveTo]"  nsresult: "0x80520006 (NS_ERROR_FILE_TARGET_DOES_NOT_EXIST)"  location: "JS frame :: resource://gre/modules/Webapps.jsm :: applyDownload :: line 1032"  data: no]" {file: "resource://gre/modules/Webapps.jsm" line: 1032}]

Comment 13

[Jason Smith [:jsmith]]

(In reply to Julien Wajsberg [:julienw] from comment #12)
> I don't know if this is this specific fix that triggers this as I have 3
> different patches about updates, but I see the following regressions:
> 
> * start install from http://everlong.org/mozilla/bigpackaged/
> * trigger a download error (for example: cancel the install)
> * restart the install from Homescreen
> * wait until the end
> => the icon doesn't change in Homescreen.
> However the notification is removed.
> 
> This probably means that downloadsuccess was sent but not downloadapplied.
> 
> I thought that on app installs we always get downlaodsuccess and
> downloadapplied in the same time but it seems that changed ?
> 
> I see the following error in logcat:
> 
> E/GeckoConsole(  489): [JavaScript Error: "[Exception... "Component returned
> failure code: 0x80520006 (NS_ERROR_FILE_TARGET_DOES_NOT_EXIST)
> [nsIFile.moveTo]"  nsresult: "0x80520006
> (NS_ERROR_FILE_TARGET_DOES_NOT_EXIST)"  location: "JS frame ::
> resource://gre/modules/Webapps.jsm :: applyDownload :: line 1032"  data:
> no]" {file: "resource://gre/modules/Webapps.jsm" line: 1032}]

This sounds like a symptom and consequence of bug 833587.

Comment 14

[Julien Wajsberg [:julienw]]

I have the patch for bug 833587 too.
And the one in bug 831644 also :)

It's one of these, I was betting more on this patch that is bigger but I'll let fabrice choose.

Comment 15

[Lukas Blakk [:lsblakk] use ?needinfo]

Comment on attachment 705048 [details] [diff] [review]
patch v2

According to comment 12 this causes regressions, please renominate when there's a patch ready that will not.

Comment 16

[[:fabrice] Fabrice Desré]

Attachment: patch v2

Fixes the issue when we apply a download that's not from an update but from a restarted download, but guarding against non-existent staged-update.webapp in the staging directory.

Comment 17

[Julien Wajsberg [:julienw]]

This fixes my testcase.

Comment 18

[[:fabrice] Fabrice Desré]

Comment on attachment 705921 [details] [diff] [review]
patch v2

[Approval Request Comment]
This is part of a set of patches that helps hardening against edge cases that may let the phone in a bad state. Risk is low compared to the reward.

Comment 19

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

Comment on attachment 705921 [details] [diff] [review]
patch v2

Review of attachment 705921 [details] [diff] [review]:
-----------------------------------------------------------------

a=me if we run this patch through Dale's tests from bug 826058

Comment 20

[[:fabrice] Fabrice Desré]

Tests are passing!

https://hg.mozilla.org/integration/mozilla-inbound/rev/be407e8f5093

Comment 21

[[:fabrice] Fabrice Desré]

https://hg.mozilla.org/releases/mozilla-b2g18/rev/96ee8f2afd10

Comment 22

[Julien Wajsberg [:julienw]]

I think that with the new landing rules, we should mark "resolved/fixed" only when it lands to master, and "status-b2g18:fixed" when it lands to mozilla-b2g18.

I don't change the "resolved fixed" here because it will hopefully migrate soon.

Comment 23

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/be407e8f5093