Closed Bug 1356826 Opened 7 years ago Closed 7 years ago

Get rid of directory scans, and especially recursive directory scan, at startup

Categories

(Toolkit :: Add-ons Manager, enhancement, P1)

Product:
Toolkit
Component:
Add-ons Manager
Type:
enhancement

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla55

People

(Reporter: kmag, Assigned: kmag)

References

(Blocks 1 open bug)

Details

(Whiteboard: triaged)

Attachments

(7 files)

Bug 1356826: Part 1 - Uninstall temporary add-ons at shutdown rather than startup.
59 bytes, text/x-review-board-request
rhelmer
: review+
Details
Bug 1356826: Part 2 - Don't scan directories for changes at startup without pref.
59 bytes, text/x-review-board-request
rhelmer
: review+
Details
Bug 1356826: Part 3 - Get rid of recursive last modified scan.
59 bytes, text/x-review-board-request
rhelmer
: review+
Details
Bug 1356826: Part 4 - Add Cu.isInAutomation and Cu.crashIfNotInAutomation helpers.
59 bytes, text/x-review-board-request
bholley
: review+
Details
Bug 1356826: Part 5 - Allow unsigned add-ons in automation.
59 bytes, text/x-review-board-request
rhelmer
: review+
Details
Bug 1356826: Part 6 - Wait for delayed startup before checking for side-loads.
59 bytes, text/x-review-board-request
aswan
: review+
Details
Bug 1356826: Part 7 - Scan for extension sideloads after final UI startup.
59 bytes, text/x-review-board-request
aswan
: review+
Details 
This is way too slow, and mostly unnecessary. If we want to detect new add-ons dropped into an install location, we can do so after startup is complete. If we want to detect modifications to unpacked developer add-ons, we can put that behind a pref. Missing add-ons we'll detect when we try to load them.

Since we rely on some of this behavior for tests, we'll probably need to preserve the startup directory scan for scopes that aren't in the autoDisableScopes pref.
Blocks: 1356828
Comment on attachment 8858570 [details]
Bug 1356826: Part 4 - Add Cu.isInAutomation and Cu.crashIfNotInAutomation helpers.

https://reviewboard.mozilla.org/r/130548/#review133304
Attachment #8858570 - Flags: review?(bobbyholley) → review+
Comment on attachment 8858567 [details]
Bug 1356826: Part 1 - Uninstall temporary add-ons at shutdown rather than startup.

https://reviewboard.mozilla.org/r/130542/#review133406

::: toolkit/mozapps/extensions/internal/XPIProvider.jsm:2259
(Diff revision 1)
>    },
>  };
>  
>  // Constructor for an ES6 Map that knows how to convert itself into a
>  // regular object for toJSON().
> -function SerializableMap() {
> +function SerializableMap(arg) {

Please use a more informative name for `arg` and add a jsdoc comment.

::: toolkit/mozapps/extensions/internal/XPIProviderUtils.js:1326
(Diff revision 1)
> +    let result;
> +    for (let [, addon] of this.addonDB) {
> +      if (addon.id != aId) {
> +        continue;
> +      }
> +      if (addon.location == aLocation) {

Why not just call `makeAddonVisible` here?
Attachment #8858567 - Flags: review?(rhelmer) → review+
Comment on attachment 8858567 [details]
Bug 1356826: Part 1 - Uninstall temporary add-ons at shutdown rather than startup.

https://reviewboard.mozilla.org/r/130542/#review133406

> Please use a more informative name for `arg` and add a jsdoc comment.

Well, it's just passing the argument along to the Map constructor. How about `(...args) { new Map(...args); `?

> Why not just call `makeAddonVisible` here?

Well, for a start, `makeAddonVisible` needs to be called with an addon object, and we need to iterate over the list of add-ons to get that. I suppose we could call `makeAddonVisible` once we have it, but then that would iterate over the list again, which seems like a bit of a waste. It would also call `saveChanges`, which would probably still be fine at this point in shutdown, but would also be unnecessary.
Comment on attachment 8858572 [details]
Bug 1356826: Part 6 - Wait for delayed startup before checking for side-loads.

https://reviewboard.mozilla.org/r/130552/#review133840
Attachment #8858572 - Flags: review?(aswan) → review+
Comment on attachment 8858568 [details]
Bug 1356826: Part 2 - Don't scan directories for changes at startup without pref.

https://reviewboard.mozilla.org/r/130544/#review133938
Attachment #8858568 - Flags: review?(rhelmer) → review+
Comment on attachment 8858569 [details]
Bug 1356826: Part 3 - Get rid of recursive last modified scan.

https://reviewboard.mozilla.org/r/130546/#review133950

::: toolkit/mozapps/extensions/test/xpcshell/test_bootstrap.js:1056
(Diff revision 1)
>  function run_test_22() {
>    shutdownManager();
>  
>    let file = manuallyInstall(do_get_addon("test_bootstrap1_1"), profileDir,
>                               ID1);
> +  if (file.isDirectory())

please use braces
Attachment #8858569 - Flags: review?(rhelmer) → review+
Comment on attachment 8858571 [details]
Bug 1356826: Part 5 - Allow unsigned add-ons in automation.

https://reviewboard.mozilla.org/r/130550/#review133962

::: toolkit/mozapps/extensions/internal/XPIProvider.jsm:5187
(Diff revision 1)
> +    } finally {
> +      // Make sure we don't have an active unsigned add-on in a release build
> +      // unless we're running in automation.
> +      if (REQUIRE_SIGNING && !aAddon.isCorrectlySigned && !aAddon.appDisabled &&
> +          SIGNED_TYPES.has(aAddon.type)) {
> +        Cu.crashIfNotInAutomation();

This code looks like it is here to protect us from breaking the code above. I feel pretty strongly that we shouldn't be crashing the user if we've made a mistake in our code.
Comment on attachment 8858571 [details]
Bug 1356826: Part 5 - Allow unsigned add-ons in automation.

https://reviewboard.mozilla.org/r/130550/#review133962

> This code looks like it is here to protect us from breaking the code above. I feel pretty strongly that we shouldn't be crashing the user if we've made a mistake in our code.

It's there to protect us from some unexpected situation where the value of `isInAutomation` during runtime, and an unsigned extension has been installed in the mean time. In that case, someone is trying to do something that they shouldn't, and we should be crashing.
Comment on attachment 8858571 [details]
Bug 1356826: Part 5 - Allow unsigned add-ons in automation.

https://reviewboard.mozilla.org/r/130550/#review133962

> It's there to protect us from some unexpected situation where the value of `isInAutomation` during runtime, and an unsigned extension has been installed in the mean time. In that case, someone is trying to do something that they shouldn't, and we should be crashing.

It doesn't do anything to correct the problem though, the user can just restart the browser and carry on. In the meantime it opens up the risk of getting the database into an inconsistent state with prefs
(In reply to Dave Townsend [:mossop] from comment #16)
> It doesn't do anything to correct the problem though, the user can just
> restart the browser and carry on. In the meantime it opens up the risk of
> getting the database into an inconsistent state with prefs

Well, it triggers a crash report, which would tell us that this is possible and happening. We do the same thing for other functionality that's only supposed to be usable in tests, which is why the native version of this function already exists.
Comment on attachment 8858573 [details]
Bug 1356826: Part 7 - Scan for extension sideloads after final UI startup.

https://reviewboard.mozilla.org/r/130554/#review134688

A few little line-item comments below but my big reaction is that I don't love the way testing is handled here.  I guess this depends on the fate of part 5 but how about an alternative approach for `browser_extension_sideloading.js`:
create something like `ExtensionsUI._setSideloaded()` that takes has most of the guts of the existing `_checkForSideloaded()`.  Make `_checkForSideloaded()` simply call `_setSideloaded()` with the results from `AddonManagerPrivate.getNewSideloaded()` but `browser_extension_sideloading.js` can keep the current MockProvider approach and call `_setSideloaded()` directly.  Then the hack to ignore unsigned addons in automation (the one in this patch) isn't necessary.

Looking at the bigger picture, I think it makes sense to carefully distinguish unit tests for the bits under toolkit/mozapps/extensions from the tests for the desktop browser UI and this would reinforce that (`browser_extension_sideloading` would bypass all the XPIProvider stuff and be clearly just about the UI).  Related to that, it seems like there should be new xpcshell tests in toolkit/mozapps/extensions for the code changed here?  Actually, I haven't looked closely but I would assume there were existing tests of the sideloading logic, how are they not broken without explicitly doing a sideload scan?

::: browser/base/content/test/webextensions/head.js:221
(Diff revision 1)
>    let icon = panel.getAttribute("icon");
>    let ul = document.getElementById("addon-webext-perm-list");
>    let header = document.getElementById("addon-webext-perm-intro");
>  
>    if (checkIcon instanceof RegExp) {
> -    ok(checkIcon.test(icon), "Notification icon is correct");
> +    ok(checkIcon.test(icon), `Notification icon is correct ${JSON.stringify(icon)} ~= ${checkIcon}`);

did you mean to include this?

::: toolkit/mozapps/extensions/internal/XPIProvider.jsm:2231
(Diff revision 1)
>   */
>  this.XPIStates = {
>    // Map(location name -> Map(add-on ID -> XPIState))
>    db: null,
>  
> +  sideLoadedAddons: new Map(),

Can you add a comment explaining what the values (and keys) are

::: toolkit/mozapps/extensions/internal/XPIProvider.jsm:3890
(Diff revision 1)
> +    Services.prefs.setCharPref(PREF_BOOTSTRAP_ADDONS,
> +                               JSON.stringify(this.bootstrappedAddons));
> +  },
> +
> +  /**
> +   * Gets an array of add-ons which were side-loaded prior to the last

nit: this is confusingly worded, how about something like "prior to startup of the current session"

::: toolkit/mozapps/extensions/internal/XPIProviderUtils.js:1707
(Diff revision 1)
>              + aInstallLocation.name);
>          aNewAddon.userDisabled = true;
>  
>          // If we don't have an old app version then this is a new profile in
>          // which case just mark any sideloaded add-ons as already seen.
> -        aNewAddon.seen = !aOldAppVersion;
> +        aNewAddon.seen = (aInstallLocation.name != KEY_APP_PROFILE &&

Why is this added?
Comment on attachment 8858573 [details]
Bug 1356826: Part 7 - Scan for extension sideloads after final UI startup.

https://reviewboard.mozilla.org/r/130554/#review134688

I'd rather we use real add-ons than a mock provider so that we're sure we're testing the actual code. I'm not opposed to adding a separate xpcshell test, but I think there are too ways that a disconnect between the two could cause things to break without us noticing.

We could get rid of that hack by just signing the add-ons, but I'd rather we not have to sign a bunch of in-tree add-ons any time we want to add a new test.

Most of the existing tests for sideloading logic only check that side loads aren't automatically enabled at startup. They don't test for the UI portions, which are the only user visible changes here.

That said, because so many xpcshell tests rely on being able to drop sideloads in extension directories, they just disable sideload protection by default, so we still do the recursive scan immediately at startup, and everything is enabled by default.

> did you mean to include this?

Yes. When I changed the check to use a regexp, I initially had a failure there, and I had no way to find out what the failing URL was, or what pattern it was supposed to match against.

> Why is this added?

This check is only meant to ignore app dir sideloads in new profiles. It doesn't make sense to apply it add-ons in the profile itself.

But more to the point, I added it because when we call this from getNewSideloads(), we don't have an `aOldAppVersion`, so seen always winds up getting set. When we're starting up from a new profile or after an upgrade, the scan happens during early startup, before we call getNewSideloads(), so this gets called with the expected `aOldAppVersion` flag, and the old logic still applies.
Comment on attachment 8858573 [details]
Bug 1356826: Part 7 - Scan for extension sideloads after final UI startup.

https://reviewboard.mozilla.org/r/130554/#review134688

This test is very similar to those in toolkit/mozapps/extensions/test/browser/, all of which use this style (MockProvider).
I understand your concern about disconnects between how real extensions appear and the test mocks in theory, but it hasn't been a problem in practice.
That all said, I would prefer that we keep this consistent with the other tests I mentioned above but if you feel really strongly, I'll take another closer look this evening.
I'm good with Kris' approach.
Blocks: 1359558
Keywords: leave-open
https://hg.mozilla.org/integration/mozilla-inbound/rev/ca9dcf386ceb81146a0ff101dc35cbee20e212b0
Bug 1356826: Part 1 - Uninstall temporary add-ons at shutdown rather than startup. r=rhelmer
Comment on attachment 8858571 [details]
Bug 1356826: Part 5 - Allow unsigned add-ons in automation.

https://reviewboard.mozilla.org/r/130550/#review138982

::: toolkit/mozapps/extensions/internal/XPIProvider.jsm:2814
(Diff revision 2)
>  
>        Services.prefs.addObserver(PREF_EM_MIN_COMPAT_APP_VERSION, this);
>        Services.prefs.addObserver(PREF_EM_MIN_COMPAT_PLATFORM_VERSION, this);
>        Services.prefs.addObserver(PREF_E10S_ADDON_BLOCKLIST, this);
>        Services.prefs.addObserver(PREF_E10S_ADDON_POLICY, this);
> -      if (!REQUIRE_SIGNING)
> +      if (!REQUIRE_SIGNING || Cu.isInAutomation)

A few of us have reservations about this, but this is not new to this bug.

I feel OK landing this as-is, we should have a larger conversation about how we handle release builds in automation soon since I was not the only one surprised.
Attachment #8858571 - Flags: review?(rhelmer) → review+
Comment on attachment 8858573 [details]
Bug 1356826: Part 7 - Scan for extension sideloads after final UI startup.

https://reviewboard.mozilla.org/r/130554/#review134688

I don't think the test is valid if we don't actually use the XPIProvider. The getNewSideloads API calls directly into XPIProvider, and that does a directory scan at the time it's called. Just trying to mock that API would be flaky at best, and even if I did, I wouldn't be happy trusting the results.
Comment on attachment 8858573 [details]
Bug 1356826: Part 7 - Scan for extension sideloads after final UI startup.

https://reviewboard.mozilla.org/r/130554/#review134688

I don't understand that comment.  `getNewSideloads()` just returns an array of extensions, mocking that seems quite straightforward.  Whether `getNewSideloads()` works properly should be covered by a separate test, which doesn't need to be a mochitest.
Comment on attachment 8858573 [details]
Bug 1356826: Part 7 - Scan for extension sideloads after final UI startup.

https://reviewboard.mozilla.org/r/130554/#review140290

Based on our IRC discussion, I'm no board if we can also get a new xpcshell test under toolkit/mozapps/extensions that covers `getNewSideloaded()`

::: browser/base/content/test/webextensions/browser_extension_sideloading.js:256
(Diff revision 2)
>    // We should have recorded 1 cancelled followed by 3 accepted sideloads.
>    expectTelemetry(["sideloadRejected", "sideloadAccepted", "sideloadAccepted", "sideloadAccepted"]);
>  
>    isnot(menuButton.getAttribute("badge-status"), "addon-alert", "Should no longer have addon alert badge");
>  
> +  yield new Promise(resolve => setTimeout(resolve, 100));

This should of course be replaced by startup listeners above.
Attachment #8858573 - Flags: review?(aswan) → review+
Priority: -- → P1
Whiteboard: triaged
https://hg.mozilla.org/integration/mozilla-inbound/rev/527435fc20dbc7d72f51580dddab18e773cd0fa1
Bug 1356826: Part 2 - Don't scan directories for changes at startup without pref. r=rhelmer

https://hg.mozilla.org/integration/mozilla-inbound/rev/237268e3d9d2d269dbb555bed54470925d54aa88
Bug 1356826: Part 3 - Get rid of recursive last modified scan. r=rhelmer

https://hg.mozilla.org/integration/mozilla-inbound/rev/c46fed6e4f6a928df2b06fca6b3f004fd4bb0cc7
Bug 1356826: Part 4 - Add Cu.isInAutomation and Cu.crashIfNotInAutomation helpers. r=bholley

https://hg.mozilla.org/integration/mozilla-inbound/rev/b02e89c671322de742762597f26354eef239bbb5
Bug 1356826: Part 5 - Allow unsigned add-ons in automation. r=rhelmer

https://hg.mozilla.org/integration/mozilla-inbound/rev/d47998fa24cd084ffba07a73834d8ffb3af81b60
Bug 1356826: Part 6 - Wait for delayed startup before checking for side-loads. r=aswan

https://hg.mozilla.org/integration/mozilla-inbound/rev/c861e23ec8ef99e19d75e6e62dad143ccb95dffd
Bug 1356826: Part 7 - Scan for extension sideloads after final UI startup. r=aswan,rhelmer

https://hg.mozilla.org/integration/mozilla-inbound/rev/459b36092b7a9e23f35b3ab9856fd8269cdd59b6
Bug 1356826: Wait for startup to finish before shutting down. r=me
https://hg.mozilla.org/integration/mozilla-inbound/rev/8b38a108097f596fd85e6a737977ef19a6637902
Bug 1356826: Part 2 - Don't scan directories for changes at startup without pref. r=rhelmer

https://hg.mozilla.org/integration/mozilla-inbound/rev/7613b01bb4bfd04c3c68d162fc96fa2c62770bc6
Bug 1356826: Part 3 - Get rid of recursive last modified scan. r=rhelmer

https://hg.mozilla.org/integration/mozilla-inbound/rev/3aa9fb530be28b150f55f84fbe0c92bd913f98e4
Bug 1356826: Part 4 - Add Cu.isInAutomation and Cu.crashIfNotInAutomation helpers. r=bholley

https://hg.mozilla.org/integration/mozilla-inbound/rev/4584ea301bd2897007d0ffcd6512e19dc61f00a0
Bug 1356826: Part 5 - Allow unsigned add-ons in automation. r=rhelmer

https://hg.mozilla.org/integration/mozilla-inbound/rev/79fbcdee024b43ba86b6d24dd7b8b2a74018fa78
Bug 1356826: Part 6 - Wait for delayed startup before checking for side-loads. r=aswan

https://hg.mozilla.org/integration/mozilla-inbound/rev/d771366888273fc96056c05a7294755f69235a01
Bug 1356826: Part 7 - Scan for extension sideloads after final UI startup. r=aswan,rhelmer

https://hg.mozilla.org/integration/mozilla-inbound/rev/6bc3416bc56efdb78550dd308d2f392dd8aa4343
Bug 1356826: Wait for startup to finish before shutting down. r=me
Comment on attachment 8858573 [details]
Bug 1356826: Part 7 - Scan for extension sideloads after final UI startup.

https://reviewboard.mozilla.org/r/130554/#review142612
Attachment #8858573 - Flags: review?(rhelmer)
Status: NEW → RESOLVED
Closed: 7 years ago
Flags: needinfo?(kmaglione+bmo)
Keywords: leave-open
Resolution: --- → FIXED
No longer depends on: 1364878
Blocks: 1367823
Target Milestone: --- → mozilla55
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: