Closed
Bug 1356826
Opened 8 years ago
Closed 8 years ago
Get rid of directory scans, and especially recursive directory scan, at startup
Categories
(Toolkit :: Add-ons Manager, enhancement, P1)
Toolkit
Add-ons Manager
Tracking
()
RESOLVED
FIXED
mozilla55
People
(Reporter: kmag, Assigned: kmag)
References
(Blocks 1 open bug)
Details
(Whiteboard: triaged)
Attachments
(7 files)
(deleted),
text/x-review-board-request
|
rhelmer
:
review+
|
Details |
(deleted),
text/x-review-board-request
|
rhelmer
:
review+
|
Details |
(deleted),
text/x-review-board-request
|
rhelmer
:
review+
|
Details |
(deleted),
text/x-review-board-request
|
bholley
:
review+
|
Details |
(deleted),
text/x-review-board-request
|
rhelmer
:
review+
|
Details |
(deleted),
text/x-review-board-request
|
aswan
:
review+
|
Details |
(deleted),
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.
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment 8•8 years ago
|
||
mozreview-review |
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 9•8 years ago
|
||
mozreview-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+
Assignee | ||
Comment 10•8 years ago
|
||
mozreview-review-reply |
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 11•8 years ago
|
||
mozreview-review |
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 12•8 years ago
|
||
mozreview-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 13•8 years ago
|
||
mozreview-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 14•8 years ago
|
||
mozreview-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.
Assignee | ||
Comment 15•8 years ago
|
||
mozreview-review-reply |
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 16•8 years ago
|
||
mozreview-review-reply |
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
Assignee | ||
Comment 17•8 years ago
|
||
(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 18•8 years ago
|
||
mozreview-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
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?
Assignee | ||
Comment 19•8 years ago
|
||
mozreview-review-reply |
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 hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment 27•8 years ago
|
||
mozreview-review-reply |
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.
Comment 28•8 years ago
|
||
I'm good with Kris' approach.
Assignee | ||
Updated•8 years ago
|
Keywords: leave-open
Assignee | ||
Comment 29•8 years ago
|
||
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 30•8 years ago
|
||
bugherder |
Comment 31•8 years ago
|
||
mozreview-review |
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+
Assignee | ||
Comment 32•8 years ago
|
||
mozreview-review-reply |
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 33•8 years ago
|
||
mozreview-review-reply |
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 34•8 years ago
|
||
mozreview-review |
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+
Updated•8 years ago
|
Priority: -- → P1
Whiteboard: triaged
Assignee | ||
Comment 35•8 years ago
|
||
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
Comment 36•8 years ago
|
||
backed out for talos error like https://treeherder.mozilla.org/logviewer.html#?job_id=98637822&repo=mozilla-inbound&lineNumber=1131
Flags: needinfo?(kmaglione+bmo)
Assignee | ||
Comment 37•8 years ago
|
||
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 38•8 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/8b38a108097f
https://hg.mozilla.org/mozilla-central/rev/7613b01bb4bf
https://hg.mozilla.org/mozilla-central/rev/3aa9fb530be2
https://hg.mozilla.org/mozilla-central/rev/4584ea301bd2
https://hg.mozilla.org/mozilla-central/rev/79fbcdee024b
https://hg.mozilla.org/mozilla-central/rev/d77136688827
https://hg.mozilla.org/mozilla-central/rev/6bc3416bc56e
Comment 39•8 years ago
|
||
mozreview-review |
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)
Assignee | ||
Updated•8 years ago
|
Status: NEW → RESOLVED
Closed: 8 years ago
Flags: needinfo?(kmaglione+bmo)
Keywords: leave-open
Resolution: --- → FIXED
Updated•7 years ago
|
Target Milestone: --- → mozilla55
You need to log in
before you can comment on or make changes to this bug.
Description
•