1231128 - about:debugging temporary file addon load does not catch errors

Status: RESOLVED FIXED

(DevTools :: about:debugging, defect, P2)

Description

[Robert Helmer [:rhelmer]]

There is a try/catch around AddonManager.installTemporaryAddon() but this does not throw it returns a promise.

Comment 1

[Robert Helmer [:rhelmer]]

Attachment: MozReview Request: Bug 1231128 - catch errors from about:debugging temporary addon install r?ochameau

Bug 1231128 - catch errors from about:debugging temporary addon install r?ochameau

Comment 2

[Robert Helmer [:rhelmer]]

Comment on attachment 8696683 [details]
MozReview Request: Bug 1231128 - catch errors from about:debugging temporary addon install r?ochameau

Actually let me add a test for this too.

Comment 3

[Robert Helmer [:rhelmer]]

Comment on attachment 8696683 [details]
MozReview Request: Bug 1231128 - catch errors from about:debugging temporary addon install r?ochameau

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/27403/diff/1-2/

Comment 4

[Alexandre Poirot [:ochameau]]

Comment on attachment 8696683 [details]
MozReview Request: Bug 1231128 - catch errors from about:debugging temporary addon install r?ochameau

https://reviewboard.mozilla.org/r/27403/#review24941

::: devtools/client/aboutdebugging/aboutdebugging.js:136
(Diff revision 2)
> +      document.getElementById("addons").appendChild(p);

The UX doesn't work there.
The error message is displayed after the addon list, which can be long. So that the error message would be hidden by the overflow.
Also, errors never go away unless you close about:debugging.

- alert() sucks, but at least don't have these issues
- if you want to improve error message by displaying it in the page, you would need some more work:
  - reset the error message on each new install
  - display it somewhere around the install button (at least before the addon list)
  - add some CSS to make it more visible

Comment 5

[Robert Helmer [:rhelmer]]

(In reply to Alexandre Poirot [:ochameau] from comment #4)
> Comment on attachment 8696683 [details]
> MozReview Request: Bug 1231128 - catch errors from about:debugging temporary
> addon install r?ochameau
> 
> https://reviewboard.mozilla.org/r/27403/#review24941
> 
> ::: devtools/client/aboutdebugging/aboutdebugging.js:136
> (Diff revision 2)
> > +      document.getElementById("addons").appendChild(p);
> 
> The UX doesn't work there.
> The error message is displayed after the addon list, which can be long. So
> that the error message would be hidden by the overflow.
> Also, errors never go away unless you close about:debugging.
> 
> - alert() sucks, but at least don't have these issues
> - if you want to improve error message by displaying it in the page, you
> would need some more work:
>   - reset the error message on each new install
>   - display it somewhere around the install button (at least before the
> addon list)
>   - add some CSS to make it more visible


Thanks! The reason I removed the alert() is because I can't figure out a great way to test it, I am happy to leave it if you can think of one?

Otherwise I will do what you suggest.

Comment 6

[Alexandre Poirot [:ochameau]]

The only thing I can think of would be overloading window.alert with a test mock...
If you are up for doing a nice html error message, I think it will be better.

Comment 7

[Robert Helmer [:rhelmer]]

(In reply to Alexandre Poirot [:ochameau] from comment #6)
> The only thing I can think of would be overloading window.alert with a test
> mock...
> If you are up for doing a nice html error message, I think it will be better.

OK will do, thanks!

Comment 8

[Andrew Swan [:aswan]]

Attachment: MozReview Request: Bug 1231128 Display errors when temporary add-on install fails

Review commit: https://reviewboard.mozilla.org/r/43807/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/43807/

Comment 9

[Andrew Swan [:aswan]]

Hi, I started working on this before noticing this bug.  I think I've addressed the suggestions from comment 4 but I'll be the first to acknowledge my lack of design skill, I'm happy to take feedback on the appearance (or of course any other aspect of the patch).

Comment 10

[Alexandre Poirot [:ochameau]]

Comment on attachment 8737216 [details]
MozReview Request: Bug 1231128 Display errors when temporary add-on install fails

https://reviewboard.mozilla.org/r/43807/#review40681

A few tweaks are necessary to have a nice looking UI, but the overall behavior is great.
The message is cleared correctly and appears in what looks like a nice spot to me.

It would be fantastic to followup in order to have exception happening during bootstrap.js evaluation or startup()/install() calls,
but it doesn't seem to be rejecting the promise your are hooking.
Instead it just seems to be logged in this "logger" object:
http://mxr.mozilla.org/mozilla-central/source/toolkit/mozapps/extensions/internal/XPIProvider.jsm#4686
Which is prints to stdout and/or the Browser console if extensions.logging.enabled pref is true.
I don't see how to easily fetch that from about:debugging and the call AddonManager.installTemporaryAddon.

::: devtools/client/aboutdebugging/components/addons-install-error.js:19
(Diff revision 1)
> +    if (!this.props.error) {
> +      return false;
> +    }
> +    let text = `There was an error during installation: ${this.props.error}`;
> +    return dom.div({ className: "addons-install-error" },
> +                   dom.img({ src: "chrome://mozapps/skin/extensions/alerticon-error.svg"}),

Could you instead use an icon from /devtools/?

We already use similar warning icon in other panels.
You can do something like this:
  dom.div({ className: "warning" })
  
and in aboutdebugging.css:

.addons-install-error .warning {
  background-image: url(chrome://devtools/skin/images/alerticon-warning.png);
  background-size: 13px 12px;
  -moz-margin-start: 5px;
  display: inline-block;
  width: 13px;
  height: 12px;
}

@media (min-resolution: 1.1dppx) {
  .addons-install-error .warning {
    background-image: url(chrome://devtools/skin/images/[e-mail]);
  }
}

Also ensure that there is enough padding in the red box, while also having a margin around the icon.
In your patch, the warning icon doesn't have any margin with the text, which looks weird.

::: devtools/client/aboutdebugging/test/browser_addons_install.js:41
(Diff revision 1)
> +  let promise = waitForMutation(top, { childList: true });
> +
> +  // This returns a promise but we don't want to yield it since it
> +  // resolves when it sees an event that happens on successful install,
> +  // but of course that won't happen with this bad addon.
> +  installAddon(document, "addons/bad/manifest.json", "test-devtools");

I don't think we want to keep a staling promise either. So it would be better to tweak installAddon by either:
* splitting it in two one very simple that just install the addon and another one which reuse the first to install and wait for update,
* or, accept a parameter to optionally wait,
* copy installAddon content here and tweak it for this very specific case. May be the simpliest.

Comment 11

[Alexandre Poirot [:ochameau]]

Attachment: Screenshot

Hi Helen,

I don't know if you follow about:debugging much, but if you have some time to feedback this change feel free to suggest any tweak.

Here is a screenshot of Andrew first patch.
It allows to see exception happening during addon installation.
It is automatically cleared on new install attempt, reload or panel change.
I suggested Andrew to add a bit of padding for the red box
and margin around the icon, while also reusing our existing warning icon:
http://mxr.mozilla.org/mozilla-central/source/devtools/client/themes/images/alerticon-warning.png

Comment 12

[Andrew Swan [:aswan]]

https://reviewboard.mozilla.org/r/43807/#review40681

I'll push a new version addressing the comments in a moment (and thanks for the css help, it would have taken me a ridculously long time to do that myself).
I've been using this for web extensions which are not supposed to be able to fail in startup() or install().  But you make a good point for existing addons, I'll file a separate follow-up issue for that...

Comment 13

[Andrew Swan [:aswan]]

Comment on attachment 8737216 [details]
MozReview Request: Bug 1231128 Display errors when temporary add-on install fails

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/43807/diff/1-2/

Comment 14

[Alexandre Poirot [:ochameau]]

Comment on attachment 8737216 [details]
MozReview Request: Bug 1231128 Display errors when temporary add-on install fails

https://reviewboard.mozilla.org/r/43807/#review40951

Thanks a lot. Did you pushed your patch to try?

::: devtools/client/aboutdebugging/aboutdebugging.css:87
(Diff revision 2)
>  
> +.addons-install-error {
> +  background-color: #f3b0b0;
> +  padding: 2px 10px 2px 10px;
> +  margin: 5px 0px 5px 0px;
> +}

I would put slightly more padding, and use the short padding definition:
  padding: 5px 10px;
  margin: 5px 4px 5px 0px;
  
Using only two values means vertical [=top,bottom] and horizontal[=left,right].
I put a 4px right margin to match the right alignment of other items like "Load temporary addon" and <hr> line under Add-ons title.

::: devtools/client/aboutdebugging/aboutdebugging.css:92
(Diff revision 2)
> +}
> +
> +.addons-install-error .warning {
> +  background-image: url(chrome://devtools/skin/images/alerticon-warning.png);
> +  background-size: 13px 12px;
> +  margin: 0px 5px 0px 5px;

And then just a right margin for the icon:
  margin-right: 10px;

::: devtools/client/aboutdebugging/components/addons-controls.js:60
(Diff revision 2)
>            ")"
>          ),
>          dom.button({
>            id: "load-addon-from-file",
>            onClick: this.loadAddonFromFile,
> -        }, Strings.GetStringFromName("loadTemporaryAddon"))
> +        }, Strings.GetStringFromName("loadTemporaryAddon"))),

It may be easier to read with following indentation and comment:
return dom.div({ className: "addons-top" },
  dom.div({ className: "addons-controls" },
    ...
    }, Strings.GetStringFromName("loadTemporaryAddon"))
  ), // end of addons-options
  AddonsInstallError({ error: this.state.installError })
);

::: devtools/client/aboutdebugging/components/addons-install-error.js:15
(Diff revision 2)
> +module.exports = createClass({
> +  displayName: "AddonsInstallError",
> +
> +  render() {
> +    if (!this.props.error) {
> +      return false;

I'm not a React expert, but shouldn't we just do  `return;` or `return null;` ?

Comment 15

[Alexandre Poirot [:ochameau]]

Attachment: Screenshot

Here is how it should look like once comments addressed.
Helen, please do not hesitate to suggest tweaks once it reach nightly.

Comment 16

[Alexandre Poirot [:ochameau]]

(In reply to Andrew Swan [:aswan] from comment #12)
> https://reviewboard.mozilla.org/r/43807/#review40681
> 
> I'll push a new version addressing the comments in a moment (and thanks for
> the css help, it would have taken me a ridculously long time to do that
> myself).
> I've been using this for web extensions which are not supposed to be able to
> fail in startup() or install().  

Oh that's true. Do we get errors happening during background page load in the Addon console?
If not, may be bug 1005193 is going to fix that. If not we should followup.

> But you make a good point for existing
> addons, I'll file a separate follow-up issue for that...

It doesn't sound that important given that we would like to promote WebExtensions first.

Comment 17

[Andy McKay]

(In reply to Alexandre Poirot [:ochameau] from comment #16)
> > But you make a good point for existing
> > addons, I'll file a separate follow-up issue for that...
> 
> It doesn't sound that important given that we would like to promote
> WebExtensions first.

Maybe make that a seperate bug so it can be picked up by someone else.

Comment 18

[Andrew Swan [:aswan]]

https://reviewboard.mozilla.org/r/43807/#review40951

> I'm not a React expert, but shouldn't we just do  `return;` or `return null;` ?

Hm, my react experience is pretty out of date.  I remember being surprised in the past when being told to use false, but looking at current docs, it seems null and false (but not undefined?) both achieve the desired effect:
http://facebook.github.io/react/docs/component-specs.html#render

Comment 19

[Andrew Swan [:aswan]]

(In reply to Alexandre Poirot [:ochameau] from comment #16)
> (In reply to Andrew Swan [:aswan] from comment #12)
> > I've been using this for web extensions which are not supposed to be able to
> > fail in startup() or install().  
> 
> Oh that's true. Do we get errors happening during background page load in
> the Addon console?
> If not, may be bug 1005193 is going to fix that. If not we should followup.

Well, I think that getting them in the console is the desired behavior, but it doesn't work right now.  See bug 1006102.

Comment 20

[Andrew Swan [:aswan]]

Comment on attachment 8737216 [details]
MozReview Request: Bug 1231128 Display errors when temporary add-on install fails

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/43807/diff/2-3/

Comment 21

[Andrew Swan [:aswan]]

Try run is here: https://treeherder.mozilla.org/#/jobs?repo=try&revision=b19d402f444d
As I seem to often do, I botched the try syntax so some unneeded tests are included..

I had hoped to have this marked for checkin today but the 32-bit windows tests have been queued on try for the last 7+ hours.

All the feedback should be addressed though if you care to look again Alexandre.  Thanks for all the feedback and help along the way!

Comment 22

[Alexandre Poirot [:ochameau]]

https://reviewboard.mozilla.org/r/43807/#review41229

::: devtools/client/aboutdebugging/test/browser_addons_install.js:36
(Diff revision 3)
> +
> +add_task(function* test_error() {
> +  let { tab, document } = yield openAboutDebugging("addons");
> +
> +  // Start an observer that looks for the install error before
> +  // actually doing th install

s/th/the/

Comment 23

[Alexandre Poirot [:ochameau]]

(In reply to Andrew Swan [:aswan] from comment #19)
> > Oh that's true. Do we get errors happening during background page load in
> > the Addon console?
> > If not, may be bug 1005193 is going to fix that. If not we should followup.
> 
> Well, I think that getting them in the console is the desired behavior, but
> it doesn't work right now.  See bug 1006102.

+1 there is no need to display them in about:debugging. The important point is about actually displaying them somewhere!

(In reply to Andrew Swan [:aswan] from comment #21)
> Try run is here:
> https://treeherder.mozilla.org/#/jobs?repo=try&revision=b19d402f444d

It looks like you have eslint error, please run ./mach eslint devtools/ and fix all errors. (Note that we have a bunch of warnings, ignore them, they are not making try to fail)

Comment 24

[Andrew Swan [:aswan]]

Comment on attachment 8737216 [details]
MozReview Request: Bug 1231128 Display errors when temporary add-on install fails

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/43807/diff/3-4/

Comment 25

[Andrew Swan [:aswan]]

Alright, lint is repaired: https://treeherder.mozilla.org/#/jobs?repo=try&revision=ad3a309176ef
Also fixed th typo, I think this is good to go.

Comment 26

[Pulsebot]

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

Comment 27

[Carsten Book [:Tomcat]]

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