1693857 - Support "beforeunload" user prompts in Marionette

Status: RESOLVED FIXED

(Remote Protocol :: Marionette, enhancement, P2)

Description

[James Nord]

it appears when firefox is started with the --marionette argument that alerts do not happen (seen in firefox 85 and 87 nightly (2021-02-19) 64bit windows and 85 on linux)

steps to reproduce

• download Jenkins from https://get.jenkins.io/war-stable/2.263.4/jenkins.war
• install JDK 8 https://adoptopenjdk.net/index.html
• run Jenkins (JENKINS_HOME=tmpfolder; java -Djenkins.install.runSetupWizard=false -jar jenkins.war --httpListenAddress=127.0.0.1

start a firefox normally

• navigate to http://localhost:8080/
• click "New View" in the left hand bar
• enter some random alpha-numeric characters for the View Name
• select "List View" option
• click OK
• enter some random text in the "Description field"
• Click "New Item" in the left hand bar

Observe a alert saying "This page is asking you to confirm that you want to leave - data you have entered may not be saved."

close firefox.

start firefox as if geckodriver had started it e.g.

• firefox.exe "--marionette" "-foreground" "-no-remote" "-profile" "C:\\Users\\jnord\\AppData\\Local\\Temp\\rust_mozprofileN6VBXM"
• navigate to http://localhost:8080/
• click "New View" in the left hand bar
• enter some random alpha-numeric characters for the View Name
• select "List View" option
• click OK
• enter some random text in the "Description field"
• Click "New Item" in the left hand bar

Observe the lack of any alert

This was extracted from observing the failure of https://github.com/jenkinsci/acceptance-test-harness/blob/master/src/test/java/core/FormValidationTest.java failing to show any alert window and the waitFor(Alert) timing out.

Comment 1

[Henrik Skupin [:whimboo][⌚️UTC+1]]

Just to be sure with alerts you only reference beforeunload prompts? Any other user prompt (alert, confirm, prompt) is still working fine?

Comment 2

[James Nord]

I think Jenkins only uses a couple of alert types and not sure if any of the others have selenium tests.
I'm more of a backend Dev and wouldn't have known this was how it's implemented until you asked the question, but I guess
https://github.com/jenkinsci/jenkins/blob/jenkins-2.280/core/src/main/resources/lib/form/confirm.js#L119 is where this happens.
I'll go poking to see if the other alert types we have work.
https://github.com/jenkinsci/jenkins/blob/jenkins-2.280/core/src/main/resources/lib/layout/confirmationLink.jelly#L48

Comment 3

[Henrik Skupin [:whimboo][⌚️UTC+1]]

Yeah, so this is a beforeunload prompt, and as given by the WebDriver spec those are implicitly dismissed:
https://w3c.github.io/webdriver/#dfn-execute-a-function-body#x16-user-prompts

User prompts that are spawned from beforeunload event handlers, are dismissed implicitly upon navigation or close window, regardless of the defined user prompt handler.

Note that right now we set a preference to turn those off globally, and as such you don't see these alerts even with navigations not triggered via Marionette itself.

Note that there is also https://github.com/w3c/webdriver/issues/1294 to get a better definition for handling beforeunload events. But as long as that hasn't been defined we will keep the code that we currently have.

Comment 4

[James Nord]

Hi Henrik,

Thanks for investigating and the link to the spec. However I disagree with your conclusion with the understanding of the spec

User prompts that are spawned from beforeunload event handlers, are dismissed implicitly upon navigation or close window, regardless of the defined user prompt handler.

Now - the navigation happens before there is a user prompt. that is the beforeunloaded prompt is not already showing then the navigation is attempted, but as a result of the navigation.

thus it is incorrect to dismiss something that does not exist at the time of the navigation

timeline

1. page is rendered
2. a navigation event occurs (the click)
3. the beforeunload should fire and a prompt should display

what the spec is saying as I read it is that if you then did another naviagion (webdriver.navigate()) then the alter would be silently dismissed.

in other words

1. page is rendered
2. a navigation event occurs (the click)
3. the beforeunload should fire and a prompt should display
4. another navigation occurs (via webdriver.navigate) the alter gets dismissed unconditionally.

I do not see why you think your interpretation is the correct one, especially in something designed to test web interations, this would basically make testing beforeunloaded prompts essentially untestable.

Note that there is also https://github.com/w3c/webdriver/issues/1294 to get a better definition for handling beforeunload events

Firefox did not used to behave like this so if you feel the spec is ambiguous I would recommend that you revert to your prior handling.

Comment 5

[Henrik Skupin [:whimboo][⌚️UTC+1]]

Well, beforeunload prompts are always opened when a navigation is about to start. And that is what the WebDriver spec covers here. It shouldn't be about user prompts that were opened before. And that's the same with the click + navigation here.

James, maybe you can weight in here and clarify the spec situation?

Comment 6

[James Graham [:jgraham]]

I think it's correct that beforeunload prompts are untestable with current WebDriver.

Every beforeunload prompt is generated either by a navigation or closing a borwsing context. And per the spec any such prompt is instantly dismissed when a WebDriver session is active.

I agree that there are a number of problems here; certain scenarios are untestable, and the spec isn't very clearly written, but I believe that the current marionette implementation is consistent with what's intended by the current spec.

Comment 7

[James Nord]

FTR I filed https://github.com/w3c/webdriver/pull/1573, seems like PRs are more active than the GH issues.

Comment 8

[James Nord]

Every beforeunload prompt is generated either by a navigation or closing a borwsing context

then why would the spec need to say the following:

User prompts that are spawned from beforeunload event handlers, are dismissed implicitly upon navigation or close window, regardless of the defined user prompt handler.

surely it would just say

User prompts that are spawned from beforeunload event handlers, are dismissed implicitly

given it is calling something out - and those callings are (as you say) the only way you can get to this situation it would seem to be superflous or alluding to something else (which would be more aligned with my understanding of it being a subsequent navigation)

A different alternative is that the href has got messed up and no one noticed.
ie the ref from navigation (upon navigation) should not be to #dfn-navigation but rather #navigation (which is section 10) and so we are talking about explicit navigation rather than implicit.

I understand now we are discussing interpretations of a spec that would be best taken to the spec itself, however the Mozilla interpretation does seem different (now) to other implementers, and would not be in the sprit of the spec.

Comment 9

[James Nord]

See the updated GH pull request. The spec had an incorrect change applied when "Go" was renamed to "Navigate To" and then subsequently missing defintions was removed causing it to link to the generic navigate.

Thus firefoxs behaviour is incorrect. the prompts should only be dismissed on close or "navigate to" (aka explicit navigation not implicit).

Comment 10

[Henrik Skupin [:whimboo][⌚️UTC+1]]

Slightly updating the bug summary so that it reflects about which topic this bug is.

Comment 11

[Henrik Skupin [:whimboo][⌚️UTC+1]]

James, until we have a decision for that issue does setting dom.disable_beforeunload to false via the capabilities work for you?

Comment 12

[Henrik Skupin [:whimboo][⌚️UTC+1]]

To get support for beforeunload user prompts we need to stop modifying the value of the dom.disable_beforeunload preference. Once done we should make sure that such a prompt is always dismissed when a navigation is started or a window is closed. At least this is the current definition of the spec.

But there is also https://github.com/w3c/webdriver/issues/1294 which requests a better handling of the user prompt especially when certain tests want to specifically test the different behavior of the page depending on if the user is accepting or dismissing the dialog.

James, not sure if we want to get this fixed for classic or need to take a special look for BiDi only. What do you think?

Comment 13

[James Graham [:jgraham]]

I think all prompt behaviour is much easier in BiDi because we can send an event and have the client handle it, rather than having to install a specific user prompt handler. If there's still demand to change classic after BiDi is fixed maybe we should look at the options.

Comment 14

[Henrik Skupin [:whimboo][⌚️UTC+1]]

Actually thinking more about it we need to get it fixed for Marionette. If we keep dom.disable_beforeunload turned off there is no way for BiDi to handle such kind of prompts. And if we turn it on we can get WebDriver Bidi working but would regress our WebDriver classic implementation.

So maybe we cover this bug in M7 and do the WebDriver BiDi implementation in M8. Lets discuss tomorrow.

Comment 15

[Henrik Skupin [:whimboo][⌚️UTC+1]]

Note that a WebDriver classic spec decision is needed first. See https://github.com/w3c/webdriver/issues/1294 for the recent status.

Comment 16

[Henrik Skupin [:whimboo][⌚️UTC+1]]

It's too late for the milestone 7 and the next milestone has been pretty much filled-up. As such lets schedule for milestone 9.

Comment 17

[Henrik Skupin [:whimboo][⌚️UTC+1]]

The work on this bug is most likely coming with M11. For now moving to backlog.

Comment 18

[Henrik Skupin [:whimboo][⌚️UTC+1]]

Hi Olivia. I'm currently working on beforeunload support for WebDriver, and trying to detect such a dialog on Android currently fails. When I check manually I do not even see it appearing when I run the wdspec tests in headful mode. Do you know if there is a limitation or difference for the testrunner compared to the Fenix? On desktop we can use the args.inPermitUnload flag from the Prompter class. But there is no such thing for mobile. Do you have a hint what to work with on Android? Thanks.

Comment 19

[Henrik Skupin [:whimboo][⌚️UTC+1]]

So the actual code that I have sets up correctly the listeners for modal dialogs, but it looks like due to the following two lines in the adb logcat the beforeunload prompt is handled by GeckoView itself?

01-16 11:19:14.350  4583  4583 D GeckoSession: handleMessage GeckoView:Prompt
01-16 11:19:14.350  4583  4583 D Prompts : handleEvent beforeUnload

Based on that we seem to not received a notification, but also the navigation does not proceed and we timeout:

01-16 11:24:36.269  5213  5238 I Gecko   : 1705400676269	Marionette	DEBUG	0 -> [0,9,"WebDriver:ElementClick",{"id":"7a30fead-c344-4730-9250-af935f154577"}]
01-16 11:24:36.274  5380  5404 I Gecko   : 1705400676274	Marionette	TRACE	Received DOM event click for https://web-platform.test:8443/webdriver/tests/support/inline.py?doc=%3C%21doctype+html%3E%0A%3Cmeta+charset%3DUTF-8%3E%0A%3Cdiv+id%3D%22foo%22%3E&mime=text%2Fhtml&charset=UTF-8
01-16 11:24:36.275  5213  5238 W GeckoViewAutofill: Disregarding old session 3dabaa87-7fe9-4556-aa68-87c6b6af1591
01-16 11:24:36.276  5213  5238 I Gecko   : 1705400676276	Marionette	TRACE	[3] Received event beforeunload for https://web-platform.test:8443/webdriver/tests/support/inline.py?doc=%3C%21doctype+html%3E%0A%3Cmeta+charset%3DUTF-8%3E%0A%0A++++++++%3Cinput+type%3D%22text%22%3E%3C%2Finput%3E%0A++++++++%3Ca+href%3D%22https%3A%2F%2Fweb-platform.test%3A8443%2Fwebdriver%2Ftests%2Fsupport%2Finline.py%3Fdoc%3D%253C%2521doctype%2Bhtml%253E%250A%253Cmeta%2Bcharset%253DUTF-8%253E%250A%253Cdiv%2Bid%253D%2522foo%2522%253E%26mime%3Dtext%252Fhtml%26charset%3DUTF-8%22%3EClick%3C%2Fa%3E%0A++++++++%3Cscript%3E%0A++++++++++++window.addEventListener%28%22beforeunload%22%2C+function+%28event%29+%7B%0A++++++++++++++++event.preventDefault%28%29%3B%0A++++++++++++%7D%29%3B%0A++++++++%3C%2Fscript%3E%0A++++++++&mime=text%2Fhtml&charset=UTF-8
01-16 11:24:36.277  5213  5213 D GeckoSession: handleMessage GeckoView:Prompt
01-16 11:24:36.277  5213  5213 D Prompts : handleEvent beforeUnload
[..]
01-16 11:24:43.286  5213  5238 I Gecko   : 1705400683286	Marionette	TRACE	Canceled page load listener because no navigation has been detected

Maybe that's indeed an issue with the testrunner?

Comment 20

[Henrik Skupin [:whimboo][⌚️UTC+1]]

Attachment: Bug 1693857 - [marionette] Use Promise.withResolvers for dialog handling.

Comment 21

[Henrik Skupin [:whimboo][⌚️UTC+1]]

Attachment: Bug 1693857 - [remote] Support "beforeunload" prompt type in dialog class.

Depends on D198679

Comment 22

[Henrik Skupin [:whimboo][⌚️UTC+1]]

Attachment: Bug 1693857 - [marionette] Implicitly accept "beforeunload" prompts in Marionette.

Depends on D198680

Comment 23

[Henrik Skupin [:whimboo][⌚️UTC+1]]

Attachment: WIP: Bug 1693857 - [wdspec] Enhance tests for "beforeunload" prompt behavior.

Depends on D198681

Comment 24

[Henrik Skupin [:whimboo][⌚️UTC+1]]

A first try build can be found at:
https://treeherder.mozilla.org/jobs?repo=try&revision=950dcc9f7fcc100aab632f2f8ec8e2bc0b5e7411

Comment 25

[Henrik Skupin [:whimboo][⌚️UTC+1]]

Attachment: Bug 1693857 - [remote] Only apply custom preferences if provided.

If we aren't checking that extra preferences are actually
provided, we will fail completely in setting recommended
preferences.

Depends on D198680

Comment 26

[Olivia Hall [:olivia]]

Hi Henrik ,

Thanks for looking into GeckoView too!

Maybe that's indeed an issue with the testrunner?

Just taking a quick look, it does look like onBeforeUnloadPrompt is missing in the TestRunnerActivity. Probably just needs to be similar to the other test prompt implementations to make sure it is intercepted as well.

Depending on the needs, we might also need to add more in GeckoViewPrompterParent to add any info or add the prompt to the window. I'm guessing it conforms to one of these types and needs intercepted over here too possibly to make everything aware of it.

• onBeforeUnloadPrompt Stub
• onBeforeUnloadPrompt missing in TestRunnerActivity
• GeckoView example implementation for onBeforeUnloadPrompt
• Android Components implementation for onBeforeUnloadPrompt - Fenix would use this one

Comment 27

[Henrik Skupin [:whimboo][⌚️UTC+1]]

I filed an upstream PR for the test improvements so that we can see how all the different browsers behave. It will be downstream synced via bug 1876824.

Comment 28

[Henrik Skupin [:whimboo][⌚️UTC+1]]

(In reply to Olivia Hall [:olivia] from comment #26)

Thanks for looking into GeckoView too!

Thank you for the feedback. I've created bug 1879324 and mentioned all the details there as well. If it's complicated to add we might just mark the test as disabled for now on Android.

Comment 29

[Henrik Skupin [:whimboo][⌚️UTC+1]]

FYI I just run the new beforeunload wdspec tests with the org.mozilla.fenix package and it works all fine. So even if we cannot run tests it would not block users of Nightly, beta and release builds on Android.

Comment 30

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/6d598a42b824
[marionette] Use Promise.withResolvers for dialog handling. r=webdriver-reviewers,jdescottes
https://hg.mozilla.org/integration/autoland/rev/badd9a6ca1a7
[remote] Support "beforeunload" prompt type in dialog class. r=webdriver-reviewers,jdescottes
https://hg.mozilla.org/integration/autoland/rev/da15aa7bdd0a
[remote] Only apply custom preferences if provided. r=webdriver-reviewers,jdescottes
https://hg.mozilla.org/integration/autoland/rev/d01d1d933b22
[marionette] Implicitly accept "beforeunload" prompts in Marionette. r=webdriver-reviewers,jdescottes

Comment 31

[Web Platform Test Sync Bot (Matrix: #interop:mozilla.org)]

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

Comment 32

[Sandor Molnar[:smolnar]]

https://hg.mozilla.org/mozilla-central/rev/6d598a42b824
https://hg.mozilla.org/mozilla-central/rev/badd9a6ca1a7
https://hg.mozilla.org/mozilla-central/rev/da15aa7bdd0a
https://hg.mozilla.org/mozilla-central/rev/d01d1d933b22

Comment 33

[Web Platform Test Sync Bot (Matrix: #interop:mozilla.org)]

Upstream PR was closed without merging

Comment 34

[Web Platform Test Sync Bot (Matrix: #interop:mozilla.org)]

Upstream PR was closed without merging

Comment 35

[Web Platform Test Sync Bot (Matrix: #interop:mozilla.org)]

Upstream PR merged by moz-wptsync-bot

Comment 36

[Web Platform Test Sync Bot (Matrix: #interop:mozilla.org)]

Upstream PR merged by moz-wptsync-bot