Closed Bug 1049142 Opened 6 years ago Closed 6 years ago

Don't scan unpacked add-on files when the add-on is disabled

Categories

(Toolkit :: Add-ons Manager, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla35

People

(Reporter: Irving, Assigned: Irving)

References

Details

Attachments

(1 file, 4 obsolete files)

I was reviewing Telemetry data gathered about add-on directory scan performance, and noticed (by a quirk of the way I was collecting data) that much of the unpacked add-on scanning we do is of disabled add-ons.

Telemetry doesn't currently record the add-on version for disabled add-ons, so in my data the add-ons listed with version '?' are disabled:

https://docs.google.com/spreadsheets/d/1oA2qY9zpKiLCKZMJzDXrbVh8TjRk67Sxt4yWCvAJGH4/edit?usp=sharing

Based on this, I think we can get a noticeable improvement in start up time (tens to perhaps one or two hundred milliseconds, at the 75th percentile) by skipping the directory scan for disabled add-ons.

We still need to check the timestamp on the install.rdf manifest, in case the add-on was updated by a 3rd party installer and can be re-enabled.
Assignee: nobody → irving
Status: NEW → ASSIGNED
This still has some rough edges to clean up around updating the new xpiState when add-ons are enabled/disabled at run time, and folding in the bootstrappedAddons and enabledAddons preferences could be added here or taken as follow up bugs.

Also, still needs some unit tests.
Attachment #8478075 - Flags: feedback?(bmcbride)
Comment on attachment 8478075 [details] [diff] [review]
WIP - Skip recursive scan for disabled add-ons

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

::: toolkit/mozapps/extensions/internal/XPIProvider.jsm
@@ -55,5 @@
>  const nsIFile = Components.Constructor("@mozilla.org/file/local;1", "nsIFile",
>                                         "initWithPath");
>  
>  const PREF_DB_SCHEMA                  = "extensions.databaseSchema";
> -const PREF_INSTALL_CACHE              = "extensions.installCache";

We should think about what do do with this pref going forward - since it can potentially hold a reasonable amount of data.

Just clearing it should be safe - anyone switching back to an earlier version of the app will just force a rebuild when its detected as stale.

@@ +2233,5 @@
> +        addonState.ft = 0;
> +      }
> +    }
> +    // Inject a convenience function to retrieve the modified time from the state
> +    addonState.mtime = function() {

The code for all this stuff is notoriously hard to follow, so I want to specifically keep an eye on making this code easier to understand. Having this mtime helper function is a good start - but I think we should go a step further by:
* Abstracting addonState out into it's own class
* Make mtime a getter
* Rename mt and ft into readable property names
* Add a toJSON method to serialise, and have it output using the shorter mt and ft names
* Refactor at least some of the related logic here into the class - eg, optionally updating mt/ft/descriptor
* Make the class handle whether it's been seen/in the DB or not, to avoid managing _seen/_inDB

@@ +2263,5 @@
> +    for (let location of this.installLocations) {
> +      // The list of things on disk in this location.
> +      let addons = location.addonLocations;
> +      // What our stored state thinks should be in this location.
> +      let locState = {};

Ditto for locState.

@@ +3249,5 @@
>          let installReason = BOOTSTRAP_REASONS.ADDON_INSTALL;
>          let extraParams = {};
>  
> +        // XXX copy add-on details (enabled, bootstrap, version, etc) to addonState
> +        aAddonState.enabled = newDBAddon.active;

Another thing a helper method on addonState would help hide away from the main algorithm here.

@@ +3471,5 @@
>      }
>  
>      // Load the list of bootstrapped add-ons first so processFileChanges can
>      // modify it
> +    // XXX eventually get rid of bootstrappedAddons

Think I'd prefer this done as a separate bug - just to make the existing changes here easier to digest.

@@ +3569,5 @@
>        if (updateReasons.length > 0) {
>          AddonManagerPrivate.recordSimpleMeasure("XPIDB_startup_load_reasons", updateReasons);
>          XPIDatabase.syncLoadDB(false);
>          try {
> +          extensionListChanged = this.processFileChanges(this.xpiState,

As a followup, now that getInstallState() is smarter, we should be able to make give us what it knows changed, and have processFileChanges() act only on that (if we know that's the only changes) rather than re-checking everything.
Attachment #8478075 - Flags: feedback?(bmcbride) → feedback+
Now with some tests, I think I'm getting close to review-ready.


(In reply to Blair McBride [:Unfocused] from comment #2)

> > -const PREF_INSTALL_CACHE              = "extensions.installCache";
> 
> We should think about what do do with this pref going forward - since it can
> potentially hold a reasonable amount of data.
> 
> Just clearing it should be safe - anyone switching back to an earlier
> version of the app will just force a rebuild when its detected as stale.

I'll clear it.

> @@ +2233,5 @@
> > +        addonState.ft = 0;
> > +      }
> > +    }
> > +    // Inject a convenience function to retrieve the modified time from the state
> > +    addonState.mtime = function() {
> 
> The code for all this stuff is notoriously hard to follow, so I want to
> specifically keep an eye on making this code easier to understand. Having
> this mtime helper function is a good start - but I think we should go a step
> further by:
> * Abstracting addonState out into it's own class
> * Make mtime a getter
> * Rename mt and ft into readable property names
> * Add a toJSON method to serialise, and have it output using the shorter mt
> and ft names
> * Refactor at least some of the related logic here into the class - eg,
> optionally updating mt/ft/descriptor

OK

> * Make the class handle whether it's been seen/in the DB or not, to avoid
> managing _seen/_inDB

If you don't like using temporary attributes for this purpose, I can use a local Set(); in both cases the use is limited to one synchronous loop over the add-on states so it doesn't make sense (to me) to manage it through an API.

> @@ +2263,5 @@
> > +    for (let location of this.installLocations) {
> > +      // The list of things on disk in this location.
> > +      let addons = location.addonLocations;
> > +      // What our stored state thinks should be in this location.
> > +      let locState = {};
> 
> Ditto for locState.

Added an API to get the per-location state.

> @@ +3249,5 @@
> >          let installReason = BOOTSTRAP_REASONS.ADDON_INSTALL;
> >          let extraParams = {};
> >  
> > +        // XXX copy add-on details (enabled, bootstrap, version, etc) to addonState
> > +        aAddonState.enabled = newDBAddon.active;
> 
> Another thing a helper method on addonState would help hide away from the
> main algorithm here.

Done.

> @@ +3471,5 @@
> >      }
> >  
> >      // Load the list of bootstrapped add-ons first so processFileChanges can
> >      // modify it
> > +    // XXX eventually get rid of bootstrappedAddons
> 
> Think I'd prefer this done as a separate bug - just to make the existing
> changes here easier to digest.

I'll file one.

> @@ +3569,5 @@
> >        if (updateReasons.length > 0) {
> >          AddonManagerPrivate.recordSimpleMeasure("XPIDB_startup_load_reasons", updateReasons);
> >          XPIDatabase.syncLoadDB(false);
> >          try {
> > +          extensionListChanged = this.processFileChanges(this.xpiState,
> 
> As a followup, now that getInstallState() is smarter, we should be able to
> make give us what it knows changed, and have processFileChanges() act only
> on that (if we know that's the only changes) rather than re-checking
> everything.

I actually started out with this approach, figured out that I'd probably end up needing to completely rewrite processFileChanges() to take advantage of the info, and chose to back away with my hands in clear view to avoid angering it.

I spent quite a bit of time going back and forth over this in my mind - I think I've come to the conclusion that the right place for xpiState is in the InstallLocation, though it could be done as a separate parallel structure. The headache with having it separate is that we end up with error checking spread through the code to make sure one or the other isn't missing an entry.

The hard part is untangling all the overlapping cases between database rebuilds, database upgrades, on-disk changes, pending installs / uninstalls etc. that are all mixed together in the current processFileChanges(). I'd love to have the time to significantly refactor this code, particularly with a view to exposing more of the core logic to unit-level testing rather than relying on the existing end-to-end testing suite, but feel like I'd have a hard time justifying the time to my manager.
Attachment #8478075 - Attachment is obsolete: true
Attachment #8479363 - Flags: feedback?(bmcbride)
Comment on attachment 8479363 [details] [diff] [review]
WIP v2 - Skip recursive scan for disabled add-ons

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

Me like! This iteration feels a lot better.

::: toolkit/mozapps/extensions/internal/XPIProvider.jsm
@@ +1584,5 @@
> +    }
> +    return this.scanTime;
> +  },
> +
> +  // Hot new JS method declaration syntax!

<3

@@ +1659,5 @@
> +   * @param aDBAddon The DBAddonInternal for this add-on.
> +   *
> +   * XXX When we enable/disable add-ons, need to update state with new time, enabled, bootstrap
> +   * ... when a bootstrap add-on is  installed / updated
> +   * ... how about delayed enable/disable/install/uninstall for restarty add-ons?

"restarty"? Geez we need better words for these things...

@@ +1712,5 @@
> +
> +  /**
> +   * Load extension state data from preferences.
> +   */
> +  loadExtensionState() {

Hm. This becomes non-deterministic due to race conditions when we move to saving via DeferredSave. Which makes me worried that this is exposed as a normal public method.

@@ +1716,5 @@
> +  loadExtensionState() {
> +    let state = {};
> +
> +    // Clear out old directory state cache.
> +    Prefs.clearUserPref(PREF_INSTALL_CACHE);

FYI, bug 1059620 will bitrot you here.

@@ +1745,5 @@
> +      // The list of add-on like file/directory names in the install location.
> +      let addons = location.addonLocations;
> +      // The results of scanning this location.
> +      let foundAddons = new Map();
> +      foundAddons.toJSON = MapToJSON;

I worry about manually setting toJSON like this, as it's too easy to forget.

Rather than a MapToJSON function, how about a SerializableMap constructor that returns a Map with toJSON already set? So you use it just like Map:
  whatever = new SerializableMap();

(Can override what you get from `new` by having the constructor explicitly return a value)

@@ +1748,5 @@
> +      let foundAddons = new Map();
> +      foundAddons.toJSON = MapToJSON;
> +
> +      // What our old state thinks should be in this location.
> +      let locState = {};

Feels like this should be a Map.

@@ +1756,5 @@
> +        delete oldState[location.name];
> +      }
> +
> +      for (let file of addons) {
> +        let scanStarted = Date.now();

One of these days I'm going to snap when I see this... it's so error prone. But that's for another bug.

@@ +1771,5 @@
> +          delete locState[id];
> +
> +          changed = xpiState.getModTime(file, id) || changed;
> +
> +          if (!('descriptor' in xpiState) || file.persistentDescriptor != xpiState.descriptor) {

First part of this condition seems redundant.

@@ +1812,5 @@
> +   * Get the Map of XPI states for a particular location.
> +   * @param aLocation The name of the install location.
> +   * @return Map (id -> XPIState) or null if there are no add-ons in the location.
> +   */
> +  getXPIStates(aLocation) {

XPIStates.getXPIStates() seems rather redundant, and not descriptive. How about something like XPIStates.getLocation() ?

@@ +1823,5 @@
> +   * @param aLocation The name of the location where the add-on is installed.
> +   * @param aId       The add-on ID
> +   * @return The XPIState entry for the add-on, or null.
> +   */
> +  getAddonXPIState(aLocation, aId) {

getAddon() ?

@@ +1833,5 @@
> +  },
> +
> +  /**
> +   * Save the current state of installed add-ons.
> +   * XXX this *totally* should be a .json file using DeferredSave...

Yesplz. Would also solve the error-prone-ness of this API.

@@ +2127,5 @@
>                                    [DIR_EXTENSIONS],
>                                    AddonManager.SCOPE_PROFILE, false);
>  
> +      // Now flip the list so that the highest priority location is first
> +      this.installLocations.reverse();

Er, can we just add them in the right order in the first place?

@@ +3518,5 @@
> +      }
> +    }
> +
> +    // Anything left in allDBAddons is a location recorded in the database,
> +    // but no longer configured for the browser.

The original comment here described this better, IMO.

@@ +4732,5 @@
>      }
>  
> +    function findAddonAndReveal(aId) {
> +      // Fortunately the Map iterator returns in order of insertion, which is
> +      // also our highest -> lowest priority order.

You made me double check the ES6 spec with this assertion ;) Looks like that is indeed guaranteed now days.

::: toolkit/mozapps/extensions/internal/XPIProviderUtils.js
@@ +73,5 @@
>                            "strictCompatibility", "locales", "targetApplications",
>                            "targetPlatforms", "multiprocessCompatible"];
>  
>  // Time to wait before async save of XPI JSON database, in milliseconds
> +const ASYNC_SAVE_DELAY_MS = 500;

Is this change based on any data?
Attachment #8479363 - Flags: feedback?(bmcbride) → feedback+
Test cases and implementation for the edge cases of install/uninstall/enable/disable for non-restartless add-ons, clean up, etc.

(In reply to Blair McBride [:Unfocused] from comment #4)
> Comment on attachment 8479363 [details] [diff] [review]
> WIP v2 - Skip recursive scan for disabled add-ons

> > +  /**
> > +   * Load extension state data from preferences.
> > +   */
> > +  loadExtensionState() {
> 
> Hm. This becomes non-deterministic due to race conditions when we move to
> saving via DeferredSave. Which makes me worried that this is exposed as a
> normal public method.

XPIStates isn't exported, and this call doesn't have side effects and is only used internally at start-up time, so I'm not concerned.

> @@ +1716,5 @@
> > +  loadExtensionState() {
> > +    let state = {};
> > +
> > +    // Clear out old directory state cache.
> > +    Prefs.clearUserPref(PREF_INSTALL_CACHE);
> 
> FYI, bug 1059620 will bitrot you here.

Race you to checkin ;->

> @@ +1745,5 @@
> Rather than a MapToJSON function, how about a SerializableMap constructor
> that returns a Map with toJSON already set? So you use it just like Map:
>   whatever = new SerializableMap();

Done.

> @@ +1748,5 @@
> > +      let foundAddons = new Map();
> > +      foundAddons.toJSON = MapToJSON;
> > +
> > +      // What our old state thinks should be in this location.
> > +      let locState = {};
> 
> Feels like this should be a Map.

I'm directly walking through (and discarding) the JSON data parsed out of the pref here, it doesn't feel worth it to me to copy it into a Map.

> @@ +1756,5 @@
> > +        delete oldState[location.name];
> > +      }
> > +
> > +      for (let file of addons) {
> > +        let scanStarted = Date.now();
> 
> One of these days I'm going to snap when I see this... it's so error prone.
> But that's for another bug.

I suspect you're looking for something like

let duration = Telemetry.time(() => { do stuff ... });

? That could piggyback on bug 906524.

> @@ +1771,5 @@
> > +          delete locState[id];
> > +
> > +          changed = xpiState.getModTime(file, id) || changed;
> > +
> > +          if (!('descriptor' in xpiState) || file.persistentDescriptor != xpiState.descriptor) {
> 
> First part of this condition seems redundant.

I was being paranoid about JS strict warnings for undefined attributes, but I tried without and didn't get warnings so I took out all the extra 'in' tests.

> @@ +1812,5 @@
> > +   * Get the Map of XPI states for a particular location.
> > +   * @param aLocation The name of the install location.
> > +   * @return Map (id -> XPIState) or null if there are no add-ons in the location.
> > +   */
> > +  getXPIStates(aLocation) {
> 
> XPIStates.getXPIStates() seems rather redundant, and not descriptive. How
> about something like XPIStates.getLocation() ?

yup

> @@ +1823,5 @@
> > +   * @param aLocation The name of the location where the add-on is installed.
> > +   * @param aId       The add-on ID
> > +   * @return The XPIState entry for the add-on, or null.
> > +   */
> > +  getAddonXPIState(aLocation, aId) {
> 
> getAddon() ?

and yup

> @@ +1833,5 @@
> > +  },
> > +
> > +  /**
> > +   * Save the current state of installed add-ons.
> > +   * XXX this *totally* should be a .json file using DeferredSave...
> 
> Yesplz. Would also solve the error-prone-ness of this API.

Follow-up bug, perhaps at the same time as we stop writing extensions.ini (so we only need to read one file sync at start up)

> @@ +2127,5 @@
> >                                    [DIR_EXTENSIONS],
> >                                    AddonManager.SCOPE_PROFILE, false);
> >  
> > +      // Now flip the list so that the highest priority location is first
> > +      this.installLocations.reverse();
> 
> Er, can we just add them in the right order in the first place?

OK, was avoiding the copypasta, but moved them around. I numbered them with comments and left the numbers on, let me know if you want that cleaned off before check-in.

> @@ +3518,5 @@
> > +      }
> > +    }
> > +
> > +    // Anything left in allDBAddons is a location recorded in the database,
> > +    // but no longer configured for the browser.
> 
> The original comment here described this better, IMO.

The semantics did change a bit, so I updated the comment but left it closer to mine than the previous.

> @@ +4732,5 @@
> >      }
> >  
> > +    function findAddonAndReveal(aId) {
> > +      // Fortunately the Map iterator returns in order of insertion, which is
> > +      // also our highest -> lowest priority order.
> 
> You made me double check the ES6 spec with this assertion ;) Looks like that
> is indeed guaranteed now days.

I liked gavin's "r- too clever" remark.

> ::: toolkit/mozapps/extensions/internal/XPIProviderUtils.js
> @@ +73,5 @@
> >                            "strictCompatibility", "locales", "targetApplications",
> >                            "targetPlatforms", "multiprocessCompatible"];
> >  
> >  // Time to wait before async save of XPI JSON database, in milliseconds
> > +const ASYNC_SAVE_DELAY_MS = 500;
> 
> Is this change based on any data?

Telemetry isn't showing a ton of extensions.json writes, but because I didn't gather "how long since the preceding write" I can't really tell if extending the delay would avoid many wasted back-to-back writes.

Mostly based on a combination of seeing a lot of back-to-back writes during test suite runs (e.g. multiple writes during the course of one background update), and feeling like 20ms is overly aggressive. I didn't really have any solid grounds for making it 20ms in the first place, but I'm happy to leave it if you prefer.
Attachment #8479363 - Attachment is obsolete: true
Attachment #8485209 - Flags: review?(bmcbride)
Blocks: 879480
(In reply to :Irving Reid from comment #5)
> I suspect you're looking for something like
> 
> let duration = Telemetry.time(() => { do stuff ... });

Yep. Even better if you can just pass in the name of the measurement too.

> That could piggyback on bug 906524.

Ah, yes. Thanks for reminding me of that.


> > > +   * Save the current state of installed add-ons.
> > > +   * XXX this *totally* should be a .json file using DeferredSave...
> > 
> > Yesplz. Would also solve the error-prone-ness of this API.
> 
> Follow-up bug, perhaps at the same time as we stop writing extensions.ini
> (so we only need to read one file sync at start up)

We don't yet have a bug for that, right? neither bug 791987 nor bug 731489 cover it, and would be WONTFIXed instead.

> OK, was avoiding the copypasta, but moved them around. I numbered them with
> comments and left the numbers on, let me know if you want that cleaned off
> before check-in.

Doesn't feel like the numbers add any benefit.


> Telemetry isn't showing a ton of extensions.json writes, but because I
> didn't gather "how long since the preceding write" I can't really tell if
> extending the delay would avoid many wasted back-to-back writes.
> 
> Mostly based on a combination of seeing a lot of back-to-back writes during
> test suite runs (e.g. multiple writes during the course of one background
> update), and feeling like 20ms is overly aggressive. I didn't really have
> any solid grounds for making it 20ms in the first place, but I'm happy to
> leave it if you prefer.

Yea, can we do this in a followup? Feels like something we should try to be intentional about, and do it isolated so we can track the effects.
Wasn't sure if you wanted an updated patch before review, and missed you on IRC, so here's an updated patch.

(In reply to Blair McBride [:Unfocused] from comment #6)
> (In reply to :Irving Reid from comment #5)
> > I suspect you're looking for something like
> > 
> > let duration = Telemetry.time(() => { do stuff ... });
> 
> Yep. Even better if you can just pass in the name of the measurement too.
> 
> > That could piggyback on bug 906524.
> 
> Ah, yes. Thanks for reminding me of that.

There's a pile of Telemetry*.jsm bugs that could really use some love. They need just a little too much design to be good-next-bug material.

> > > > +   * Save the current state of installed add-ons.
> > > > +   * XXX this *totally* should be a .json file using DeferredSave...
> > > 
> > > Yesplz. Would also solve the error-prone-ness of this API.
> > 
> > Follow-up bug, perhaps at the same time as we stop writing extensions.ini
> > (so we only need to read one file sync at start up)
> 
> We don't yet have a bug for that, right? neither bug 791987 nor bug 731489
> cover it, and would be WONTFIXed instead.

I'll file an new bug that blocks those and any others I can find that overlap (e.g. bug 1058899 probably gets fixed en passant)

> > OK, was avoiding the copypasta, but moved them around. I numbered them with
> > comments and left the numbers on, let me know if you want that cleaned off
> > before check-in.
> 
> Doesn't feel like the numbers add any benefit.

Removed.

> > Mostly based on a combination of seeing a lot of back-to-back writes during
> > test suite runs (e.g. multiple writes during the course of one background
> > update), and feeling like 20ms is overly aggressive. I didn't really have
> > any solid grounds for making it 20ms in the first place, but I'm happy to
> > leave it if you prefer.
> 
> Yea, can we do this in a followup? Feels like something we should try to be
> intentional about, and do it isolated so we can track the effects.

I'll just leave it unchanged, it's not a big deal.
Attachment #8485209 - Attachment is obsolete: true
Attachment #8485209 - Flags: review?(bmcbride)
Attachment #8487215 - Flags: review?(bmcbride)
(In reply to :Irving Reid from comment #7)
> Wasn't sure if you wanted an updated patch before review, and missed you on
> IRC, so here's an updated patch.

Sorry for the lag here - I was (very) slowly making my way through the patch. Will aim to finish the review on the updated patch today.
Comment on attachment 8487215 [details] [diff] [review]
Skip recursive scan for disabled add-ons, v2

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

::: toolkit/mozapps/extensions/internal/XPIProvider.jsm
@@ +1581,5 @@
> +    return this.scanTime;
> +  },
> +
> +  toJSON() {
> +    let r = {};

Nit: Down with single character variable names!

@@ +1702,5 @@
> +    if (!this.db) {
> +      return 0;
> +    }
> +    let count = 0;
> +    for (let [, loc] of this.db) {

Nit: global s/loc/location/

Also, you may want db.values() here

@@ +4745,5 @@
>        // the revealed add-on
>        AddonManagerPrivate.callAddonListeners("onInstalled", wrappedAddon);
>      }
>  
> +    function findAddonAndReveal(aId) {

Nit: Feels like this could do with a refactoring, so it ends up something like:

  let location = XPIStates.findLocation(id);
  XPIDatabase.getAddonInLocation(id, location.name, revealAddon);

::: toolkit/mozapps/extensions/internal/XPIProviderUtils.js
@@ +1170,5 @@
>          aAddon => (aAddon.visible &&
>                     (aAddon.pendingUninstall ||
> +                    // Logic here is tricky. If we're active but disabled,
> +                    // we're pending disable; !active && !disabled, we're pending enable
> +                    (aAddon.active == aAddon.disabled)) &&

Hm, interesting, the semantics changed here a bit - now taking into account softDisabled, which I think is correct.
Attachment #8487215 - Flags: review?(bmcbride) → review+
Nits fixed, carrying forward r+

Updated to depend on bug 1059620 (convert to Preferences.jsm), joint try run at https://tbpl.mozilla.org/?tree=Try&rev=dfc290f6d73c

(In reply to Blair McBride [:Unfocused] from comment #9)
> Comment on attachment 8487215 [details] [diff] [review]
> > +    let r = {};
> 
> Nit: Down with single character variable names!

heh, I have a bit of bias towards golang's style of very short variable names when they're extremely local, http://wordaligned.org/articles/go-for-short-variable-names, but I'm not dogmatic about it.

> @@ +1702,5 @@
> Nit: global s/loc/location/

except the one function where it's s/loc/locale/

> Also, you may want db.values() here

yes.

> @@ +4745,5 @@
> Nit: Feels like this could do with a refactoring, so it ends up something
> like:
> 
>   let location = XPIStates.findLocation(id);
>   XPIDatabase.getAddonInLocation(id, location.name, revealAddon);

I generalized from zero examples to make XPIStates.findAddon return [locationName, addon]

> ::: toolkit/mozapps/extensions/internal/XPIProviderUtils.js
> @@ +1170,5 @@
> >          aAddon => (aAddon.visible &&
> >                     (aAddon.pendingUninstall ||
> > +                    // Logic here is tricky. If we're active but disabled,
> > +                    // we're pending disable; !active && !disabled, we're pending enable
> > +                    (aAddon.active == aAddon.disabled)) &&
> 
> Hm, interesting, the semantics changed here a bit - now taking into account
> softDisabled, which I think is correct.

I meant to call that out when I posted the patch...
Attachment #8487215 - Attachment is obsolete: true
Attachment #8490154 - Flags: review+
(In reply to :Irving Reid from comment #10)
> > @@ +1702,5 @@
> > Nit: global s/loc/location/
> 
> except the one function where it's s/loc/locale/

Heh, I think that illustrates my point nicely :)
https://hg.mozilla.org/mozilla-central/rev/adb01d557168
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Whiteboard: [fixed-in-fx-team]
Target Milestone: --- → mozilla35
Depends on: 1117858
You need to log in before you can comment on or make changes to this bug.