816448 - [App Install] Handle errors during app download & install

Status: VERIFIED FIXED

(Firefox OS Graveyard :: Gaia::System, defect, P3)

Description

[Ben Francis [:benfrancis]]

Currently all errors during app install display the same "[app name] download stopped" message. But there are lots of different ways a download or install could fail and we could do a better job of informing the user why it failed. At least to differentiate between a download failure and an install failure.

Josh suggests:

Generic catch-all: 
Banner: "[app name] download stopped" 

INSUFFICIENT_STORAGE: 
Prompt, per current spec

NETWORK_ERROR (dns, 404, etc.)
DOWNLOAD_ERROR (can't save the package)
Banner: "[app name] download failed."
Would be good to extend to "[app name] download failed. Connection lost." if we can fit it...

MISSING_MANIFEST (no manifest.webapp in the package)
INVALID_MANIFEST (bad json, no name, ...)
INSTALL_FROM_DENIED (install origin is not in the allow_install_from array)
INVALID_SECURITY_LEVEL (eg. you pretend to be certified while you're not)
INVALID_PACKAGE (generic error reading the zip)
Banner: "[app name] install failed"

"The lack of space within the Banner forces us to be very stingy with our wording, but at least the above provides more granular information about the _why_ of the failure."

Comment 1

[Dylan Oliver [:doliver]]

blocking-, would take a patch for this

Comment 2

[Julien Wajsberg [:julienw]]

Renominating because I think the part about differentiating between install errors and download errors could be BB+ because this is really confusing.

The complete customization is polish and we could address that later. However it seems simpler to address this now than doing only the first part, because this would allow us to add entries like "install.<name_of_error>" in locale and reference them very easily in the code. Plus, the sooner we add new localized string, the better.

Comment 3

[Jason Smith [:jsmith]]

I agree with Julien, disagree on the blocking- call here - app review team has expressed quite a lot of concern that need effective error specified to understand what's wrong with the app in order to know why they would reject an app for X reason.

Comment 4

[Lisa Brewster [:adora]]

The app review team doesn't necessarily need this info in a pretty UI, we just need to know how to figure out what the problem is so we can tell the developer what to fix.  Getting it from a log file would be fine.

Comment 5

[Jason Smith [:jsmith]]

(In reply to Lisa Brewster [:adora] from comment #4)
> The app review team doesn't necessarily need this info in a pretty UI, we
> just need to know how to figure out what the problem is so we can tell the
> developer what to fix.  Getting it from a log file would be fine.

Oh well in that case, why wouldn't getting it from adb logcat work then?

Comment 6

[Julien Wajsberg [:julienw]]

I was more concerned by the current generic message which is : "[app name] download stopped". This message would happen even if the download has not started yet, as soon as the user taps "install". This seems confusing at best.

Comment 7

[Donovan Preston [:fzzzy]]

Since this affects developers trying to write apps, I think it's a pretty high priority. The current behavior is not good at all.

I just checked, there is no info at all in adb logcat about what is wrong when trying to install the dictionary.com app. It simply does not install and flashes the "download stopped" message briefly.

Comment 8

[David Scravaglieri [:scravag]]

No direct user impact: blocking-, would take a patch for this

Comment 9

[Jason Smith [:jsmith]]

(In reply to dscravaglieri from comment #8)
> No direct user impact: blocking-, would take a patch for this

See comment 7. We have developers building apps - so the developer workflow can't suck. Try again.

Comment 10

[Donovan Preston [:fzzzy]]

The direct user impact is that if developers can't figure out how to fix problems with loading apps on the phone, users won't have any apps.

Comment 11

[David Scravaglieri [:scravag]]

Does the user is aware about problem storage (or others) while app download & install ? With a clear UI ?

Comment 12

[Julien Wajsberg [:julienw]]

(In reply to dscravaglieri from comment #11)
> Does the user is aware about problem storage (or others) while app download
> & install ? With a clear UI ?

When the app is a packaged app with size that is specified in the manifest, then the platform will check for available space and display a clear error to the user. In all other cases we'll have a generic error in the banner.

Comment 13

[Ben Francis [:benfrancis]]

As Julien said.

We will always show an error if there is an error, but sometimes it may be inaccurate, like saying "download stopped" when the download hasn't even started or "download stopped" when really the problem was an invalid manifest.

Comment 14

[Jason Smith [:jsmith]]

Let's back up and ask this question:

What do we think acceptable for v1 errors-wise?

I think that's the question that'll determine what, if anything, blocks. That's probably what's spurring the disagreement here. Can we narrow down the v1 piece?

Comment 15

[Ben Francis [:benfrancis]]

We just need to keep the current "[app name] download stopped" message and add two more specific ones

* [app name] download failed.
* [app name] install failed.

as proposed by Josh.

It's just a switch statement and let's users/developers differentiate between a download failure and an installation failure.

Comment 16

[David Scravaglieri [:scravag]]

Julien, are there some clear feedback for the user ?

Comment 17

[Alex Keybl [:akeybl]]

Mass Modify: All un-milestoned, unresolved blocking-basecamp+ bugs are being moved into the C3 milestone. Note that the target milestone does not mean that these bugs can't be resolved prior to 12/10, rather C2 bugs should be prioritized ahead of C3 bugs.

Comment 18

[Julien Wajsberg [:julienw]]

I found APP_CACHE_DOWNLOAD_ERROR a well.

I put the same error message than NETWORK_ERROR as it seems to be the same thing but for hosted apps. I'm wondering if they shouldn't be merged somehow but I don't really mind.

Comment 19

[Julien Wajsberg [:julienw]]

Attachment: patch v1

(apply with |git am|)

Comment 20

[Etienne Segonzac (:etienne)]

Comment on attachment 690447 [details] [diff] [review]
patch v1

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

r-, you test suite is not webscale! :)

More seriously the app install manager test file needs to be kept in control.
My recommendation would be to test the changes from this patch more atomically and then "continuously-improve" our way out of having recursions in the test file.

::: apps/system/js/app_install_manager.js
@@ +166,5 @@
> +      'INSTALL_FROM_DENIED': 1,
> +      'INVALID_SECURITY_LEVEL': 1,
> +      'INVALID_PACKAGE': 1,
> +      'APP_CACHE_DOWNLOAD_ERROR': 1
> +    };

Why not an array?

::: apps/system/test/unit/app_install_manager_test.js
@@ +503,5 @@
> +            assert.equal(fakeNotif.childElementCount, 0);
> +          });
> +
> +          beforeFirstProgressSuite();
> +          downloadEventsSuite(/*afterError*/ true);

Yep, I'm pretty sure this is where we taking things too far...

Comment 21

[Julien Wajsberg [:julienw]]

Attachment: patch v2

Comment 22

[Julien Wajsberg [:julienw]]

Comment on attachment 690447 [details] [diff] [review]
patch v1

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

::: apps/system/js/app_install_manager.js
@@ +166,5 @@
> +      'INSTALL_FROM_DENIED': 1,
> +      'INVALID_SECURITY_LEVEL': 1,
> +      'INVALID_PACKAGE': 1,
> +      'APP_CACHE_DOWNLOAD_ERROR': 1
> +    };

I want to know if an error code is one of these, I think this is the correct structure for this.

However I create it only once in |init| in my new patch.

Comment 23

[Axel Hecht [:Pike]]

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

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

We're string frozen, and adding a host of the same strings sounds like a bad idea.

::: apps/system/locales/system.en-US.properties
@@ +73,5 @@
> +download-stopped-missing-manifest={{ appName }} install failed
> +download-stopped-invalid-manifest={{ appName }} install failed
> +download-stopped-install-from-denied={{ appName }} install failed
> +download-stopped-invalid-security-level={{ appName }} install failed
> +download-stopped-invalid-package={{ appName }} install failed

I don't see a reason to expose localizers to the details here.

Comment 24

[Julien Wajsberg [:julienw]]

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

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

::: apps/system/locales/system.en-US.properties
@@ +73,5 @@
> +download-stopped-missing-manifest={{ appName }} install failed
> +download-stopped-invalid-manifest={{ appName }} install failed
> +download-stopped-install-from-denied={{ appName }} install failed
> +download-stopped-invalid-security-level={{ appName }} install failed
> +download-stopped-invalid-package={{ appName }} install failed

I thought it was a good idea because we could change this easily later without having to change the code.

Would it be ok to have 2 strings :
download-failed-error={{ appName }} download failed
install-failed-error={{ appName }} install failed

and all other strings referencing these ones, with a comment saying to not translate them, eg:
# please don't translate all other download-stopped-* properties
download-stopped-network-error={{ download-failed-error }}

(I'll have to check this would work though)

Comment 25

[Axel Hecht [:Pike]]

(In reply to Julien Wajsberg [:julienw] from comment #24)
> Comment on attachment 691390 [details] [diff] [review]
> patch v2
> 
> Review of attachment 691390 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: apps/system/locales/system.en-US.properties
> @@ +73,5 @@
> > +download-stopped-missing-manifest={{ appName }} install failed
> > +download-stopped-invalid-manifest={{ appName }} install failed
> > +download-stopped-install-from-denied={{ appName }} install failed
> > +download-stopped-invalid-security-level={{ appName }} install failed
> > +download-stopped-invalid-package={{ appName }} install failed
> 
> I thought it was a good idea because we could change this easily later
> without having to change the code.

That'd be breaking the l10n story anyway, if you change the string, you need to change the key, and thus the code. Otherwise, localizers won't get signaled that they actually need to do something.

> Would it be ok to have 2 strings :
> download-failed-error={{ appName }} download failed
> install-failed-error={{ appName }} install failed
> 
> and all other strings referencing these ones, with a comment saying to not
> translate them, eg:
> # please don't translate all other download-stopped-* properties
> download-stopped-network-error={{ download-failed-error }}
> 
> (I'll have to check this would work though)

Please put the redirect into your code itself. Generally it's a good idea to write code once, and not N times.

Also see my comment above about how you won't be able to tweak the text for an existing message and get localizers to pay attention to that without creating a new key.

Comment 26

[Julien Wajsberg [:julienw]]

Attachment: patch v3

implemented pike's follow-up

Comment 27

[Axel Hecht [:Pike]]

Comment on attachment 691756 [details] [diff] [review]
patch v3

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

Thanks, this looks good.

Comment 28

[Etienne Segonzac (:etienne)]

Comment on attachment 691756 [details] [diff] [review]
patch v3

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

r=me, let's discuss the test comment over thaï food :)

::: apps/system/test/unit/app_install_manager_test.js
@@ +512,5 @@
> +            setup(function() {
> +              mockApp.mTriggerDownloadError(errorName);
> +            });
> +
> +            downloadErrorTests(errorName);

I know tests will be fast soon, but I see really little value in testing the icon removal and the notification removal for *each* knownError.

It does not align well with the goal of keeping the test more unit-like, it's more like fuzz-testing :)

Comment 29

[Julien Wajsberg [:julienw]]

https://github.com/mozilla-b2g/gaia/commit/8f84252b59c306e8d843a1012e3345ffbf6e36ea with etienne's comment addressed

Comment 30

[Julien Wajsberg [:julienw]]

ah I didn't really address it completely because I misread it, I still test the icon/notification removal for each errors, but I did this for the whole suites that I commented before. Except that I uncommented then (it works correctly fast when they're executing only once) and left the comment above :/

Should we remove the comment now or wait for a subsequent bug in the system app ?

Comment 31

[Etienne Segonzac (:etienne)]

:/

Comment 32

[Julien Wajsberg [:julienw]]

follow-up : https://github.com/mozilla-b2g/gaia/commit/4568b2136b91e84cca0922837972874f38540dae

sorry about that, I commited to fast. That's why final review are not bad ;)

Comment 33

[Julien Wajsberg [:julienw]]

Attachment: final patch

final patch that was committed

Comment 34

[Julien Wajsberg [:julienw]]

Attachment: follow-up

follow-up to address the latest bits in the tests

Comment 35

[Jason Smith [:jsmith]]

Verified through a bunch of tests with invalid errors such as invalid appcache manifest, manifest mismatch, invalid security level, etc. Very useful for debugging too.