Implement persistent event capability for WebExtensions

RESOLVED FIXED in Firefox 61

Status

enhancement
P1
normal
RESOLVED FIXED
a year ago
11 months ago

People

(Reporter: aswan, Assigned: aswan)

Tracking

unspecified
mozilla61
Dependency tree / graph
Bug Flags:
qe-verify -

Firefox Tracking Flags

(firefox61 fixed)

Details

Attachments

(3 attachments)

Assignee

Description

a year ago
Forking bug 1447551 even further so that we can separate review/discussion of the way we handle events during startup from issues specific to webRequest (or other apis)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Assignee

Updated

a year ago
Attachment #8964076 - Flags: review?(kmaglione+bmo)
Attachment #8964077 - Flags: review?(kmaglione+bmo)
Attachment #8964078 - Flags: review?(kmaglione+bmo)

Comment 4

a year ago
mozreview-review
Comment on attachment 8964076 [details]
Bug 1450388 Part 1 Refactor EventManager

https://reviewboard.mozilla.org/r/232864/#review238638

::: browser/components/extensions/parent/ext-windows.js:36
(Diff revision 1)
> + * @param {function} listener
> + *        The listener function to call when a DOM event is received.
> + *
> + * @returns {object} An injectable api for the new event.
> + */
> +function WindowEventManager(context, name, event, listener) {

Please remove from globals list in .eslintrc

::: mobile/android/components/extensions/ext-utils.js:208
(Diff revision 1)
> -global.GlobalEventManager = class extends EventManager {
> -  constructor(context, name, event, listener) {
> +global.makeGlobalEvent = function makeGlobalEvent(context, name, event, listener) {
> +  return new EventManager({

Why can't this keep being a subclass?

::: toolkit/components/extensions/ExtensionCommon.jsm:1775
(Diff revision 1)
> - *        A function called whenever a new listener is added.
> +   *        A function called whenever a new listener is added.
> - */
> -function EventManager(context, name, register) {
> +   * @param {boolean} [params.inputHandling=false]
> +   *        If true, the "handling user input" flag is set while handlers
> +   *        for this event are executing.
> +   */
> +  constructor(params, ...rest) {

Rest args are fairly expensive. Since we call this a lot and they're only used for a corner case, please just use `arguments` instead.
Attachment #8964076 - Flags: review?(kmaglione+bmo) → review+

Comment 5

a year ago
mozreview-review
Comment on attachment 8964077 [details]
Bug 1450388 Part 2: Expose startupData to webextensions

https://reviewboard.mozilla.org/r/232866/#review238642

::: toolkit/components/extensions/test/xpcshell/test_ext_startupData.js:22
(Diff revision 1)
> +
> +  const DATA = {test: "i am some startup data"};
> +  extension.startupData = DATA;
> +  await extension.saveStartupData();
> +
> +  await AddonTestUtils.promiseRestartManager();

Should `await wrapper.startupPromise` after restarting the add-on manager, or we might wind up with some ugly races.

::: toolkit/mozapps/extensions/internal/XPIProvider.jsm:3471
(Diff revision 1)
> +    let state = XPIStates.findAddon(aID);
> +    state.startupData = aData;
> +    XPIStates.save();
> +
> +    let addon = await new Promise(r => XPIDatabase.getVisibleAddonForID(aID, r));
> +    XPIDatabase.setAddonProperties(addon, {startupData: aData});

If we're going to store this in both the DB and in XPIStates, we should probably just set the DB property and then call updateXPIStates(addon).

We really shouldn't need to store this in the DB, though...
Attachment #8964077 - Flags: review?(kmaglione+bmo) → review+

Comment 6

a year ago
mozreview-review
Comment on attachment 8964078 [details]
Bug 1450388 Part 3: Implement persistent option for EventManager

https://reviewboard.mozilla.org/r/232868/#review238644

::: toolkit/components/extensions/ExtensionCommon.jsm:1742
(Diff revision 1)
> - * new EventManager(context, "api.subAPI", fire => {
> - *   let listener = (...) => {
> - *     // Fire any listeners registered with addListener.
> - *     fire.async(arg1, arg2);
> - *   };
> - *   // Register the listener.
> - *   SomehowRegisterListener(listener);
> - *   return () => {
> - *     // Return a way to unregister the listener.
> - *     SomehowUnregisterListener(listener);
> - *   };
> + * new EventManager({
> + *   context,
> + *   name: "api.subAPI",
> + *   register:  fire => {
> + *     let listener = (...) => {
> + *       // Fire any listeners registered with addListener.
> + *       fire.async(arg1, arg2);
> + *     };
> + *     // Register the listener.
> + *     SomehowRegisterListener(listener);
> + *     return () => {
> + *       // Return a way to unregister the listener.
> + *       SomehowUnregisterListener(listener);
> + *     };
> + *   }

Should be in part 1?

::: toolkit/components/extensions/ExtensionCommon.jsm:1880
(Diff revision 1)
> +
> +  // Record the fact that there is a listener for the given event in
> +  // the given extension.  `args` is an Array containing any extra
> +  // arguments that were passed to addListener().
> +  static savePersistentListener(extension, module, event, args) {
> +    if (!extension.startupData.persistentListeners) {

Can we have something like `let {startupData} = extension;` here?

::: toolkit/components/extensions/ExtensionCommon.jsm:1900
(Diff revision 1)
> +  // startup data.  `args` is a string, resulting from calling uneval()
> +  // on the array of extra arguments originally passed to addListener().
> +  static clearPersistentListener(extension, module, event, args) {
> +    let {startupData} = extension;
> +    let listeners = startupData.persistentListeners[module][event];
> +    let i = listeners.findIndex(a => uneval(a) == args);

Not ideal. Please make this an object with `uneval(args)` as the key. We don't really have a way to handle multiple listeners with the same args, anyway.

::: toolkit/components/extensions/ExtensionCommon.jsm:1900
(Diff revision 1)
> +  // startup data.  `args` is a string, resulting from calling uneval()
> +  // on the array of extra arguments originally passed to addListener().
> +  static clearPersistentListener(extension, module, event, args) {
> +    let {startupData} = extension;
> +    let listeners = startupData.persistentListeners[module][event];
> +    let i = listeners.findIndex(a => uneval(a) == args);

Not ideal. Please make this an object with `uneval(args)` as the key. We don't really have a way to handle multiple listeners with the same args, anyway.

::: toolkit/components/extensions/ExtensionCommon.jsm:1909
(Diff revision 1)
> +      delete startupData.persistentListeners[module][event];
> +      if (Object.keys(startupData.persistentListeners[module]).length == 0) {
> +        delete startupData.persistentListeners[module];

This is super expensive. Both the `delete`s and the `Object.keys()`s. We're probably better off just leaving the empty objects around.

It would probably be better to build up a new persistent listeners object each startup rather than updating the old one, though.

::: toolkit/components/extensions/parent/ext-backgroundPage.js:68
(Diff revision 1)
> +    extension.once("start-background-page", async () => {
> +      await this.bgPage.build();

Hm. I'm not sure we really want this before the browser window is visible...

::: toolkit/components/extensions/parent/ext-toolkit.js:83
(Diff revision 1)
> +    let promise = new Promise(resolve => {
> +      Services.obs.addObserver(function observer() {
> +        Services.obs.removeObserver(observer, "sessionstore-windows-restored");
> +        resolve();
> +      }, "sessionstore-windows-restored");
> +    });

`promiseObserved("sessionstore-windows-restored")`

::: toolkit/components/extensions/parent/ext-toolkit.js:89
(Diff revision 1)
> +      Services.obs.addObserver(function observer() {
> +        Services.obs.removeObserver(observer, "sessionstore-windows-restored");
> +        resolve();
> +      }, "sessionstore-windows-restored");
> +    });
> +    delete global.browserStartupPromise;

Please never delete properties if you can help it. It has the side-effect of putting the object into dictionary mode, and therefore breaking lots of JIT optimizations. That's especially bad in the case of an object on a scope chain.

::: toolkit/components/extensions/test/xpcshell/test_ext_persistent_events.js:127
(Diff revision 1)
> +  function listener(subject, _topic, data) {
> +    results.push(subject.wrappedJSObject);

Would really be better not to use the observer service for this... The XPCOM gunk is more trouble than it's worth for pure JS code. An EventEmitter would be better.

::: toolkit/components/extensions/test/xpcshell/test_ext_persistent_events.js:152
(Diff revision 1)
> +}
> +
> +add_task(async function() {
> +  AddonTestUtils.init(global);
> +  AddonTestUtils.overrideCertDB();
> +  AddonTestUtils.createAppInfo("xpcshell@tests.mozilla.org", "XPCShell", "1", "1");

Should be something like "43" rather than "1". The AOM tests set some hacky prefs to make "1" work...
Attachment #8964078 - Flags: review?(kmaglione+bmo)
Assignee

Comment 7

a year ago
mozreview-review-reply
Comment on attachment 8964076 [details]
Bug 1450388 Part 1 Refactor EventManager

https://reviewboard.mozilla.org/r/232864/#review238638

> Why can't this keep being a subclass?

No reason, but a subclass that only overrides the constructor (and changes the signature from the base class signature) seemed like a poor use of inheritance.  There are some other similar cases that I didn't change though, I can change it back to a subclass if you feel strongly.

Comment 8

a year ago
mozreview-review-reply
Comment on attachment 8964076 [details]
Bug 1450388 Part 1 Refactor EventManager

https://reviewboard.mozilla.org/r/232864/#review238638

> No reason, but a subclass that only overrides the constructor (and changes the signature from the base class signature) seemed like a poor use of inheritance.  There are some other similar cases that I didn't change though, I can change it back to a subclass if you feel strongly.

I don't care all that much, but if you keep this change, please also update the globals list in .eslintrc
Assignee

Comment 9

a year ago
mozreview-review-reply
Comment on attachment 8964077 [details]
Bug 1450388 Part 2: Expose startupData to webextensions

https://reviewboard.mozilla.org/r/232866/#review238642

> If we're going to store this in both the DB and in XPIStates, we should probably just set the DB property and then call updateXPIStates(addon).
> 
> We really shouldn't need to store this in the DB, though...

The reason to put it in the DB would be that `syncWithDB()` will clobber saved startupData if its not also in the DB, and I believe that gets called on active extensions every time we get into `processFileChanges()` (ie on every app update plus any time we rebuild the DB due to eg a schema change)
Assignee

Comment 10

a year ago
mozreview-review-reply
Comment on attachment 8964078 [details]
Bug 1450388 Part 3: Implement persistent option for EventManager

https://reviewboard.mozilla.org/r/232868/#review238644

> Not ideal. Please make this an object with `uneval(args)` as the key. We don't really have a way to handle multiple listeners with the same args, anyway.

This is the data we get directly from addonStartup.json, if we store it as uneval'ed strings, then we need to turn those strings back into arrays in `primeListeners()` (or hand an uneval'ed string to the api implementation, but then each api would need to actual arguments).
Assignee

Comment 11

a year ago
mozreview-review-reply
Comment on attachment 8964078 [details]
Bug 1450388 Part 3: Implement persistent option for EventManager

https://reviewboard.mozilla.org/r/232868/#review238644

> This is super expensive. Both the `delete`s and the `Object.keys()`s. We're probably better off just leaving the empty objects around.
> 
> It would probably be better to build up a new persistent listeners object each startup rather than updating the old one, though.

I converted these to just assign undefined which will get removed when we stringify

> Hm. I'm not sure we really want this before the browser window is visible...

You mean we should wait for browserStartupPromise regardless?  That's doable, it means that events that happen during startup will be sitting in this interim state for a while (which could just mean queueing the event details for non-blocking listeners or suspending requests for something like a blocking webRequest listener)
I don't really know how to evaluate that tradeoff...

> Please never delete properties if you can help it. It has the side-effect of putting the object into dictionary mode, and therefore breaking lots of JIT optimizations. That's especially bad in the case of an object on a scope chain.

I just grabbed this from `XPCOMUtils.defineLazyGetter()` which we use all over the place and which uses delete...

> Would really be better not to use the observer service for this... The XPCOM gunk is more trouble than it's worth for pure JS code. An EventEmitter would be better.

That's not as easy as it seems, as described in the comment above the API class, that code is serialized into a string then loaded and executed elsewhere, so I would have to jump through some hoops to put the EventEmitter somewhere that both API and the test code could get to it.
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)

Comment 15

a year ago
mozreview-review
Comment on attachment 8964078 [details]
Bug 1450388 Part 3: Implement persistent option for EventManager

https://reviewboard.mozilla.org/r/232868/#review239914

::: modules/libpref/init/all.js:5100
(Diff revision 2)
>  pref("extensions.webextensions.protocol.remote", true);
>  
>  // Disable tab hiding API by default.
>  pref("extensions.webextensions.tabhide.enabled", false);
>  
> +pref("webextensions.background-delayed-startup", false);

Can we call this "extensions.webextensions..." so I don't have to twitch every time I see it grouped with those other preferences?

::: toolkit/components/extensions/ExtensionCommon.jsm:1805
(Diff revision 2)
> +
>      this.unregister = new Map();
> +    this.remove = new Map();
> +
> +    if (this.persistent) {
> +      if (this.context.viewType != "background") {

Nit: !==

::: toolkit/components/extensions/ExtensionCommon.jsm:1869
(Diff revision 2)
> +        startupListeners[module][event] = [];
> +        for (let listener of eventEntry.values()) {
> +          startupListeners[module][event].push(listener.params);

Nit: `= Array.from(eventEntry.values(), listener => listener.params)`

::: toolkit/components/extensions/parent/ext-backgroundPage.js:64
(Diff revision 2)
> -    let {manifest} = this.extension;
> -
> +    let {extension} = this;
> +    let {manifest} = extension;
> -    this.bgPage = new BackgroundPage(this.extension, manifest.background);
>  
> -    return this.bgPage.build();
> +    this.bgPage = new BackgroundPage(extension, manifest.background);
> +    if (extension.startupReason != "APP_STARTUP" || !DELAYED_STARTUP) {

Nit: !==

::: toolkit/components/extensions/test/xpcshell/test_ext_persistent_events.js:138
(Diff revision 2)
> +  Services.obs.addObserver(listener, topic);
> +
> +  try {
> +    await Promise.all([
> +      new Promise(resolve => { _countResolve = resolve; }),
> +      Promise.resolve(fn && fn()),

Nit: No need for the `Promise.resolve`. `Promise.all` handles non-promise values the same way that `Promise.resolve` does.
Attachment #8964078 - Flags: review?(kmaglione+bmo) → review+
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)

Comment 19

a year ago
Pushed by aswan@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/bcacd467f867
Part 1 Refactor EventManager r=kmag
https://hg.mozilla.org/integration/autoland/rev/d5e58b02d335
Part 2: Expose startupData to webextensions r=kmag
https://hg.mozilla.org/integration/autoland/rev/b40f4cad613b
Part 3: Implement persistent option for EventManager r=kmag

Comment 20

a year ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/bcacd467f867
https://hg.mozilla.org/mozilla-central/rev/d5e58b02d335
https://hg.mozilla.org/mozilla-central/rev/b40f4cad613b
Status: NEW → RESOLVED
Last Resolved: a year ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla61

Comment 21

a year ago
Is manual testing required on this bug? If yes, please provide some STR and the proper extension(if required) or set the “qe-verify -“ flag.

Thanks!
Flags: needinfo?(aswan)
Assignee

Updated

a year ago
Flags: needinfo?(aswan) → qe-verify-
Assignee

Updated

a year ago
Blocks: 1447361

Updated

11 months ago
Product: Toolkit → WebExtensions
You need to log in before you can comment on or make changes to this bug.