1487358 - "Accept Alert" and "Dismiss Alert" should check for remaining user prompts

Status: RESOLVED FIXED

(Remote Protocol :: Marionette, enhancement, P1)

Description

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

Partly a regression because my patch on bug 1479368 got rid of the race condition when closing user prompts.

I propose this as a workaround until we have bug 1477977 sanely implemented. for now it would give us the opportunity to handle multiple existent user prompts.

Both methods currently reset the internal modal dialog reference to `null`, whereby they should check for remaining open user prompts:

https://dxr.mozilla.org/mozilla-central/rev/c2e3be6a1dd352b969a45f0b85e87674e24ad284/testing/marionette/driver.js#3165

Comment 1

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

Attachment: [marionette] "Accept Alert" and "Dismiss Alert" should check for remaining user prompts

Workaround until we have a sane dynamic user prompt implementation
(see bug 1477977). At least for now this patch will give us the
opportunity to handle multiple open user prompts.

Comment 2

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

https://treeherder.mozilla.org/#/jobs?repo=try&revision=184ef46493367cc67a1d7df4759082760d6858ab

Comment 3

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

Comment on attachment 9005183 [details] [diff] [review]
[marionette] "Accept Alert" and "Dismiss Alert" should check for remaining user prompts

If you think having wdspec tests right now, I could also add them. Otherwise we can do that later when we work on bug 1477977.

Comment 4

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/baa98a473b15
[marionette] "Accept Alert" and "Dismiss Alert" should check for remaining user prompts. r=ato

Comment 5

[Raul Gurzau (:RaulG)]

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

Comment 6

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

Please backout this change from any landed branch given that it caused a regression in Selenium as filed as bug 1487705.

We will re-land once the underlying problem has been fixed in Selenium, or Firefox.

Comment 7

[Pulsebot]

Backout by [e-mail]:
https://hg.mozilla.org/mozilla-central/rev/cda13fb48e71
Backed out 1 changesets for causing a regression in Selenium a=backout

Comment 8

[Liz Henry (:lizzard) (relman/hg->git project)]

Henrik, I'm assuming this won't land for 63 at this point and marking 65 as affected. Let me know if that isn't correct though.

Comment 9

[Liz Henry (:lizzard) (relman/hg->git project)]

Getting late to fix in 64, but we could still take a patch for 65.

Comment 10

[Liz Henry (:lizzard) (relman/hg->git project)]

Too late to fix in 64. Marking this issue as fix-optional for 65; if you land a patch in nightly and think it's low-risk for beta, please request uplift.

Comment 11

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

This bug is unblocked now! But I will wait with landing the patch until early next week, just to make sure no regression from landing the patch on bug 1370630 causes backout hell.

Comment 12

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

Attachment: Bug 1487358 - [marionette] "Accept Alert" and "Dismiss Alert" should check for remaining user prompts. r=#webdriver

Workaround until we have a sane dynamic user prompt implementation
(see bug 1477977). At least for now this patch will give us the
opportunity to handle multiple open user prompts.

Comment 13

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

Note that we only want to land this patch for 69, but not 68 because bug 1487705 won't be fixed for that ESR release.

Comment 14

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

https://treeherder.mozilla.org/#/jobs?repo=try&revision=64e03da47384521d8660bbe2b01df2b072bd0748

Comment 15

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

It's actually not a regression. So removing this keyword to stop the release bot to change the bug type.

Comment 16

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/3350eb992270
[marionette] "Accept Alert" and "Dismiss Alert" should check for remaining user prompts. r=webdriver-reviewers,maja_zf

Comment 17

[Pulsebot]

Backout by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/c6a17846e2d2
Backed out changeset 3350eb992270 for causing the Mn and Wd intermittent/perma failures

Comment 18

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

It's interesting that this didn't cause a single failure in the recent try push before landing the patch. I will have to investigate it.

Comment 19

[Cristina Coroiu [:ccoroiu]]

Backed out changeset 3350eb992270 (bug 1487358) for causing the Mn and Wd intermittent/perma failures

Backout: https://hg.mozilla.org/integration/autoland/rev/c6a17846e2d2ff62c36b879dbfa234dad0d02565

Comment 20

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

Here the list of changesets and tests for the time period my patch was on autoland:

https://treeherder.mozilla.org/#/jobs?repo=autoland&bugfiler=&fromchange=3350eb992270&tochange=c6a17846e2d2ff62c36b879dbfa234dad0d02565

Comment 21

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

The Wd failures were actually not a problem with my patch but just a bad coincidence with bug 1553855, which was backed out via:
https://hg.mozilla.org/integration/autoland/rev/795082d54c998ff568d64472e8f0af828a011eae

As such the only failures are from the Mn job, and here for the newly added test test_handle_two_modal_dialogs. After a couple of runs I was able to see this failure on MacOS too. Reason is most likely that after closing the first alert, the second alert isn't immediately shown but it takes some milliseconds.

Maybe accept and dismiss need to spin the event loop.

Comment 22

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

It looks like just spinning the event loop before checking for remaining alerts on the page after a prompt has been accepted/dismissed is not enough. Only when I add sleeps of about 30ms and more, a remaining prompt will be detected. Here is the HTML/JS snippet:

  <script type="text/javascript">
    function setInnerText(id, value) {
      document.getElementById(id).innerHTML = "<p>" + value + "</p>";
    }

    function handleTwoDialogs() {
      setInnerText("text1", prompt("First"));
      setInnerText("text2", prompt("Second"));
    }
  </script>
[..]
  <a href="#" id="open-two-dialogs" onclick="handleTwoDialogs()">Open two prompts.</a>

Olli, right now my code is waiting for the DOMModalDialogClosed event before it immediately checks for any remaining prompt via this method:
https://searchfox.org/mozilla-central/rev/9eb2f739c165b4e294929f7b99fbb4d90f8a396b/testing/marionette/modal.js#62

Maybe you know if there is some other kind of event Marionette would have to wait for before checking for remaining alerts? Or is the first prompt just blocking, and the second opens after the first has been closed? If that is the case maybe we need a sleep call for now until bug 1477977 has been fixed.

Comment 23

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

Actually I found the problem with bug 1477977, so I'm going to fix that first. But then also the test has to wait for the second alert, given that in cases of high CPU load the tabmodal-dialog-loaded observer notification is delayed, which means that the alert isn't getting opened immediately by Firefox. Not sure if that is expected or not. As such I will leave the needinfo request for Olli.

Comment 24

[Olli Pettay [:smaug][[e-mail]]]

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

Or is the first prompt just
blocking, and the second opens after the first has been closed?
This

If that is
the case maybe we need a sleep call for now until bug 1477977 has been fixed.
Hmm, sleep where?
Can't you just handle DOMModalDialogClosed synchronously and once that is handled, the next prompt will be opened?

Perhaps I'm misunderstanding the issue.

Comment 25

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

We don't have control over this behavior. Marionette just needs to be able to handle websites which might make use of that. Current Marionette code is located here:

https://searchfox.org/mozilla-central/rev/153172de0c5bfca31ef861bd8fc0995f44cada6a/testing/marionette/driver.js#3185-3194

But as said in my last comment I will fix bug 1477977 first, so when a tab modal appears Marionette immediately gets notified, and we set the internal flag for an open prompt, and vice versa.

Comment 26

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

Now that bug 1477977 is fixed the patch for this bug works fine locally. I also include the enabling of the currently disabled wdspec tests.

Here is a try push:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=2379544527902f13db9915a0f982fd40d7c215db

Comment 27

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

Attachment: Bug 1487358 - [wdspec] Enable test_abort_by_user_prompt_twice for "Execute Script" and "Execute Async Script". r=#webdriver

Depends on D32666

Comment 28

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/9c28135106d3
[marionette] "Accept Alert" and "Dismiss Alert" should check for remaining user prompts. r=webdriver-reviewers,maja_zf
https://hg.mozilla.org/integration/autoland/rev/64f906609a52
[wdspec] Enable test_abort_by_user_prompt_twice for "Execute Script" and "Execute Async Script". r=webdriver-reviewers,ato

Comment 29

[Bogdan Tara[:bogdan_tara | bogdant]]

https://hg.mozilla.org/mozilla-central/rev/9c28135106d3
https://hg.mozilla.org/mozilla-central/rev/64f906609a52

Comment 30

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

(In reply to Bogdan Tara[:bogdan_tara] from comment #29)

https://hg.mozilla.org/mozilla-central/rev/64f906609a52

Can someone please backout only this commit? It causes a high frequent intermittent for wdspec tests, and I would like to get this fixed in a separate bug. There is no need to reopen this bug after the backout. Thanks.

Comment 31

[Cosmin Sabou [:CosminS]]

It was backed out by: https://hg.mozilla.org/integration/autoland/rev/2af46ed2e59b9aab02bda25eebd5c610ef373e02