Closed Bug 1350522 Opened 7 years ago Closed 7 years ago

Lazily load and instantiate APIs when they're needed

Categories

(WebExtensions :: General, enhancement)

enhancement
Not set
normal

Tracking

(firefox55 fixed)

RESOLVED FIXED
mozilla55
Tracking Status
firefox55 --- fixed

People

(Reporter: kmag, Assigned: kmag)

References

Details

Attachments

(7 files)

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 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 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 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 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 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 on attachment 8851270 [details]
Bug 1350522: Part 5 - Remove registerSchemaAPI().

https://reviewboard.mozilla.org/r/123630/#review127836
Attachment #8851270 - Flags: review?(aswan) → 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+
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.
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.
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.
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.
(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 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+
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)
Flags: needinfo?(kmaglione+bmo)
Depends on: 1375002
Product: Toolkit → WebExtensions
Depends on: 1499067
You need to log in before you can comment on or make changes to this bug.