Closed Bug 1170844 Opened 6 years ago Closed 6 years ago

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

Categories

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

35 Branch
defect
Not set
normal

Tracking

(firefox41 verified)

VERIFIED FIXED
Firefox 41
Tracking Status
firefox41 --- verified

People

(Reporter: Margaret, Assigned: Margaret)

References

Details

Attachments

(3 files)

Android version of bug 1149696 (implemented in bug 1038068).
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.
Flags: needinfo?(dtownsend)
(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.
Flags: needinfo?(dtownsend)
Bug 1170844 - Display error message when add-on fails to be installed because it isn't signed. r=Mossop
Attachment #8616772 - Flags: review?(dtownsend)
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"?
Flags: needinfo?(alam)
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 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
Attachment #8616772 - Flags: review?(dtownsend) → review+
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".
Flags: needinfo?(alam)
Attachment #8616772 - Attachment description: MozReview Request: Bug 1170844 - Display error message when add-on fails to be installed because it isn't signed. r=Mossop → MozReview Request: Bug 1170844 - Display error message when add-on fails to be installed because it isn't signed. r=Mossop,mfinkle
Attachment #8616772 - Flags: review+ → review?(mark.finkle)
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
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.
I updated our error message logic to always show a prompt, instead of showing a toast. Here's what that looks like.
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.
Flags: needinfo?(jsavage)
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!
Attachment #8616772 - Flags: review?(mark.finkle) → review+
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.
Flags: needinfo?(jsavage)
(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 :)
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?
Flags: needinfo?(margaret.leibovic)
https://hg.mozilla.org/mozilla-central/rev/0e0370946c50
Status: NEW → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 41
(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.
Flags: needinfo?(margaret.leibovic)
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.
Status: RESOLVED → VERIFIED
Product: Firefox for Android → Firefox for Android Graveyard
You need to log in before you can comment on or make changes to this bug.