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

RESOLVED FIXED in Firefox 69

Status

enhancement
P1
normal
RESOLVED FIXED
11 months ago
29 days ago

People

(Reporter: whimboo, Assigned: whimboo)

Tracking

(Blocks 1 bug)

Trunk
mozilla69
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox-esr52 unaffected, firefox-esr60 unaffected, firefox61 unaffected, firefox62 unaffected, firefox63 wontfix, firefox64 wontfix, firefox65 wontfix, firefox67 wontfix, firefox67.0.1 wontfix, firefox68 wontfix, firefox69 fixed)

Details

()

Attachments

(2 attachments, 1 obsolete attachment)

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
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 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.
Attachment #9005183 - Flags: review?(ato)
Attachment #9005183 - Flags: review?(ato) → review+
Pushed by hskupin@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/baa98a473b15
[marionette] "Accept Alert" and "Dismiss Alert" should check for remaining user prompts. r=ato
https://hg.mozilla.org/mozilla-central/rev/baa98a473b15
Status: ASSIGNED → RESOLVED
Closed: 11 months ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla63
Keywords: regression
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.
Flags: needinfo?(sheriffs)
Backout by aciure@mozilla.com:
https://hg.mozilla.org/mozilla-central/rev/cda13fb48e71
Backed out 1 changesets for causing a regression in Selenium a=backout
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Target Milestone: mozilla63 → ---
Flags: needinfo?(sheriffs)
No longer blocks: 1487705
Depends on: 1487705
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.
Getting late to fix in 64, but we could still take a patch for 65.
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.

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.

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.

Type: defect → enhancement
Priority: P2 → P1

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.

Type: enhancement → defect
Version: Version 3 → Trunk

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

Keywords: regression
Pushed by hskupin@mozilla.com:
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
Backout by ccoroiu@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/c6a17846e2d2
Backed out changeset 3350eb992270 for causing the Mn and Wd intermittent/perma failures

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.

Status: REOPENED → ASSIGNED

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

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

Flags: needinfo?(hskupin)

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.

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.

Flags: needinfo?(bugs)

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.

No longer blocks: 1477977
Depends on: 1477977

(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.

Flags: needinfo?(bugs)

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.

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

Pushed by atolfsen@mozilla.com:
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
Status: ASSIGNED → RESOLVED
Closed: 11 months agoLast month
Resolution: --- → FIXED
Target Milestone: --- → mozilla69
Regressions: 1559836

(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.

Flags: needinfo?(sheriffs)
Attachment #9072122 - Attachment is obsolete: true
You need to log in before you can comment on or make changes to this bug.