1686741 - Add complete support for the new tab modal dialog

Status: RESOLVED FIXED

(Remote Protocol :: Marionette, enhancement, P3)

Description

[Micah [:mtigley] (she/her)]

Marionette tests currently have the new modal UI disabled. This issue is for updating the existing tests.

Comment 1

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

It's not only around updating the tests in here:
https://searchfox.org/mozilla-central/source/testing/marionette/harness/marionette_harness/tests/unit/test_modal_dialogs.py

Most of the tests actually work, but the ones that create a modal dialog before the Marionette session gets actually started cannot be found.

When starting to work on this bug the preference as set in recommended preferences has to be removed:
https://searchfox.org/mozilla-central/source/testing/marionette/components/marionette.js

Comment 2

[BugBot [:suhaib / :marco/ :calixte]]

Set release status flags based on info from the regressing bug 1680637

Comment 3

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

Micah, would you have the time to get the support added to Marionette? I would kinda appreciate. If not mind giving some advice in how to use the new modal dialog type?

Comment 4

[Micah [:mtigley] (she/her)]

Hey Henrik, sorry I dropped the ball on this. I've been swept up with other work.

(In reply to Henrik Skupin (:whimboo) [⌚️UTC+1] from comment #3)

Micah, would you have the time to get the support added to Marionette? I would kinda appreciate. If not mind giving some advice in how to use the new modal dialog type?

Of course! Here are some things we've been using in mochitests to test the new modal dialog type:

• We can get a modal container for a tab using gBrowser.getTabDialogBox.

• Modals opened by JS are stored in a separate manager on TabDialogBox. To get access to this manager we need to call TabDialogBox.getContentManager.

• We get access to its opened modal dialogs by accessing its ._dialogs member.

• To examine the contents of a single dialog. We have to access its dialog._frame.contentWindow object.

It might be easier to show an example of all these pieces being used in an existing test. I recently wrote a test for checking the title text of a dialog at: https://searchfox.org/mozilla-central/rev/fd853f4aea89186efdb368e759a71b7a90c2b89c/browser/base/content/test/tabdialogs/browser_tabdialogbox_content_prompts.js#50-53.

Comment 5

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

(In reply to Micah Tigley [:mtigley] from comment #4)

It might be easier to show an example of all these pieces being used in an existing test. I recently wrote a test for checking the title text of a dialog at: https://searchfox.org/mozilla-central/rev/fd853f4aea89186efdb368e759a71b7a90c2b89c/browser/base/content/test/tabdialogs/browser_tabdialogbox_content_prompts.js#50-53.

Thanks! I seem to have gotten it working for tab modals like alert and such, but I still wonder about those like for HTTP authentication. Those are still using getTabDialogManager(), right? So it's only alert, confirm, and prompt that implement the SubDialog path? It means we have to keep those existing checks?

Comment 6

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

Attachment: Bug 1686741 - [marionette] Add support for the new content modal dialog.

Comment 7

[Micah [:mtigley] (she/her)]

(In reply to Henrik Skupin (:whimboo) [⌚️UTC+1] from comment #5)

Thanks! I seem to have gotten it working for tab modals like alert and such, but I still wonder about those like for HTTP authentication. Those are still using getTabDialogManager(), right? So it's only alert, confirm, and prompt that implement the SubDialog path? It means we have to keep those existing checks?

Yes the HTTP auth modal is still using getTabDialogManager. Both the HTTP auth and alert/confirm/prompt modals used SubDialog, which begins around here: https://searchfox.org/mozilla-central/source/browser/actors/PromptParent.jsm#293-305.

The only difference between them is that prompts like HTTP auth are caused by webcontent but need to look like it's part of secure browser UI whereas alert/confirm/prompt don't need to be. It's mostly a styling difference. And to manage the two prompt types, we store them in separate managers, which are retrievable via getTabDialogManager() (for "tab" prompt types) or getContentDialogManager() (for "content" prompt types).

Comment 8

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

Making this change actually triggers a known intermittent test failure for TestTabModalAlerts.test_handle_two_dialogs (bug 1559992) pretty permanently. And as it looks like the first "WebDriver:AcceptAlert command returns to early and as such fires off immediately the next command, whereby the common-dialog-loaded observer notification is arrived afterward:

Here how it looks like when it works:

1613406564562   Marionette  DEBUG   10 -> [0,16,"WebDriver:ElementClick",{"id":"7991fec3-763f-cd44-8dba-302a6b1cc3a5"}]
1613406564605   Marionette  TRACE   Received observer notification common-dialog-loaded
1613406564605   Marionette  TRACE   Received observer notification common-dialog-loaded
1613406564605   Marionette  TRACE   Canceled page load listener because a dialog opened
1613406564605   Marionette  DEBUG   10 <- [1,16,null,{"value":null}]
1613406564643   Marionette  DEBUG   10 -> [0,17,"WebDriver:SendAlertText",{"text":"foo"}]
1613406564643   Marionette  DEBUG   10 <- [1,17,null,{"value":null}]
1613406564646   Marionette  DEBUG   10 -> [0,18,"WebDriver:AcceptAlert",{}]
1613406564650   Marionette  TRACE   Received event DOMModalDialogClosed
1613406564650   Marionette  TRACE   Received event DOMModalDialogClosed
1613406564650   Marionette  TRACE   Received DOM event DOMModalDialogClosed for [object XULFrameElement]
1613406564655   Marionette  DEBUG   10 <- [1,18,null,{"value":null}]
1613406564712   Marionette  TRACE   Received observer notification common-dialog-loaded
1613406564712   Marionette  TRACE   Received observer notification common-dialog-loaded
1613406564712   Marionette  DEBUG   10 -> [0,19,"WebDriver:SendAlertText",{"text":"bar"}]
1613406564712   Marionette  DEBUG   10 <- [1,19,null,{"value":null}]

And here when it fails:

1613406565071   Marionette  DEBUG   11 -> [0,16,"WebDriver:ElementClick",{"id":"65fb781e-b867-f94b-ae23-2f5ccd4c6516"}]
1613406565142   Marionette  TRACE   Received observer notification common-dialog-loaded
1613406565142   Marionette  TRACE   Received observer notification common-dialog-loaded
1613406565142   Marionette  TRACE   Canceled page load listener because a dialog opened
1613406565142   Marionette  DEBUG   11 <- [1,16,null,{"value":null}]
1613406565182   Marionette  DEBUG   11 -> [0,17,"WebDriver:SendAlertText",{"text":"foo"}]
1613406565182   Marionette  DEBUG   11 <- [1,17,null,{"value":null}]
1613406565184   Marionette  DEBUG   11 -> [0,18,"WebDriver:AcceptAlert",{}]
1613406565187   Marionette  TRACE   Received event DOMModalDialogClosed
1613406565187   Marionette  TRACE   Received event DOMModalDialogClosed
1613406565187   Marionette  TRACE   Received DOM event DOMModalDialogClosed for [object XULFrameElement]
1613406565188   Marionette  DEBUG   11 <- [1,18,null,{"value":null}]
1613406565216   Marionette  DEBUG   11 -> [0,19,"WebDriver:SendAlertText",{"text":"bar"}]
1613406565216   Marionette  DEBUG   11 <- [1,19,{"error":"no such alert","message":"","stacktrace":"WebDriverError@chrome://marionette/content/error.js:181:5\nNoSuchAl ... t@chrome://marionette/content/server.js:239:9\n_onJSONObjectReady/<@chrome://marionette/content/transport.js:504:20\n"},null]
1613406565222   Marionette  TRACE   Received observer notification common-dialog-loaded
1613406565222   Marionette  TRACE   Received observer notification common-dialog-loaded

Comment 9

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

Micah, could it be that the creation of the new tab modals is a bit slower than before? In the above case it's actually for the 2nd prompt as opened from here:

https://searchfox.org/mozilla-central/rev/b32d4ca055ca9cf717be480df640f8970724a0ce/testing/marionette/harness/marionette_harness/www/test_tab_modal_dialogs.html#27-30,37

Is there any kind of additional logging available for prompts? Not sure how to best investigate that race condition.

Comment 10

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

We clarified the problem, and actually have to wait for the second user prompt to appear.

Comment 11

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

Attachment: Bug 1686741 - [marionette] Use dialogObserver when awaiting dialog to be closed.

Using the "waitForEvent" promise will cause an extra
"DOMModalDialogClosed" event to be logged, which is
confusing.

Comment 12

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

Attachment: Bug 1686741 - [marionette] "WebDriver:SwitchToWindow" needs to search for currently open user prompts.

Comment 13

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/213c96fc82bd
[marionette] "WebDriver:SwitchToWindow" needs to search for currently open user prompts. r=marionette-reviewers,jdescottes
https://hg.mozilla.org/integration/autoland/rev/edb492e25698
[marionette] Use dialogObserver when awaiting dialog to be closed. r=marionette-reviewers,jdescottes
https://hg.mozilla.org/integration/autoland/rev/68ceed0186b8
[marionette] Add support for the new tab modal dialog. r=marionette-reviewers,mtigley,jdescottes

Comment 14

[Alexandru Michis [:malexandru]]

Backed out 3 changesets (bug 1686741) for causing awsy failures in TestMemoryUsage.test_open_tabs
Backout link: https://hg.mozilla.org/integration/autoland/rev/df94cbb3007d4edf4ccd67b72ad7eda50c074603
Push with failures, failure log.

Comment 15

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

I tried to reproduce locally but I'm not able to so far. Not sure what's causing this test to fail in CI. Interesting is at least that it always happens for the page https://yandex.ru/search/?text=barack%20obama&lr=10115. The failure output that we get is:

[task 2021-03-09T22:45:03.833Z] 22:45:03     INFO - starting checkpoint TabsOpenSettled...
[task 2021-03-09T22:45:04.126Z] 22:45:04     INFO - checkpoint created, stored in /builds/worker/workspace/build/tests/results/memory-report-TabsOpenSettled-0.json.gz
[task 2021-03-09T22:45:05.797Z] 22:45:05     INFO - TEST-UNEXPECTED-FAIL | awsy/test_memory_usage.py TestMemoryUsage.test_open_tabs | AssertionError: Checkpoint was recorded

The related code can be found here:

https://searchfox.org/mozilla-central/rev/2b99ea2e97eef00a8a1c7e24e5fe51ab5304bc42/testing/awsy/awsy/awsy_test_case.py#183-229

Based on the log output the problem happens for the TabsOpenSettled checkpoint. It is getting created via an asynchronous script with a timeout of 60s. But it looks like that the script hasn't even been run at all, because checkpoint created is logged right after. As such there is no expected return value and checkpoint will not be updated and keeps None, and the else block is entered to print the path of the checkpoint.

The gecko.log artifact doesn't show any helpful information.

As such I triggered a try build that enables trace logs for Marionette:
https://treeherder.mozilla.org/jobs?repo=try&revision=8c0ad3ac5f90d89846c1da07ed759ce44a582807

Comment 16

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

Btw we are not going to uplift the patches given that this feature is still only available in nightly and early beta.

Comment 17

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

The problem here is indeed a dialog that pops-up when trying to execute the async script:

https://treeherder.mozilla.org/logviewer?job_id=332653490&repo=try&lineNumber=3188-3193

[task 2021-03-10T08:54:29.555Z] 08:54:29     INFO -  1615366469548	Marionette	DEBUG	2 -> [0,384,"WebDriver:ExecuteAsyncScript",{"scriptTimeout":60000,"newSandbox":true,"args":[],"filename":"../../venv/lib/python2.7 ...                 /* anonymize */ false,\n                /* minimize memory usage */ false);","sandbox":"default","line":212}]
[task 2021-03-10T08:54:29.555Z] 08:54:29     INFO -  1615366469551	Marionette	TRACE	Received event DOMModalDialogClosed
[task 2021-03-10T08:54:29.555Z] 08:54:29     INFO -  1615366469552	Marionette	TRACE	Received event DOMModalDialogClosed
[task 2021-03-10T08:54:29.760Z] 08:54:29     INFO -  1615366469757	Marionette	TRACE	Received observer notification common-dialog-loaded
[task 2021-03-10T08:54:29.760Z] 08:54:29     INFO -  1615366469757	Marionette	TRACE	Received observer notification common-dialog-loaded
[task 2021-03-10T08:54:29.760Z] 08:54:29     INFO -  1615366469757	Marionette	DEBUG	2 <- [1,384,null,{"value":null}]

Because the dialog gets opened while the script executes the command immediately returns with value null as expected. I wonder which kind of dialog this actually is that interferes with AWSY. I will do another try build with more details in the log message.

Comment 18

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

The alert that pops-up has the following text:

https://treeherder.mozilla.org/logviewer?job_id=332693230&repo=try&lineNumber=3253-3254

S-a produs o eroare de server. Apăsați pe „„Reload” (Reîncărcați) în browserul dvs.

That basically means A server error has occurred. Click "Reload" in your browser.

I'm not sure why this error is now visible with my patches. I might want to trigger individual try builds for each of the commits to at least get an idea where the regression got introduced.

Comment 19

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

The failure comes indeed from the patch to add support for the new tab modal dialog. Without it the test job is green:
https://treeherder.mozilla.org/jobs?repo=try&revision=04bf06bb09052c59219bbc00555560731b4fb13b

Micah, do you have an idea why this comes up now? It's not clear to me why we didn't detect that prompt before.

Given that I cannot see the alert text in the recording of the Yandex website, I wonder if this could be an alert that got injected by mitmproxy. But having that in Romanian language is strange. Greg, any idea?

Comment 20

[Greg Mierzwinski [:sparky]]

Most of our recordings get recorded in Romania so that's likely why it's in Romanian.

Comment 21

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

(In reply to Greg Mierzwinski [:sparky] from comment #20)

Most of our recordings get recorded in Romania so that's likely why it's in Romanian.

Ok, but do you have an idea where this error is coming from? I do not see it in the recording. But maybe I missed to check other locations awsy is putting files into.

This is the command that I used locally (and that doesn't reproduce it):

mach awsy-test testing/awsy/awsy/test_memory_usage.py --settle-wait-time 1 --per-tab-pause 1 --tp6 --gecko-log - -vv

Comment 22

[Greg Mierzwinski [:sparky]]

Oh sorry! I'm not sure where this error might be coming from, maybe re-recording the page could solve it.

Comment 23

[BugBot [:suhaib / :marco/ :calixte]]

There are some r+ patches which didn't land and no activity in this bug for 2 weeks.
:whimboo, could you have a look please?
For more information, please visit auto_nag documentation.

Comment 24

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

Micah, I would still be interested to hear from you. Does the change to the new tab modal in any way miss some kind of alerts?

Comment 25

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

I actually pushed a new try build to see which platforms actually fail on Linux:
https://treeherder.mozilla.org/jobs?repo=try&revision=5e49d100f4ccbf06ebd3d33305e48d8e05ae2956

I still have to try to get it reproduced locally.

Comment 26

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

On bug 1686707 we also have to turn off the new tab modal dialogs for wdspec tests. So if that other patch lands first, I have to remove the pref there too.

Comment 27

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

I can now reproduce the problem. It's not that the alert opens for the page in the currently selected tab, but in a different tab. Specifically here for the TP6 Google Sheet page. I'll check on Monday why we don't filter out that alert. Not sure if that's a problem with my patch or the new tab modal implementation.

Comment 28

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

The work on this bug is now blocked by bug 1701686. I still don't know why it starts to fail now.

Comment 29

[Phabricator Automation]

Comment on attachment 9203455 [details]
Bug 1686741 - [marionette] Use dialogObserver when awaiting dialog to be closed.

Revision D105316 was moved to bug 1701686. Setting attachment 9203455 [details] to obsolete.

Comment 30

[Phabricator Automation]

Comment on attachment 9207436 [details]
Bug 1686741 - [marionette] "WebDriver:SwitchToWindow" needs to search for currently open user prompts.

Revision D107460 was moved to bug 1701686. Setting attachment 9207436 [details] to obsolete.

Comment 31

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

Now that bug 1701686 got fixed I had a look at enabling the content modal dialogs. Finding already open content modal dialogs works fine with the following code:

    const tabDialogBox = context.tabBrowser.getTabDialogBox(contentBrowser);
    const dialogs = tabDialogBox.getContentDialogManager().dialogs;

But somehow tab modal dialogs cannot be found anymore. Whether with the above code now with what currently exists in modal.js.

I also tried with .browserSidebarContainer but not sure where exactly in that DOM tree the tabs are listed, and if that would be the correct approach.

Gijs, maybe you can help again so that we can get a reasonable and stable implementation for Marionette? Thanks in advance!

Comment 32

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

Mike, maybe you can also help in case Gijs isn't around or has lesser time for my questions? Thanks.

Comment 33

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

(In reply to Henrik Skupin (:whimboo) [⌚️UTC+1] from comment #32)

Mike, maybe you can also help in case Gijs isn't around or has lesser time for my questions? Thanks.

Comment 34

[Mike Conley (:mconley) (:⚙️)]

I think for tab modal dialogs, you don't want to use getContentDialogManager().dialogs, but getTabDialogManager().dialogs.

Comment 35

[:Gijs (he/him)]

Gah, sorry, the last few days have been a bit weird. I see Mike has answered your question; I think that's the correct thing to use.

Comment 36

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

(In reply to Mike Conley (:mconley) (:⚙️) (Catching up on needinfos) from comment #34)

I think for tab modal dialogs, you don't want to use getContentDialogManager().dialogs, but getTabDialogManager().dialogs.

Ah, the naming makes actually sense and it works like a charm! Thanks a lot.

I'll upload a patch pretty soon that also keeps support for the old non SubDialog implemented content modals that may be still used in other Gecko based applications and we want to keep support for them.

Comment 37

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/d14b6e704299
[marionette] Add support for the new content modal dialog. r=Gijs,marionette-reviewers,jdescottes

Comment 38

[Narcis Beleuzu [:NarcisB]]

https://hg.mozilla.org/mozilla-central/rev/d14b6e704299

Comment 39

[BugBot [:suhaib / :marco/ :calixte]]

Since the status are different for nightly and release, what's the status for beta?
For more information, please visit auto_nag documentation.

Comment 40

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

Gijs, how important would it be for the Firefox team to see support for that new feature in Marionette for 89 beta? Should we considering an uplift? If yes, we would also have to uplift bug 1701686, and fix its memory regression first.

Comment 41

[:Gijs (he/him)]

(In reply to Henrik Skupin (:whimboo) [⌚️UTC+1] from comment #40)

Gijs, how important would it be for the Firefox team to see support for that new feature in Marionette for 89 beta?

I don't think it matters for the Firefox team, unless without this patch there is perma-orange somewhere or something. Otherwise, as far as we're concerned it's just tests. What I don't know is if other marionette consumers will be surprised that in the default configuration of Firefox, they can't handle dialogs anymore...

Comment 42

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

(In reply to :Gijs (he/him) from comment #41)

I don't think it matters for the Firefox team, unless without this patch there is perma-orange somewhere or something.

No, there is no such case. So lets not uplift the patch then. Thanks!

Otherwise, as far as we're concerned it's just tests. What I don't know is if other marionette consumers will be surprised that in the default configuration of Firefox, they can't handle dialogs anymore...

They can because we disabled the new content modal dialogs via the preference for both Marionette client and geckodriver. So nothing changes for users when upgrading to Firefox 89. The patch on this bug actually removed the preference again on all necessary places.