MarionetteTestharnessProtocolPart._close_windows() doesn't close any user prompts

RESOLVED FIXED in Firefox 66

Status

enhancement
P2
normal
RESOLVED FIXED
8 months ago
8 months ago

People

(Reporter: whimboo, Assigned: whimboo)

Tracking

unspecified
mozilla66
Points:
---

Firefox Tracking Flags

(firefox66 fixed)

Details

Attachments

(1 attachment)

While trying to fix a wpt failure as introduced by my patches on bug 1504756 I noticed that the method `MarionetteTestharnessProtocolPart.dismiss_alert()` is a no-op when called from `_close_windows()` because `switch_to_window()` doesn't raise a `UnexpectedAlertOpen` error.

Why does `dismiss_alert()` need a callback at all? Why can't we just use `self.marionette.switch_to_alert()` to check for an existing user prompt, and use `NoAlertPresentException` to return from the function?

https://searchfox.org/mozilla-central/rev/fef7f858efb695a76010b4c624da5277c16e95b3/testing/web-platform/tests/tools/wptrunner/wptrunner/executors/executormarionette.py#160

https://searchfox.org/mozilla-central/rev/fef7f858efb695a76010b4c624da5277c16e95b3/testing/web-platform/tests/tools/wptrunner/wptrunner/executors/executormarionette.py#177-188
Flags: needinfo?(james)
The callback is an optimistaion; it means that in the most commn case where there are 0 alerts open we just perform the action directly rather than going through an extra command and an exception, bot of which are slow. It may be that it doesn't matter in this case, but generally putting extra netwerk traffic and exceptions on the common path seems like a bad idea.
Flags: needinfo?(james)
Well, in this case we also issue two commands which is the `Switch to Window` one. See line 161 of the first link above. 

But as said above this won't trigger the error which is used to close the left-open alerts, and as such they will remain and cause a real failure later when closing the window.
Right, sure there's a bug here that we run `switch_to_window` twice. I'm a little surprised that we don't throw an error, but I guess it makes sense. I think in this case it could make sense to rearrange the commands to:

                self.marionette.switch_to_window(handle)
                self.logger.info("Closing window %s" % handle)
                self.dismiss_alert(lambda: self.marionette.close())
Not really, given that in case of a user prompt being open, it will close it but not the window.
The point of `dismiss_alert` is that it runs the original command until there isn't an unexpected alert open exception and closed the alert when there is. So it would close the window afaict.
Oh, wait. You are right. I missed that the break statement is in the else case of the try block. Let me just patch that.
Assignee: nobody → hskupin
Status: NEW → ASSIGNED
Priority: -- → P2
Using the "Switch To Window" command to check if a user prompt is open
doesn't work because that command doesn't raise a "unexpected alert open"
error. To fix that the "Close Window" command can be used for.
Pushed by hskupin@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/fcb026c5194b
[wpt] Correctly dismiss user prompts in MarionetteTestharnessProtocolPart._close_windows(). r=jgraham
Created web-platform-tests PR https://github.com/web-platform-tests/wpt/pull/14737 for changes under testing/web-platform/tests
Upstream web-platform-tests status checks passed, PR will merge once commit reaches central.
https://hg.mozilla.org/mozilla-central/rev/fcb026c5194b
Status: ASSIGNED → RESOLVED
Closed: 8 months ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla66
You need to log in before you can comment on or make changes to this bug.