Closed Bug 1452618 Opened 6 years ago Closed 6 years ago

Make getAddonBlocklistEntry API asynchronous

Categories

(Toolkit :: Blocklist Policy Requests, enhancement)

enhancement
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla61
Tracking Status
firefox61 --- fixed

People

(Reporter: Gijs, Assigned: Gijs)

References

Details

Attachments

(2 files)

This API has 1 non-test callsite outside the blocklist code itself,

https://dxr.mozilla.org/mozilla-central/rev/cb2bb4a7c1c39a017aa8c24c30b62a5f48922677/toolkit/mozapps/extensions/internal/XPIProvider.jsm#4577

which unfortunately has an inverse calltree that is a bit intimidating.

Based on the comments in bug 1447680, I think that in the general case we can just do an async lookup and (if necessary) wait for the blocklist to load.

However, on startup, I think we should probably let the install go ahead while the blocklist starts in parallel. We don't particularly need to know the blocklist state for sideloaded add-ons immediately: malicious actors could just wipe the blocklist anyway if they managed to sideload an add-on, and if the add-on managed to get installed by the user through the UI, it should have been checked against the blocklist then anyway.
Comment on attachment 8966240 [details]
Bug 1452618 - make getAddonBlocklistEntry asynchronous,

https://reviewboard.mozilla.org/r/234986/#review240610

::: toolkit/mozapps/extensions/internal/XPIProviderUtils.js:1264
(Diff revision 1)
> +    // If we only just got our new add-on, we do another blocklist update check
> +    // based on the data we had for the old add-on.
> +    // We do it here because this way we're guaranteed to have a DB add-on, and
> +    // `updateBlocklistState` will take care of saving the new updated blocklist
> +    // state into the DB for us (asynchronously).
> +    if (reDoBlocklistState) {
> +      dbAddon.updateBlocklistState({oldAddon: aOldAddon});
> +    }

This and the other change in this file feel extremely hacky to me. However, I don't really understand the startup flow of the add-on manager well enough to make alternative suggestions. The callpaths for both `updateMetadata` and `updateCompatibility` seem to be fully synchronous. While I could follow the lead of `syncLoadManifestFromFile` and add event loop spinning in order to load the blocklist asynchronously, I wasn't very happy with that idea, either.

Kris, can you recommend alternatives if you see any, and/or let me know if what I'm doing is likely to break things? To be clear, I have no idea what happens in terms of conflict resolution if saving some add-on into the db async - what if some other consumer overwrites some of the data inbetween when this code currently writes and when the async blocklist load returns and we write for that? Do we just clobber any other state changes? That would be bad... how does the add-on manager deal with this right now, given it has so many other async APIs? I have no idea...

::: toolkit/mozapps/extensions/internal/XPIProviderUtils.js
(Diff revision 1)
> -   * @param  aOldAppVersion
> -   *         The version of the application last run with this profile or null
> -   *         if it is a new profile or the version is unknown
> -   * @param  aOldPlatformVersion
> -   *         The version of the platform last run with this profile or null
> -   *         if it is a new profile or the version is unknown

These seemed to be unused, and the callsite in this file seems to be the only callsite, so I just removed them.
Status: NEW → ASSIGNED
Comment on attachment 8966240 [details]
Bug 1452618 - make getAddonBlocklistEntry asynchronous,

https://reviewboard.mozilla.org/r/234986/#review240614

::: toolkit/mozapps/extensions/internal/XPIInstall.jsm:787
(Diff revision 1)
>    addon._sourceBundle = aDir.clone();
>    addon._installLocation = aInstallLocation;
>    addon.size = getFileSize(aDir);
>    addon.signedState = await verifyDirSignedState(aDir, addon)
>      .then(({signedState}) => signedState);
> -  addon.updateBlocklistState();
> +  await addon.updateBlocklistState();

I would also like to make this code (and the similar zip reader one) not block on loading the update state, but that's technically not necessary for fixing this bug, and again, due to my lack of familiarity with the addon manager, I don't know how many other things would need updating in order to accomplish that. I do know that `isUsableAddon` needs the results of `updateBlocklistState` immediately, and as a result the disabled state of the add-on would be wrong if we just didn't bother getting the blocklist state sync. I'm sure we could work around that somehow, but I finally decided not to try to do that in this bug.
Looks like the patch as-is causes several xpcshell failures in add-ons/blocklist-related tests. Going to leave review open for now to get feedback on the approach before trying to fix tests.
Comment on attachment 8966240 [details]
Bug 1452618 - make getAddonBlocklistEntry asynchronous,

https://reviewboard.mozilla.org/r/234986/#review240786

::: toolkit/mozapps/extensions/internal/XPIProviderUtils.js:1264
(Diff revision 1)
> +    // If we only just got our new add-on, we do another blocklist update check
> +    // based on the data we had for the old add-on.
> +    // We do it here because this way we're guaranteed to have a DB add-on, and
> +    // `updateBlocklistState` will take care of saving the new updated blocklist
> +    // state into the DB for us (asynchronously).
> +    if (reDoBlocklistState) {
> +      dbAddon.updateBlocklistState({oldAddon: aOldAddon});
> +    }

This shouldn't be necessary. The syncLoadManifest gunk already calls updateBlocklistState() and spins the event loop while it waits for all of the initialization stuff to complete. We should probably just pass the old add-on through to the parsing code, and use it in the initial updateBlocklistState call.

::: toolkit/mozapps/extensions/internal/XPIProviderUtils.js:1351
(Diff revision 1)
> -    aOldAddon.updateBlocklistState({updateDatabase: false});
> -    aOldAddon.appDisabled = !isUsableAddon(aOldAddon);
> +    // We don't check the blocklist here. We'll do this after the rest of
> +    // our checks. We assume that any updates to the app don't need to
> +    // synchronously require loading the blocklist to check for issues.

We need to keep the appDisabled update here, since we call this during startup on app update to update the compatibility based on the app version.

Deferring the blocklist update is probably fine, though. App version blocklist entries are much less important for WebExtensions than they were for legacy extensions (and particularly binary extensions).

(Also, this is the reason some of the tests fail in your try push)

::: toolkit/mozapps/extensions/internal/XPIProviderUtils.js:1685
(Diff revision 1)
> +      if (changed.includes(true)) {
> +        XPIDatabase.saveChanges();
> +      }

Honestly, this optimization probably isn't necessary. `XPIDatabase.saveChanges()` should already group updates that happen close together.

::: toolkit/mozapps/extensions/nsBlocklistService.js:781
(Diff revision 1)
> +      return;
> +    }
> +    if (!this._preloadPromise) {
> +      this._preloadPromise = this._loadBlocklistAsyncInternal();
> +    }
> +    await this._preloadPromise;

Nit: s/await/return/
Attachment #8966240 - Flags: review?(kmaglione+bmo)
(In reply to Kris Maglione [:kmag] (long backlog; ping on IRC if you're blocked) from comment #6)

Thanks for the detailed review!

> ::: toolkit/mozapps/extensions/internal/XPIProviderUtils.js:1351
> (Diff revision 1)
> > -    aOldAddon.updateBlocklistState({updateDatabase: false});
> > -    aOldAddon.appDisabled = !isUsableAddon(aOldAddon);
> > +    // We don't check the blocklist here. We'll do this after the rest of
> > +    // our checks. We assume that any updates to the app don't need to
> > +    // synchronously require loading the blocklist to check for issues.
> 
> We need to keep the appDisabled update here, since we call this during
> startup on app update to update the compatibility based on the app version.
> 
> Deferring the blocklist update is probably fine, though. App version
> blocklist entries are much less important for WebExtensions than they were
> for legacy extensions (and particularly binary extensions).
> 
> (Also, this is the reason some of the tests fail in your try push)

Ah. I think I was confused because updateBlocklistState() itself sets appDisabled anyway, but I guess isUsableAddon checks other stuff as well...

> ::: toolkit/mozapps/extensions/nsBlocklistService.js:781
> (Diff revision 1)
> > +      return;
> > +    }
> > +    if (!this._preloadPromise) {
> > +      this._preloadPromise = this._loadBlocklistAsyncInternal();
> > +    }
> > +    await this._preloadPromise;
> 
> Nit: s/await/return/

That triggers an eslint warning because of the early `return;`, ie we sometimes return a value and sometimes don't. I can update the early return to `return undefined` but this seemed cleaner to me. Do you prefer the explicit return, or is there some other alternative I'm missing?
Flags: needinfo?(kmaglione+bmo)
(In reply to :Gijs (he/him) from comment #7)
> Ah. I think I was confused because updateBlocklistState() itself sets
> appDisabled anyway, but I guess isUsableAddon checks other stuff as well...

Right. In this case, it's mostly about updating compatibility ranges.

> > Nit: s/await/return/
> 
> That triggers an eslint warning because of the early `return;`, ie we
> sometimes return a value and sometimes don't. I can update the early return
> to `return undefined` but this seemed cleaner to me. Do you prefer the
> explicit return, or is there some other alternative I'm missing?

No, it's probably fine in that case. I just tend to read `await` as the last step of an async function a bug. We should probably turn off the consistent returns rule for async functions...
Flags: needinfo?(kmaglione+bmo)
(In reply to Kris Maglione [:kmag] (long backlog; ping on IRC if you're blocked) from comment #6)
> Deferring the blocklist update is probably fine, though. App version
> blocklist entries are much less important for WebExtensions than they were
> for legacy extensions (and particularly binary extensions).
> 
> (Also, this is the reason some of the tests fail in your try push)


So this still bit me a bit, because when running test_upgrade_incompatible, first various bits of code set appDisabled to true. But then we somehow (asynchronously) end up setting it back to false in updateBlocklistState, with this stack:

 0:01.32 INFO "CONSOLE_MESSAGE: (error) [JavaScript Error: "Updating appDisabled: false" {file: "resource://gre/modules/addons/XPIProvider.jsm" line: 4636}]
updateBlocklistState@resource://gre/modules/addons/XPIProvider.jsm:4636:7
async*processFileChanges/<@resource://gre/modules/addons/XPIProvider.jsm -> resource://gre/modules/addons/XPIProviderUtils.js:1672:14
processFileChanges@resource://gre/modules/addons/XPIProvider.jsm -> resource://gre/modules/addons/XPIProviderUtils.js:1671:17
checkForChanges@resource://gre/modules/addons/XPIProvider.jsm:3091:34
startup@resource://gre/modules/addons/XPIProvider.jsm:2122:25

I fixed by making the block at the end of processFileChanges always fetch the add-on from the AddonManager, to make sure it gets an up-to-date object/wrapper, but I assume there must be a cheaper / better way of doing this - just can't really tell what that is. `processFileChanges` as a method is pretty huge. I'm also nervous that in production code, waiting for the blocklist to load could introduce enough asynchronicity that even the new wrapper might be outdated. Kris, is there something I can/should do to avoid this? (I'll put up the updated patch in a sec so you can have a look...)
Comment on attachment 8966240 [details]
Bug 1452618 - make getAddonBlocklistEntry asynchronous,

https://reviewboard.mozilla.org/r/234986/#review241500


Code analysis found 2 defects in this patch:
 - 2 defects found by mozlint

You can run this analysis locally with:
 - `./mach lint path/to/file` (JS/Python)


If you see a problem in this automated review, please report it here: http://bit.ly/2y9N9Vx


::: toolkit/mozapps/extensions/internal/XPIProviderUtils.js:1673
(Diff revision 2)
> +    // Do some blocklist checks. These will happen after we've just saved everything,
> +    // because they're async and depend on the blocklist loading. When we're done, save
> +    // the data if any of the add-ons' blocklist state has changed.
> +    Promise.all(addonsToCheckAgainstBlocklist.map(id => {
> +      return AddonManager.getAddonByID(id).then(addon =>
> +        addon && addon.updateBlocklistState({updateDatabase: false}), () => {/* swallow rejections */});

Error: Requires a space after '{' [eslint: block-spacing]

::: toolkit/mozapps/extensions/internal/XPIProviderUtils.js:1673
(Diff revision 2)
> +    // Do some blocklist checks. These will happen after we've just saved everything,
> +    // because they're async and depend on the blocklist loading. When we're done, save
> +    // the data if any of the add-ons' blocklist state has changed.
> +    Promise.all(addonsToCheckAgainstBlocklist.map(id => {
> +      return AddonManager.getAddonByID(id).then(addon =>
> +        addon && addon.updateBlocklistState({updateDatabase: false}), () => {/* swallow rejections */});

Error: Requires a space before '}' [eslint: block-spacing]
(In reply to :Gijs (he/him) from comment #9)
> `processFileChanges` as a method is pretty huge.

Yeah, sorry about that. I've been working on making all of this
stuff sane, but it's a slow process.

> I'm also nervous that in production code, waiting for the
> blocklist to load could introduce enough asynchronicity that even the new
> wrapper might be outdated.

Wrappers don't become outdated. But there are some corner cases
where they don't mean what you think they mean and we don't
handle it properly. For instance, after an add-on is updated,
the new version gets a new Addon object, but the old one
continues to more or less work. We also have separate
AddonInternal instances that are not DBAddon instances
sometimes, which is... annoying.
Comment on attachment 8966240 [details]
Bug 1452618 - make getAddonBlocklistEntry asynchronous,

https://reviewboard.mozilla.org/r/234986/#review241632

Thanks

::: toolkit/mozapps/extensions/internal/XPIProviderUtils.js:1461
(Diff revision 3)
>                // the manifests.
>                newAddon = this.updateCompatibility(installLocation, oldAddon, xpiState,
> -                                                  aOldAppVersion, aOldPlatformVersion,
>                                                    aSchemaChange);
> +              // We need to do a blocklist check later, but the add-on may have changed by then.
> +              // Avoid storing the current copy and fetch it out of XPIStates at that point

Nit: XPIStates doesn't have all that much to do with this.

::: toolkit/mozapps/extensions/internal/XPIProviderUtils.js:1671
(Diff revision 3)
> +    Promise.all(addonsToCheckAgainstBlocklist.map(id => {
> +      return AddonManager.getAddonByID(id).then(

`AddonManager.getAddonsByIDs(addonsToCheckAgainstBlocklist).then(addons => ...)`

::: toolkit/mozapps/extensions/internal/XPIProviderUtils.js:1674
(Diff revision 3)
> +    // because they're async and depend on the blocklist loading. When we're done, save
> +    // the data if any of the add-ons' blocklist state has changed.
> +    Promise.all(addonsToCheckAgainstBlocklist.map(id => {
> +      return AddonManager.getAddonByID(id).then(
> +        addon => addon && addon.updateBlocklistState({updateDatabase: false}),
> +        () => { /* swallow rejections */ });

I'd kind of rather we not swallow rejections...

::: toolkit/mozapps/extensions/internal/XPIProviderUtils.js:1676
(Diff revision 3)
> +    Promise.all(addonsToCheckAgainstBlocklist.map(id => {
> +      return AddonManager.getAddonByID(id).then(
> +        addon => addon && addon.updateBlocklistState({updateDatabase: false}),
> +        () => { /* swallow rejections */ });
> +    })).then(() => {
> +      // May have shut down in the meantime.

Hm. It would probably be a good idea to add a shutdown blocker for this so we don't wind up in an inconsistent state if we shutdown before blocklist updates are complete...
Attachment #8966240 - Flags: review?(kmaglione+bmo) → review+
Comment on attachment 8966240 [details]
Bug 1452618 - make getAddonBlocklistEntry asynchronous,

https://reviewboard.mozilla.org/r/234986/#review241776

::: toolkit/mozapps/extensions/nsBlocklistService.js:807
(Diff revisions 3 - 4)
> +    if (this.isLoaded) {
> +      return;
> +    }

Realized just in time that we should probably keep this while we still support sync loading, because otherwise the blocklist could load sync while between the 2 attempts to load the profile/appdir versions of the blocklist. We can get rid of it once we remove the final sync consumers here.
Hm, so the new trypush has new orange:

https://treeherder.mozilla.org/#/jobs?repo=try&revision=60d255c65f5b778967a0a3c7c0c5f916d56a00d8&selectedJob=173267988

TEST-UNEXPECTED-FAIL | xpcshell.ini:toolkit/mozapps/extensions/test/xpcshell/test_blocklistchange.js | app_update_step_2 - [app_update_step_2 : 662] 0 == 1

What seems to be happening is that the blocklistchange test installs an add-on that is at first not blocklisted, and then becomes blocklisted once the app is updated (after an AM restart) and then later becomes non-blocklisted. The test fails very quickly, because after the first restart the add-on is still unblocklisted.

At first I thought this was just the asynchronicity of the blocklist-based update, but adding a dumb timeout doesn't fix anything. On closer inspection, when running this test, this code:

    AddonManager.shutdown.addBlocker(
      "Update add-on blocklist state into add-on DB",
      AddonManager.getAddonsByIDs(addonsToCheckAgainstBlocklist).then(addons => {
        for (let addon of addons) {
          if (addon) {
            addon.updateBlocklistState({updateDatabase: false});
          }
        }
        XPIDatabase.saveChanges();
      }).catch(Cu.reportError)
    );


is getting `null` for every add-on the test uses at that point. The IDs in the `addonsToCheckAgainstBlocklist` list are correct. I assume this means that somehow asking the AddonManager at this point in time is not right, either. I think that maybe this is because the XPIDatabase update is async so at this point no add-ons are available yet - `saveChanges` has a backoff timer so presumably that'd explain it. But I'm not sure what the "right" way is either.

I also just realized this code should be waiting for the blocklist state update before writing, so should be an async method and use `await` for the updateBlocklistState call...

Closer reading of the earlier code in this method suggests that `currentVisible` is a map of id=>addon object for currently "visible" add-ons, and `previousVisible` is one for ones that used to be visible. I don't know if using the 'current' one here is correct, or if I need to check both, or neither - depends what "visible" means...

Kris, can you make a suggestion on how to resolve this conundrum? Sorry that this isn't as smooth a process as you might like...
Flags: needinfo?(kmaglione+bmo)
The other bit of the confusion I have here is that apparently the AM keeps track of "startup changes", and disabling as a result of these blocklist changes won't be in that set unless we do something (AIUI). I can't tell if that's a problem or not.
Using the add-on objects that are already in scope brings back other failures, like the one in comment 9.

I now suspect that the issue is that updateBlocklistState in XPIProvider.jsm sets appDisabled based on isAddonUsable(), but there are some failure cases where we set appDisabled directly (e.g. if the manifest fails to load), and doing the blocklist check later means that that state gets clobbered if the add-on isn't blocklisted (ie we set appDisabled to false when it should be true).

I'm... not sure what the best way of fixing that is. It feels like persistent state like "couldn't load this manifest" ought to be tracked in some bool other than the appDisabled thing to which people write semi-willy-nilly, and then we can update isAddonUsable to take that bool into account...
(In reply to :Gijs (he/him) from comment #17)
>     AddonManager.shutdown.addBlocker(
>       "Update add-on blocklist state into add-on DB",
>       AddonManager.getAddonsByIDs(addonsToCheckAgainstBlocklist).then(addons
> => {
>         for (let addon of addons) {
>           if (addon) {
>             addon.updateBlocklistState({updateDatabase: false});
>           }
>         }
>         XPIDatabase.saveChanges();
>       }).catch(Cu.reportError)
>     );
> 
> 
> is getting `null` for every add-on the test uses at that point.

These are basically just timing issues. You can't call getAddonsByIDs before startup has finished. Something like this fixes the immediate problems, though the exact details are obviously less than ideal:

diff --git a/toolkit/mozapps/extensions/internal/XPIProviderUtils.js b/toolkit/mozapps/extensions/internal/XPIProviderUtils.js
--- a/toolkit/mozapps/extensions/internal/XPIProviderUtils.js
+++ b/toolkit/mozapps/extensions/internal/XPIProviderUtils.js
@@ -1664,21 +1664,22 @@ this.XPIDatabaseReconcile = {
     XPIDatabase.migrateData = null;
     XPIDatabase.saveChanges();
 
     // Do some blocklist checks. These will happen after we've just saved everything,
     // because they're async and depend on the blocklist loading. When we're done, save
     // the data if any of the add-ons' blocklist state has changed.
     AddonManager.shutdown.addBlocker(
       "Update add-on blocklist state into add-on DB",
-      AddonManager.getAddonsByIDs(addonsToCheckAgainstBlocklist).then(addons => {
-        for (let addon of addons) {
-          if (addon) {
-            addon.updateBlocklistState({updateDatabase: false});
+      Promise.resolve().then(() =>
+        AddonManager.getAddonsByIDs(addonsToCheckAgainstBlocklist)).then(addons => {
+          for (let addon of addons) {
+            if (addon) {
+              addon.updateBlocklistState({updateDatabase: false});
+            }
           }
-        }
-        XPIDatabase.saveChanges();
-      }).catch(Cu.reportError)
+          XPIDatabase.saveChanges();
+        }).catch(Cu.reportError)
     );
 
     return true;
   },
 };
diff --git a/toolkit/mozapps/extensions/test/xpcshell/test_blocklistchange.js b/toolkit/mozapps/extensions/test/xpcshell/test_blocklistchange.js
--- a/toolkit/mozapps/extensions/test/xpcshell/test_blocklistchange.js
+++ b/toolkit/mozapps/extensions/test/xpcshell/test_blocklistchange.js
@@ -794,23 +794,27 @@ add_task(async function run_app_update_s
   check_addon(s1, "1.0", false, false, Ci.nsIBlocklistService.STATE_NOT_BLOCKED);
   check_addon(s2, "1.0", false, false, Ci.nsIBlocklistService.STATE_NOT_BLOCKED);
   check_addon(s3, "1.0", false, false, Ci.nsIBlocklistService.STATE_NOT_BLOCKED);
   check_addon(s4, "1.0", true, false, Ci.nsIBlocklistService.STATE_NOT_BLOCKED);
   check_addon(h, "1.0", false, false, Ci.nsIBlocklistService.STATE_NOT_BLOCKED);
   check_addon(r, "1.0", false, false, Ci.nsIBlocklistService.STATE_NOT_BLOCKED);
 });
 
+ChromeUtils.import("resource://gre/modules/Timer.jsm");
+
 add_task(async function update_schema_2() {
   await promiseShutdownManager();
 
   await changeXPIDBVersion(100);
   gAppInfo.version = "2";
   startupManager(true);
 
+  await new Promise(resolve => setTimeout(resolve, 0));
+
   let [s1, s2, s3, s4, h, r] = await promiseAddonsByIDs(ADDON_IDS);
 
   check_addon(s1, "1.0", true, true, Ci.nsIBlocklistService.STATE_SOFTBLOCKED);
   check_addon(s2, "1.0", true, true, Ci.nsIBlocklistService.STATE_SOFTBLOCKED);
   check_addon(s3, "1.0", true, true, Ci.nsIBlocklistService.STATE_SOFTBLOCKED);
   check_addon(s4, "1.0", true, false, Ci.nsIBlocklistService.STATE_SOFTBLOCKED);
   check_addon(h, "1.0", false, false, Ci.nsIBlocklistService.STATE_BLOCKED);
   check_addon(r, "1.0", false, false, Ci.nsIBlocklistService.STATE_BLOCKED);
@@ -825,32 +829,36 @@ add_task(async function update_schema_2(
 add_task(async function update_schema_3() {
   await promiseRestartManager();
 
   await promiseShutdownManager();
   await changeXPIDBVersion(100);
   gAppInfo.version = "2.5";
   startupManager(true);
 
+  await new Promise(resolve => setTimeout(resolve, 0));
+
   let [s1, s2, s3, s4, h, r] = await promiseAddonsByIDs(ADDON_IDS);
 
   check_addon(s1, "1.0", true, true, Ci.nsIBlocklistService.STATE_SOFTBLOCKED);
   check_addon(s2, "1.0", true, false, Ci.nsIBlocklistService.STATE_SOFTBLOCKED);
   check_addon(s3, "1.0", false, false, Ci.nsIBlocklistService.STATE_SOFTBLOCKED);
   check_addon(s4, "1.0", true, false, Ci.nsIBlocklistService.STATE_SOFTBLOCKED);
   check_addon(h, "1.0", false, false, Ci.nsIBlocklistService.STATE_BLOCKED);
   check_addon(r, "1.0", false, false, Ci.nsIBlocklistService.STATE_BLOCKED);
 });
 
 add_task(async function update_schema_4() {
   await promiseShutdownManager();
 
   await changeXPIDBVersion(100);
   startupManager(false);
 
+  await new Promise(resolve => setTimeout(resolve, 0));
+
   let [s1, s2, s3, s4, h, r] = await promiseAddonsByIDs(ADDON_IDS);
 
   check_addon(s1, "1.0", true, true, Ci.nsIBlocklistService.STATE_SOFTBLOCKED);
   check_addon(s2, "1.0", true, false, Ci.nsIBlocklistService.STATE_SOFTBLOCKED);
   check_addon(s3, "1.0", false, false, Ci.nsIBlocklistService.STATE_SOFTBLOCKED);
   check_addon(s4, "1.0", true, false, Ci.nsIBlocklistService.STATE_SOFTBLOCKED);
   check_addon(h, "1.0", false, false, Ci.nsIBlocklistService.STATE_BLOCKED);
   check_addon(r, "1.0", false, false, Ci.nsIBlocklistService.STATE_BLOCKED);
@@ -858,16 +866,18 @@ add_task(async function update_schema_4(
 
 add_task(async function update_schema_5() {
   await promiseShutdownManager();
 
   await changeXPIDBVersion(100);
   gAppInfo.version = "1";
   startupManager(true);
 
+  await new Promise(resolve => setTimeout(resolve, 0));
+
   let [s1, s2, s3, s4, h, r] = await promiseAddonsByIDs(ADDON_IDS);
 
   check_addon(s1, "1.0", false, false, Ci.nsIBlocklistService.STATE_NOT_BLOCKED);
   check_addon(s2, "1.0", true, false, Ci.nsIBlocklistService.STATE_NOT_BLOCKED);
   check_addon(s3, "1.0", false, false, Ci.nsIBlocklistService.STATE_NOT_BLOCKED);
   check_addon(s4, "1.0", true, false, Ci.nsIBlocklistService.STATE_NOT_BLOCKED);
   check_addon(h, "1.0", false, false, Ci.nsIBlocklistService.STATE_NOT_BLOCKED);
   check_addon(r, "1.0", false, false, Ci.nsIBlocklistService.STATE_NOT_BLOCKED);

(In reply to :Gijs (he/him) from comment #18)
> The other bit of the confusion I have here is that apparently the AM keeps
> track of "startup changes", and disabling as a result of these blocklist
> changes won't be in that set unless we do something (AIUI). I can't tell if
> that's a problem or not.

Don't even get me started on startup changes. The short answer is that if no tests fail, don't worry about it. The long answer is that after these patches, the changes probably happen late enough for anything that still cares about startup changes to see them. But I really wish startup changes weren't a thing, and now that non-bootstrapped add-ons are gone, and legacy extensions don't matter, I'd really like to get rid of them.
Flags: needinfo?(kmaglione+bmo)
Comment on attachment 8966240 [details]
Bug 1452618 - make getAddonBlocklistEntry asynchronous,

Eh, these 2 csets show up in the wrong order in the bug, but it shouldn't really matter. Just doublechecking that this way of fixing up tests and asynchronicity is OK to ship.
Attachment #8966240 - Flags: review+ → review?(kmaglione+bmo)
Comment on attachment 8966240 [details]
Bug 1452618 - make getAddonBlocklistEntry asynchronous,

https://reviewboard.mozilla.org/r/234986/#review242254

::: toolkit/mozapps/extensions/internal/AddonTestUtils.jsm:733
(Diff revision 5)
>        this.addonStartup.remove(true);
>  
>      this.addonIntegrationService = Cc["@mozilla.org/addons/integration;1"]
>            .getService(Ci.nsIObserver);
>  
> +    let blocklistLoaded = appChanged ? TestUtils.topicObserved("addonmanager-blocklist-processed") : Promise.resolve();

It seems this just causes other tests to time out because they pass `true`, because the default here is `true`, which then causes us to wait for the blocklist change, which doesn't happen if the addons manager doesn't find any reason to call `processFileChanges` .

Not sure how best to fix that yet. I could add an extra flag, but then I have to update a bunch of tests, or I could change the default for the property - and then I'd probably also have to update a lot of tests. :-\
(In reply to :Gijs (he/him) from comment #24)
> Comment on attachment 8966240 [details]
> Bug 1452618 - make getAddonBlocklistEntry asynchronous,
> 
> https://reviewboard.mozilla.org/r/234986/#review242254
> 
> ::: toolkit/mozapps/extensions/internal/AddonTestUtils.jsm:733
> (Diff revision 5)
> >        this.addonStartup.remove(true);
> >  
> >      this.addonIntegrationService = Cc["@mozilla.org/addons/integration;1"]
> >            .getService(Ci.nsIObserver);
> >  
> > +    let blocklistLoaded = appChanged ? TestUtils.topicObserved("addonmanager-blocklist-processed") : Promise.resolve();
> 
> It seems this just causes other tests to time out because they pass `true`,
> because the default here is `true`, which then causes us to wait for the
> blocklist change, which doesn't happen if the addons manager doesn't find
> any reason to call `processFileChanges` .
> 
> Not sure how best to fix that yet. I could add an extra flag, but then I
> have to update a bunch of tests, or I could change the default for the
> property - and then I'd probably also have to update a lot of tests. :-\

Huh, actually, it seems like the changes in the first cset mean we don't need to do more waiting in the second. Or something. At least, I can't reproduce any more failures after just removing the code waiting for the blocklist update to complete...
Comment on attachment 8966240 [details]
Bug 1452618 - make getAddonBlocklistEntry asynchronous,

Stupid mozreview.
Attachment #8966240 - Flags: review?(kmaglione+bmo)
Comment on attachment 8966240 [details]
Bug 1452618 - make getAddonBlocklistEntry asynchronous,

Hopefully this is the last one...
Attachment #8966240 - Flags: review?(kmaglione+bmo)
Comment on attachment 8967726 [details]
Bug 1452618 - track force-disabling things so we don't accidentally re-enable them by using `isUsableAddon` later,

https://reviewboard.mozilla.org/r/236436/#review242382

::: toolkit/mozapps/extensions/internal/XPIProviderUtils.js:1324
(Diff revision 1)
>          let file = new nsIFile(aAddonState.path);
>          manifest = syncLoadManifestFromFile(file, aInstallLocation);
>        } catch (err) {
>          // If we can no longer read the manifest, it is no longer compatible.
> -        aOldAddon.appDisabled = true;
> +        aOldAddon.brokenManifest = true;
> +        aOldAddon.appDisabled = !isUsableAddon(aOldAddon);

This can just be `appDisabled = true`
Attachment #8967726 - Flags: review?(kmaglione+bmo) → review+
Comment on attachment 8966240 [details]
Bug 1452618 - make getAddonBlocklistEntry asynchronous,

https://reviewboard.mozilla.org/r/234986/#review242384

::: toolkit/mozapps/extensions/internal/XPIProviderUtils.js:1678
(Diff revision 8)
> +        for (let addon of addons) {
> +          if (addon) {
> +            await addon.updateBlocklistState({updateDatabase: false});

This should probably be something like:

    await Promise.all(addons.map(addon => addon && addon.updateBlocklistState()));

It probably doesn't matter much now, but when we switch to an indexedDB backend, each update will have its own latency rather than just the one-time latency of loading the blocklist file.
Attachment #8966240 - Flags: review?(kmaglione+bmo) → review+
Pushed by gijskruitbosch@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/fdcb9f2ec9d9
track force-disabling things so we don't accidentally re-enable them by using `isUsableAddon` later, r=kmag
https://hg.mozilla.org/integration/autoland/rev/5bf3bfedd867
make getAddonBlocklistEntry asynchronous, r=kmag
Backout by rgurzau@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/e63607c0931b
Backed out 2 changesets for frequently failing mochitest on Android on a CLOSED TREE
We're sorry, Autoland could not rebase your commits for you automatically. Please manually rebase your commits and try again.

hg error in cmd: hg rebase -s f5349905640c1c7403c11df9370f9a38c1d35584 -d 38a419def0a7: rebasing 458569:f5349905640c "Bug 1452618 - track force-disabling things so we don't accidentally re-enable them by using `isUsableAddon` later, r=kmag"
merging toolkit/mozapps/extensions/internal/XPIProvider.jsm
merging toolkit/mozapps/extensions/internal/XPIProviderUtils.js
merging toolkit/mozapps/extensions/test/xpcshell/test_upgrade_incompatible.js
warning: conflicts while merging toolkit/mozapps/extensions/internal/XPIProvider.jsm! (edit, then use 'hg resolve --mark')
unresolved conflicts (see hg resolve, then hg rebase --continue)
Pushed by gijskruitbosch@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/72b3036ce450
track force-disabling things so we don't accidentally re-enable them by using `isUsableAddon` later, r=kmag
https://hg.mozilla.org/integration/autoland/rev/a0a108e7e7b3
make getAddonBlocklistEntry asynchronous, r=kmag
https://hg.mozilla.org/mozilla-central/rev/72b3036ce450
https://hg.mozilla.org/mozilla-central/rev/a0a108e7e7b3
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla61
Depends on: 1454563
You need to log in before you can comment on or make changes to this bug.