1261522 - Support reloading an add-on for about:debugging

Status: RESOLVED FIXED

(Toolkit :: Add-ons Manager, defect)

Description

[Kumar McMillan [:kumar]]

In bug 1246030 we'd like to provide a Reload button next to a temporarily installed add-on. We need some helpers in the add-on wrapper for this.

To keep it simple, the first iteration will only allow reloading restartless add-ons so we also need a helper for that.

Comment 1

[Kumar McMillan [:kumar]]

Attachment: MozReview Request: Bug 1261522 - Support reloading an add-on. r=Mossop

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

Comment 2

[Dave Townsend [:mossop]]

Comment on attachment 8737395 [details]
MozReview Request: Bug 1261522 - Support reloading an add-on. r=Mossop

https://reviewboard.mozilla.org/r/43945/#review40543

::: toolkit/mozapps/extensions/internal/XPIProvider.jsm:6993
(Diff revision 1)
>      return icons;
>    },
>  
> +  get isRestartless() {
> +    let addon = addonFor(this);
> +    return addon.bootstrap;

This isn't correct, there are various cases where a bootstrapped add-on isn't "restartless".

Besides we already expose this information to the rest of the world with the operationsRequiringRestart property so there is no need to add this.

::: toolkit/mozapps/extensions/internal/XPIProvider.jsm:7286
(Diff revision 1)
>  
> +  reload: function() {
> +    this.userDisabled = true;
> +    flushStartupCache();
> +    this.userDisabled = false;
> +  },

This needs to verify that the add-on isn't currently disabled and that it can be disabled and enabled without restart.

Please make this an async operation returning a promise, we are likely to need to do work to clear caches in the child process as a part of this.

Comment 3

[Kumar McMillan [:kumar]]

Comment on attachment 8737395 [details]
MozReview Request: Bug 1261522 - Support reloading an add-on. r=Mossop

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

Comment 4

[Kumar McMillan [:kumar]]

Thanks for the review! Here are some notes about what I changed:

- Since I still needed something internally to check if the add-on can be reloaded, I thought it would be ok to expose this for use in about:debugging. I could also make a function that mimics the check in about:debugging, let me know.
- The userDisabled setter already is a noop when disabled (https://dxr.mozilla.org/mozilla-central/source/toolkit/mozapps/extensions/internal/XPIProvider.jsm#7142) so I couldn't figure out how to test a check for this. Plus, it seems redundant. Should I add a test anyway for safety?

Comment 5

[Kumar McMillan [:kumar]]

Comment on attachment 8737395 [details]
MozReview Request: Bug 1261522 - Support reloading an add-on. r=Mossop

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

Comment 6

[Dave Townsend [:mossop]]

Comment on attachment 8737395 [details]
MozReview Request: Bug 1261522 - Support reloading an add-on. r=Mossop

https://reviewboard.mozilla.org/r/43945/#review41137

::: toolkit/mozapps/extensions/internal/XPIProvider.jsm:7281
(Diff revision 3)
>  
> +  get isReloadable() {
> +    let addon = addonFor(this);
> +    return (!XPIProvider.enableRequiresRestart(addon) &&
> +            !XPIProvider.disableRequiresRestart(addon));
> +  },

Yeah I'd much rather you just do this in about:debugging by checking the flags in addon.operationsRequiringRestart. The unfortunate fact is that there isn't a surefire way to say that a given add-on is restartless is all cases, we can only say whether a given operation can complete given the current state of the system. In fact checking whether it can be enabled without a restart can technically only be done once disabled, but we're pretty safe to say that if it can be disabled without a restart then it can be enabled without a restart immediately after. At least for now anyway!

I look forward to the day when XUL add-ons can die and all this complexity goes away :(

::: toolkit/mozapps/extensions/internal/XPIProvider.jsm:7285
(Diff revision 3)
> +            !XPIProvider.disableRequiresRestart(addon));
> +  },
> +
> +  reload: function() {
> +    return new Promise(resolve => {
> +      if (!this.isReloadable) {

So to my mind if an add-on is currently disabled then calling reload is an error, maybe that's mistaken though, what are your thoughts?

Even without that though we need to at least check that appDisabled is false or it won't possible to enable the add-on at all.

Comment 7

[Kumar McMillan [:kumar]]

Comment on attachment 8737395 [details]
MozReview Request: Bug 1261522 - Support reloading an add-on. r=Mossop

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

Comment 8

[Kumar McMillan [:kumar]]

https://reviewboard.mozilla.org/r/43945/#review41137

> So to my mind if an add-on is currently disabled then calling reload is an error, maybe that's mistaken though, what are your thoughts?
> 
> Even without that though we need to at least check that appDisabled is false or it won't possible to enable the add-on at all.

Personally, I don't think it would be an error to reload a user disabled add-on. There are no ill side effects. If we turn it into an error, the caller would need some hooks to flush the cache before enabling it which might be annoying (the calling code would have to be a lot more defensive too). Also, it seems nice to encapsulate the reload() behavior all in one place.

Comment 9

[Dave Townsend [:mossop]]

Comment on attachment 8737395 [details]
MozReview Request: Bug 1261522 - Support reloading an add-on. r=Mossop

https://reviewboard.mozilla.org/r/43945/#review41629

::: toolkit/mozapps/extensions/test/xpcshell/xpcshell.ini:29
(Diff revision 4)
>  [test_provider_markSafe.js]
>  [test_provider_shutdown.js]
>  [test_provider_unsafe_access_shutdown.js]
>  [test_provider_unsafe_access_startup.js]
>  [test_ProductAddonChecker.js]
> +[test_reload.js]

Please put this in xpcshell-shared.ini instead so we test both packed and unpacked cases. You'll find you will also need to add a newline to xpcshell.ini or treeherder won't spot the change.

Comment 10

[Kumar McMillan [:kumar]]

Comment on attachment 8737395 [details]
MozReview Request: Bug 1261522 - Support reloading an add-on. r=Mossop

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

Comment 11

[Kumar McMillan [:kumar]]

Comment on attachment 8737395 [details]
MozReview Request: Bug 1261522 - Support reloading an add-on. r=Mossop

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/43945/diff/5-6/

Comment 12

[Kumar McMillan [:kumar]]

I think these Try failures are unrelated but I am still trying to understand how to comprehend Try results. Let me know if I need to rebase or something to get a passing Try run.

Comment 13

[Pulsebot]

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

Comment 14

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

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

Comment 15

[Carsten Book [:Tomcat]]

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