1344674 - Error message for unsigned non-temporary file should clarify what's wrong

Status: RESOLVED INVALID

(Toolkit :: Add-ons Manager, defect)

Description

[Tooru Fujisawa [:arai]]

Steps to Reproduce:
  1. Follow this example
       https://developer.mozilla.org/en-US/Add-ons/WebExtensions/Your_first_WebExtension
  2. package it with "web-ext build"
  3. try to install it to Firefox developers edition with xpinstall.signatures.required==false

Actual result:
  Door hanger says
    undefined - this add-on couldn't be installed because it appears to be corrupt.
  and Browser console says
    addons.xpi	WARN Invalid XPI: Error: Cannot find id for addon {PATH}/web-ext-artifacts/borderify-1.0.xpi (resource://gre/modules/addons/XPIProvider.jsm:5571:19) JS Stack trace: loadManifest/<@XPIProvider.jsm:5571:19

Expected result:
  it should clarify unsigned non-temporary WebExtension is not supported,
  and tell the developer to sign it, or use it only from about:debugging

also, "undefined" title is too bad :P

Comment 1

[Tooru Fujisawa [:arai]]

To be clear, "undefined - " part is observed on developer edition 53.
release 51 doesn't have that field

Comment 2

[Tooru Fujisawa [:arai]]

Attachment: Clarify WebExtensions package should be singed in the error message for unsigned local package.

Added special path for WebExtensions when addon.id is not found.
(since WebExtensions already has special path in loadManifestFromZipReader, that developers cannot guess what's happening inside it)

that says the following message for local package:
  WebExtensions package should be signed.  Please sign this add-on, or install it temporarily.

for non-local, it says the same thing as unsigned addon:
  %1$S has prevented this site from installing an unverified add-on.

Comment 3

[Tooru Fujisawa [:arai]]

apparently the "undefined" is popup-notification-origin.
I'll file another bug for it

Comment 4

[Tooru Fujisawa [:arai]]

filed bug 1344693

Comment 5

[Andrew Swan [:aswan]]

I think the intention here is that only a developer is really in a position to do something about signing (i.e., get the extension signed or use temporary installation) and they are already encouraged to either use temporary installation and/or to check the browser console during development.  As a corollary, error messages displayed to users don't include details that are only or primarily meaningful to developers.
Markus, what do you think?

Comment 6

[Tooru Fujisawa [:arai]]

(In reply to Andrew Swan [:aswan] from comment #5)
> they are already encouraged to either use
> temporary installation and/or to check the browser console during
> development.

even in that case, saying "corrupt" in the popup is misleading.
also, "Cannot find id for addon" in browser console doesn't make any sense,
that's completely the internal details and developers have no idea what it means, since WebExtensions has no ID at all.

how about just saying "%1$S has prevented this site from installing an unverified add-on." for all case and displaying the detail (WebExtensions should be signed even with xpinstall.signatures.required==false) in browser console?

Comment 7

[Andrew Swan [:aswan]]

(In reply to Tooru Fujisawa [:arai] from comment #6)
> (In reply to Andrew Swan [:aswan] from comment #5)
> > they are already encouraged to either use
> > temporary installation and/or to check the browser console during
> > development.
> 
> even in that case, saying "corrupt" in the popup is misleading.

Okay, this message is generally about the scenario where we download the xpi file successfully (for network installs which are much more common than installs from the local file system) but there is something about the contents of the file that means we cannot install it.  To be clear, you're suggesting that the message should be clarified, not arguing for finer-grained error messages here right?

> also, "Cannot find id for addon" in browser console doesn't make any sense,
> that's completely the internal details and developers have no idea what it
> means, since WebExtensions has no ID at all.

That's not true, see: https://developer.mozilla.org/en-US/Add-ons/WebExtensions/Add-on_ID

> how about just saying "%1$S has prevented this site from installing an
> unverified add-on." for all case and displaying the detail (WebExtensions
> should be signed even with xpinstall.signatures.required==false) in browser
> console?

Because that's not at all accurate.

Comment 8

[Tooru Fujisawa [:arai]]

(In reply to Andrew Swan [:aswan] from comment #7)
> Okay, this message is generally about the scenario where we download the xpi
> file successfully (for network installs which are much more common than
> installs from the local file system) but there is something about the
> contents of the file that means we cannot install it.  To be clear, you're
> suggesting that the message should be clarified, not arguing for
> finer-grained error messages here right?

I meant, error message should provide the information about what actually goes wrong, and how to recover from the error, as much as possible.


> > also, "Cannot find id for addon" in browser console doesn't make any sense,
> > that's completely the internal details and developers have no idea what it
> > means, since WebExtensions has no ID at all.
> 
> That's not true, see:
> https://developer.mozilla.org/en-US/Add-ons/WebExtensions/Add-on_ID

sorry, I didn't know that page, and my comment was incorrect.

when I saw the following note in "Your first WebExtension" page, I thought it's completely legacy thing and removed.
> If you're using a Firefox version before 48, you'll also need an additional key called applications:

so, what's the actual supposed way to recover from this issue?
the case that developer follows the example, and pack it, and try to install, and failed.
as far as I saw, all examples have no ID.

if signing is the solution, the error should say "it's not signed",
if using temporary installation is the solution (I hope not), it should suggest it,
if adding an ID is really the solution, ID should be in the all examples, and any documentation shouldn't suggest omitting it without explaining the exceptional cases.

Comment 9

[Markus Jaritz [:markus] (UX)]

As Andrew already started we should separate 2 kinds of messaging here.
add-on-user facing messaging and addon-developer facing messaging

In both cases I agree that error messages should offer *a way to recover from the error*.

For add-on users saying the extension seams corrupt is enough info about the error - we should however help recover with stating they should re-attempt install and contact the developer if that fails.
(which does not improve the developer-case described here...)

For add-on developers it seams our console messages could be more detailed to help developers recover from errors. What to say there I don't know.

Comment 10

[Andrew Swan [:aswan]]

(In reply to Tooru Fujisawa [:arai] from comment #8)
> (In reply to Andrew Swan [:aswan] from comment #7)
> > Okay, this message is generally about the scenario where we download the xpi
> > file successfully (for network installs which are much more common than
> > installs from the local file system) but there is something about the
> > contents of the file that means we cannot install it.  To be clear, you're
> > suggesting that the message should be clarified, not arguing for
> > finer-grained error messages here right?
> 
> I meant, error message should provide the information about what actually
> goes wrong, and how to recover from the error, as much as possible.

But once again, the point here is that the error message is that "something in the contents of this xpi file have prevented it from being installed".  It was a conscious decision to keep the more specific errors confined to separate developer-focused workflows.

The developer-focused workflows that we recommend and document on MDN are using the web-ext command line tool and using temporary debugging.

Maybe you could explain why you're focused on the "bundle an XPI and install it via drag-and-drop" workflow instead?

Comment 11

[Tooru Fujisawa [:arai]]

(In reply to Andrew Swan [:aswan] from comment #10)
> But once again, the point here is that the error message is that "something
> in the contents of this xpi file have prevented it from being installed". 

Why should we say "something" while we know what it is?
I believe that hiding the information and providing useless error message is the reason why people don't read error message...


> It was a conscious decision to keep the more specific errors confined to
> separate developer-focused workflows.

Okay, then we should at least fix the error message in browser console,
to help developers transitioning to WebExtensions.

What the current information lacks is:
  * why it complains about missing ID while it didn't while temporary debugging
  * what ID is
  * when ID is required
  * how to solve this issue
    * what actual entry in manifest.json is missing
    * suggest signing (maybe?)
      * why signing is necessary while xpinstall.signatures.required is set to false


> The developer-focused workflows that we recommend and document on MDN are
> using the web-ext command line tool and using temporary debugging.
> 
> Maybe you could explain why you're focused on the "bundle an XPI and install
> it via drag-and-drop" workflow instead?

because I finished debugging and try to use it in main profile to see it really works in wild,
and installing a package is how I was doing until I try to port my extension to WebExtensions.

also, I did use "web-ext build" command to create the package, as described in the document, not "instead".

https://developer.mozilla.org/en-US/Add-ons/WebExtensions/Getting_started_with_web-ext#Packaging_your_extension
> 
>   web-ext build
> 
> This will output a full path to the generated .zip file that can be loaded into a browser.

isn't that true?
isn't this an expected workflow?


Then, please tell me whether ID is really required/expected or not.
I still don't understand what the expected thing is.

Comment 12

[Tooru Fujisawa [:arai]]

I added the following to manifest.json, and it still says "Cannot find id for addon":

>   "applications": {
>     "gecko": {
>       "id": "foo@localhost",
>       "strict_min_version": "52.0"
>     }
>   }

what's actually missing?

Comment 13

[Tooru Fujisawa [:arai]]

(In reply to Tooru Fujisawa [:arai] from comment #12)
> I added the following to manifest.json, and it still says "Cannot find id
> for addon":
> 
> >   "applications": {
> >     "gecko": {
> >       "id": "foo@localhost",
> >       "strict_min_version": "52.0"
> >     }
> >   }
> 
> what's actually missing?

looks like it was trying to install from cache, not the actual file I dropped :/

Comment 14

[Tooru Fujisawa [:arai]]

Comment on attachment 8843932 [details] [diff] [review]
Clarify WebExtensions package should be singed in the error message for unsigned local package.

I'll rephrase the message.

Comment 15

[Andrew Swan [:aswan]]

(In reply to Tooru Fujisawa [:arai] from comment #11)
> (In reply to Andrew Swan [:aswan] from comment #10)
> > But once again, the point here is that the error message is that "something
> > in the contents of this xpi file have prevented it from being installed". 
> 
> Why should we say "something" while we know what it is?
> I believe that hiding the information and providing useless error message is
> the reason why people don't read error message...

I think you can also argue the opposite: error messages that include specific information that is not comprehensible to most users is a big reason for not reading error messages.  In any case, this is an argument against the concept described in previous comments of keeping details that are only meaningful to developers out of the messages that non-developer add-on users see.  I think that decision was made with some careful thought so it would take some compelling new information to reverse that decision.

> Okay, then we should at least fix the error message in browser console,
> to help developers transitioning to WebExtensions.
> 
> What the current information lacks is:
>   * why it complains about missing ID while it didn't while temporary
> debugging
>   * what ID is
>   * when ID is required
>   * how to solve this issue
>     * what actual entry in manifest.json is missing
>     * suggest signing (maybe?)
>       * why signing is necessary while xpinstall.signatures.required is set
> to false

That's a lot to put into a browser console message but I think it is all covered on the MDN page I linked in comment 7.
I think adding a link to that page might be okay, but at the same time, there are other ways for people to arrive at that message than the particular problem you are having.

> also, I did use "web-ext build" command to create the package, as described
> in the document, not "instead".
> 
> [...]
> isn't this an expected workflow?

The usual flow is to use "web-ext run" during development, then "web-ext sign".

> Then, please tell me whether ID is really required/expected or not.
> I still don't understand what the expected thing is.

This is all explained on the MDN page, including the circumstances in which it is required and the format.

Comment 16

[Andy McKay]

I don't think we should be showing anything more to the end user. But I really like the idea of linking to MDN in the browser console messages, I think we should do that.

Comment 17

[Tooru Fujisawa [:arai]]

(In reply to Andrew Swan [:aswan] from comment #15)
> > Okay, then we should at least fix the error message in browser console,
> > to help developers transitioning to WebExtensions.
> > 
> > What the current information lacks is:
> >   * why it complains about missing ID while it didn't while temporary
> > debugging
> >   * what ID is
> >   * when ID is required
> >   * how to solve this issue
> >     * what actual entry in manifest.json is missing
> >     * suggest signing (maybe?)
> >       * why signing is necessary while xpinstall.signatures.required is set
> > to false
> 
> That's a lot to put into a browser console message but I think it is all
> covered on the MDN page I linked in comment 7.

I don't understand why this kind of error message needs to be short.
it's not user-facing one that you says shouldn't explain any details.


> I think adding a link to that page might be okay, but at the same time,
> there are other ways for people to arrive at that message than the
> particular problem you are having.

I cannot think of other ways that loadManifestFromZipReader doesn't fail and this.addon.id is missing for WebExtensions.
it's just because the package doesn't have corresponding entry for id and it's unsigned and it's not temporary.
if the package is really corrupted, the zip reader should fail.


> > also, I did use "web-ext build" command to create the package, as described
> > in the document, not "instead".
> > 
> > [...]
> > isn't this an expected workflow?
> 
> The usual flow is to use "web-ext run" during development, then "web-ext
> sign".

Okay, then "web-ext build" shouldn't say it's "ready" nor "can be loaded",
or, at least say so after verification for whether it's really ready, and explanation for why,
especially the difference between temporary installation and package.

currently documentation says it can be loaded:
  https://developer.mozilla.org/en-US/Add-ons/WebExtensions/Getting_started_with_web-ext#Packaging_your_extension

and "web-ext build" itself says "Your web extension is ready"


> > Then, please tell me whether ID is really required/expected or not.
> > I still don't understand what the expected thing is.
> 
> This is all explained on the MDN page, including the circumstances in which
> it is required and the format.

so, what the "Add-on ID" page says is correct, right?
  https://developer.mozilla.org/en-US/Add-ons/WebExtensions/Add-on_ID
(btw, that page is cropped at the middle of the sentence, and also the list of conditions there is not clear whether it's "AND" or "OR")


then, the "Your first WebExtension" page is wrong, and it should link to Add-on ID page.
  https://developer.mozilla.org/en-US/Add-ons/WebExtensions/Your_first_WebExtension
> If you're using a Firefox version before 48, you'll also need an additional key called applications:

Comment 18

[Tooru Fujisawa [:arai]]

Fixed "Your first WebExtension" page to say id is required in more cases and link to Add-on ID page.
  https://developer.mozilla.org/en-US/Add-ons/WebExtensions/Your_first_WebExtension$compare?locale=en-US&to=1215337&from=1213125

Comment 19

[Tooru Fujisawa [:arai]]

I don't understand this

https://developer.mozilla.org/en-US/Add-ons/WebExtensions/Add-on_ID
>  * you do not use the web-ext tool: instead, you test your add-on using the "Load Temporary Add-on" button in about:debugging

what does the former part actually mean?
isn't using web-ext suggested workflow?

Comment 20

[Tooru Fujisawa [:arai]]

I don't think the "Add-on ID" page actually explains the requirement, but just explains one case that doesn't need it.

is the following requirement correct? (if so, I'll update the document)
  * you are developing on Firefox 48 or later
  * you are going to sign the extension whenever you create a package
    (either with "web-ext sign" or AMO web interface maybe?)
  * you use "Load Temporary Add-on" button in about:debugging, or "web-ext run" whenever you debug the addon
  * you are going to use web interface on AMO whenever updating the add-on listed on AMO (I don't understand what this implies)

the 2nd one is especially different than current one, and also I don't understand the current 3rd one.

also, it would be better explaining when id is needed, with explicit examples:
  * you are developing on Firefox 47 or older
  * you create a package without signing
  * (maybe some more cases related to update?)

the 2nd one is hard to figure out from current documentation.

also, the page should also explain what actually happens with signed addon.
(as far as I can see from the code, it's assigning certificate's name)

Comment 21

[Tooru Fujisawa [:arai]]

Attachment: Clarify packed WebExtensions needs explicit id or signature in the error message for unsigned non-temporary file.

Changed to use AddonManager.ERROR_SIGNEDSTATE_REQUIRED, since it sounds like signing is expected workflow than adding id, and signing it surely fixes the issue.

then, in browser console it shows the following message:

> Cannot find the the applications.gecko.id property in manifest.json,
>  or the signature for addon ${file.path}.
>  Packaged WebExtensions requires either the explicit addon id or the signature.
>  For more details about the addon id, please refer
>  https://developer.mozilla.org/en-US/Add-ons/WebExtensions/Add-on_ID

I think it should be an error message instead of warning.
I filed bug 1345010 for it.

Comment 22

[Tooru Fujisawa [:arai]]

to be clear, looks like Add-on ID page now redirects to different page.
  https://developer.mozilla.org/en-US/Add-ons/WebExtensions/WebExtensions_and_the_Add-on_ID

Comment 23

[Tooru Fujisawa [:arai]]

filed https://github.com/mozilla/web-ext/issues/847 for web-ext tool

Comment 24

[Andrew Swan [:aswan]]

Comment on attachment 8844325 [details] [diff] [review]
Clarify packed WebExtensions needs explicit id or signature in the error message for unsigned non-temporary file.

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

A couple of things here
1. ERROR_SIGNEDSTATE_REQUIRED is not accurate here
2. Embedding a bunch of user-facing text in an exception message property is clumsy, lets just use Services.console to log an appropriate message in this case.
3. Bonus points for using the existing mechanism for "learn more" links in the console, though with a few minutes of experimenting I couldn't figure out how to make it work.  Folks in #devtools may be able to help or see bug 1261877

Comment 25

[Tooru Fujisawa [:arai]]

(In reply to Andrew Swan [:aswan] from comment #24)
> 1. ERROR_SIGNEDSTATE_REQUIRED is not accurate here

what's correct/expected one?
everyone says different things and I have no idea what to use...

Comment 26

[Andrew Swan [:aswan]]

The existing error code is correct

Comment 27

[Tooru Fujisawa [:arai]]

(In reply to Andrew Swan [:aswan] from comment #26)
> The existing error code is correct

then the popup should show detailed error message at least on Nightly and Developers Edition.

Comment 28

[Tooru Fujisawa [:arai]]

anyway, current error code is *not* correct.

Comment 29

[Tooru Fujisawa [:arai]]

how about this?

  this add-on couldn't be installed because it appears to be incompatible or corrupt.

at least it describes the situation a bit better,
and that provides the information that the issue can be not from packaging, but somewhere else.

Comment 30

[Andrew Swan [:aswan]]

(In reply to Tooru Fujisawa [:arai] from comment #27)
> then the popup should show detailed error message at least on Nightly and
> Developers Edition.

Eh, we've been over this already, this isn't productive.  I made a suggestion for what to do with the console message in comment 24.  If you really want to argue for changing the user-facing message, take it up with :maritz