Bug 1701686 - [marionette] Only handle user prompts from the currently selected tab. r=marionette-reviewers,webdriver-reviewers,Gijs,jdescottes

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

testing/marionette/driver.js

@@ -226,12 +226,7 @@ GeckoDriver.prototype.QueryInterface = ChromeUtils.generateQI([
  * Callback used to observe the creation of new modal or tab modal dialogs
  * during the session's lifetime.
  */
-GeckoDriver.prototype.handleModalDialog = function(action, dialog, win) {
-  // Only care about modals of the currently selected window.
-  if (win !== this.curBrowser.window) {
-    return;
-  }
-
+GeckoDriver.prototype.handleModalDialog = function(action, dialog) {
   if (action === modal.ACTION_OPENED) {
     this.dialog = new modal.Dialog(() => this.curBrowser, dialog);
     this.getActor().notifyDialogOpened();
@@ -620,7 +615,7 @@ GeckoDriver.prototype.newSession = async function(cmd) {
   }
 
   // Setup observer for modal dialogs
-  this.dialogObserver = new modal.DialogObserver(this);
+  this.dialogObserver = new modal.DialogObserver(() => this.curBrowser);
   this.dialogObserver.add(this.handleModalDialog.bind(this));
 
   Services.obs.addObserver(this, "browsing-context-attached");
@@ -1449,7 +1444,9 @@ GeckoDriver.prototype.setWindowHandle = async function(
   // Check for existing dialogs for the new window
   this.dialog = modal.findModalDialogs(this.curBrowser);
 
-  if (focus) {
+  // If there is an open window modal dialog the underlying chrome window
+  // cannot be focused.
+  if (focus && !this.dialog?.isWindowModal) {
     await this.curBrowser.focusWindow();
   }
 };
@@ -2627,13 +2624,14 @@ GeckoDriver.prototype.dismissDialog = async function() {
   assert.open(this.getBrowsingContext({ top: true }));
   this._checkIfAlertIsPresent();
 
-  const win = this.getCurrentWindow();
-  const dialogClosed = this.dialogObserver.dialogClosed(win);
+  const dialogClosed = this.dialogObserver.dialogClosed();
 
   const { button0, button1 } = this.dialog.ui;
   (button1 ? button1 : button0).click();
 
   await dialogClosed;
+
+  const win = this.getCurrentWindow();
   await new IdlePromise(win);
 };
 
@@ -2648,13 +2646,14 @@ GeckoDriver.prototype.acceptDialog = async function() {
   assert.open(this.getBrowsingContext({ top: true }));
   this._checkIfAlertIsPresent();
 
-  const win = this.getCurrentWindow();
-  const dialogClosed = this.dialogObserver.dialogClosed(win);
+  const dialogClosed = this.dialogObserver.dialogClosed();
 
   const { button0 } = this.dialog.ui;
   button0.click();
 
   await dialogClosed;
+
+  const win = this.getCurrentWindow();
   await new IdlePromise(win);
 };
 

testing/marionette/harness/marionette_harness/tests/unit/test_modal_dialogs.py

@@ -29,6 +29,7 @@ class TestModalDialogs(WindowManagerMixin, MarionetteTestCase):
             pass
 
         self.close_all_tabs()
+        self.close_all_windows()
 
         super(TestModalDialogs, self).tearDown()
 
@@ -43,7 +44,7 @@ class TestModalDialogs(WindowManagerMixin, MarionetteTestCase):
     def wait_for_alert(self, timeout=None):
         Wait(self.marionette, timeout=timeout).until(lambda _: self.alert_present)
 
-    def open_custom_prompt(self, modal_type):
+    def open_custom_prompt(self, modal_type, delay=0):
         browsing_context_id = self.marionette.execute_script(
             """
             return window.browsingContext.id;
@@ -56,7 +57,7 @@ class TestModalDialogs(WindowManagerMixin, MarionetteTestCase):
                 """
                 const { Services } = ChromeUtils.import("resource://gre/modules/Services.jsm");
 
-                const [ modalType, browsingContextId ] = arguments;
+                const [ modalType, browsingContextId, delay ] = arguments;
 
                 const modalTypes = {
                   1: Services.prompt.MODAL_TYPE_CONTENT,
@@ -65,21 +66,27 @@ class TestModalDialogs(WindowManagerMixin, MarionetteTestCase):
                   4: Services.prompt.MODAL_TYPE_INTERNAL_WINDOW,
                 }
 
-                const bc = (modalType === 3) ? null : BrowsingContext.get(browsingContextId);
-                Services.prompt.alertBC(bc, modalTypes[modalType], "title", "text");
+                window.setTimeout(() => {
+                  Services.prompt.alertBC(
+                    BrowsingContext.get(browsingContextId),
+                    modalTypes[modalType],
+                    "title",
+                    "text"
+                  );
+                }, delay);
             """,
-                script_args=(modal_type, browsing_context_id),
+                script_args=(modal_type, browsing_context_id, delay * 1000),
             )
 
     @parameterized("content", 1)
     @parameterized("tab", 2)
     @parameterized("window", 3)
     @parameterized("internal_window", 4)
-    def test_detect_modal_type(self, type):
+    def test_detect_modal_type_in_current_tab_for_type(self, type):
         self.open_custom_prompt(type)
         self.wait_for_alert()
 
-        self.marionette.switch_to_alert()
+        self.assertTrue(self.alert_present)
 
         # Restart the session to ensure we still find the formerly left-open dialog.
         self.marionette.delete_session()
@@ -88,6 +95,53 @@ class TestModalDialogs(WindowManagerMixin, MarionetteTestCase):
         alert = self.marionette.switch_to_alert()
         alert.dismiss()
 
+    @parameterized("content", 1)
+    @parameterized("tab", 2)
+    def test_dont_detect_content_and_tab_modal_type_in_another_tab_for_type(self, type):
+        self.open_custom_prompt(type, delay=0.25)
+
+        self.marionette.switch_to_window(self.start_tab)
+        with self.assertRaises(errors.TimeoutException):
+            self.wait_for_alert(2)
+
+        self.marionette.switch_to_window(self.new_tab)
+        alert = self.marionette.switch_to_alert()
+        alert.dismiss()
+
+    @parameterized("window", 3)
+    @parameterized("internal_window", 4)
+    def test_detect_window_modal_type_in_another_tab_for_type(self, type):
+        self.open_custom_prompt(type, delay=0.25)
+
+        self.marionette.switch_to_window(self.start_tab)
+        self.wait_for_alert()
+
+        alert = self.marionette.switch_to_alert()
+        alert.dismiss()
+
+        self.marionette.switch_to_window(self.new_tab)
+        self.assertFalse(self.alert_present)
+
+    @parameterized("window", 3)
+    @parameterized("internal_window", 4)
+    def test_detect_window_modal_type_in_another_window_for_type(self, type):
+        self.new_window = self.open_window()
+
+        self.marionette.switch_to_window(self.new_window)
+
+        self.open_custom_prompt(type, delay=0.25)
+
+        self.marionette.switch_to_window(self.new_tab)
+        with self.assertRaises(errors.TimeoutException):
+            self.wait_for_alert(2)
+
+        self.marionette.switch_to_window(self.new_window)
+        alert = self.marionette.switch_to_alert()
+        alert.dismiss()
+
+        self.marionette.switch_to_window(self.new_tab)
+        self.assertFalse(self.alert_present)
+
     def test_http_auth_dismiss(self):
         with self.marionette.using_prefs({self.http_auth_pref: True}):
             self.marionette.navigate(self.marionette.absolute_url("http_auth"))

testing/marionette/modal.js

@@ -47,7 +47,8 @@ modal.findModalDialogs = function(context) {
       win.opener &&
       win.opener === context.window
     ) {
-      return new modal.Dialog(() => context, Cu.getWeakReference(win));
+      logger.trace("Found open modal prompt");
+      return new modal.Dialog(() => context, win);
     }
   }
 
@@ -60,6 +61,7 @@ modal.findModalDialogs = function(context) {
     let prompts = promptManager.listPrompts();
 
     if (prompts.length) {
+      logger.trace("Found open tab modal prompt");
       return new modal.Dialog(() => context, null);
     }
   }
@@ -76,10 +78,8 @@ modal.findModalDialogs = function(context) {
     );
 
     if (dialogs.length) {
-      return new modal.Dialog(
-        () => context,
-        Cu.getWeakReference(dialogs[0]._frame.contentWindow)
-      );
+      logger.trace("Found open content prompt");
+      return new modal.Dialog(() => context, dialogs[0]._frame.contentWindow);
     }
   }
 
@@ -89,11 +89,16 @@ modal.findModalDialogs = function(context) {
 /**
  * Observer for modal and tab modal dialogs.
  *
+ * @param {function(): browser.Context} curBrowserFn
+ *     Function that returns the current |browser.Context|.
+ *
  * @return {modal.DialogObserver}
  *     Returns instance of the DialogObserver class.
  */
 modal.DialogObserver = class {
-  constructor() {
+  constructor(curBrowserFn) {
+    this._curBrowserFn = curBrowserFn;
+
     this.callbacks = new Set();
     this.register();
   }
@@ -128,33 +133,66 @@ modal.DialogObserver = class {
   handleEvent(event) {
     logger.trace(`Received event ${event.type}`);
 
-    let chromeWin = event.target.opener
+    const chromeWin = event.target.opener
       ? event.target.opener.ownerGlobal
       : event.target.ownerGlobal;
 
-    let targetRef = Cu.getWeakReference(event.target);
+    if (chromeWin != this._curBrowserFn().window) {
+      return;
+    }
 
     this.callbacks.forEach(callback => {
-      callback(modal.ACTION_CLOSED, targetRef, chromeWin);
+      callback(modal.ACTION_CLOSED, event.target);
     });
   }
 
   observe(subject, topic) {
     logger.trace(`Received observer notification ${topic}`);
 
+    const curBrowser = this._curBrowserFn();
+
     switch (topic) {
-      case "common-dialog-loaded":
+      // This topic is only used by the old-style content modal dialogs like
+      // alert, confirm, and prompt. It can be removed when only the new
+      // subdialog based content modals remain. Those will be made default in
+      // Firefox 89, and this case is deprecated.
       case "tabmodal-dialog-loaded":
-        let chromeWin = subject.opener
-          ? subject.opener.ownerGlobal
-          : subject.ownerGlobal;
+        const container = curBrowser.contentBrowser.closest(
+          ".browserSidebarContainer"
+        );
+        if (!container.contains(subject)) {
+          return;
+        }
+        this.callbacks.forEach(callback =>
+          callback(modal.ACTION_OPENED, subject)
+        );
+        break;
 
-        // Always keep a weak reference to the current dialog
-        let targetRef = Cu.getWeakReference(subject);
+      case "common-dialog-loaded":
+        const modalType = subject.Dialog.args.modalType;
 
-        this.callbacks.forEach(callback => {
-          callback(modal.ACTION_OPENED, targetRef, chromeWin);
-        });
+        if (
+          modalType === Services.prompt.MODAL_TYPE_TAB ||
+          modalType === Services.prompt.MODAL_TYPE_CONTENT
+        ) {
+          // Find the container of the dialog in the parent document, and ensure
+          // it is a descendant of the same container as the current browser.
+          const container = curBrowser.contentBrowser.closest(
+            ".browserSidebarContainer"
+          );
+          if (!container.contains(subject.docShell.chromeEventHandler)) {
+            return;
+          }
+        } else if (
+          subject.ownerGlobal != curBrowser.window &&
+          subject.opener?.ownerGlobal != curBrowser.window
+        ) {
+          return;
+        }
+
+        this.callbacks.forEach(callback =>
+          callback(modal.ACTION_OPENED, subject)
+        );
         break;
 
       case "toplevel-window-ready":
@@ -191,14 +229,11 @@ modal.DialogObserver = class {
 
   /**
    * Returns a promise that waits for the dialog to be closed.
-   *
-   * @param {window} win
-   *     The window containing the modal dialog to close.
    */
-  async dialogClosed(win) {
+  async dialogClosed() {
     return new Promise(resolve => {
-      const dialogClosed = (action, dialog, window) => {
-        if (action == modal.ACTION_CLOSED && window == win) {
+      const dialogClosed = (action, dialog) => {
+        if (action == modal.ACTION_CLOSED) {
           this.remove(dialogClosed);
           resolve();
         }
@@ -214,13 +249,13 @@ modal.DialogObserver = class {
  *
  * @param {function(): browser.Context} curBrowserFn
  *     Function that returns the current |browser.Context|.
- * @param {nsIWeakReference=} winRef
- *     A weak reference to the current |ChromeWindow|.
+ * @param {DOMWindow} dialog
+ *     DOMWindow of the dialog.
  */
 modal.Dialog = class {
-  constructor(curBrowserFn, winRef = undefined) {
+  constructor(curBrowserFn, dialog) {
     this.curBrowserFn_ = curBrowserFn;
-    this.win_ = winRef;
+    this.win_ = Cu.getWeakReference(dialog);
   }
 
   get curBrowser_() {
@@ -254,6 +289,13 @@ modal.Dialog = class {
     return tm ? tm.args : null;
   }
 
+  get isWindowModal() {
+    return [
+      Services.prompt.MODAL_TYPE_WINDOW,
+      Services.prompt.MODAL_TYPE_INTERNAL_WINDOW,
+    ].includes(this.args.modalType);
+  }
+
   get ui() {
     let tm = this.tabModal;
     return tm ? tm.ui : null;

testing/marionette/navigate.js

@@ -236,12 +236,7 @@ navigate.waitForNavigationCompleted = async function waitForNavigationCompleted(
     }
   };
 
-  const onDialogOpened = (action, dialog, win) => {
-    // Only care about modals of the currently selected window.
-    if (win !== chromeWindow) {
-      return;
-    }
-
+  const onDialogOpened = action => {
     if (action === modal.ACTION_OPENED) {
       logger.trace("Canceled page load listener because a dialog opened");
       checkDone({ finished: true });

testing/marionette/test/unit/test_modal.js

@@ -4,20 +4,31 @@
 
 "use strict";
 
+const { Services } = ChromeUtils.import("resource://gre/modules/Services.jsm");
 const { modal } = ChromeUtils.import("chrome://marionette/content/modal.js");
 
+const chromeWindow = {};
+
 const mockModalDialog = {
+  docShell: {
+    chromeEventHandler: null,
+  },
   opener: {
-    ownerGlobal: "foo",
+    ownerGlobal: chromeWindow,
+  },
+  Dialog: {
+    args: {
+      modalType: Services.prompt.MODAL_TYPE_WINDOW,
+    },
   },
 };
 
-const mockTabModalDialog = {
-  ownerGlobal: "foo",
+const mockCurBrowser = {
+  window: chromeWindow,
 };
 
 add_test(function test_addCallback() {
-  let observer = new modal.DialogObserver();
+  let observer = new modal.DialogObserver(() => mockCurBrowser);
   let cb1 = () => true;
   let cb2 = () => false;
 
@@ -33,7 +44,7 @@ add_test(function test_addCallback() {
 });
 
 add_test(function test_removeCallback() {
-  let observer = new modal.DialogObserver();
+  let observer = new modal.DialogObserver(() => mockCurBrowser);
   let cb1 = () => true;
   let cb2 = () => false;
 
@@ -53,7 +64,7 @@ add_test(function test_removeCallback() {
 });
 
 add_test(function test_registerDialogClosedEventHandler() {
-  let observer = new modal.DialogObserver();
+  let observer = new modal.DialogObserver(() => mockCurBrowser);
   let mockChromeWindow = {
     addEventListener(event, cb) {
       equal(
@@ -70,40 +81,22 @@ add_test(function test_registerDialogClosedEventHandler() {
 });
 
 add_test(function test_handleCallbackOpenModalDialog() {
-  let observer = new modal.DialogObserver();
+  let observer = new modal.DialogObserver(() => mockCurBrowser);
 
-  observer.add((action, target, win) => {
+  observer.add((action, dialog) => {
     equal(action, modal.ACTION_OPENED, "'opened' action has been passed");
-    equal(
-      target.get(),
-      mockModalDialog,
-      "weak reference has been created for target"
-    );
-    equal(
-      win,
-      mockModalDialog.opener.ownerGlobal,
-      "chrome window has been passed"
-    );
+    equal(dialog, mockModalDialog, "dialog has been passed");
     run_next_test();
   });
   observer.observe(mockModalDialog, "common-dialog-loaded");
 });
 
 add_test(function test_handleCallbackCloseModalDialog() {
-  let observer = new modal.DialogObserver();
+  let observer = new modal.DialogObserver(() => mockCurBrowser);
 
-  observer.add((action, target, win) => {
+  observer.add((action, dialog) => {
     equal(action, modal.ACTION_CLOSED, "'closed' action has been passed");
-    equal(
-      target.get(),
-      mockModalDialog,
-      "weak reference has been created for target"
-    );
-    equal(
-      win,
-      mockModalDialog.opener.ownerGlobal,
-      "chrome window has been passed"
-    );
+    equal(dialog, mockModalDialog, "dialog has been passed");
     run_next_test();
   });
   observer.handleEvent({
@@ -112,49 +105,14 @@ add_test(function test_handleCallbackCloseModalDialog() {
   });
 });
 
-add_test(function test_handleCallbackOpenTabModalDialog() {
-  let observer = new modal.DialogObserver();
-
-  observer.add((action, target, win) => {
-    equal(action, modal.ACTION_OPENED, "'opened' action has been passed");
-    equal(
-      target.get(),
-      mockTabModalDialog,
-      "weak reference has been created for target"
-    );
-    equal(win, mockTabModalDialog.ownerGlobal, "chrome window has been passed");
-    run_next_test();
-  });
-  observer.observe(mockTabModalDialog, "tabmodal-dialog-loaded");
-});
-
 add_test(function test_dialogClosed() {
-  let observer = new modal.DialogObserver();
+  let observer = new modal.DialogObserver(() => mockCurBrowser);
 
-  observer.dialogClosed(mockTabModalDialog.ownerGlobal).then(() => {
+  observer.dialogClosed().then(() => {
     run_next_test();
   });
   observer.handleEvent({
     type: "DOMModalDialogClosed",
-    target: mockTabModalDialog,
-  });
-});
-
-add_test(function test_handleCallbackCloseTabModalDialog() {
-  let observer = new modal.DialogObserver();
-
-  observer.add((action, target, win) => {
-    equal(action, modal.ACTION_CLOSED, "'closed' action has been passed");
-    equal(
-      target.get(),
-      mockTabModalDialog,
-      "weak reference has been created for target"
-    );
-    equal(win, mockTabModalDialog.ownerGlobal, "chrome window has been passed");
-    run_next_test();
-  });
-  observer.handleEvent({
-    type: "DOMModalDialogClosed",
-    target: mockTabModalDialog,
+    target: mockModalDialog,
   });
 });

testing/web-platform/tests/webdriver/tests/execute_async_script/execute_async.py

@@ -43,6 +43,25 @@ def test_abort_by_user_prompt(session, dialog_type):
     session.alert.accept()
 
 
+@pytest.mark.parametrize("dialog_type", ["alert", "confirm", "prompt"])
+def test_no_abort_by_user_prompt_in_other_tab(session, dialog_type):
+    original_handle = session.window_handle
+    new_handle = session.new_window()
+
+    session.execute_script("setTimeout(() => {}('foo'), 250);".format(dialog_type))
+
+    session.window_handle = new_handle
+    response = execute_async_script(
+        session,
+        "setTimeout(() => arguments[0](42), 1000);")
+    assert_success(response, 42)
+
+    session.window.close()
+
+    session.window_handle = original_handle
+    session.alert.accept()
+
+
 @pytest.mark.parametrize("dialog_type", ["alert", "confirm", "prompt"])
 def test_abort_by_user_prompt_twice(session, dialog_type):
     response = execute_async_script(