1170844 - Display error message when add-on fails to be installed because it isn't signed

Status: VERIFIED FIXED

(Firefox for Android Graveyard :: Add-on Manager, defect)

Description

[:Margaret Leibovic]

Android version of bug 1149696 (implemented in bug 1038068).

Comment 1

[:Margaret Leibovic]

So... I'm having a tough time understanding how some of our current XPInstallObserver logic works (I think it doesn't). I was hoping to just add new error case strings like Mossop did in bug 1038068, but our current code that uses those strings only gets called in the onDownloadCancelled handler (which doesn't seem to make much sense):
http://mxr.mozilla.org/mozilla-central/source/mobile/android/chrome/content/browser.js#6317

Should this logic be in onInstallFailed instead? Should I replace that message with and "addon-install-failed" notification observer like desktop does?

I don't want to yak-shave too much here, but this seems pretty busted right now.

Comment 2

[Dave Townsend [:mossop]]

(In reply to :Margaret Leibovic from comment #1)
> So... I'm having a tough time understanding how some of our current
> XPInstallObserver logic works (I think it doesn't). I was hoping to just add
> new error case strings like Mossop did in bug 1038068, but our current code
> that uses those strings only gets called in the onDownloadCancelled handler
> (which doesn't seem to make much sense):
> http://mxr.mozilla.org/mozilla-central/source/mobile/android/chrome/content/
> browser.js#6317
> 
> Should this logic be in onInstallFailed instead? Should I replace that
> message with and "addon-install-failed" notification observer like desktop
> does?
> 
> I don't want to yak-shave too much here, but this seems pretty busted right
> now.

Surprisingly this is actually sane and it comes down to a difference between the raw add-ons manager API and the code that handles installing add-ons from webpages:

The raw API is pretty open, it will let you install any add-on that isn't corrupt even if it isn't compatible or blocklisted (though in those cases the API won't let the add-on become enabled).

The web installer code which uses the raw API underneath however filters out incompatible and blocked add-ons. It does this by waiting for the download to complete and then if they shouldn't be installed cancels them. So you get an onDownloadCancelled for those cases.

The case for add-ons that have broken signatures or are unsigned and signing requirements are enabled are slightly different. Not even the raw API will allow those to be installed so they never complete download. You'll pick up those cases in onDownloadFailed. install.error will be ERROR_CORRUPT_FILE for the broken case and ERROR_SIGNEDSTATE_REQUIRED for the missing signature case.

Comment 3

[:Margaret Leibovic]

Attachment: MozReview Request: Bug 1170844 - Display error message when add-on fails to be installed because it isn't signed. r=Mossop,mfinkle

Bug 1170844 - Display error message when add-on fails to be installed because it isn't signed. r=Mossop

Comment 4

[:Margaret Leibovic]

I decided to factor out this error message logic block into a helper function.

I'm not sure why we currently show a vague "Installation failed" message in the failure case. Is there a failure situation that wouldn't be handled properly by this logic block? If so, I could just restore that vague message as a fallback.

I wish we had automated tests for these on mobile... I'd like to be able to verify that this handles all the different cases we can encounter.

Also, on desktop, we use doorhanger notifications for these error message, and we show a "Learn more" link for the unsigned add-on case. Anthony, should we make the toast message into a button toast in this case? "Learn more" seems like too much to have on the right side of a toast, so we would need to come up with a short string to put there. Maybe "DETAILS"?

Comment 5

[:Margaret Leibovic]

Oh, wait... I just realized that the mock-up in bug 1170043 show a prompt for this case, as well as for the generic error case.

Anthony, should be switching this logic so that we always show a prompt instead of a toast? I'm all for improving our add-on install UI more generally, but I just don't want to scope-creep this signed add-ons project too much.

Comment 6

[Dave Townsend [:mossop]]

Comment on attachment 8616772 [details]
MozReview Request: Bug 1170844 - Display error message when add-on fails to be installed because it isn't signed. r=Mossop,mfinkle

https://reviewboard.mozilla.org/r/10469/#review9333

::: mobile/android/chrome/content/browser.js:6303
(Diff revision 1)
>      // Don't create a notification for distribution add-ons.
>      if (Distribution.pendingAddonInstalls.has(aInstall)) {
>        Cu.reportError("Error installing distribution add-on: " + aInstall.addon.id);
>        Distribution.pendingAddonInstalls.delete(aInstall);
>        return;
>      }

I'd suggest moving this block into _showErrorMessage

Comment 7

[Anthony Lam (:antlam) Find me, @antlam on Slack!]

If it's not too much trouble, I'd like to move it to a prompt dialog. 

The experience is more easily extensible if we move it to a dialog. It also carries the messaging much better (in terms of feeling like an important action) when compared to toasts. \

It's also more similar to the Desktop experience of using "panels".

Comment 8

[:Margaret Leibovic]

Comment on attachment 8616772 [details]
MozReview Request: Bug 1170844 - Display error message when add-on fails to be installed because it isn't signed. r=Mossop,mfinkle

Bug 1170844 - Display error message when add-on fails to be installed because it isn't signed. r=Mossop,mfinkle

Comment 9

[:Margaret Leibovic]

Attachment: screenshot blocked add-on

I decided to add the host name to the error message, since our UI is different than desktop's, so we don't have anywhere else where we're showing the host name. This is consistent with the other add-on install error messages we show.

Comment 10

[:Margaret Leibovic]

Attachment: screenshot generic error

I updated our error message logic to always show a prompt, instead of showing a toast. Here's what that looks like.

Comment 11

[:Margaret Leibovic]

I modeled the "Learn more" URL after what desktop does, using the base support url concatenated with "unsigned-addons". However, this leads to a 404 on mobile.

Joni, can you help make a SUMO page that we can point to for Android? It can probably have mostly the same content as the desktop page.

Comment 12

[Mark Finkle (:mfinkle) (use needinfo?)]

Comment on attachment 8616772 [details]
MozReview Request: Bug 1170844 - Display error message when add-on fails to be installed because it isn't signed. r=Mossop,mfinkle

https://reviewboard.mozilla.org/r/10469/#review9385

Ship It!

Comment 13

[Joni Chan]

Margaret, sure, I've set up a placeholder for the article. Please use this URL: https://support.mozilla.org/1/mobile/%VERSION%/%OS%/%LOCALE%/unsigned-addons (replacing the Version/OS/Locale tuplet)

We'll have content on that page in a day or two.

Comment 14

[:Margaret Leibovic]

(In reply to Joni Savage from comment #13)
> Margaret, sure, I've set up a placeholder for the article. Please use this
> URL:
> https://support.mozilla.org/1/mobile/%VERSION%/%OS%/%LOCALE%/unsigned-addons
> (replacing the Version/OS/Locale tuplet)
> 
> We'll have content on that page in a day or two.

Excellent! That's the URL I already have in my patch, and it looks like it's already working :)

Comment 15

[Pulsebot]

https://hg.mozilla.org/integration/fx-team/rev/0e0370946c50

Comment 16

[Roland Tanglao needinfo please :rolandtanglao, :mohnkuchen, :adobo, :sinigang, :roland]

margaret et al: is the warning in FF for Android in 40 or 41? And when will it become an error: in Android 41 or 42?

Comment 17

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/0e0370946c50

Comment 18

[:Margaret Leibovic]

(In reply to Roland Tanglao :rolandtanglao from comment #16)
> margaret et al: is the warning in FF for Android in 40 or 41? And when will
> it become an error: in Android 41 or 42?

All the functionality should be in place by Firefox 41, but I'm not sure if we've committed to turning this on by default in that release.

Comment 19

[u421692]

Verified as fixed on Firefox 41 Beta 1 with xpinstall.signatures.required pref set to true.
Currently xpinstall.signatures.required is set to false by default.