Closed Bug 1461062 Opened 6 years ago Closed 6 years ago

Make bootstrap scope callbacks and lifetime management marginally maintainable

Categories

(Toolkit :: Add-ons Manager, enhancement)

Product:
Toolkit
Component:
Add-ons Manager
Type:
enhancement
Priority:
Not set
Severity:
normal

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla62
Tracking Flags:
Tracking Status
firefox62 --- fixed

People

(Reporter: kmag, Assigned: kmag)

References

Details

Attachments

(2 files)

Bug 1461062: Refactor bootstrap lifecycle management to be somewhat maintainable.
59 bytes, text/x-review-board-request
aswan
: review+
Details
Bug 1461062: Part 2 - Remove dead code.
59 bytes, text/x-review-board-request
jdescottes
: review+
rpl
: review+
Details 
Our handling of bootstrap scopes is currently a rats nest of duplicated logic spewed across dozens of classes and files. The logic for handling bootstrap life cycles should be consolidated in a single place, so we have some faint hope of being able to reason about it.
Comment on attachment 8975224 [details]
Bug 1461062: Refactor bootstrap lifecycle management to be somewhat maintainable.

https://reviewboard.mozilla.org/r/243572/#review249734

::: toolkit/mozapps/extensions/internal/XPIDatabase.jsm:1983
(Diff revision 2)
>      }
>  
>      if (aAddon.dependencies.length) {
>        let isActive = id => {
>          let active = XPIProvider.activeAddons.get(id);
> -        return active && !active.disable;
> +        return active && !active._disable;

can we name this something more descriptive?  pendingDisable or something?

::: toolkit/mozapps/extensions/internal/XPIDatabase.jsm:2938
(Diff revision 2)
>              !previousAddon._sourceBundle.equals(currentAddon._sourceBundle)) {
> -          this.callBootstrapUninstall(previousAddon, installReason,
> -                                      { newVersion: currentAddon.version });
> +          XPIInternal.BootstrapScope.get(previousAddon).update(
> +            currentAddon);
> +        } else {
> +          let reason = XPIInstall.newVersionReason(previousAddon.version, currentAddon.version);
> +          XPIInternal.BootstrapScope.get(currentAddon).install(

shouldn't we be using update() here?

::: toolkit/mozapps/extensions/internal/XPIInstall.jsm:3520
(Diff revision 2)
>      addon.importMetadata(metadata);
>  
> -    var oldBootstrap = null;
>      logger.debug(`Processing install of ${id} in ${location.name}`);
>      let existingAddon = XPIStates.findAddon(id);
> -    if (existingAddon && existingAddon.bootstrapped) {
> +    if (existingAddon) {

Should this be in bug 1461045?

::: toolkit/mozapps/extensions/internal/XPIProvider.jsm:1262
(Diff revision 2)
> + *
> + * @param {Object} addon
> + *        The add-on which owns this scope. Should be either an
> + *        AddonInternal or XPIState object.
> + */
> +class BootstrapScope {

Naming bikeshed: The fact that instances of this class are often called `scope` but it also has a property called `scope` that is often an instance of a different class also called `BootstrapScope` is confusing.
How about calling this something like BootstrapScopeWrapper?  If we do that, I think I can live with `scope.scope`

::: toolkit/mozapps/extensions/internal/XPIProvider.jsm:1422
(Diff revision 2)
> +  }
> +
> +  /**
> +   * Loads a bootstrapped add-on's bootstrap.js into a sandbox and the reason
> +   * values as constants in the scope. This will also add information about the
> +   * add-on to the bootstrappedAddons dictionary and notify the crash reporter

Remove the bit about "the bootstrappedAddons dictionary"

::: toolkit/mozapps/extensions/internal/XPIProvider.jsm:1629
(Diff revision 2)
> +   *        the old add-on and installing the new one. This callback
> +   *        should update any database state which is necessary for the
> +   *        startup of the new add-on.
> +   */
> +  update(newAddon, startup = false, updateCallback) {
> +    let reason = XPIInstall.newVersionReason(this.addon.version, newAddon.version);

Any reason not to move newVersionReason from XPIInstall.jsm to here now?

::: toolkit/mozapps/extensions/test/xpcshell/head_addons.js:249
(Diff revision 2)
>  
>    checkMatches(cached, current) {
>      Assert.notEqual(cached, undefined);
>      Assert.equal(current.data.version, cached.data.version);
>      Assert.equal(current.data.installPath, cached.data.installPath);
> -    Assert.equal(current.data.resourceURI, cached.data.resourceURI);
> +    Assert.ok(Services.io.newURI(current.data.resourceURI).equals(Services.io.newURI(cached.data.resourceURI)),

I can't tell from the reviewboard presentation but it looks like the indentation here is off?
Attachment #8975224 - Flags: review?(aswan)
Comment on attachment 8975224 [details]
Bug 1461062: Refactor bootstrap lifecycle management to be somewhat maintainable.

https://reviewboard.mozilla.org/r/243572/#review249734

> shouldn't we be using update() here?

Nope. There's no existing previous add-on to call shutdown/uninstall on in this case, so we can only call install() on the new version. This can only happen in changed side-loads, though.

> Should this be in bug 1461045?

Nope. You'd think so, but this code is actually broken, because the `existingAddon` object is an `AddonInternal` object, and never had a .bootstrapped flag. And if it actually *did* run, it would be super broken, since `oldVersion` is an `AddonInternal` object rather than a version string.

> Any reason not to move newVersionReason from XPIInstall.jsm to here now?

Well, it's more code added to XPIProvider to load even when it's not needed. We should only actually need to call this during install/upgrade, which doesn't happen in most sessions (and in which cases XPIInstall.jsm will already be loaded). It probably doesn't matter *that* much, though...

> I can't tell from the reviewboard presentation but it looks like the indentation here is off?

Could be.
Comment on attachment 8975224 [details]
Bug 1461062: Refactor bootstrap lifecycle management to be somewhat maintainable.

https://reviewboard.mozilla.org/r/243572/#review249734

> Could be.

Nope, just reviewboard.
Comment on attachment 8975224 [details]
Bug 1461062: Refactor bootstrap lifecycle management to be somewhat maintainable.

https://reviewboard.mozilla.org/r/243572/#review249734

> Nope. There's no existing previous add-on to call shutdown/uninstall on in this case, so we can only call install() on the new version. This can only happen in changed side-loads, though.

Hm, we should handle this better.  I think all the meaningful uninstall (or upgrade) cleanup that we do in webextensions is keyed just by the extension id, but we need the right notification from here to know to do that cleanup.

> Nope. You'd think so, but this code is actually broken, because the `existingAddon` object is an `AddonInternal` object, and never had a .bootstrapped flag. And if it actually *did* run, it would be super broken, since `oldVersion` is an `AddonInternal` object rather than a version string.

Ugh

> Well, it's more code added to XPIProvider to load even when it's not needed. We should only actually need to call this during install/upgrade, which doesn't happen in most sessions (and in which cases XPIInstall.jsm will already be loaded). It probably doesn't matter *that* much, though...

Well by that logic we don't need the install/uninstall/update methods on BootstrapScope at all in XPIProvider.jsm.
I vote for just making this straightforward here though by moving newVersionReason.
Comment on attachment 8975224 [details]
Bug 1461062: Refactor bootstrap lifecycle management to be somewhat maintainable.

https://reviewboard.mozilla.org/r/243572/#review249734

> Hm, we should handle this better.  I think all the meaningful uninstall (or upgrade) cleanup that we do in webextensions is keyed just by the extension id, but we need the right notification from here to know to do that cleanup.

Perhaps. But that's a separate bug. This preserves the existing behavior, but just makes it more comprehensible.

> Well by that logic we don't need the install/uninstall/update methods on BootstrapScope at all in XPIProvider.jsm.
> I vote for just making this straightforward here though by moving newVersionReason.

We don't, but in this case, I think the benefits of having all of the bootstrap logic in one place overrides the benefits of not having install logic in XPIProvider.jsm. And the newVersionReason function is used in other modules as well, so I'd prefer to keep it in XPIInstall.
Comment on attachment 8975224 [details]
Bug 1461062: Refactor bootstrap lifecycle management to be somewhat maintainable.

https://reviewboard.mozilla.org/r/243572/#review249734

> Perhaps. But that's a separate bug. This preserves the existing behavior, but just makes it more comprehensible.

okay filed bug 1461485
Comment on attachment 8975224 [details]
Bug 1461062: Refactor bootstrap lifecycle management to be somewhat maintainable.

https://reviewboard.mozilla.org/r/243572/#review249782
Attachment #8975224 - Flags: review?(aswan) → review+
Comment on attachment 8975225 [details]
Bug 1461062: Part 2 - Remove dead code.

https://reviewboard.mozilla.org/r/243574/#review249784

I'm unfamiliar with this code, redirecting
Attachment #8975225 - Flags: review?(aswan) → review?(lgreco)
Comment on attachment 8975225 [details]
Bug 1461062: Part 2 - Remove dead code.

https://reviewboard.mozilla.org/r/243574/#review250070

This (unfortunately) doesn't seem to be dead code yet, at least not until we completely remove any support for internal usage of the bootstrapped addons (more details below).

It doesn't seem that this change is actually mandatory for the changes applied by the other patch in this queue (not at a first glance at least), how about moving it in a separate issue?

----

Follows some additional details (that can be helpful to determine if we can really remove it right now or we have to defer it a bit, and what other changes are needed to completely remove any usage of it from the Firefox codebase):

The webextensions debugging doesn't need or use this, but I'm also pretty sure that this is still used by the legacy addon debugging window, which is the one opened to debug any plain bootstrapped addon or hybrid bootstrapped addon with an embedded webextensions (which are hopefully already fading out in favor of "webextensions with embedded experiment API"), 
e.g. I would expect that clicking the debug button on most of the system addons would give a broken webconsole panel with this patch applied.

And so, we can safely proceed to remove this code as soon as:
- none of the internal addons are bootstrapped
- or we have determined that none of the people that is working on a bootstrapped system addon (or other internal addons) needs (or is using) the legacy addon debugging window, and we made them aware that we are going to remove it immediately.

Either way, once we are ready to do it, we should also bring into the review someone from the DevTools team (e.g. ochameau or jdescottes), so that we can be sure that we don't leave anything half-broken (or half-cleaned), and that we are also cleaning up (or removing) any other components and tests that are related to it, e.g. the following are for sure some of them:

- the following test case (which I would expect to fail with this patch applied): https://searchfox.org/mozilla-central/source/devtools/client/aboutdebugging/test/browser_addons_debug_bootstrapped.js

- the following actors should also be removed when we are killing that part (none of these actors is used by the webextensions debugging):
  - https://searchfox.org/mozilla-central/source/devtools/server/actors/addon.js 

- BrowserAddonList should be cleaned up by any usage of the above addon actors:
  - https://searchfox.org/mozilla-central/rev/2b9779c59390ecc47be7a70d99753653d8eb5afc/devtools/server/actors/webbrowser.js#873

- the following methods on the DebuggingServer and DebuggingServerConnection are related to the code removed by this patch:
  - https://searchfox.org/mozilla-central/rev/2b9779c59390ecc47be7a70d99753653d8eb5afc/devtools/server/main.js#542-564
  - https://searchfox.org/mozilla-central/rev/2b9779c59390ecc47be7a70d99753653d8eb5afc/devtools/server/main.js#1651-1674

I'm clearing the review request in the meantime (mainly to signal that this is not a safe change until we have verified that we are ready to do it, and that it is not complete if we don't also remove the other code and tests that depends on this).
Attachment #8975225 - Flags: review?(lgreco)
(In reply to Luca Greco [:rpl] from comment #15)
> Comment on attachment 8975225 [details]
> Bug 1461062: Part 2 - Remove dead code.
> 
> https://reviewboard.mozilla.org/r/243574/#review250070
> 
> This (unfortunately) doesn't seem to be dead code yet, at least not until we
> completely remove any support for internal usage of the bootstrapped addons
> (more details below).
> 
> It doesn't seem that this change is actually mandatory for the changes
> applied by the other patch in this queue (not at a first glance at least),
> how about moving it in a separate issue?

It is dead code in the sense that it never runs. The devtools code adds a preference observer, while the add-on manager code dispatches a normal observer. Which means that the code that I removed never runs, and is therefore dead.

And since it hasn't run for years and apparently hasn't been missed, there doesn't seem to be any point in keeping it around.
(In reply to Kris Maglione [:kmag] (long backlog; ping on IRC if you're blocked) from comment #16)
> It is dead code in the sense that it never runs. The devtools code adds a
> preference observer, while the add-on manager code dispatches a normal
> observer. 

He, mentioning that (the mismatch between the observer and the dispatching) somewhere in the commit message would have helped to speed up a more complete investigation ;-)

> Which means that the code that I removed never runs, and is
> therefore dead.
> And since it hasn't run for years and apparently hasn't been missed, there
> doesn't seem to be any point in keeping it around.

That actually means (at least from my point of view): something was broken from the last change applied near there 
(which is not actually "years" yet ;-) most of that code has been most recently changed by Bug 1375809, 
which has been landed in Firefox 56 less than an year ago, https://hg.mozilla.org/mozilla-central/rev/fceeb41c167e).

Anyway, you are clearly right that Toolbox.jsm is currently listening "for nothing" on the preference service.

It has been broken in Bug 1375809 by mistake and got unnoticed because that code was covering an uncommon scenario that is not explicitly tested in automation, which is (even just based on the inline comments part of the removed lines):

"if the addon is disabled and/or enabled, then the target global of the addon debugging window should be updated accordingly".

It is clearly not a common scenario: the only extensions that are still bootstrapped are just a subset of the internal addons, it is also unlikely that they are often enabled/disabled "while" they are being debugged, and so I really doubt that many people may have seen this issue.

To confirm that this was the scenario already broken I tried it on mozilla-central (by manually restarting
the system addon from the browser console) and the webconsole becomes broken as soon as the system addon is
restarted once, fixing the listener subscribed in Toolbox.jsm by changing it into 
`Services.obs.addObserver((subject) => {}, "...");` keeps the webconsole working across the system addon restarts
(as it should).

Personally I would prefer to fix this (pre-existing) issue, at least as long as we keep this "legacy addon debugging window" around, but I'm also adding jdescottes as a reviewer on it, he may have different opinions (about remove and/or fixing it, and how), and he worked on Bug 1375809.
(In reply to Luca Greco [:rpl] from comment #17)
> Personally I would prefer to fix this (pre-existing) issue, at least as long
> as we keep this "legacy addon debugging window" around, but I'm also adding
> jdescottes as a reviewer on it, he may have different opinions (about remove
> and/or fixing it, and how), and he worked on Bug 1375809.

So, at this point, my position on any code relating to legacy extensions in the add-on manager is basically: If it's not absolutely necessary at this point, and it adds complexity, delete it.

This code adds complexity to an already complex part of the bootstrap management cycle. And since it's been broken for over a year without being noticed, it definitely does not seem absolutely necessary.
Comment on attachment 8975225 [details]
Bug 1461062: Part 2 - Remove dead code.

Hi Julian,
as described in more details in Comment 17, it looks that in Bug 1375809 we have broken the webconsole in the legacy addon debugging window on the addon reloads
(only the legacy bootstrapped addons may be affected).

This patch is currently going to remove the broken code, but based on what I saw I'm currently proposing to fix the issue and keep this code as long as we keep the "legacy addon debugging window" around.

do you mind to take a look at Comment 17 to double-check that I'm not reading it wrong? 
(and also to review the changes on the devtools side of the final version of this patch, once we agreed on the outcome that we prefer).
Attachment #8975225 - Flags: review?(jdescottes)
Comment on attachment 8975224 [details]
Bug 1461062: Refactor bootstrap lifecycle management to be somewhat maintainable.

https://reviewboard.mozilla.org/r/243572/#review250184

::: toolkit/mozapps/extensions/internal/XPIProvider.jsm:2810
(Diff revision 3)
>        connection.setAddonOptions(
>          id, { global: val.bootstrapScope });

This should be 
`connection.setAddonOptions(id, { global: val.scope });` 

(it used to be `activeAddon.bootstrapScope`, but in the refactored version it looks like it should be `BootstrapScope.scope` now).

To double-check it, I've just pulled this changes locally:

devtools/client/aboutdebugging/test/browser_addons_debug_bootstrapped.js (the one mentioned in comment 15) is failing with these patches applied, but the additional change above is enough to fix that test failure. 
(and I tried to use the webconsole on one of the system addons manually and it is working correctly once fixed).
Just to clarify my last response:

Legacy extensions are entirely unsupported at this point, which means the default action for any code relating to them which no longer works should be to remove that code, not to fix it.

I don't have any opinion on whether the devtools team should continue to maintain the legacy extension debugger, but add-on manager code that relates to it needs to have a very good reason to continue existing. I'm happy to keep the working portions of that code around until we remove support for bootstrapped system extensions. But broken or unused legacy extension code needs to go away as soon as possible. The add-on manager code as it stands is far too difficult to understand and maintain, and legacy add-on cruft is one of the biggest contributing factors to that.
https://hg.mozilla.org/integration/mozilla-inbound/rev/fcfdc7f53add8571ac073d84f67a5b31fbb24332
Bug 1461062: Refactor bootstrap lifecycle management to be somewhat maintainable. r=aswan
https://hg.mozilla.org/integration/mozilla-inbound/rev/34d13ae107cc912aee5847a70c8ad750166f9394
Bug 1461062: Follow-up: Update counts in test_cacheflush. r=bustage,test-only DONTBUILD
https://hg.mozilla.org/integration/mozilla-inbound/rev/6be9c9eef3775e34b8c2bbea33fe4554999574c6
Bug 1461062: Follow-up: Temporarily disable Windows-only failures for rebase bustage. r=bustage,test-only DONTBUILD
(In reply to Kris Maglione [:kmag] (long backlog; ping on IRC if you're blocked) from comment #21)
> Just to clarify my last response:
> 
> Legacy extensions are entirely unsupported at this point, which means the
> default action for any code relating to them which no longer works should be
> to remove that code, not to fix it.
> 
> I don't have any opinion on whether the devtools team should continue to
> maintain the legacy extension debugger, but add-on manager code that relates
> to it needs to have a very good reason to continue existing. I'm happy to
> keep the working portions of that code around until we remove support for
> bootstrapped system extensions. But broken or unused legacy extension code
> needs to go away as soon as possible. The add-on manager code as it stands
> is far too difficult to understand and maintain, and legacy add-on cruft is
> one of the biggest contributing factors to that.

Fine by me, I only expressed my opinion, but it is not a strong opinion, like I said
I doubt that many people would notice the issue on the extension reload 
(also given that it has been broken for 11 months now).

My only goal was actually to help you to spare one additional 
"Bug 1461062: Follow-up: Fix browser_addons_debug_bootstrapped.js failure. r=bustage"
pushed in an hurry to prevent the patch from being backed out, and it seems that
we've already achieved that.

I don't gain absolutely anything in making you feel "blocked" on the additional change
that you want to apply, but there is nothing wrong on having different opinions, a
different opinion is just an "additional value" if it is being expressed in a non-aggressive way
(which is what I want and I always try to do), you may even happily ignore it if you feel that 
it doesn't matter, I assure you that I will not feel offended, I'm going to live fine even 
if my opinion isn't always taken into account. 

Anyway, if we agree on proceeding with removing that code, I'm happy to r+ the changes
applied on the XPIProvider.jsm side, I added the r? assigned to jdescottes because
most of the code that is going to be removed in attachment 8975225 [details] is actually in
ToolboxProcess.jsm and he may notice if we can contextually remove from their side
other code related to it (and it is also better to make the DevTools aware of this
change in any case).
(In reply to Luca Greco [:rpl] from comment #28)
> My only goal was actually to help you to spare one additional "Bug 1461062:
> Follow-up: Fix browser_addons_debug_bootstrapped.js failure. r=bustage" pushed
> in an hurry to prevent the patch from being backed out, and it seems that
> we've already achieved that.

Eh, that showed up in my try run, anyway. The only reason for the other
follow-ups was that my last try run had patches for other cleanup bugs on top of
it, and a bunch of things only failed with the partial patchset applied.
Comment on attachment 8975225 [details]
Bug 1461062: Part 2 - Remove dead code.

https://reviewboard.mozilla.org/r/243574/#review250920

Since the feature has been broken for around a year and bootstrapped extensions are going away in release 64 now, I don't have any strong argument to keep this code around. I would prefer Luca's appraoch, since the fix is just one line of code. But if you feel like removing the code from toolkit/mozapps/extensions/internal/XPIProvider.jsm will make your life easier, it's your call.
Attachment #8975225 - Flags: review?(jdescottes) → review+
(In reply to Kris Maglione [:kmag] (long backlog; ping on IRC if you're blocked) from comment #30)
> (In reply to Luca Greco [:rpl] from comment #28)
> > My only goal was actually to help you to spare one additional "Bug 1461062:
> > Follow-up: Fix browser_addons_debug_bootstrapped.js failure. r=bustage" pushed
> > in an hurry to prevent the patch from being backed out, and it seems that
> > we've already achieved that.
> 
> Eh, that showed up in my try run, anyway. The only reason for the other
> follow-ups was that my last try run had patches for other cleanup bugs on
> top of
> it, and a bunch of things only failed with the partial patchset applied.

eh, but that's not the point (and you could easily recognize it by yourself, you just need to think about it from a less "self-centric" point of view): 

being able to deal with any issue by yourself is absolutely great and clearly important 
(and I would never doubt that you will be always able to deal with any issue by yourself)

But it doesn't change the fact that "asking/accepting/getting help from a team-mate" doesn't make you less smart, it doesn't make you less great or awesome, on the contrary it could just help to make "everyone a bit better day after day", your "queue" potentially shorter / faster, and the entire team stronger.

Anyway... you know... that's just my opinion :-)
Comment on attachment 8975225 [details]
Bug 1461062: Part 2 - Remove dead code.

(In reply to Julian Descottes [:jdescottes][:julian] from comment #31)
> Since the feature has been broken for around a year and bootstrapped
> extensions are going away in release 64 now, I don't have any strong
> argument to keep this code around. I would prefer Luca's appraoch, since the
> fix is just one line of code. But if you feel like removing the code from
> toolkit/mozapps/extensions/internal/XPIProvider.jsm will make your life
> easier, it's your call.

As already stated in Comment 28, I'm also fine with whatever decision Kris will make (removing or fixing).
Attachment #8975225 - Flags: review+
Is manual testing required on this bug? If yes, please provide some STR and the proper extension(if required) or set the “qe-verify -“ flag.

Thanks!
Flags: needinfo?(kmaglione+bmo)
Flags: needinfo?(kmaglione+bmo) → qe-verify-
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: