Closed Bug 1261522 Opened 4 years ago Closed 4 years ago

Support reloading an add-on for about:debugging

Categories

(Toolkit :: Add-ons Manager, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla48
Tracking Status
firefox48 --- fixed

People

(Reporter: kumar, Assigned: kumar)

References

Details

Attachments

(1 file)

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.
Assignee: nobody → kumar.mcmillan
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.
Attachment #8737395 - Flags: review?(dtownsend)
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/
Attachment #8737395 - Flags: review?(dtownsend)
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 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 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.
Attachment #8737395 - Flags: review?(dtownsend)
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/
Attachment #8737395 - Flags: review?(dtownsend)
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 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.
Attachment #8737395 - Flags: review?(dtownsend) → review+
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 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/
Attachment #8737395 - Attachment description: MozReview Request: Bug 1261522 - Support reloading an add-on. r?Mossop → MozReview Request: Bug 1261522 - Support reloading an add-on. r=Mossop
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.
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/b98d8b88dfc7
Status: NEW → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla48
You need to log in before you can comment on or make changes to this bug.