Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Retry navigation if unload alert disappears #1965

Conversation

elliterate
Copy link
Contributor

@elliterate elliterate commented Feb 14, 2018

When a Selenium driver interaction raises an unexpected alert error, Firefox 59+ automatically dismisses the user prompt that caused it. Therefore, if the driver reset triggers an unload alert, we cannot wait to handle it until we receive an unexpected alert error. Instead, we must retry navigation and proactively attempt to accept any alert that appears.

@elliterate
Copy link
Contributor Author

This fixes the "Timed out waiting for Selenium session reset" errors in the builds using Firefox Beta and Firefox Nightly.

@twalpole
Copy link
Member

Has this been reported to firefox/geckodriver? Is it intentional behavior?

@elliterate
Copy link
Contributor Author

Yes and yes. I reported it as mozilla/geckodriver#1171 and was informed that it was deliberately implemented in https://bugzilla.mozilla.org/show_bug.cgi?id=1416284.

@elliterate
Copy link
Contributor Author

BTW, I realized after I submitted the pull request that my implementation wouldn't be sufficient, since it adds a delay to every session reset to handle a rare scenario. I'll keep working on it, but I'm also happy to hear any thoughts you have on how best to handle this.

(Also, as the build shows, it appears to be getting stuck on some test.)

@twalpole
Copy link
Member

twalpole commented Feb 14, 2018

Well that's a depressing behavior decision - So you tell the browser to go to another page and FF decides to auto-dismiss any confirm/prompt (rather than accept) and prevent the page change - why would anyone want that as the default? It also obviously doesn't align it with chromedriver (as stated) or things would still work (since it currently works fine with chromedriver)

@twalpole
Copy link
Member

@elliterate Yeah -- it would be better to catch the error and then try navigating agin within accept_modal so the sleep isn't always needed.

@elliterate
Copy link
Contributor Author

So you tell the browser to go to another page and FF decides to auto-dismiss any confirm/prompt (rather than accept) and prevent the page change

It doesn't actually auto-dismiss on navigation—only if you do something that triggers an unexpected alert error. But that's exactly what Capybara does when it proceeds to poll for an empty page body.

And because it auto-dismisses the alert at the same time that it raises an unexpected alert error, attempting to accept the alert in the error handler will no longer work. You'll have to re-attempt the navigation, then immediately go looking for an alert to accept.

Well that's a depressing behavior decision

We're in violent agreement.

But it might get worse. According to the latest draft WebDriver specification, they intend for user prompts triggered by beforeunload handlers to always be dismissed, which, if my understanding is correct, means that one would effectively be trapped on such a page.

@twalpole
Copy link
Member

twalpole commented Feb 14, 2018

@elliterate Ugh --- Definitely sounds like that would be the case, hopefully someone pays attention to your issue report - I often have the feeling the people writing the spec don't actually ever use the things they're writing the specs for, but maybe it's just an oversight.

When a Selenium driver interaction raises an unexpected alert error,
Firefox 59+ [automatically dismisses the user prompt][1] that caused it.
Therefore, if the driver reset triggers an unload alert, we cannot wait
to handle it until we receive an unexpected alert error. Instead, we
must retry navigation and proactively attempt to accept any alert that
appears.

[1]: https://bugzilla.mozilla.org/show_bug.cgi?id=1416284
@elliterate elliterate force-pushed the bugs/fix-firefox-beta-unload-prompt-handling branch from 14c3d7d to 32e1c64 Compare February 15, 2018 23:15
@elliterate
Copy link
Contributor Author

While I've updated my branch with what I believe is a reasonable solution, I discovered mozilla/geckodriver#1181 in the process, which likely blocks me from verifying it with passing tests.

@elliterate elliterate changed the title Accept any unload prompt that appears on reset Retry navigation if unload alert disappears Feb 16, 2018
@twalpole
Copy link
Member

@elliterate The beta build still fails, are you saying that's because of the geckodriver issue? If so tis there any benefit at all to merging this at the current time?

@elliterate
Copy link
Contributor Author

The beta build still fails, are you saying that's because of the geckodriver issue?

That's correct.

If so tis there any benefit at all to merging this at the current time?

I don't believe we'd see an immediate benefit, no. That issue is preventing this fix from being useful, as it's preventing the find_xpath('/html/body/*') from raising an unexpected/unhandled alert error.

@twalpole
Copy link
Member

Closing in favor of a66ff2e

@twalpole twalpole closed this Feb 28, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants