Bug 1535674 Fix race with webextension persistent listeners r=kmag
authorAndrew Swan <aswan@mozilla.com>
Wed, 17 Apr 2019 10:49:24 -0700
changeset 470286 1305877ca306a4ff075c9ef84acf8ba4141e8888
parent 470285 b330e84e1458ac4e11343638f622329a48bb952b
child 470287 66ff290649f725c50c4b1815ab2294cbe1fc2f00
push id35892
push userrgurzau@mozilla.com
push dateSat, 20 Apr 2019 09:55:32 +0000
treeherdermozilla-central@a092972b53f0 [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewerskmag
bugs1535674, 1495072
milestone68.0a1
first release with
nightly linux32
nightly linux64
nightly mac
nightly win32
nightly win64
last release without
nightly linux32
nightly linux64
nightly mac
nightly win32
nightly win64
Bug 1535674 Fix race with webextension persistent listeners r=kmag Bug 1495072 uncovered a race in the webextension persistent listener logic where the Promise returned by a listener that is not re-registered during extension startup may never resolve. When this occurs with a blocking webRequest listener, content loads just hang forever. Fix this by forcing primed listeners to reject is they are invoked after the background page has started. Differential Revision: https://phabricator.services.mozilla.com/D27942
toolkit/components/extensions/ExtensionCommon.jsm
toolkit/components/extensions/parent/ext-backgroundPage.js
toolkit/components/extensions/parent/ext-webRequest.js
toolkit/components/extensions/test/xpcshell/test_ext_persistent_events.js
toolkit/components/extensions/test/xpcshell/test_ext_webRequest_startup.js
--- a/toolkit/components/extensions/ExtensionCommon.jsm
+++ b/toolkit/components/extensions/ExtensionCommon.jsm
@@ -2110,26 +2110,30 @@ class EventManager {
   // about all primed listeners in the extension's persistentListeners Map.
   static primeListeners(extension) {
     EventManager._initPersistentListeners(extension);
 
     for (let [module, moduleEntry] of extension.persistentListeners) {
       let api = extension.apiManager.getAPI(module, extension, "addon_parent");
       for (let [event, eventEntry] of moduleEntry) {
         for (let listener of eventEntry.values()) {
-          let primed = {pendingEvents: []};
+          let primed = {pendingEvents: [], cleared: false};
           listener.primed = primed;
 
           let bgStartupPromise = new Promise(r => extension.once("startup", r));
           let wakeup = () => {
             extension.emit("background-page-event");
             return bgStartupPromise;
           };
 
           let fireEvent = (...args) => new Promise((resolve, reject) => {
+            if (primed.cleared) {
+              reject(new Error("listener not re-registered"));
+              return;
+            }
             primed.pendingEvents.push({args, resolve, reject});
             extension.emit("background-page-event");
           });
 
           let fire = {
             wakeup,
             sync: fireEvent,
             async: fireEvent,
@@ -2172,16 +2176,17 @@ class EventManager {
           for (let evt of primed.pendingEvents) {
             evt.reject(new Error("listener not re-registered"));
           }
 
           if (clearPersistent) {
             EventManager.clearPersistentListener(extension, module, event, key);
           }
           primed.unregister();
+          primed.cleared = true;
         }
       }
     }
   }
 
   // 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().
--- a/toolkit/components/extensions/parent/ext-backgroundPage.js
+++ b/toolkit/components/extensions/parent/ext-backgroundPage.js
@@ -61,16 +61,23 @@ class BackgroundPage extends HiddenExten
 
     if (context) {
       // Wait until all event listeners registered by the script so far
       // to be handled.
       await Promise.all(context.listenerPromises);
       context.listenerPromises = null;
     }
 
+    if (extension.persistentListeners) {
+      // |this.extension| may be null if the extension was shut down.
+      // In that case, we still want to clear the primed listeners,
+      // but not update the persistent listeners in the startupData.
+      EventManager.clearPrimedListeners(extension, !!this.extension);
+    }
+
     extension.emit("startup");
   }
 
   shutdown() {
     this.extension._backgroundPageFrameLoader = null;
     super.shutdown();
   }
 }
@@ -108,20 +115,16 @@ this.backgroundPage = class extends Exte
 
     extension.once("start-background-page", async () => {
       if (!this.extension) {
         // Extension was shut down. Don't build the background page.
         // Primed listeners have been cleared in onShutdown.
         return;
       }
       await this.build();
-      // |this.extension| may be null if the extension was shut down.
-      // In that case, we still want to clear the primed listeners,
-      // but not update the persistent listeners in the startupData.
-      EventManager.clearPrimedListeners(extension, !!this.extension);
     });
 
     // There are two ways to start the background page:
     // 1. If a primed event fires, then start the background page as
     //    soon as we have painted a browser window.  Note that we have
     //    to touch browserPaintedPromise here to initialize the listener
     //    or else we can miss it if the event occurs after the first
     //    window is painted but before #2
--- a/toolkit/components/extensions/parent/ext-webRequest.js
+++ b/toolkit/components/extensions/parent/ext-webRequest.js
@@ -21,16 +21,20 @@ function registerEvent(extension, eventN
     if (filter.windowId != null && browserData.windowId != filter.windowId) {
       return;
     }
 
     let event = data.serialize(eventName);
     event.tabId = browserData.tabId;
 
     if (data.registerTraceableChannel) {
+      // If this is a primed listener, no tabParent was passed in here,
+      // but the convert() callback later in this function will be called
+      // when the background page is started.  Force that to happen here
+      // after which we'll have a valid tabParent.
       if (fire.wakeup) {
         await fire.wakeup();
       }
       data.registerTraceableChannel(extension.policy, tabParent);
     }
 
     return fire.sync(event);
   };
--- a/toolkit/components/extensions/test/xpcshell/test_ext_persistent_events.js
+++ b/toolkit/components/extensions/test/xpcshell/test_ext_persistent_events.js
@@ -27,70 +27,72 @@ const SCHEMA = [
 // serialized into a string which is later loaded by the WebExtensions
 // framework in the same context as other extension APIs.  By writing it
 // this way rather than as a big string constant we get lint coverage.
 // But eslint doesn't understand that this code runs in a different context
 // where the EventManager class is available so just tell it here:
 /* global EventManager */
 const API = class extends ExtensionAPI {
   primeListener(extension, event, fire, params) {
-    let data = {wrappedJSObject: {event, params}};
-    Services.obs.notifyObservers(data, "prime-event-listener");
+    Services.obs.notifyObservers({event, params}, "prime-event-listener");
 
     const FIRE_TOPIC = `fire-${event}`;
 
-    async function listener(subject, topic, _data) {
+    async function listener(subject, topic, data) {
       try {
-        await fire.async(subject.wrappedJSObject);
+        if (subject.wrappedJSObject.waitForBackground) {
+          await fire.wakeup();
+        }
+        await fire.async(subject.wrappedJSObject.listenerArgs);
       } catch (err) {
-        Services.obs.notifyObservers(data, "listener-callback-exception");
+        Services.obs.notifyObservers({event}, "listener-callback-exception");
       }
     }
     Services.obs.addObserver(listener, FIRE_TOPIC);
 
     return {
       unregister() {
-        Services.obs.notifyObservers(data, "unregister-primed-listener");
+        Services.obs.notifyObservers({event, params}, "unregister-primed-listener");
         Services.obs.removeObserver(listener, FIRE_TOPIC);
       },
       convert(_fire) {
-        Services.obs.notifyObservers(data, "convert-event-listener");
+        Services.obs.notifyObservers({event, params}, "convert-event-listener");
         fire = _fire;
       },
     };
   }
 
   getAPI(context) {
     return {
       eventtest: {
         onEvent1: new EventManager({
           context,
           name: "test.event1",
           persistent: {
             module: "eventtest",
             event: "onEvent1",
           },
           register: (fire, ...params) => {
-            let data = {wrappedJSObject: {event: "onEvent1", params}};
+            let data = {event: "onEvent1", params};
             Services.obs.notifyObservers(data, "register-event-listener");
             return () => {
               Services.obs.notifyObservers(data, "unregister-event-listener");
             };
           },
         }).api(),
 
         onEvent2: new EventManager({
           context,
           name: "test.event1",
           persistent: {
             module: "eventtest",
             event: "onEvent2",
           },
           register: (fire, ...params) => {
-            let data = {wrappedJSObject: {event: "onEvent2", params}};
+            let data = {event: "onEvent2", params};
             Services.obs.notifyObservers(data, "register-event-listener");
             return () => {
               Services.obs.notifyObservers(data, "unregister-event-listener");
             };
           },
         }).api(),
       },
     };
@@ -247,24 +249,23 @@ add_task(async function() {
   info = await p;
 
   check(info, "convert");
 
   await extension.awaitMessage("ready");
 
   // Check that when the event is triggered, all the plumbing worked
   // correctly for the primed-then-converted listener.
-  let eventDetails = {test: "kaboom"};
-  let eventSubject = {wrappedJSObject: eventDetails};
-  Services.obs.notifyObservers(eventSubject, "fire-onEvent1");
+  let listenerArgs = {test: "kaboom"};
+  Services.obs.notifyObservers({listenerArgs}, "fire-onEvent1");
 
   let details = await extension.awaitMessage("listener1");
-  deepEqual(details, eventDetails, "Listener 1 fired");
+  deepEqual(details, listenerArgs, "Listener 1 fired");
   details = await extension.awaitMessage("listener2");
-  deepEqual(details, eventDetails, "Listener 2 fired");
+  deepEqual(details, listenerArgs, "Listener 2 fired");
 
   // Check that the converted listener is properly unregistered at
   // browser shutdown.
   [info] = await Promise.all([
     promiseObservable("unregister-primed-listener", 3),
     AddonTestUtils.promiseShutdownManager(),
   ]);
   check(info, "unregister");
@@ -275,24 +276,24 @@ add_task(async function() {
     AddonTestUtils.promiseStartupManager(),
   ]);
   check(info, "prime");
 
   // Check that triggering the event before the listener has been converted
   // causes the background page to be loaded and the listener to be converted,
   // and the listener is invoked.
   p = promiseObservable("convert-event-listener", 3);
-  eventDetails.test = "startup event";
-  Services.obs.notifyObservers(eventSubject, "fire-onEvent2");
+  listenerArgs.test = "startup event";
+  Services.obs.notifyObservers({listenerArgs}, "fire-onEvent2");
   info = await p;
 
   check(info, "convert");
 
   details = await extension.awaitMessage("listener3");
-  deepEqual(details, eventDetails, "Listener 3 fired for event during startup");
+  deepEqual(details, listenerArgs, "Listener 3 fired for event during startup");
 
   await extension.awaitMessage("ready");
 
   // Check that the unregister process works when we manually remove
   // a listener.
   p = promiseObservable("unregister-primed-listener", 1);
   extension.sendMessage("unregister2");
   info = await p;
@@ -315,20 +316,20 @@ add_task(async function() {
   // starts up.
   p = promiseObservable("unregister-primed-listener", 1,
                         () => extension.awaitMessage("ready"));
   Services.obs.notifyObservers(null, "sessionstore-windows-restored");
   info = await p;
   check(info, "unregister", {listener1: false, listener3: false});
 
   // Just listener1 should be registered now, fire event1 to confirm.
-  eventDetails.test = "third time";
-  Services.obs.notifyObservers(eventSubject, "fire-onEvent1");
+  listenerArgs.test = "third time";
+  Services.obs.notifyObservers({listenerArgs}, "fire-onEvent1");
   details = await extension.awaitMessage("listener1");
-  deepEqual(details, eventDetails, "Listener 1 fired");
+  deepEqual(details, listenerArgs, "Listener 1 fired");
 
   // Tell the extension not to re-register listener1 on the next startup
   extension.sendMessage("unregister1");
   await extension.awaitMessage("unregistered");
 
   // Shut down, start up
   info = await promiseObservable("unregister-primed-listener", 1,
                                  () => AddonTestUtils.promiseShutdownManager());
@@ -336,17 +337,17 @@ add_task(async function() {
 
   info = await promiseObservable("prime-event-listener", 1,
                                  () => AddonTestUtils.promiseStartupManager());
   check(info, "register", {listener2: false, listener3: false});
 
   // Check that firing event1 causes the listener fire callback to
   // reject.
   p = promiseObservable("listener-callback-exception", 1);
-  Services.obs.notifyObservers(eventSubject, "fire-onEvent1");
+  Services.obs.notifyObservers({listenerArgs, waitForBackground: true}, "fire-onEvent1");
   await p;
   ok(true, "Primed listener that was not re-registered received an error when event was triggered during startup");
 
   await extension.awaitMessage("ready");
 
   await extension.unload();
 
   await AddonTestUtils.promiseShutdownManager();
--- a/toolkit/components/extensions/test/xpcshell/test_ext_webRequest_startup.js
+++ b/toolkit/components/extensions/test/xpcshell/test_ext_webRequest_startup.js
@@ -118,17 +118,17 @@ add_task(async function test_2() {
 
   Services.obs.notifyObservers(null, "sessionstore-windows-restored");
   await extension.awaitMessage("ready");
 
   await extension.unload();
   await promiseShutdownManager();
 });
 
-// Test that a block listener that uses filterResponseData() works
+// Test that a blocking listener that uses filterResponseData() works
 // properly (i.e., that the delayed call to registerTraceableChannel
 // works properly).
 add_task(async function test_3() {
   const DATA = `<!DOCTYPE html>
 <html>
 <body>
   <h1>This is a modified page</h1>
 </body>