Closed Bug 1434872 Opened 6 years ago Closed 6 years ago

Navigation and close window commands don't handle alerts as being opened from beforeunload events

Categories

(Remote Protocol :: Marionette, enhancement, P1)

Product:
Remote Protocol
Component:
Marionette
Type:
enhancement

Tracking

(firefox61 fixed, firefox62 fixed)

Status:
RESOLVED FIXED
Milestone:
mozilla62
Tracking Flags:
Tracking Status
firefox61 --- fixed
firefox62 --- fixed

People

(Reporter: whimboo, Assigned: whimboo)

References

(Blocks 1 open bug,
URL
)

Details

Attachments

(4 files)

Bug 1434872 - [marionette] Firefox has to automatically dismiss "beforeunload" prompts.
59 bytes, text/x-review-board-request
ato
: review+
Details
Bug 1434872 - [wdclient] Handle SessionNotCreatedException in session.send_command().
59 bytes, text/x-review-board-request
ato
: review+
Details
Bug 1434872 - [marionette] Fix remote_page checks in bf_cache navigtion tests.
59 bytes, text/x-review-board-request
ato
: review+
Details
Bug 1434872 - [wdclient] Store original capabilities as used for the driver.
59 bytes, text/x-review-board-request
ato
: review+
Details 
Originally from: https://github.com/mozilla/geckodriver/issues/1151

When requesting a quit of Firefox, and the page has a `onbeforeunload` event listener registered, the command doesn't handle that and Firefox will not quit.

So quit might have to register for those events and close the alerts, or if possible disable that feature by temporarily turning off a preference. The latter might not be ideal, because we will not be able to reset the value and it will continue to exist in the next session.
I thought this was solved by geckodriver passing eForceQuit to Marionette:Quit?
Sadly not. It can be reproduced with the following Marionette test:

> self.marionette.navigate("http://www.4guysfromrolla.com/demos/OnBeforeUnloadDemo1.htm")
> self.marionette.find_element(By.XPATH, "//input[@name='name']").send_keys("test")
> self.marionette.quit(in_app=True)

There seem to be multiple problems:

1) Interestingly from the marionette-client side we never specify eForceQuit but use eAttemptQuit. I think we should fix that.

2) So if eForceQuit should really stop Firefox from bringing up such alerts, something is wrong. Are you sure that this is the expected behavior? I'm actually not.
OS: Unspecified → All
Priority: -- → P3
Hardware: Unspecified → All
Version: Version 3 → Trunk
Due to this issue, the geckodriver process is not killed. Please let us know when is this planned to get addressed.
This issue causes execution to hang indefinitely for both quit and close. 
https://github.com/mozilla/geckodriver/issues/1151
https://github.com/mozilla/geckodriver/issues/1146

Is there some kind of a workaround?
Flags: needinfo?(dburns)
(In reply to Henrik Skupin (:whimboo) from comment #2)

> 2) So if eForceQuit should really stop Firefox from bringing up
> such alerts, something is wrong. Are you sure that this is the
> expected behavior? I'm actually not.

It’s in the name that eForceQuit forces the process to stop.  This
doesn’t wait for the modal dialogue to be discarded.

But you could also imagine making Marionette:Quit close any
modal dialogues if there are any.  We don’t need to override
beforeunload for that.
(In reply to L&ON from comment #4)

> Is there some kind of a workaround?

Not aware of a workaround.  This bug is on the backlog and not
currently on any plans.
Flags: needinfo?(dburns)
I guess we could theoretically claim that this is a conformance issue.
Priority: P3 → P2
If it is indeed the case that Services.startup.quit(eForceQuit)
will not block and not close these dialogues, this needs to depend
on bug 1264259 (again depending on the window tracking refactoring).
Depends on: 1264259
I wonder if `windowUtils.disableDialogs()` could help us here:

https://dxr.mozilla.org/mozilla-central/rev/6276ec7ebbf33e3484997b189f20fc1511534187/dom/interfaces/base/nsIDOMWindowUtils.idl#1602-1606

With that we seem to be able to disable all kinds of dialogs, which is something we indeed want to have here.
Please note what the spec says:

"User prompts that are spawned from beforeunload event handlers, are dismissed implicitly upon navigation or or close window, regardless of the defined user prompt handler."

It means that we should not only dismiss this dialog implicitly for closing a window (like on quit), but even for navigation. 


https://support.mozilla.org/en-US/questions/1043508

(In reply to Henrik Skupin (:whimboo) from comment #9)
> I wonder if `windowUtils.disableDialogs()` could help us here:

This solution actually doesn't work, because it would have to be run each time after a navigation completed, and before the actual script gets run. In all of our cases it will be too late to decide.

So I wonder if we should use the preference `dom.disable_beforeunload` to just let Firefox unload them automatically without user interaction.

Andreas, I would like to hear your feedback on this.
Flags: needinfo?(ato)
That seems like the right thing to do.  Thanks for digging up the
correct spec prose.
Flags: needinfo?(ato)
Depends on: 1455282
Assignee: nobody → hskupin
Blocks: webdriver
Status: NEW → ASSIGNED
Priority: P2 → P1
Summary: "Marionette:Quit" command doesn't handle alerts as being opened from onbeforeunload → "Marionette:Quit" and "WebDriver:CloseWindow" commands don't handle alerts as being opened from beforeunload events
Comment on attachment 8973037 [details]
Bug 1434872 - [marionette] Firefox has to automatically dismiss "beforeunload" prompts.

https://reviewboard.mozilla.org/r/241564/#review247654

::: testing/marionette/harness/marionette_harness/tests/unit/test_click.py:396
(Diff revision 2)
> +          </script>
> +        """))
>          self.marionette.find_element(By.TAG_NAME, "a").click()
>  
> -        # click returns immediately when a beforeunload handler is invoked
> -        alert = self.marionette.switch_to_alert()
> +        # navigation auto-dismisses beforeunload prompt
> +        self.assertIn("#foo", self.marionette.get_url())

Would it be ok to ask Alex on https://github.com/w3c/web-platform-tests/pull/8379 to get the case added? I'm sure my patch lands first, and I don't want to collide with his work.
Actually the spec refers to all navigation and the close window command. So I will also have to add a few more testcases for `get`, `back`, `forward`, `refresh`, and `element click` + navigation. As such the bug summary should be more generic.
Summary: "Marionette:Quit" and "WebDriver:CloseWindow" commands don't handle alerts as being opened from beforeunload events → Navigation and close window commands don't handle alerts as being opened from beforeunload events
I actually have problems to understand this remaining failure:

https://treeherder.mozilla.org/#/jobs?repo=try&revision=be92e2d0fe87962cf94c4c8b749fff6892ef77b7&selectedJob=177131329

When a wdspec test calls `session.end()`, the next test is not able to load a locally served page by `inline.py` because wptserve doesn't seem to run on `http://web-platform.test:8000` anymore, or the DNS entry cannot be found. Are we doing some magic in the wptrunner which fiddles with the DNS entries during tests?
Flags: needinfo?(james)
Flags: needinfo?(ato)
The simplest test to reproduce this is actually:

> def test(session):
>     session.end()
>     session.start()
>     session.url = inline("<html><head><title>Cheese</title><body>Peas")
Ok, so the problem here is that with end() and start() we actually restart Firefox, and as such also create a fresh profile with geckodriver. Sadly we do not set the preference `network.dns.localDomains` anymore, which is actually causing this problem because `web-platform.tests` is not known as local domain.
Flags: needinfo?(james)
Flags: needinfo?(ato)
It's a bug in wdclient which re-assigns the capabilities as returned by the driver to `self.capabilities` and as such looses all the requested capabilities. Which means a second time `start()` gets called, all the preferences as initially requested are not set anymore. I will fix it in wdclient.
Attachment #8973312 - Flags: review?(ato)
Attachment #8973645 - Flags: review?(ato)
Attachment #8973463 - Flags: review?(ato)
Attachment #8973037 - Flags: review?(ato)
Comment on attachment 8973037 [details]
Bug 1434872 - [marionette] Firefox has to automatically dismiss "beforeunload" prompts.

https://reviewboard.mozilla.org/r/241564/#review247868

::: testing/marionette/harness/marionette_harness/tests/unit/test_navigation.py:424
(Diff revision 5)
>      def test_no_history_items(self):
>          # Both methods should not raise a failure if no navigation is possible
>          self.marionette.go_back()
>          self.marionette.go_forward()
>  
> +    def test_dismissed_beforeunload_prompt(self):

Tests for navigation are getting added with bug 1392274. So I would post-ponse those additions.
Comment on attachment 8973312 [details]
Bug 1434872 - [wdclient] Handle SessionNotCreatedException in session.send_command().

https://reviewboard.mozilla.org/r/241784/#review248208

::: testing/web-platform/tests/tools/webdriver/webdriver/client.py:440
(Diff revision 2)
> +            if isinstance(err, error.SessionNotCreatedException):
> +                # The driver could have already been deleted the session.
> +                self.session_id = None

Normally I would argue that this should be done in the
webdriver.Client#close function by inspecting the return value array
for being empty, but I guess we can’t do that because this client
allows direct calls to the remote outside the API.

Is that basically the issue here?
Attachment #8973312 - Flags: review?(ato) → review+
Comment on attachment 8973645 [details]
Bug 1434872 - [wdclient] Store original capabilities as used for the driver.

https://reviewboard.mozilla.org/r/242010/#review248212

::: commit-message-0ebf2:3
(Diff revision 2)
> +To allow Session.start() to be called multiple times the originally
> +requested capabilities have to be stored. Currently those are
> +overwritten with the actual ones as returned by the driver.

It should in theory be perfectly safe to start a new session with
the exact negotiated capabilities as before.  Is this a problem
currently for some reason?

I suppose I agree with the premise that we should be passing what
the usuer had originally requested when constructing the object,
however.
Attachment #8973645 - Flags: review?(ato) → review+
Comment on attachment 8973463 [details]
Bug 1434872 - [marionette] Fix remote_page checks in bf_cache navigtion tests.

https://reviewboard.mozilla.org/r/241850/#review248214
Attachment #8973463 - Flags: review?(ato) → review+
Comment on attachment 8973037 [details]
Bug 1434872 - [marionette] Firefox has to automatically dismiss "beforeunload" prompts.

https://reviewboard.mozilla.org/r/241564/#review248216

Very good!

::: testing/marionette/harness/marionette_harness/tests/unit/test_click.py:389
(Diff revision 5)
> +            window.addEventListener("beforeunload", function (event) {{
> +              event.preventDefault();
> +            }});

Double {{?

::: testing/marionette/harness/marionette_harness/tests/unit/test_navigation.py:387
(Diff revision 5)
>                                       page["url"], page["is_remote"]))
>  
> +            if "callback" in page and callable(page["callback"]):
> +                page["callback"]()
> +
> +        for index, page in enumerate(test_pages):

index not in use.

::: testing/marionette/harness/marionette_harness/tests/unit/test_navigation.py:426
(Diff revision 5)
>          self.marionette.go_back()
>          self.marionette.go_forward()
>  
> +    def test_dismissed_beforeunload_prompt(self):
> +        url_beforeunload = inline("""
> +          <input type="text"/>

FWIW the type="text"/> part of this has a certain wiff of XHTML
about it.

::: testing/marionette/harness/marionette_harness/tests/unit/test_navigation.py:660
(Diff revision 5)
>          self.marionette.refresh()
>          self.assertEqual(self.test_page_file_url, self.marionette.get_url())
>  
> +    def test_dismissed_beforeunload_prompt(self):
> +        self.marionette.navigate(inline("""
> +          <input type="text"></input>

<input> does not have a closing tag.

::: testing/marionette/harness/marionette_harness/tests/unit/test_quit_restart.py:297
(Diff revision 5)
>          with self.assertRaisesRegexp(ValueError, "is not callable"):
>              self.marionette.restart(in_app=True, callback=4)
>  
> +    def test_in_app_quit_with_dismissed_beforeunload_prompt(self):
> +        self.marionette.navigate(inline("""
> +          <input type="text"></input>

Closing tag.

::: testing/marionette/harness/marionette_harness/tests/unit/test_window_close_content.py:80
(Diff revision 5)
> +    def test_close_window_with_dismissed_beforeunload_prompt(self):
> +        tab = self.open_tab()
> +        self.marionette.switch_to_window(tab)
> +
> +        self.marionette.navigate(inline("""
> +          <input type="text"></input>

Closing tag.

::: testing/web-platform/tests/webdriver/tests/close_window/close.py:36
(Diff revision 5)
>      assert session.handles == handles
>      assert new_handle not in value
>  
>  
> +def test_close_browsing_context_with_dismissed_beforeunload_prompt(session, create_window):
> +    handles = session.handles

Can we call this original_handles for clarity?

::: testing/web-platform/tests/webdriver/tests/close_window/close.py:42
(Diff revision 5)
> +
> +    new_handle = create_window()
> +    session.window_handle = new_handle
> +
> +    session.url = inline("""
> +      <input type="text"></input>

Closing tag.

::: testing/web-platform/tests/webdriver/tests/close_window/close.py:50
(Diff revision 5)
> +          event.preventDefault();
> +        });
> +      </script>
> +    """)
> +
> +    input = session.find.css("input", all=False)

‘input’ is a built-in function in Python, so let’s try to find a
different name for it so that it doesn’t overload the internals.

::: testing/web-platform/tests/webdriver/tests/close_window/close.py:55
(Diff revision 5)
> +    input = session.find.css("input", all=False)
> +    input.send_keys("foo")
> +
> +    response = close(session)
> +    value = assert_success(response, handles)
> +    assert session.handles == handles

Do you mean value == handles here?

::: testing/web-platform/tests/webdriver/tests/delete_session/delete.py:14
(Diff revision 5)
> +    return session.transport.send("DELETE", "session/{session_id}".format(**vars(session)))
> +
> +
> +def test_delete_session_with_dismissed_beforeunload_prompt(session):
> +    session.url = inline("""
> +      <input type="text"></input>

Closing tag.

::: testing/web-platform/tests/webdriver/tests/delete_session/delete.py:22
(Diff revision 5)
> +          event.preventDefault();
> +        });
> +      </script>
> +    """)
> +
> +    input = session.find.css("input", all=False)

Variable name.
Attachment #8973037 - Flags: review?(ato) → review+
Comment on attachment 8973312 [details]
Bug 1434872 - [wdclient] Handle SessionNotCreatedException in session.send_command().

https://reviewboard.mozilla.org/r/241784/#review248326
Comment on attachment 8973645 [details]
Bug 1434872 - [wdclient] Store original capabilities as used for the driver.

https://reviewboard.mozilla.org/r/242010/#review248212

> It should in theory be perfectly safe to start a new session with
> the exact negotiated capabilities as before.  Is this a problem
> currently for some reason?
> 
> I suppose I agree with the premise that we should be passing what
> the usuer had originally requested when constructing the object,
> however.

As requested there are preferences to be set (local DNS domains) which are getting passed to the driver, but the response doesn't contain those preferences. Should it contain all of them, and should we consider that a bug on our end?
Comment on attachment 8973037 [details]
Bug 1434872 - [marionette] Firefox has to automatically dismiss "beforeunload" prompts.

https://reviewboard.mozilla.org/r/241564/#review248216

> Double {{?

This is expected given that format would take it as placeholder instead. By using `{{` it will be converted to `{`.

> index not in use.

Interesting case! `index` is actually used but as variable in that scope inside of `check_page_status`. As such it also didn't fail. I will add an extra parameter to make it lesser confusing.

> FWIW the type="text"/> part of this has a certain wiff of XHTML
> about it.

Not sure I understand this comment.

> Can we call this original_handles for clarity?

We can. And I will update `test_close_browsing_context` at the same time.

> ‘input’ is a built-in function in Python, so let’s try to find a
> different name for it so that it doesn’t overload the internals.

Lets use chaining FWIW.

> Do you mean value == handles here?

No, this is already checked in `assert_success` the line above. Here we make sure that the window has really been closed and that `close()` is not just lying.
Comment on attachment 8973645 [details]
Bug 1434872 - [wdclient] Store original capabilities as used for the driver.

https://reviewboard.mozilla.org/r/242010/#review248212

> As requested there are preferences to be set (local DNS domains) which are getting passed to the driver, but the response doesn't contain those preferences. Should it contain all of them, and should we consider that a bug on our end?

Here the requested capabilities:

> {"capabilities": {"alwaysMatch": {"moz:firefoxOptions": {"binary": "/Volumes/data/code/gecko/obj/debug/dist/NightlyDebug.app/Contents/MacOS/firefox", "prefs": {"network.dns.localDomains": "web-platform.test,www.web-platform.test,xn--n8j6ds53lwwkrqhv28a.web-platform.test,xn--lve-6lad.web-platform.test,www2.web-platform.test,www1.web-platform.test"}}}}}

Having a more closer look at this I forgot about the driver layer. The capabilities as requested here will also contain entries which are getting extracted by any driver. In this case those will be the preferences. They are not getting forwarded to the browser, but are used to setup the profile.

I should update the commit message.
Comment on attachment 8973645 [details]
Bug 1434872 - [wdclient] Store original capabilities as used for the driver.

https://reviewboard.mozilla.org/r/242010/#review248212

> Here the requested capabilities:
> 
> > {"capabilities": {"alwaysMatch": {"moz:firefoxOptions": {"binary": "/Volumes/data/code/gecko/obj/debug/dist/NightlyDebug.app/Contents/MacOS/firefox", "prefs": {"network.dns.localDomains": "web-platform.test,www.web-platform.test,xn--n8j6ds53lwwkrqhv28a.web-platform.test,xn--lve-6lad.web-platform.test,www2.web-platform.test,www1.web-platform.test"}}}}}
> 
> Having a more closer look at this I forgot about the driver layer. The capabilities as requested here will also contain entries which are getting extracted by any driver. In this case those will be the preferences. They are not getting forwarded to the browser, but are used to setup the profile.
> 
> I should update the commit message.

Mh, maybe geckodriver should return the `moz:firefoxOptions` and not just remove them? Anyway this shouldn't block us here and if needed should become its own bug.
Comment on attachment 8973037 [details]
Bug 1434872 - [marionette] Firefox has to automatically dismiss "beforeunload" prompts.

https://reviewboard.mozilla.org/r/241564/#review247654

> Would it be ok to ask Alex on https://github.com/w3c/web-platform-tests/pull/8379 to get the case added? I'm sure my patch lands first, and I don't want to collide with his work.

I added a comment on that PR with a reference to this bug.
Andreas, I decided to wait with the landing until I got your final thoughts. Thanks.
Flags: needinfo?(ato)
Comment on attachment 8973037 [details]
Bug 1434872 - [marionette] Firefox has to automatically dismiss "beforeunload" prompts.

https://reviewboard.mozilla.org/r/241564/#review248216

> Not sure I understand this comment.

<input type="text"/> is functionally equivalent to just <input>.
The ending slash, /, is an XHTML acronism leftover.
Comment on attachment 8973645 [details]
Bug 1434872 - [wdclient] Store original capabilities as used for the driver.

https://reviewboard.mozilla.org/r/242010/#review248212

> Mh, maybe geckodriver should return the `moz:firefoxOptions` and not just remove them? Anyway this shouldn't block us here and if needed should become its own bug.

We decided not to return moz:firefoxOptions in the matched session
capabilities because we can’t control the data it holds.  E.g.
someone might ship a several-megabyte Base64 encoded string as
profile.

Just to reiterate what I said in my original note: I agree with the
premise that we should store the requested capabilities separately
and re-issue these when starting a new session.  I was just making
a note about this, so thank you for answering my questions.
Flags: needinfo?(ato)
Comment on attachment 8973037 [details]
Bug 1434872 - [marionette] Firefox has to automatically dismiss "beforeunload" prompts.

https://reviewboard.mozilla.org/r/241564/#review248216

> <input type="text"/> is functionally equivalent to just <input>.
> The ending slash, /, is an XHTML acronism leftover.

Ah ok, that was were you are referring to. It got fixed. :)
Comment on attachment 8973645 [details]
Bug 1434872 - [wdclient] Store original capabilities as used for the driver.

https://reviewboard.mozilla.org/r/242010/#review248212

> We decided not to return moz:firefoxOptions in the matched session
> capabilities because we can’t control the data it holds.  E.g.
> someone might ship a several-megabyte Base64 encoded string as
> profile.
> 
> Just to reiterate what I said in my original note: I agree with the
> premise that we should store the requested capabilities separately
> and re-issue these when starting a new session.  I was just making
> a note about this, so thank you for answering my questions.

Great. Then all is fine, and I wasn't aware of that decision, which might have been made during one of the TPAC sessions. But it makes sense not to hold those base64 strings.
We're sorry, Autoland could not rebase your commits for you automatically. Please manually rebase your commits and try again.

hg error in cmd: hg rebase -s 2fb0c9c54d38a83a7cb8f0c16bddcec155e2a86d -d 087157c4e7c3: rebasing 462861:2fb0c9c54d38 "Bug 1434872 - [wdclient] Handle SessionNotCreatedException in session.send_command(). r=ato"
rebasing 462862:d14a6b7a0733 "Bug 1434872 - [wdclient] Store original capabilities as used for the driver. r=ato"
rebasing 462863:a33ed1e2785e "Bug 1434872 - [marionette] Fix remote_page checks in bf_cache navigtion tests. r=ato"
rebasing 462864:9840d7ef3a98 "Bug 1434872 - [marionette] Firefox has to automatically dismiss "beforeunload" prompts. r=ato" (tip)
merging testing/web-platform/meta/MANIFEST.json
merging testing/web-platform/tests/webdriver/tests/close_window/close.py
warning: conflicts while merging testing/web-platform/meta/MANIFEST.json! (edit, then use 'hg resolve --mark')
unresolved conflicts (see hg resolve, then hg rebase --continue)
Pushed by hskupin@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/f22e8468bc85
[wdclient] Handle SessionNotCreatedException in session.send_command(). r=ato
https://hg.mozilla.org/integration/autoland/rev/14c67a3e28d5
[wdclient] Store original capabilities as used for the driver. r=ato
https://hg.mozilla.org/integration/autoland/rev/c69d500931d2
[marionette] Fix remote_page checks in bf_cache navigtion tests. r=ato
https://hg.mozilla.org/integration/autoland/rev/2ec04569f9bd
[marionette] Firefox has to automatically dismiss "beforeunload" prompts. r=ato
Created web-platform-tests PR https://github.com/w3c/web-platform-tests/pull/10936 for changes under testing/web-platform/tests
Will this fix be put into Firefox 60 ESR?
No, most likely not. It's not critical nor security related.
Maybe I'm reading the webdriver spec wrong. But with Firefox 61 and geckdriver 0.21 it will always discard the beforeunload user prompt. 
I thought this only applied to closing the browser/session and navigation actions according to section 9 of the specification. But now clicking a link on the page will also discard the beforeunload user prompt.
Does clicking the link cause a navigation to happen? I'm fairly sure it is, given otherwise the onbeforeunload prompt wouldn't appear anyway.
Flags: needinfo?(bugzilla)
Yes, it will cause a navigation to happen. But it is not a "webdriver navigation" command, i.e.
POST /session/{session id}/url
POST /session/{session id}/back
POST /session/{session id}/forward
POST /session/{session id}/refresh

Reading the webdriver spec again it actually refers to "navigation" as defined in the whatwg HTML standard, which does include any kind of navigation action (so including following links). Which means that validating beforeunload behavior is not possible with webdriver.
Flags: needinfo?(bugzilla)
Michiel, feel free to file an issue here: https://github.com/w3c/webdriver/issues/
And as workaround for now you could set "dom.disable_beforeunload" to false. But this is not a supported feature at the moment, so misbehavior could happen.
Blocks: 1450876
No longer blocks: 1450876
Product: Testing → Remote Protocol
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: