Closed Bug 1311350 Opened 8 years ago Closed 8 years ago

close() and closeChromeWindow() do not wait until the underlying window has disappeared

Categories

(Remote Protocol :: Marionette, defect)

Product:
Remote Protocol
Component:
Marionette
Version:
Version 3
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

(firefox50 wontfix, firefox51 wontfix, firefox52 fixed, firefox53 fixed)

Status:
RESOLVED FIXED
Milestone:
mozilla53
Tracking Flags:
Tracking Status
firefox50 --- wontfix
firefox51 --- wontfix
firefox52 --- fixed
firefox53 --- fixed

People

(Reporter: whimboo, Assigned: whimboo)

References

(Blocks 2 open bugs,
URL
)

Details

(Whiteboard: [spec][17/01])

Attachments

(2 files)

Bug 1311350 - Make close window commands synchronous and return remaining window handles.
59 bytes, text/x-review-board-request
ato
: review+
Details
Bug 1311350 - Add unit tests for close window commands.
59 bytes, text/x-review-board-request
ato
: review+
Details 
Both methods only call close() on the appropriate window object but do not wait for the object actually being closed and disappeared. It means that tests via Marionette client or Selenium (via geckodriver) can experience race conditions when working with new tabs or chrome windows.

I will work on this once I'm done with bug 1259055.
Blocks: webdriver
OS: Unspecified → All
Hardware: Unspecified → All
I'm currently experiencing a bug in Marionette when closing a tab where the `visibilitychange` event on the next tab fires before the `visibilitychange` event on the current tab (note this bug doesn't happen when using Firefox normally).

Could that be related to this bug or is it worth opening a new one?
We might need a testcase to see that, but from what I read it could definitely be. I would suggest you file a new bug and mark it as dependent on this one. We can check later, once this bug is fixed, if it is still an issue. Otherwise we might forget about this problem. Thanks for raising this.
Blocks: 1318098
Blocks: 1322383
Something else we are missing here is step 4) from the spec:

4. Return the result of running the remote end steps for the Get Window Handles command. 

Given that this bug blocks my work on bug 1322383 I will work on it.
Assignee: nobody → hskupin
URL: https://w3c.github.io/webdriver/webdr...
Status: NEW → ASSIGNED
Currently our close methods delete the session if the last window gets closed. But this is not specified in the spec. Andreas, is it something we have to remove on our side? Or is it missing in the spec?
Flags: needinfo?(ato)
So close() is using removeTab() from gBrowser which is specified as:
https://developer.mozilla.org/en-US/docs/Mozilla/Tech/XUL/Method/removeTab

If only one tab is displayed, this method does nothing (unless the preference browser.tabs.closeWindowWithLastTab is true, in which case the window containing the tab is closed). 

I wonder if the last tab should be allowed to be closed or not. At least with this pref we have the possibility to enforce that the last tab cannot be closed.
(In reply to Henrik Skupin (:whimboo) from comment #4)
> Currently our close methods delete the session if the last window gets
> closed. But this is not specified in the spec. Andreas, is it something we
> have to remove on our side? Or is it missing in the spec?

This is missing from the spec. It should be added if its not there.
Flags: needinfo?(ato)
I had a WIP patch for this and some of the things I will discuss in this comment, but I’m very happy for whimboo to take it over.  Thank you!

Firstly, we should obviously make close() and closeChromeWindow() synchronous so that we only return to the user after the operation has happened.

One bit of complexity is that Firefox does not let us close the last tab.  There is apparently a preference switch to let you do that, but I’m not sure if that causes Firefox to quit on non-macOS systems.  On macOS, the process will live on due to the unique characteristics of its windowing system.

My plan was to make GeckoDriver#close() lie about the number of window handles when there is exactly one window and one tab left, and send an empty array.  Because WebDriver says we should end the session (meaning, close the session and shut down the browser), it would let geckodriver know that it is time to stop the Firefox process.  Because Firefox was spawned by geckodriver, it seems natural for it to control its lifetime.

The reason we can’t use Marionette to shut down Firefox directly is due to the inadequate abstractions of the socket interface.  When Marionette triggers the exit sequence, we cannot guarantee that the response to the client will be sent.  This may cause the client to end up with a garbled/incomplete packet, a socket timeout, or a process termination signal.  The XPCOM abstractions do not let us set the SO_LINGER option on the socket, which would delay the exit(2) syscall until all socket buffers had been flushed/are empty.

I also think _not shutting down_ Firefox from within Marionette makes some sense.  You would not expect a remote control protocol that isn’t the owner of its own process to shut itself down; it would in most common cases be the responsibility of the parent process.  This means we shift one more bit of the WebDriver implementation over to geckodriver.  One downside to this is that it will not be possible to shut down Firefox when geckodriver connects to an existing Firefox instance, but this is a pretty rare case.

So in summary, this patch should (a) make the commands synchronous, (b) remove the close session behaviour currently in GeckoDriver#close(), (c) send back an empty array when there is exactly one tab left.
(In reply to Andreas Tolfsen ‹:ato› from comment #7)
> One bit of complexity is that Firefox does not let us close the last tab. 
> There is apparently a preference switch to let you do that, but I’m not sure
> if that causes Firefox to quit on non-macOS systems.  On macOS, the process
> will live on due to the unique characteristics of its windowing system.

Yes, on non-OSX systems Firefox will close. So to get a consistent behavior (if that is really wanted) we should not let the user close the last tab, and as such should not modify this preference.

> My plan was to make GeckoDriver#close() lie about the number of window
> handles when there is exactly one window and one tab left, and send an empty
> array.  Because WebDriver says we should end the session (meaning, close the
> session and shut down the browser), it would let geckodriver know that it is
> time to stop the Firefox process.  Because Firefox was spawned by
> geckodriver, it seems natural for it to control its lifetime.

Andreas, where does webdriver say that? Can you please give a link to the spec? I cannot find it, and as such I think the spec would need an update.

> The reason we can’t use Marionette to shut down Firefox directly is due to
> the inadequate abstractions of the socket interface.  When Marionette
> triggers the exit sequence, we cannot guarantee that the response to the
> client will be sent.  This may cause the client to end up with a
> garbled/incomplete packet, a socket timeout, or a process termination
> signal.  The XPCOM abstractions do not let us set the SO_LINGER option on
> the socket, which would delay the exit(2) syscall until all socket buffers
> had been flushed/are empty.

I haven't the knowledge about all that so I trust you on this judgement.

> I also think _not shutting down_ Firefox from within Marionette makes some
> sense.  You would not expect a remote control protocol that isn’t the owner
> of its own process to shut itself down; it would in most common cases be the
> responsibility of the parent process.  This means we shift one more bit of
> the WebDriver implementation over to geckodriver.  One downside to this is
> that it will not be possible to shut down Firefox when geckodriver connects
> to an existing Firefox instance, but this is a pretty rare case.

Why should we shutdown Firefox in such a case? If Geckodriver isn't the actual parent process which controls Firefox, there should be another process which is actually doing this job. So I don't see a problem with this.

> So in summary, this patch should (a) make the commands synchronous, (b)
> remove the close session behaviour currently in GeckoDriver#close(), (c)
> send back an empty array when there is exactly one tab left.

Ok, that's what I have for close() now. Next step is to update close_chrome_window() for all that.
Flags: needinfo?(ato)
(In reply to Henrik Skupin (:whimboo) from comment #8)
> (In reply to Andreas Tolfsen ‹:ato› from comment #7)
> > My plan was to make GeckoDriver#close() lie about the number of window
> > handles when there is exactly one window and one tab left, and send an empty
> > array.  Because WebDriver says we should end the session (meaning, close the
> > session and shut down the browser), it would let geckodriver know that it is
> > time to stop the Firefox process.  Because Firefox was spawned by
> > geckodriver, it seems natural for it to control its lifetime.
> 
> Andreas, where does webdriver say that? Can you please give a link to the
> spec? I cannot find it, and as such I think the spec would need an update.

Under the ‘Close Window’ command, WebDriver says as step3, “[i]f there are no more open top-level browsing contexts, then close the session”.  Closing the session means to shut down the browser, since a “WebDriver session” is analogous to the browser lifetime.

> > I also think _not shutting down_ Firefox from within Marionette makes some
> > sense.  You would not expect a remote control protocol that isn’t the owner
> > of its own process to shut itself down; it would in most common cases be the
> > responsibility of the parent process.  This means we shift one more bit of
> > the WebDriver implementation over to geckodriver.  One downside to this is
> > that it will not be possible to shut down Firefox when geckodriver connects
> > to an existing Firefox instance, but this is a pretty rare case.
> 
> Why should we shutdown Firefox in such a case? If Geckodriver isn't the
> actual parent process which controls Firefox, there should be another
> process which is actually doing this job. So I don't see a problem with this.

Indeed, but there may be cases where you have an existing Firefox process running that isn’t controlled by any user-level process that you wish to connect geckodriver to.  But as I said, I don’t think it’s a problem that you won’t be allowed to shut this Firefox down.
Flags: needinfo?(ato)
(In reply to Andreas Tolfsen ‹:ato› from comment #9)
> > Andreas, where does webdriver say that? Can you please give a link to the
> > spec? I cannot find it, and as such I think the spec would need an update.
> 
> Under the ‘Close Window’ command, WebDriver says as step3, “[i]f there are
> no more open top-level browsing contexts, then close the session”.  Closing
> the session means to shut down the browser, since a “WebDriver session” is
> analogous to the browser lifetime.

Interesting. I totally missed that. Maybe also due to the fact that for Marionette a deleteSession is not doing the same thing. Looks like there is a bit of inconsistency here.

I assume we have to update geckodriver ASAP to this change? Or does it already handle that? If not, how about backward compatibility?
With my updated code I also have the problem with unicode strings now but on Linux and that permanently:

> test_window_management.py TestSwitchWindow.test_windows | AssertionError: u'11' != 11

Maybe this gives us the chance to trace this down to the underlying problem.
I figured out this problem which was related to one of my changes. I removed some B2G specific lines which internally converted the window id from an int to a string. So with those removed we have to call toString() now.
I totally messed up my patch series with a histedit late yesterday. It means that everything has been condensed to a 2-patch only series. I hope that this isn't a problem for you when reviewing the changes.
Attachment #8825177 - Flags: review?(ato)
Attachment #8825178 - Flags: review?(ato)
Comment on attachment 8825177 [details]
Bug 1311350 - Make close window commands synchronous and return remaining window handles.

https://reviewboard.mozilla.org/r/103372/#review104172

::: testing/marionette/browser.js:9
(Diff revision 4)
> +Cu.import("resource://gre/modules/Log.jsm");
> +const logger = Log.repository.getLogger("Marionette");
> +

I can’t see this is used?

::: testing/marionette/browser.js:162
(Diff revision 4)
> +   * @return {Promise}
> +   *     A promise which is resolved when the current window has been closed.
> +   */
> +  closeWindow() {
> +    return new Promise(resolve => {
> +      this.window.addEventListener('unload', ev => {

Double quotes.

::: testing/marionette/browser.js:194
(Diff revision 4)
> +        this.tab.addEventListener("TabClose", ev => {
> +          resolve();
> +        }, {once: true});
> -      this.browser.removeTab(this.tab);
> +        this.browser.removeTab(this.tab);
> +      } else {
> +        reject(new Error(`closeTab() not supported in ${this.driver.appName}`));

Use `UnsupportedOperationError`.

::: testing/marionette/client/marionette_driver/marionette.py:1480
(Diff revision 4)
>      def close(self):
>          """Close the current window, ending the session if it's the last
>          window currently open.
>  
>          """
> -        self._send_message("close")
> +        return self._send_message("close")
>  
>      def close_chrome_window(self):
>          """Close the currently selected chrome window, ending the session
>          if it's the last window open.
>  
>          """
> -        self._send_message("closeChromeWindow")
> +        return self._send_message("closeChromeWindow")

Document that these return the open window handles.

::: testing/marionette/driver.js:165
(Diff revision 4)
> +Object.defineProperty(GeckoDriver.prototype, "chromeWindowHandles", {
> +  get : function () {
> +    let hs = [];
> +    let winEn = Services.wm.getEnumerator(null);
> +
> +    while (winEn.hasMoreElements()) {
> +      let foundWin = winEn.getNext();
> +      let winId = foundWin.QueryInterface(Ci.nsIInterfaceRequestor)
> +          .getInterface(Ci.nsIDOMWindowUtils)
> +          .outerWindowID;
> +      hs.push(winId.toString());
> +    }
> +
> +    return hs;
> +  },
> +});

Perhaps move this down after the `windowHandles` getter?

::: testing/marionette/driver.js:2099
(Diff revision 4)
>    this.mm.addMessageListener("Marionette:deleteCookie", cb);
>    yield this.listener.deleteCookie(cmd.parameters.name);
>  };
>  
>  /**
> - * Close the current window, ending the session if it's the last
> + * Close the currently selected window (tab).

It is window _or_ tab.  Change this to “window/tab”.

::: testing/marionette/driver.js:2124
(Diff revision 4)
>    if (nwins == 1) {
> -    this.sessionTearDown();
> +    return [];
> -    return;
>    }

Add similar comment as for `closeChromeWindow` here.

::: testing/marionette/driver.js:2157
(Diff revision 4)
> -  // if there is only 1 window left, delete the session
> + // If there is only 1 window left, do not close it. Instead return a faked
> +  // empty array of window handles. This will instruct geckodriver to terminate
> +  // the session.
>    if (nwins == 1) {
> -    this.sessionTearDown();
> +    return [];
> -    return;
>    }

s/terminate the session/terminate Firefox/
Attachment #8825177 - Flags: review?(ato) → review+
Comment on attachment 8825178 [details]
Bug 1311350 - Add unit tests for close window commands.

https://reviewboard.mozilla.org/r/103374/#review104174

::: testing/marionette/harness/marionette_harness/tests/unit/test_window_close_chrome.py:26
(Diff revision 4)
> +        win = self.open_window(trigger=open_window_with_js)
> +        self.marionette.switch_to_window(win)
> +
> +        self.assertEqual(2, len(self.marionette.chrome_window_handles))

Do an assertion also at the beginning of the test that we are in the correct state.  Possibly this can be done in `startUp`?

::: testing/marionette/harness/marionette_harness/tests/unit/test_window_close_content.py:15
(Diff revision 4)
> +    @skip_if_mobile("Interacting with chrome windows not available for Fennec")
> +    def test_close_chrome_window_handle(self):
> +
> +        def open_window_with_js():
> +            with self.marionette.using_context("chrome"):
> +                self.marionette.execute_script("""
> +                  window.open('chrome://marionette/content/test.xul',
> +                              'foo', 'chrome,centerscreen');
> +                """)
> +
> +        win = self.open_window(trigger=open_window_with_js)
> +        self.marionette.switch_to_window(win)
> +
> +        self.assertEqual(2, len(self.marionette.chrome_window_handles))
> +        chrome_window_handles = self.marionette.close_chrome_window()
> +        self.assertNotIn(win, chrome_window_handles)
> +        self.assertListEqual(self.marionette.chrome_window_handles, chrome_window_handles)

I don’t think we need this test for content?

::: testing/marionette/harness/marionette_harness/tests/unit/test_window_close_content.py:33
(Diff revision 4)
> +    @skip_if_mobile("Interacting with chrome windows not available for Fennec")
> +    def test_close_chrome_window_last_handle(self):
> +        self.assertEqual(1, len(self.marionette.chrome_window_handles))
> +        self.assertListEqual([], self.marionette.close_chrome_window())
> +        self.assertIsNotNone(self.marionette.session)

Chrome window handles?

::: testing/marionette/harness/marionette_harness/tests/unit/test_window_close_content.py:67
(Diff revision 4)
> +        self.assertNotIn(win, window_handles)
> +        self.assertListEqual(self.marionette.window_handles, window_handles)
> +        self.assertEqual(1, len(self.marionette.chrome_window_handles))
> +        self.assertEqual(1, len(self.marionette.window_handles))
> +
> +    def test_close_window_last_handle(self):

Can we also add a test that closes a child window by JS?
Attachment #8825178 - Flags: review?(ato) → review+
Comment on attachment 8825177 [details]
Bug 1311350 - Make close window commands synchronous and return remaining window handles.

https://reviewboard.mozilla.org/r/103372/#review104172

> Document that these return the open window handles.

I have known that I missed something here! ;) Thanks for catching that.

> Perhaps move this down after the `windowHandles` getter?

I put it here to have an alphabetic order of all properties. Do you think it's better to have them ordered by functionality?

> It is window _or_ tab.  Change this to “window/tab”.

I will change it to "tab/window" because tab is the target we usually close here.

> s/terminate the session/terminate Firefox/

Better is "terminate the application" because we support different ones.
Comment on attachment 8825177 [details]
Bug 1311350 - Make close window commands synchronous and return remaining window handles.

https://reviewboard.mozilla.org/r/103372/#review104172

> I put it here to have an alphabetic order of all properties. Do you think it's better to have them ordered by functionality?

I think for this, functionality is more important so that context is established.

> Better is "terminate the application" because we support different ones.

Yes, that sonuds alright.
Comment on attachment 8825178 [details]
Bug 1311350 - Add unit tests for close window commands.

https://reviewboard.mozilla.org/r/103374/#review104174

> Do an assertion also at the beginning of the test that we are in the correct state.  Possibly this can be done in `startUp`?

I assume you meant setUp()? I was instructed by David not add any assertion in this method.

When checking the tests again I think it might be better to rely on the initial number of tabs and windows instead - so no hard-coded lenghts.

> I don’t think we need this test for content?

We need. Reason is that all window and chrome window methods have to work side by side whether in which scope you are currently in.

> Chrome window handles?

Not sure what you mean with this comment. Can you please clarify?

> Can we also add a test that closes a child window by JS?

This is somewhat outside the scope of those changes. What value should it give us regarding close() and close_chrome_window()?
Comment on attachment 8825177 [details]
Bug 1311350 - Make close window commands synchronous and return remaining window handles.

https://reviewboard.mozilla.org/r/103372/#review104172

> I think for this, functionality is more important so that context is established.

Ok, done.
(In reply to Henrik Skupin (:whimboo) from comment #10)
> I assume we have to update geckodriver ASAP to this change? Or does it
> already handle that? If not, how about backward compatibility?

Andreas, mind giving me some help in what has to be done here? I can see that I have to update at least geckodriver for the return values, and add the chrome window closing method. In webdriver-rust I cannot find anything. thanks.
Flags: needinfo?(ato)
Comment on attachment 8825178 [details]
Bug 1311350 - Add unit tests for close window commands.

https://reviewboard.mozilla.org/r/103374/#review104174

> I assume you meant setUp()? I was instructed by David not add any assertion in this method.
> 
> When checking the tests again I think it might be better to rely on the initial number of tabs and windows instead - so no hard-coded lenghts.

Assertions in `setUp` are perfectly fine: they are checking that the test setup is in the expected state before beginning the tests.

> We need. Reason is that all window and chrome window methods have to work side by side whether in which scope you are currently in.

OK.

> Not sure what you mean with this comment. Can you please clarify?

This test is for content, why is this test function testing chrome windows?

> This is somewhat outside the scope of those changes. What value should it give us regarding close() and close_chrome_window()?

If the window has been spawned from JS inside an existing window, e.g. by clicking a button, it should report one more window handle.  When this window is closed, either through the Marionette close command, from the new window closing itself, or from the parent closing it, it should report one less handle.
(In reply to Henrik Skupin (:whimboo) from comment #32)
> (In reply to Henrik Skupin (:whimboo) from comment #10)
> > I assume we have to update geckodriver ASAP to this change? Or does it
> > already handle that? If not, how about backward compatibility?
> 
> Andreas, mind giving me some help in what has to be done here? I can see
> that I have to update at least geckodriver for the return values, and add
> the chrome window closing method. In webdriver-rust I cannot find anything.
> thanks.

It doesn’t yet.  I can work on the termination of Firefox as I already have a patch in progress for that.  Fixing the return values seems like an related change that can be done in parallel.
Flags: needinfo?(ato)
Comment on attachment 8825178 [details]
Bug 1311350 - Add unit tests for close window commands.

https://reviewboard.mozilla.org/r/103374/#review104174

> This test is for content, why is this test function testing chrome windows?

As said above both chrome and content windows have to be handled correctly whether in which scope you are in. One test module handles everything for content scope (this one), and the other for chrome scope. This separation was necessary to be able to correctly skip for Fennec.
Comment on attachment 8825178 [details]
Bug 1311350 - Add unit tests for close window commands.

https://reviewboard.mozilla.org/r/103374/#review104174

> Assertions in `setUp` are perfectly fine: they are checking that the test setup is in the expected state before beginning the tests.

So we have two different opinions then. Whether whom I ask I will get a different reply. :D

> If the window has been spawned from JS inside an existing window, e.g. by clicking a button, it should report one more window handle.  When this window is closed, either through the Marionette close command, from the new window closing itself, or from the parent closing it, it should report one less handle.

I think that this is a better fit for test_window_management.py which has basic window handling tests. Specifically test_should_load_and_close_a_window(). I'm happy to file a new bug for getting those checks done.
Comment on attachment 8825178 [details]
Bug 1311350 - Add unit tests for close window commands.

https://reviewboard.mozilla.org/r/103374/#review104174

> I think that this is a better fit for test_window_management.py which has basic window handling tests. Specifically test_should_load_and_close_a_window(). I'm happy to file a new bug for getting those checks done.

I will have a look at the test update while checking bug 1223277.
Another try push has shown that due to some reason the test test_execute_script.py starts failing for chrome tests in Fennec now:

https://treeherder.mozilla.org/#/jobs?repo=try&revision=5a837ea4eeb6&filter-tier=1&filter-tier=2&filter-tier=3&selectedJob=67955621

I don't know why but I would like to not land this patch with this deficit.
I’m personally fine with landing the changes as they are, since they are rather important, and fix the Fennec failures later.  But of course, this all depends on whether it is easy to fix.
Yes, I noticed that there is a bigger issue with chunking. So I filed bug 1330321. Lets get the failures fixed in a follow-up bug similar to latest modal dialog failures.
Pushed by hskupin@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/5691fa597a36
Make close window commands synchronous and return remaining window handles. r=ato
https://hg.mozilla.org/integration/autoland/rev/f39600950aea
Add unit tests for close window commands. r=ato
https://hg.mozilla.org/mozilla-central/rev/5691fa597a36
https://hg.mozilla.org/mozilla-central/rev/f39600950aea
Status: ASSIGNED → RESOLVED
Closed: 8 years ago
status-firefox53: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla53
Please uplift this test-only patch to aurora. Thanks.
status-firefox50: affected → wontfix
status-firefox51: affected → wontfix
Whiteboard: [checkin-needed-aurora]
Whiteboard: [spec][17/01]
Hi

I am using firefox 52.0b6 and gecko driver 0.14.
Issue still exist there.Its not closing the opened browser window.
We have unit tests which cover various scenarios of closing tabs and windows. So I doubt it's a Marionette issue. If you have issues please file a new bug, also this is a totally unrelated bug to your issue. Thanks.
Today I tested with gecko driver 0.15 and selenium client library 3.3.0
Firefox 52.0

Still its not closing the browser using driver.close();
Flags: needinfo?(hskupin)
This is totally unrelated to this bug. You are most likely seeing https://github.com/mozilla/geckodriver/issues/285. So please subscribe there for updates. The bug in Marionette is fixed.
Flags: needinfo?(hskupin)
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: