Closed
Bug 1350522
Opened 7 years ago
Closed 7 years ago
Lazily load and instantiate APIs when they're needed
Categories
(WebExtensions :: General, enhancement)
WebExtensions
General
Tracking
(firefox55 fixed)
RESOLVED
FIXED
mozilla55
Tracking | Status | |
---|---|---|
firefox55 | --- | fixed |
People
(Reporter: kmag, Assigned: kmag)
References
Details
Attachments
(7 files)
59 bytes,
text/x-review-board-request
|
aswan
:
review+
|
Details |
59 bytes,
text/x-review-board-request
|
aswan
:
review+
|
Details |
59 bytes,
text/x-review-board-request
|
aswan
:
review+
|
Details |
59 bytes,
text/x-review-board-request
|
aswan
:
review+
|
Details |
59 bytes,
text/x-review-board-request
|
aswan
:
review+
|
Details |
59 bytes,
text/x-review-board-request
|
aswan
:
review+
|
Details |
59 bytes,
text/x-review-board-request
|
aswan
:
review+
|
Details |
We currently load the code for all extension APIs as soon as the first extension is instantiated, and instantiate all APIs the extension has privileges for, regardless of how long it will be before they use them. We should instead load and instantiate the APIs the first time they're used, and in the parent process, we should do so asynchronously.
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 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 hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment 21•7 years ago
|
||
mozreview-review |
Comment on attachment 8851266 [details] Bug 1350522: Part 1 - Allow lazily loading and instantiating API modules. https://reviewboard.mozilla.org/r/123622/#review127788 I spent a fair amount of time with this and I still don't feel like I have my head wrapped completely around it. A few thoughts: - The detailed class-by-class and method-by-method docs are pretty good, but a big picture overview of all the different components and how they work together would be very helpful for people unfamiliar with this code (or even people are familiar but don't work with it frequently) - Related to that, I don't have a clear picture in my head of exactly where enforcement happens of whether particular code should be loaded in a particular context (due to allowed_contexts as well as permissions etc), so I can't really notice errors related to that in this patch, so we are relying on a combination of unit tests and you not making errors. I am mildly worried about that. - The separate synchronous and asynchrnous implementations in CanOfAPIs are kind of annoying, it would be nice to avoid that but I don't see any good way to do so - I don't understand why the xpcshell test was removed rather than being modified to work with the new code ::: toolkit/components/extensions/ExtensionChild.jsm:767 (Diff revision 3) > - let obj = namespace.split(".").reduce( > - (object, prop) => object && object[prop], > + this.apiCan.findAPIPath(`${namespace}.${name}`); > + let obj = this.apiCan.findAPIPath(namespace); hm, yuck ::: toolkit/components/extensions/ExtensionCommon.jsm:575 (Diff revision 3) > + * context. > + * > + * @param {BaseContext} context > + * The context to manage APIs for. > + * @param {SchemaAPIManager} apiManager > + * The API manager holding the APIs to manager. nit "to manage" or "to be managed" ::: toolkit/components/extensions/ExtensionCommon.jsm:631 (Diff revision 3) > + if (!Schemas.checkPermissions(name, extension)) { > + return; > + } > + > + let api = await this.apiManager.asyncGetAPI(name, extension, this.scopeName); > + // Check again, because async; can we just handle this inside apiManager? ::: toolkit/components/extensions/ExtensionCommon.jsm:842 (Diff revision 3) > + /** > + * Emits an `onManifestEntry` event for the top-level manifest entry > + * on all relevant {@link ExtensionAPI} instances for the given > + * extension. > + * > + * The API modules will be synchronously loaded if they have not been > + * loaded already. > + * > + * @param {Extension} extension > + * The extension for which to emit the events. > + * @param {string} entry > + * The name of the top-level manifest entry. > + * > + * @returns {*} > + */ > + emitManifestEntry(extension, entry) { > + let apiName = this.manifestKeys.get(entry); > + if (apiName) { > + let api = this.getAPI(apiName, extension); > + return api.onManifestEntry(entry); > + } this isn't used?
Attachment #8851266 -
Flags: review?(aswan) → review+
Comment 22•7 years ago
|
||
mozreview-review |
Comment on attachment 8851267 [details] Bug 1350522: Part 2 - Convert toolkit APIs to lazy loading. https://reviewboard.mozilla.org/r/123624/#review127790 ::: toolkit/components/extensions/ext-notifications.js:72 (Diff revision 3) > emitAndDelete("closed"); > } > }, > }; > > -/* eslint-disable mozilla/balanced-listeners */ > +let nextId = 0; can we just make this a member of the class below? the ids won't be unique across extensions but we won't have to worry about this relatively generic name clashing with some future code. ::: toolkit/components/extensions/ext-toolkit.js:222 (Diff revision 3) > + ["webRequest"], > + ], > + }, > +}); > + > +if (AppConstants.MOZ_BUILD_APP === "browser") { why isn't this just in browser/components/extensions ?
Attachment #8851267 -
Flags: review?(aswan) → review+
Comment 23•7 years ago
|
||
mozreview-review |
Comment on attachment 8851268 [details] Bug 1350522: Part 3 - Convert android APIs to lazy loading. https://reviewboard.mozilla.org/r/123626/#review127830
Attachment #8851268 -
Flags: review?(aswan) → review+
Comment 24•7 years ago
|
||
mozreview-review |
Comment on attachment 8851268 [details] Bug 1350522: Part 3 - Convert android APIs to lazy loading. https://reviewboard.mozilla.org/r/123626/#review127832 ::: mobile/android/components/extensions/extensions-mobile.manifest:2 (Diff revision 3) > # scripts > -category webextension-scripts pageAction chrome://browser/content/ext-pageAction.js > +category webextension-scripts android chrome://browser/content/ext-android.js this file (and ext-c-android.js) appear to be missing? ::: mobile/android/components/extensions/extensions-mobile.manifest:2 (Diff revision 3) > # scripts > -category webextension-scripts pageAction chrome://browser/content/ext-pageAction.js > +category webextension-scripts android chrome://browser/content/ext-android.js this file (and ext-c-android.js) appear to be missing?
Comment 25•7 years ago
|
||
mozreview-review |
Comment on attachment 8851269 [details] Bug 1350522: Part 4 - Convert browser APIs to lazy loading. https://reviewboard.mozilla.org/r/123628/#review127834
Attachment #8851269 -
Flags: review?(aswan) → review+
Comment 26•7 years ago
|
||
mozreview-review |
Comment on attachment 8851270 [details] Bug 1350522: Part 5 - Remove registerSchemaAPI(). https://reviewboard.mozilla.org/r/123630/#review127836
Attachment #8851270 -
Flags: review?(aswan) → review+
Comment 27•7 years ago
|
||
mozreview-review |
Comment on attachment 8851271 [details] Bug 1350522: Part 6 - Cleanup per-api-instance state logic. https://reviewboard.mozilla.org/r/123632/#review127838 ::: browser/components/extensions/ext-commands.js:235 (Diff revision 3) > > getAPI(context) { > - let {extension} = context; > return { > commands: { > - getAll() { > + getAll: () => { why change the syntax here? ::: browser/components/extensions/ext-omnibox.js:34 (Diff revision 3) > > getAPI(context) { > let {extension} = context; > return { > omnibox: { > - setDefaultSuggestion(suggestion) { > + setDefaultSuggestion: (suggestion) => { same as above ::: browser/components/extensions/ext-omnibox.js:75 (Diff revision 3) > }; > }).api(), > }, > > omnibox_internal: { > - addSuggestions(id, suggestions) { > + addSuggestions: (id, suggestions) => { same as above ::: toolkit/components/extensions/ext-theme.js:186 (Diff revision 3) > getAPI(context) { > let {extension} = context; > + > return { > theme: { > - update(details) { > + update: (details) => { same as above
Attachment #8851271 -
Flags: review?(aswan) → review+
Assignee | ||
Comment 28•7 years ago
|
||
mozreview-review-reply |
Comment on attachment 8851271 [details] Bug 1350522: Part 6 - Cleanup per-api-instance state logic. https://reviewboard.mozilla.org/r/123632/#review127838 > why change the syntax here? Because we need to capture the outer `this`, which arrow functions do, and ordinary methods don't.
Assignee | ||
Comment 29•7 years ago
|
||
mozreview-review-reply |
Comment on attachment 8851268 [details] Bug 1350522: Part 3 - Convert android APIs to lazy loading. https://reviewboard.mozilla.org/r/123626/#review127832 > this file (and ext-c-android.js) appear to be missing? Urgh. I guess I forgot to `hg add` it.
Assignee | ||
Comment 30•7 years ago
|
||
mozreview-review-reply |
Comment on attachment 8851267 [details] Bug 1350522: Part 2 - Convert toolkit APIs to lazy loading. https://reviewboard.mozilla.org/r/123624/#review127790 > why isn't this just in browser/components/extensions ? Because it's meant to eventually support Android, it just doesn't yet.
Assignee | ||
Comment 31•7 years ago
|
||
mozreview-review-reply |
Comment on attachment 8851266 [details] Bug 1350522: Part 1 - Allow lazily loading and instantiating API modules. https://reviewboard.mozilla.org/r/123622/#review127788 > hm, yuck Yeah, at some point I want to change that to work more like the version in the parent process, so it doesn't need this. > can we just handle this inside apiManager? Nope. apiManager handles making sure we don't load the API twice, but we still have to make sure we don't instantiate it twice here. > this isn't used? It is.
Assignee | ||
Comment 32•7 years ago
|
||
(In reply to Andrew Swan [:aswan] from comment #21) > - Related to that, I don't have a clear picture in my head of exactly where > enforcement happens of whether particular code should be loaded in a > particular context (due to allowed_contexts as well as permissions etc), so > I can't really notice errors related to that in this patch, so we are > relying on a combination of unit tests and you not making errors. I am > mildly worried about that. The unit tests should be enough. They explicitly test that only the expected APIs are available in the expected contexts. > - The separate synchronous and asynchrnous implementations in CanOfAPIs are > kind of annoying, it would be nice to avoid that but I don't see any good > way to do so Yeah, I'd rather not have it either, but I don't have a better option. > - I don't understand why the xpcshell test was removed rather than being > modified to work with the new code Because it was testing for things that no longer applied. I'm not sure it ever made sense to have that test in the first place, anyway.
Comment 33•7 years ago
|
||
mozreview-review |
Comment on attachment 8851296 [details] Bug 1350522: Part 7 - Merge pageAction/browserAction/sidebarAction/commands helper classes into API instances. https://reviewboard.mozilla.org/r/123642/#review128216
Attachment #8851296 -
Flags: review?(aswan) → review+
Assignee | ||
Comment 34•7 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/1b4e77a9d11d97779fce333dbc1fdf73489c78dc Bug 1350522: Part 1 - Allow lazily loading and instantiating API modules. r=aswan https://hg.mozilla.org/integration/mozilla-inbound/rev/a3ed5ad1bc338e7fd8055c2efcf73695c25e09e5 Bug 1350522: Part 2 - Convert toolkit APIs to lazy loading. r=aswan https://hg.mozilla.org/integration/mozilla-inbound/rev/42015d3bfe49a09a28435ca3691358848c9dc2e5 Bug 1350522: Part 3 - Convert android APIs to lazy loading. r=aswan https://hg.mozilla.org/integration/mozilla-inbound/rev/7fd4ade8811856e835506310a57725dbd355c786 Bug 1350522: Part 4 - Convert browser APIs to lazy loading. r=aswan https://hg.mozilla.org/integration/mozilla-inbound/rev/8f8891e5193623da5630324041ba1533b4e0b2c7 Bug 1350522: Part 5 - Remove registerSchemaAPI(). r=aswan https://hg.mozilla.org/integration/mozilla-inbound/rev/650808cfe044339ceae30dbd79ef931330f78049 Bug 1350522: Part 6 - Cleanup per-api-instance state logic. r=aswan https://hg.mozilla.org/integration/mozilla-inbound/rev/069dbaa8300ef045b157d6fa081829d4b2a77cd5 Bug 1350522: Part 7 - Merge pageAction/browserAction/sidebarAction/commands helper classes into API instances. r=aswan
Comment 35•7 years ago
|
||
Backed out for leaking in browser_ext_incognito_popup.js: https://hg.mozilla.org/integration/mozilla-inbound/rev/36b190b455af7f396af7ab48a8b99ce54409a427 https://hg.mozilla.org/integration/mozilla-inbound/rev/90074920ac93744e7d0e77bf6ce673dcf020abb8 https://hg.mozilla.org/integration/mozilla-inbound/rev/22f6c9cb1fbe38057d30a15740b2e1580073c9ab https://hg.mozilla.org/integration/mozilla-inbound/rev/42c071ba0a725930bd33cb4347f3a64125aacc9d https://hg.mozilla.org/integration/mozilla-inbound/rev/552b31373020a246d3b65591070d3c9e1ecc8170 https://hg.mozilla.org/integration/mozilla-inbound/rev/d89932e97500409947cde6516d52678af4e99815 https://hg.mozilla.org/integration/mozilla-inbound/rev/98bdace78aee6988ff79af7330d4b6635befd5be Push with failures: https://treeherder.mozilla.org/#/jobs?repo=mozilla-inbound&revision=069dbaa8300ef045b157d6fa081829d4b2a77cd5&filter-resultStatus=testfailed&filter-resultStatus=busted&filter-resultStatus=exception&filter-resultStatus=retry&filter-resultStatus=usercancel&filter-resultStatus=runnable Failure log: https://treeherder.mozilla.org/logviewer.html#?job_id=88043596&repo=mozilla-inbound [task 2017-04-01T07:08:01.610862Z] 07:08:01 ERROR - TEST-UNEXPECTED-FAIL | browser/components/extensions/test/browser/browser_ext_incognito_popup.js | leaked 1 window(s) until shutdown [url = about:blank] [task 2017-04-01T07:08:01.624250Z] 07:08:01 ERROR - TEST-UNEXPECTED-FAIL | browser/components/extensions/test/browser/browser_ext_incognito_popup.js | leaked 1 window(s) until shutdown [url = chrome://browser/content/browser.xul]
Flags: needinfo?(kmaglione+bmo)
Assignee | ||
Comment 36•7 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/e7e4a3616132b18939681cc34c036ded5fba1e9c Bug 1350522: Part 1 - Allow lazily loading and instantiating API modules. r=aswan https://hg.mozilla.org/integration/mozilla-inbound/rev/f0bb8c65ee241c6b676a1b6f68cadbe0a95f823c Bug 1350522: Part 2 - Convert toolkit APIs to lazy loading. r=aswan https://hg.mozilla.org/integration/mozilla-inbound/rev/e57cadf2719f0b713d320991c49399429e4b4c72 Bug 1350522: Part 3 - Convert android APIs to lazy loading. r=aswan https://hg.mozilla.org/integration/mozilla-inbound/rev/76196b979de5f4161e61506496109fcf220691e3 Bug 1350522: Part 4 - Convert browser APIs to lazy loading. r=aswan https://hg.mozilla.org/integration/mozilla-inbound/rev/aaf9ba09dff1a7cb4eb1e2bdae3f9193921796ed Bug 1350522: Part 5 - Remove registerSchemaAPI(). r=aswan https://hg.mozilla.org/integration/mozilla-inbound/rev/e0a3f778fab97317a4a620eb9cdeda8bdef2f6dc Bug 1350522: Part 6 - Cleanup per-api-instance state logic. r=aswan https://hg.mozilla.org/integration/mozilla-inbound/rev/31dd0a3e931a22133fe3d4e21abfc1c2e1fb891f Bug 1350522: Part 7 - Merge pageAction/browserAction/sidebarAction/commands helper classes into API instances. r=aswan
Comment 37•7 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/e7e4a3616132 https://hg.mozilla.org/mozilla-central/rev/f0bb8c65ee24 https://hg.mozilla.org/mozilla-central/rev/e57cadf2719f https://hg.mozilla.org/mozilla-central/rev/76196b979de5 https://hg.mozilla.org/mozilla-central/rev/aaf9ba09dff1 https://hg.mozilla.org/mozilla-central/rev/e0a3f778fab9 https://hg.mozilla.org/mozilla-central/rev/31dd0a3e931a
Status: NEW → RESOLVED
Closed: 7 years ago
status-firefox55:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla55
Assignee | ||
Updated•7 years ago
|
Flags: needinfo?(kmaglione+bmo)
Updated•6 years ago
|
Product: Toolkit → WebExtensions
You need to log in
before you can comment on or make changes to this bug.
Description
•