Bug 1588193 - Fix BrowserTestUtils.waitForContentEvent with Fission, r=mccr8
authorKashav Madan <kmadan@mozilla.com>
Thu, 07 Nov 2019 21:16:01 +0000
changeset 501169 9944bddc7b997b433f348d5ad3603952f7a258b8
parent 501168 3c4ee65f29c6507e470e75aa26d644eda63d09d1
child 501170 7c24449d6802f24cb122e0d2d06ee1bf772884d9
push id36781
push usercsabou@mozilla.com
push dateFri, 08 Nov 2019 05:21:04 +0000
treeherdermozilla-central@dff542b772e5 [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewersmccr8
bugs1588193
milestone72.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 1588193 - Fix BrowserTestUtils.waitForContentEvent with Fission, r=mccr8 This also changes BrowserTestUtils.addContentEventListener to use browsing contexts to track added listeners and their associated targets. Depends on D52105 Differential Revision: https://phabricator.services.mozilla.com/D51441
testing/mochitest/BrowserTestUtils/BrowserTestUtils.jsm
testing/mochitest/BrowserTestUtils/ContentEventListenerParent.jsm
--- a/testing/mochitest/BrowserTestUtils/BrowserTestUtils.jsm
+++ b/testing/mochitest/BrowserTestUtils/BrowserTestUtils.jsm
@@ -1146,70 +1146,47 @@ var BrowserTestUtils = {
    *        Name of the event to listen to.
    * @param {bool} capture [optional]
    *        Whether to use a capturing listener.
    * @param {function} checkFn [optional]
    *        Called with the Event object as argument, should return true if the
    *        event is the expected one, or false if it should be ignored and
    *        listening should continue. If not specified, the first event with
    *        the specified name resolves the returned promise.
-   * @param {bool} wantsUntrusted [optional]
+   * @param {bool} wantUntrusted [optional]
    *        Whether to accept untrusted events
    *
-   * @note Because this function is intended for testing, any error in checkFn
-   *       will cause the returned promise to be rejected instead of waiting for
-   *       the next event, since this is probably a bug in the test.
+   * @note As of bug 1588193, this function no longer rejects the returned
+   *       promise in the case of a checkFn error. Instead, since checkFn is now
+   *       called through eval in the content process, the error is thrown in
+   *       the listener created by ContentEventListenerChild. Work to improve
+   *       error handling (eg. to reject the promise as before and to preserve
+   *       the filename/stack) is being tracked in bug 1593811.
    *
    * @returns {Promise}
    */
   waitForContentEvent(
     browser,
     eventName,
     capture = false,
     checkFn,
-    wantsUntrusted = false
+    wantUntrusted = false
   ) {
-    let parameters = {
-      eventName,
-      capture,
-      checkFnSource: checkFn ? checkFn.toSource() : null,
-      wantsUntrusted,
-    };
-    /* eslint-disable no-eval */
-    return ContentTask.spawn(browser, parameters, function({
-      eventName,
-      capture,
-      checkFnSource,
-      wantsUntrusted,
-    }) {
-      let checkFn;
-      if (checkFnSource) {
-        checkFn = eval(`(() => (${checkFnSource}))()`);
-      }
-      return new Promise((resolve, reject) => {
-        addEventListener(
-          eventName,
-          function listener(event) {
-            let completion = resolve;
-            try {
-              if (checkFn && !checkFn(event)) {
-                return;
-              }
-            } catch (e) {
-              completion = () => reject(e);
-            }
-            removeEventListener(eventName, listener, capture);
-            completion();
-          },
-          capture,
-          wantsUntrusted
-        );
-      });
+    return new Promise(resolve => {
+      let removeEventListener = this.addContentEventListener(
+        browser,
+        eventName,
+        () => {
+          removeEventListener();
+          resolve();
+        },
+        { capture, wantUntrusted },
+        checkFn
+      );
     });
-    /* eslint-enable no-eval */
   },
 
   /**
    * Like waitForEvent, but acts on a popup. It ensures the popup is not already
    * in the expected state.
    *
    * @param {Element} popup
    *        The popup element that should receive the event.
@@ -1256,17 +1233,20 @@ var BrowserTestUtils = {
     browser,
     eventName,
     listener,
     listenerOptions = {},
     checkFn
   ) {
     let id = gListenerId++;
     let contentEventListeners = this._contentEventListeners;
-    contentEventListeners.set(id, { listener, browser });
+    contentEventListeners.set(id, {
+      listener,
+      browsingContext: browser.browsingContext,
+    });
 
     let eventListenerState = this._contentEventListenerSharedState;
     eventListenerState.set(id, {
       eventName,
       listenerOptions,
       checkFnSource: checkFn ? checkFn.toSource() : "",
     });
 
@@ -1291,22 +1271,22 @@ var BrowserTestUtils = {
     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) {
+  _receivedContentEventListener(listenerId, browsingContext) {
     let listenerData = this._contentEventListeners.get(listenerId);
     if (!listenerData) {
       return;
     }
-    if (listenerData.browser != browser) {
+    if (listenerData.browsingContext != browsingContext) {
       return;
     }
     listenerData.listener();
   },
 
   /**
    * This is an internal method that cleans up any state from content event
    * listeners.
--- a/testing/mochitest/BrowserTestUtils/ContentEventListenerParent.jsm
+++ b/testing/mochitest/BrowserTestUtils/ContentEventListenerParent.jsm
@@ -9,25 +9,17 @@ var EXPORTED_SYMBOLS = ["ContentEventLis
 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
+          this.browsingContext.top
         );
         break;
       }
     }
   }
 }