Closed Bug 1721982 Opened 3 years ago Closed 3 years ago

"WebDriver:AcceptAlert" and "WebDriver:DismissAlert" can hang for alerts in opened popup windows

Categories

(Remote Protocol :: Marionette, defect, P3)

Firefox 90
defect

Tracking

(firefox-esr78 unaffected, firefox90 wontfix, firefox91 fixed, firefox92 fixed)

RESOLVED FIXED
92 Branch
Tracking Status
firefox-esr78 --- unaffected
firefox90 --- wontfix
firefox91 --- fixed
firefox92 --- fixed

People

(Reporter: kovalev007, Assigned: whimboo)

References

(Regression)

Details

(Keywords: regression)

Attachments

(6 files)

User Agent: Mozilla/5.0 (Windows NT 10.0; Win64; x64) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/92.0.4515.107 Safari/537.36

Steps to reproduce:

Testcase:
https://output.jsbin.com/zelecaj

click on button with text "open"
wait new window opened
click on button with text "alert"
accept alert

Actual results:

geckodriver hung after accept alert

Expected results:

test should be continue

Bug exists in Firefox beta and Firefox nightly

Attached file Test.java
Attachment #9232795 - Attachment mime type: application/octet-stream → text/plain

From mozregression_log_from_89_to_90.txt
Last good revision: ce8036ee1a2c572f653b79fe42108815f23af15d
First bad revision: 150c94fd5ad36145cb171c217702e9cb7c589793
Pushlog: https://hg.mozilla.org/releases/mozilla-release/pushloghtml?fromchange=ce8036ee1a2c572f653b79fe42108815f23af15d&tochange=150c94fd5ad36145cb171c217702e9cb7c589793

I tried to narrow down my search - with no success
mozregression --good ce8036ee1a2c572f653b79fe42108815f23af15d --bad 150c94fd5ad36145cb171c217702e9cb7c589793 --command 'performance-crawler.bat'

0:02.15 INFO: bits option not specified, using 64-bit builds.
0:03.43 INFO: 150c94fd5ad36145cb171c217702e9cb7c589793 is not a release, assuming it's a hash...
0:04.74 INFO: ce8036ee1a2c572f653b79fe42108815f23af15d is not a release, assuming it's a hash...
0:04.75 INFO: Getting mozilla-central builds between ce8036ee1a2c572f653b79fe42108815f23af15d and 150c94fd5ad36145cb171c217702e9cb7c589793
0:05.95 ERROR: The url 'https://hg.mozilla.org/mozilla-central/json-pushes?changeset=ce8036ee1a2c572f653b79fe42108815f23af15d' returned a 404 error because the push is not in this repo (e.g., not merged yet).

Also I tried use mozregression by days
From mozregression_log_from_2021-04-25_to_202104-26.txt
Last good revision: 82d59bab21dadcad3c63aa1537fc48fa3c8a4e98
First bad revision: 15253a3168d2f2543a5348d47157f033995e928b
Pushlog:
https://hg.mozilla.org/integration/autoland/pushloghtml?fromchange=82d59bab21dadcad3c63aa1537fc48fa3c8a4e98&tochange=15253a3168d2f2543a5348d47157f033995e928b

Henrik Skupin you can confirm that the regression appeared after Bug 1701686?

Flags: needinfo?(hskupin)

Thanks a lot for the report George! It's great to see that you get back to us with such detailed information for regressions. I appreciate that!

I had a look at the example code and I'm able to reproduce it with Marionette itself. It fails since bug 1701686 has been landed. Specifically the following changeset is responsible:

https://hg.mozilla.org/integration/autoland/rev/e352f905ecf8

I assume that the window check if (action == modal.ACTION_CLOSED && window == win) { is failing here. I'll have a look at it.

We should be able to easily get it fixed for the 91 release.

Status: UNCONFIRMED → NEW
Ever confirmed: true
Keywords: regression
Regressed by: 1701686

The problem here is actually that we no longer wait for a DOMModalDialogClosed event, but rely on the toplevel-window-ready observer notification, which centrally registers such an event listener for the newly opened window. But that's not done here. Due to whatever reason no toplevel-window-ready notification is fired when a new window (popup) gets opened. As such no such listener is active.

From the MDN docs I see Called just after a new top level window has been opened and is ready, but has not yet loaded a document:
https://github.com/mdn/archived-content/blob/main/files/en-us/mozilla/tech/xpcom/observer_notifications/index.html

Olli, do you know why we do not get such an observer? Is there some other notification we should register for? I tried some others from the above page, but none seem to be helpful.

Flags: needinfo?(hskupin) → needinfo?(bugs)
Summary: Hang after accept alert → "WebDriver:AcceptAlert" and "WebDriver:DismissAlert" can hang for alerts in opened popup windows

The reason why a new window gets opened here is because status: no is passed as attribute into the window.open() call. If that is removed we always open a new tab instead.

Attached can be found a minimized Marionette test to reproduce it. It can be run via:

./mach marionette-test --gecko-log - %path%

As discussed with Arai on Elements it looks like that we will have to switch to the domwindowopened observer notification. That gets really fired for each and every new window.

Will have to check if that causes some kind of side-effect in marionette through..

Flags: needinfo?(bugs)

FYI there is a standards discussion regarding the topic when a new popup should be opened:
https://github.com/whatwg/html/issues/5872

Given that it should always happen across all browsers when a width and height is specified I will add a new Wdspec test to cover this specific scenario.

Not sure how many pages are out there that use real popups and are affected, but I think it's worth getting fixed for the 91 release. Will take this bug.

Assignee: nobody → hskupin
Severity: -- → S3
Status: NEW → ASSIGNED
Priority: -- → P3
Attachment #9232868 - Attachment description: Bug 1721982 - [wdspec] Add test to accept or dismiss a user prompt in a popup window. → WIP: Bug 1721982 - [wdspec] Add test to accept or dismiss a user prompt in a popup window.
Attachment #9232869 - Attachment description: Bug 1721982 - [marionette] Use domwindowopened observer to detect newly opened windows. → WIP: Bug 1721982 - [marionette] Use domwindowopened observer to detect newly opened windows.
Attachment #9232868 - Attachment description: WIP: Bug 1721982 - [wdspec] Add test to accept or dismiss a user prompt in a popup window. → Bug 1721982 - [wdspec] Add test to accept or dismiss a user prompt in a popup window.
Attachment #9232869 - Attachment description: WIP: Bug 1721982 - [marionette] Use domwindowopened observer to detect newly opened windows. → Bug 1721982 - [marionette] Use domwindowopened observer to detect newly opened windows.
Pushed by hskupin@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/2520da75e848
[wdspec] Add test to accept or dismiss a user prompt in a popup window. r=webdriver-reviewers,jdescottes
https://hg.mozilla.org/integration/autoland/rev/926af858a573
[marionette] Use domwindowopened observer to detect newly opened windows. r=webdriver-reviewers,jdescottes
Created web-platform-tests PR https://github.com/web-platform-tests/wpt/pull/29782 for changes under testing/web-platform/tests
Status: ASSIGNED → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → 92 Branch

(In reply to Web Platform Test Sync Bot (Matrix: #interop:mozilla.org) from comment #14)

Created web-platform-tests PR
https://github.com/web-platform-tests/wpt/pull/29782 for changes under
testing/web-platform/tests

James, can you please check why the PR doesn't get automatically merged? Thanks!

Flags: needinfo?(james)

Comment on attachment 9232869 [details]
Bug 1721982 - [marionette] Use domwindowopened observer to detect newly opened windows.

Beta/Release Uplift Approval Request

  • User impact if declined: If web pages under test by WebDriver open a new popup window any user prompt that gets opened from within this window cannot be closed and Marionette hangs.
  • Is this code covered by automated tests?: Yes
  • Has the fix been verified in Nightly?: No
  • Needs manual test from QE?: No
  • If yes, steps to reproduce:
  • List of other uplifts needed: None
  • Risk to taking this patch: Low
  • Why is the change risky/not risky? (and alternatives if risky): The change only affects the handling of user prompts (content, tab, modal) and simply swaps the observer notification to listen for.
  • String changes made/needed: no
Attachment #9232869 - Flags: approval-mozilla-beta?
Attachment #9232868 - Flags: approval-mozilla-beta?

Comment on attachment 9232869 [details]
Bug 1721982 - [marionette] Use domwindowopened observer to detect newly opened windows.

Approved for 91 beta 8, thanks.

Attachment #9232869 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
Attachment #9232868 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
Upstream PR merged by jgraham

Thanks everyone!

Flags: needinfo?(james)
Flags: in-testsuite+
Product: Testing → Remote Protocol
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: