Bug 1586393, part 2 - Fix BrowserTestUtils.addContentEventListener() with Fission. r=mconley

The current implementation of addContentEventListener() is centered
around the message manager. When any message manager goes away, it
cleans up everything, which does not work when Fission is enabled and
we do a cross-process navigation.

My new implementation instead uses window actors. Message manager
shared state is used to store the set of expected event listeners. New
windows, after the function is called for the first time in a test,
will get listeners added properly. Of windows that exist at the first
time the function is called in a test, only windows associated with
the BC of the browser that is passed in will get added, which is a
disadvantage relative to the current setup. That could probably be
fixed.

We auto remove at the end of the test, not when any message manager is
torn down, so there's never a need to disable auto removal.

Differential Revision: https://phabricator.services.mozilla.com/D48777

--HG--
extra : moz-landing-system : lando

browser/base/content/test/favicons/browser.ini

@@ -17,7 +17,6 @@ prefs =
 support-files =
   file_favicon_change.html
 [browser_favicon_change_not_in_document.js]
-fail-if = fission
 support-files =
   file_favicon_change_not_in_document.html
 [browser_multiple_icons_in_short_timeframe.js]

browser/base/content/test/plugins/browser.ini

@@ -50,7 +50,6 @@ support-files =
 [browser_bug743421.js]
 tags = blocklist
 [browser_bug744745.js]
-skip-if = fission # Fails with Fission, and we're unlikely to spend time to fix it.
 [browser_bug787619.js]
 [browser_bug797677.js]
 [browser_bug812562.js]

testing/mochitest/BrowserTestUtils/BrowserTestUtils.jsm

@@ -10,8 +10,7 @@
  */
 
 // This file uses ContentTask & frame scripts, where these are available.
-/* global addEventListener, removeEventListener, sendAsyncMessage,
-          addMessageListener, removeMessageListener, ContentTaskUtils */
+/* global addEventListener, removeEventListener, ContentTaskUtils */
 
 "use strict";
 
@@ -98,6 +97,25 @@ function registerActor() {
   ChromeUtils.registerWindowActor("BrowserTestUtils", actorOptions);
 }
 
+function registerContentEventListenerActor() {
+  let actorOptions = {
+    parent: {
+      moduleURI: "resource://testing-common/ContentEventListenerParent.jsm",
+    },
+
+    child: {
+      moduleURI: "resource://testing-common/ContentEventListenerChild.jsm",
+      events: {
+        // We need to see the creation of all new windows, in case they have
+        // a browsing context we are interesting in.
+        DOMWindowCreated: { capture: true },
+      },
+    },
+    allFrames: true,
+  };
+  ChromeUtils.registerWindowActor("ContentEventListener", actorOptions);
+}
+
 registerActor();
 
 var BrowserTestUtils = {
@@ -505,6 +523,12 @@ var BrowserTestUtils = {
 
   _webProgressListeners: new Set(),
 
+  _contentEventListenerSharedState: new Map(),
+
+  _contentEventListeners: new Map(),
+
+  _contentEventListenerActorRegistered: false,
+
   /**
    * Waits for the web progress listener associated with this tab to fire a
    * STATE_STOP for the toplevel document.
@@ -1215,6 +1239,10 @@ var BrowserTestUtils = {
    * fire until it is removed. A callable object is returned that,
    * when called, removes the event listener. Note that this function
    * works even if the browser's frameloader is swapped.
+   * Note: This will only listen for events that either have the browsing
+   * context of the browser element at the time of the call, or that are fired
+   * on windows that were created after any call to the function since the start
+   * of the test. This could be improved if needed.
    *
    * @param {xul:browser} browser
    *        The browser element to listen for events in.
@@ -1231,10 +1259,6 @@ var BrowserTestUtils = {
    *        listening should continue. If not specified, the first event with
    *        the specified name resolves the returned promise. This is called
    *        within the content process and can have no closure environment.
-   * @param {bool} autoremove [optional]
-   *        Whether the listener should be removed when |browser| is removed
-   *        from the DOM. Note that, if this flag is true, it won't be possible
-   *        to listen for events after a frameloader swap.
    *
    * @returns function
    *        If called, the return value will remove the event listener.
@@ -1244,84 +1268,115 @@ var BrowserTestUtils = {
     eventName,
     listener,
     listenerOptions = {},
-    checkFn,
-    autoremove = true
+    checkFn
   ) {
     let id = gListenerId++;
-    let checkFnSource = checkFn
-      ? encodeURIComponent(escape(checkFn.toSource()))
-      : "";
+    let contentEventListeners = this._contentEventListeners;
+    contentEventListeners.set(id, { listener, browser });
 
-    // To correctly handle frameloader swaps, we load a frame script
-    // into all tabs but ignore messages from the ones not related to
-    // |browser|.
+    let eventListenerState = this._contentEventListenerSharedState;
+    eventListenerState.set(id, {
+      eventName,
+      listenerOptions,
+      checkFnSource: checkFn ? checkFn.toSource() : "",
+    });
 
-    /* eslint-disable no-eval */
-    function frameScript(id, eventName, listenerOptions, checkFnSource) {
-      let checkFn;
-      if (checkFnSource) {
-        checkFn = eval(`(() => (${unescape(checkFnSource)}))()`);
-      }
+    Services.ppmm.sharedData.set(
+      "BrowserTestUtils:ContentEventListener",
+      eventListenerState
+    );
+    Services.ppmm.sharedData.flush();
 
-      function listener(event) {
-        if (checkFn && !checkFn(event)) {
-          return;
+    if (!this._contentEventListenerActorRegistered) {
+      this._contentEventListenerActorRegistered = true;
+      registerContentEventListenerActor();
+
+      // We hadn't registered the actor yet, so any existing window
+      // for browser's BC will not have been created yet. Explicitly
+      // make sure the actors are created and ready to go now. This
+      // happens after the updating of sharedData so that the actors
+      // don't have to get created and do nothing, and then later have
+      // to be updated.
+      // Note: As mentioned in the comment at the start of this function,
+      // this will miss any windows that existed at the time that the function
+      // was initially called during this test, but that did not have the
+      // browser's browsing context.
+      let contextsToVisit = [browser.browsingContext];
+      while (contextsToVisit.length) {
+        let currentContext = contextsToVisit.pop();
+        let global = currentContext.currentWindowGlobal;
+        if (!global) {
+          continue;
         }
-        sendAsyncMessage("ContentEventListener:Run", id);
-      }
-      function removeListener(msg) {
-        if (msg.data == id) {
-          removeMessageListener("ContentEventListener:Remove", removeListener);
-          removeEventListener(eventName, listener, listenerOptions);
-        }
-      }
-      addMessageListener("ContentEventListener:Remove", removeListener);
-      addEventListener(eventName, listener, listenerOptions);
-    }
-    /* eslint-enable no-eval */
-
-    let frameScriptSource = `data:,(${frameScript.toString()})(${id}, "${eventName}", ${uneval(
-      listenerOptions
-    )}, "${checkFnSource}")`;
-
-    let mm = Services.mm;
-
-    function runListener(msg) {
-      if (msg.data == id && msg.target == browser) {
-        listener();
+        let actor = browser.browsingContext.currentWindowGlobal.getActor(
+          "ContentEventListener"
+        );
+        actor.sendAsyncMessage("ContentEventListener:LateCreate");
+        contextsToVisit.push(...currentContext.getChildren());
       }
     }
-    mm.addMessageListener("ContentEventListener:Run", runListener);
-
-    let needCleanup = true;
 
     let unregisterFunction = function() {
-      if (!needCleanup) {
+      if (!eventListenerState.has(id)) {
         return;
       }
-      needCleanup = false;
-      mm.removeMessageListener("ContentEventListener:Run", runListener);
-      mm.broadcastAsyncMessage("ContentEventListener:Remove", id);
-      mm.removeDelayedFrameScript(frameScriptSource);
-      if (autoremove) {
-        Services.obs.removeObserver(cleanupObserver, "message-manager-close");
-      }
+      eventListenerState.delete(id);
+      contentEventListeners.delete(id);
+      Services.ppmm.sharedData.set(
+        "BrowserTestUtils:ContentEventListener",
+        eventListenerState
+      );
+      Services.ppmm.sharedData.flush();
     };
-
-    function cleanupObserver(subject, topic, data) {
-      if (subject == browser.messageManager) {
-        unregisterFunction();
-      }
-    }
-    if (autoremove) {
-      Services.obs.addObserver(cleanupObserver, "message-manager-close");
-    }
-
-    mm.loadFrameScript(frameScriptSource, true);
-
     return unregisterFunction;
   },
 
+  /**
+   * This is an internal method to be invoked by
+   * BrowserTestUtilsParent.jsm when a content event we were listening for
+   * happens.
+   */
+  _receivedContentEventListener(listenerId, browser) {
+    let listenerData = this._contentEventListeners.get(listenerId);
+    if (!listenerData) {
+      return;
+    }
+    if (listenerData.browser != browser) {
+      return;
+    }
+    listenerData.listener();
+  },
+
+  /**
+   * This is an internal method that cleans up any state from content event
+   * listeners.
+   */
+  _cleanupContentEventListeners() {
+    this._contentEventListeners.clear();
+
+    if (this._contentEventListenerSharedState.size != 0) {
+      this._contentEventListenerSharedState.clear();
+      Services.ppmm.sharedData.set(
+        "BrowserTestUtils:ContentEventListener",
+        this._contentEventListenerSharedState
+      );
+      Services.ppmm.sharedData.flush();
+    }
+
+    if (this._contentEventListenerActorRegistered) {
+      this._contentEventListenerActorRegistered = false;
+      ChromeUtils.unregisterWindowActor("ContentEventListener");
+    }
+  },
+
+  observe(subject, topic, data) {
+    switch (topic) {
+      case "test-complete":
+        this._cleanupContentEventListeners();
+        break;
+    }
+  },
+
   /**
    * Like browserLoaded, but waits for an error page to appear.
    * This explicitly deals with cases where the browser is not currently remote and a
@@ -2279,3 +2334,5 @@ var BrowserTestUtils = {
     return actor.sendQuery(aMessageName, aMessageData);
   },
 };
+
+Services.obs.addObserver(BrowserTestUtils, "test-complete");

testing/mochitest/BrowserTestUtils/ContentEventListenerChild.jsm

@@ -0,0 +1,186 @@
+/* vim: set ts=2 sw=2 sts=2 et tw=80: */
+/* This Source Code Form is subject to the terms of the Mozilla Public
+ * License, v. 2.0. If a copy of the MPL was not distributed with this
+ * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
+"use strict";
+
+var EXPORTED_SYMBOLS = ["ContentEventListenerChild"];
+
+const { Services } = ChromeUtils.import("resource://gre/modules/Services.jsm");
+
+class ContentEventListenerChild extends JSWindowActorChild {
+  actorCreated() {
+    this._contentEvents = new Map();
+    this._shutdown = false;
+    this._chromeEventHandler = null;
+    Services.cpmm.sharedData.addEventListener("change", this);
+  }
+
+  willDestroy() {
+    this._shutdown = true;
+    Services.cpmm.sharedData.removeEventListener("change", this);
+    this._updateContentEventListeners(/* clearListeners = */ true);
+    if (this._contentEvents.size != 0) {
+      throw new Error(`Didn't expect content events after willDestroy`);
+    }
+  }
+
+  receiveMessage(msg) {
+    if (msg.name != "ContentEventListener:LateCreate") {
+      return;
+    }
+    this._updateContentEventListeners();
+  }
+
+  handleEvent(event) {
+    switch (event.type) {
+      case "DOMWindowCreated": {
+        this._updateContentEventListeners();
+        break;
+      }
+
+      case "change": {
+        if (
+          !event.changedKeys.includes("BrowserTestUtils:ContentEventListener")
+        ) {
+          return;
+        }
+        this._updateContentEventListeners();
+        break;
+      }
+    }
+  }
+
+  /**
+   * This method first determines the desired set of content event listeners
+   * for the window. This is either the empty set, if clearListeners is true,
+   * or is retrieved from the message manager's shared data. It then compares
+   * this event listener data to the existing set of listeners that we have
+   * registered, as recorded in this._contentEvents. Each content event listener
+   * has been assigned a unique id by the parent process. If a listener was
+   * added, but is not in the new event data, it is removed. If a listener was
+   * not present, but is in the new event data, it is added. If it is in both,
+   * then a basic check is done to see if they are the same.
+   *
+   * @param {bool} clearListeners [optional]
+   *        If this is true, then instead of checking shared data to decide
+   *        what the desired set of listeners is, just use the empty set. This
+   *        will result in any existing listeners being cleared, and is used
+   *        when the window is going away.
+   */
+  _updateContentEventListeners(clearListeners = false) {
+    // If we've already begun the destruction process, any new event
+    // listeners for our bc id can't possibly really be for us, so ignore them.
+    if (this._shutdown && !clearListeners) {
+      throw new Error(
+        "Tried to update after we shut down content event listening"
+      );
+    }
+
+    let newEventData;
+    if (!clearListeners) {
+      newEventData = Services.cpmm.sharedData.get(
+        "BrowserTestUtils:ContentEventListener"
+      );
+    }
+    if (!newEventData) {
+      newEventData = new Map();
+    }
+
+    // Check that entries that continue to exist are the same and remove entries
+    // that no longer exist.
+    for (let [
+      listenerId,
+      { eventName, listener, listenerOptions },
+    ] of this._contentEvents.entries()) {
+      let newData = newEventData.get(listenerId);
+      if (newData) {
+        if (newData.eventName !== eventName) {
+          // Could potentially check if listenerOptions are the same, but
+          // checkFnSource can't be checked unless we store it, and this is
+          // just a smoke test anyways, so don't bother.
+          throw new Error(
+            "Got new content event listener that disagreed with existing data"
+          );
+        }
+        continue;
+      }
+      if (!this._chromeEventHandler) {
+        throw new Error(
+          "Trying to remove an event listener for waitForContentEvent without a cached event handler"
+        );
+      }
+      this._chromeEventHandler.removeEventListener(
+        eventName,
+        listener,
+        listenerOptions
+      );
+      this._contentEvents.delete(listenerId);
+    }
+
+    let actorChild = this;
+
+    // Add in new entries.
+    for (let [
+      listenerId,
+      { eventName, listenerOptions, checkFnSource },
+    ] of newEventData.entries()) {
+      let oldData = this._contentEvents.get(listenerId);
+      if (oldData) {
+        // We checked that the data is the same in the previous loop.
+        continue;
+      }
+
+      /* eslint-disable no-eval */
+      let checkFn;
+      if (checkFnSource) {
+        checkFn = eval(`(() => (${unescape(checkFnSource)}))()`);
+      }
+      /* eslint-enable no-eval */
+
+      function listener(event) {
+        if (checkFn && !checkFn(event)) {
+          return;
+        }
+        actorChild.sendAsyncMessage("ContentEventListener:Run", {
+          listenerId,
+        });
+      }
+
+      // Cache the chrome event handler because this.docShell won't be
+      // available during shut down.
+      if (!this._chromeEventHandler) {
+        if (!this.docShell) {
+          throw new Error(
+            "Trying to add a new event listener for waitForContentEvent without a docshell"
+          );
+        }
+        this._chromeEventHandler = this.docShell.chromeEventHandler;
+      }
+
+      // Some windows, like top-level browser windows, maybe not have a chrome
+      // event handler set up as this point, but we don't actually care about
+      // events on those windows, so ignore them.
+      if (!this._chromeEventHandler) {
+        continue;
+      }
+
+      this._chromeEventHandler.addEventListener(
+        eventName,
+        listener,
+        listenerOptions
+      );
+      this._contentEvents.set(listenerId, {
+        eventName,
+        listener,
+        listenerOptions,
+      });
+    }
+
+    // If there are no active content events, clear our reference to the chrome
+    // event handler to prevent leaks.
+    if (this._contentEvents.size == 0) {
+      this._chromeEventHandler = null;
+    }
+  }
+}

testing/mochitest/BrowserTestUtils/ContentEventListenerParent.jsm

@@ -0,0 +1,33 @@
+/* vim: set ts=2 sw=2 sts=2 et tw=80: */
+/* This Source Code Form is subject to the terms of the Mozilla Public
+ * License, v. 2.0. If a copy of the MPL was not distributed with this
+ * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
+"use strict";
+
+var EXPORTED_SYMBOLS = ["ContentEventListenerParent"];
+
+const { BrowserTestUtils } = ChromeUtils.import(
+  "resource://testing-common/BrowserTestUtils.jsm"
+);
+
+class ContentEventListenerParent extends JSWindowActorParent {
+  receiveMessage(aMessage) {
+    switch (aMessage.name) {
+      case "ContentEventListener:Run": {
+        let browser = this.browsingContext.top.embedderElement;
+        if (!browser) {
+          break;
+        }
+        // Handle RDM.
+        if (browser.outerBrowser) {
+          browser = browser.outerBrowser;
+        }
+        BrowserTestUtils._receivedContentEventListener(
+          aMessage.data.listenerId,
+          browser
+        );
+        break;
+      }
+    }
+  }
+}

testing/mochitest/BrowserTestUtils/moz.build

@@ -9,6 +9,8 @@ TESTING_JS_MODULES += [
     'BrowserTestUtilsChild.jsm',
     'BrowserTestUtilsParent.jsm',
     'content/content-task.js',
+    'ContentEventListenerChild.jsm',
+    'ContentEventListenerParent.jsm',
     'ContentTask.jsm',
     'ContentTaskUtils.jsm',
 ]

toolkit/mozapps/extensions/test/browser/browser_history_navigation.js

@@ -165,8 +165,7 @@ function wait_for_page_show(browser) {
       "pageshow",
       listener,
       {},
-      event => event.target.location == "http://example.com/",
-      /* autoremove = */ false
+      event => event.target.location == "http://example.com/"
     );
   });
   return promise;