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 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.

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...

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...

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

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)

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).

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 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.

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

https://hg.mozilla.org/mozilla-central/rev/bcacd467f867 https://hg.mozilla.org/mozilla-central/rev/d5e58b02d335 https://hg.mozilla.org/mozilla-central/rev/b40f4cad613b

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!