Lazily load and instantiate APIs when they're needed

RESOLVED FIXED in Firefox 55

Status

()

Toolkit
WebExtensions: General
RESOLVED FIXED
3 months ago
3 months ago

People

(Reporter: kmag, Assigned: kmag)

Tracking

(Blocks: 1 bug)

unspecified
mozilla55
Points:
---

Firefox Tracking Flags

(firefox55 fixed)

Details

MozReview Requests

()

Submitter Diff Changes Open Issues Last Updated
Loading...
Error loading review requests:

Attachments

(7 attachments)

(Assignee)

Description

3 months ago
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

3 months 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

3 months 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

3 months 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

3 months 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

3 months 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

3 months 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

3 months 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

3 months 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

3 months 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

3 months 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

3 months 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

3 months 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

3 months 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

3 months 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
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

3 months 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

3 months 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
Last Resolved: 3 months ago
status-firefox55: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla55
(Assignee)

Updated

3 months ago
Flags: needinfo?(kmaglione+bmo)
You need to log in before you can comment on or make changes to this bug.