Closed Bug 1381459 Opened 7 years ago Closed 7 years ago

Intermittent /html/browsers/windows/auxiliary-browsing-contexts/opener.html | Current window does not have a content browser

Categories

(Testing :: Marionette Client and Harness, defect, P5)

Product:
Testing
Component:
Marionette Client and Harness
Type:
defect

Tracking

(firefox58 fixed, firefox59 fixed)

Status:
RESOLVED FIXED
Milestone:
mozilla59
Tracking Flags:
Tracking Status
firefox58 --- fixed
firefox59 --- fixed

People

(Reporter: intermittent-bug-filer, Assigned: nnebel, Mentored)

References

Details

(Keywords: intermittent-failure, Whiteboard: [lang=py][lang=js])

Attachments

(1 file, 2 obsolete files)

Bug 1381459 - Allow closing of browserless tabs.
59 bytes, text/x-review-board-request
whimboo
: review+
Details
Priority: -- → P5
The problem here is not in Core:DOM but in Marionette. So we are trying to close a tab, which doesn't have a content browser yet. This in fact can happen due to various reasons like:

1) The tab has not been restored yet from a former session after a restart. This happens to background tabs, and those would need an explicit focus before the content browser gets loaded.

2) There is eg. a remoteness change and the content browser is not ready yet, when this check happens.

Personally I don't think that we should care if the content browser is available or not. In any case the tab should be closable an no exception should be thrown.

To get this fixed we simply have to change the following assertion to:

> assert.window(this.getCurrentWindow(Context.CONTENT));

at:

https://dxr.mozilla.org/mozilla-central/rev/11fe0a2895aab26c57bcfe61b3041d7837e954cd/testing/marionette/driver.js#2729

A testcase here could look like:

* Open two tabs with test pages
* Restart the browser and ensure that all tabs have been restored
* Switch to the background tab 
* Try to close the background tab

The last step should not raise an error.

Vedant, might you be interested in this bug? I'm happy to mentor.
Mentor: hskupin
Component: DOM → Marionette
Flags: needinfo?(vedantc98)
Product: Core → Testing
Whiteboard: [lang=py][lang=js]
Thanks Henrik, I would like to take this one up. 
I'll work on this later today and let you know if there are any problems.
Flags: needinfo?(vedantc98)
Amazing to hear that! Based on the last patches I'm sure it's a bug you will enjoy working on.
Vedant, do you still want to work on this bug? Please let me know if you have any issues/blockers. Thanks.
Flags: needinfo?(vedantc98)
Hello, I would be interested in tackling this bug.
It's good to hear. Let me know if you have questions, and once a patch got uploaded I will assign the bug to you. Thanks!
Flags: needinfo?(vedantc98)
(In reply to Henrik Skupin (:whimboo) from comment #12)
> A testcase here could look like:
> * Open two tabs with test pages
> * Restart the browser and ensure that all tabs have been restored
> * Switch to the background tab 
> * Try to close the background tab

To prevent us from having to create a restart test, I wonder if there is a method available for the tabbrowser API which let us unload the contentBrower, and keep the tab open. The tab should be in a state like a background tab would be after a restart with session restore enabled.

Dao, is there such a method available which we could use here? Thanks.
Flags: needinfo?(dao+bmo)
gBrowser.discardBrowser(browser);
Flags: needinfo?(dao+bmo)
Thanks. So it looks like a Firefox only feature, and as such the test to be created cannot run for Fennec. But that's not a problem given that all restart tests cannot be run in Fennec neither.

Nathaniel, here you can find some updated steps a test has to perform:

* Open a new tab and optionally load a test page
* Discard the browser by using execute_script() and `gBrowser.discardBrowser()` (check test_navigation.py in how to get the selected browser)
* Close the current tab
Hello Henrik, I just attached a patch for the proposed resolution to this issue. Let me know if you see any problems, have any suggestion, or any questions.
Hello Henrik, I have pushed the changes to ReviewBoard https://reviewboard.mozilla.org/r/196616/ . Sorry for the delay made a mistake and had to fight hg.
Attachment #8925465 - Attachment is obsolete: true
Nathaniel, I checked the review request but cannot operate on given that you seem to have pushed an update, but forgot to publish it. Can you please check? When you do that please also add my nickname to the list of reviewers in the mozreview ui.
Assignee: nobody → me
Status: NEW → ASSIGNED
Flags: needinfo?(me)
Attachment #8925482 - Flags: review?(hskupin)
Flags: needinfo?(me)
Attachment #8926288 - Attachment is obsolete: true
Comment on attachment 8925482 [details]
Bug 1381459 - Allow closing of browserless tabs.

https://reviewboard.mozilla.org/r/196616/#review202054

::: testing/marionette/harness/marionette_harness/tests/unit/test_window_status_content.py:119
(Diff revision 1)
>          with self.assertRaises(NoSuchWindowException):
>              self.marionette.switch_to_window(tab)
> +
> +    def test_closed_browserless_tab(self):
> +        with self.marionette.using_context("content"):
> +            tab = self.open_tab(self.open_tab_in_foreground)

Typo, corrected to self.open_tab() . Didn't catch that before commit.

::: testing/marionette/harness/marionette_harness/tests/unit/test_window_status_content.py:130
(Diff revision 1)
> +              Components.utils.import("resource:///modules/RecentWindow.jsm");
> +          
> +              let win = RecentWindow.getMostRecentBrowserWindow();
> +              let browser = win.gBrowser.selectedBrowser;
> +              gBrowser.discardBrowser(browser);
> +            """)

Script does not appear to discard the browser as the page does not turn white.
(In reply to Dão Gottwald [::dao] from comment #23)
> gBrowser.discardBrowser(browser);

So we have the following code now, and I also get the TabBrowserDiscarded event fired, but when I print the value of the linkedBrowser, it still says XULElement but not null:

              Components.utils.import("resource:///modules/RecentWindow.jsm");

              let win = RecentWindow.getMostRecentBrowserWindow();
              win.addEventListener("TabBrowserDiscarded", ev => {
                dump(`* TabBrowserDiscarded: ${win.gBrowser.tabs[1].linkedBrowser}\n`);
                marionetteScriptFinished(true);
              }, { once: true});
              win.gBrowser.discardBrowser(win.gBrowser.tabs[1].linkedBrowser);

Is there something wrong with the code, or how can I really get the content brower to become null?
Flags: needinfo?(dao+bmo)
Comment on attachment 8925482 [details]
Bug 1381459 - Allow closing of browserless tabs.

https://reviewboard.mozilla.org/r/196616/#review203222

Thank you for the patch Nathaniel. Please have a look at the open issues in terms of what needs to be fixed. In case of issues we can figure out details together on IRC.

::: commit-message-dc45e:1
(Diff revision 3)
> +Fix Bug#1381459

The commit message of a bug should have a proper description of what this patch is going to fix. If you cannot find a proper one, I can clearly help you.

::: testing/marionette/harness/marionette_harness/tests/unit/test_window_status_content.py:117
(Diff revision 3)
>          self.marionette.switch_to_window(self.start_tab)
>  
>          with self.assertRaises(NoSuchWindowException):
>              self.marionette.switch_to_window(tab)
> +
> +    def test_closed_browserless_tab(self):

It would be better to get this test added to `test_window_close_content.py`.

::: testing/marionette/harness/marionette_harness/tests/unit/test_window_status_content.py:124
(Diff revision 3)
> +            tab = self.open_tab()
> +            self.marionette.switch_to_window(tab)
> +            self.marionette.navigate(self.test_page)
> +
> +        with self.marionette.using_context("chrome"):
> +            self.marionette.execute_script("""

Please note that discarding the browser will only work when the tab is not selected. So the first tab has to be selected first again.

The original tab can be accessed via `self.start_tab`.

::: testing/marionette/harness/marionette_harness/tests/unit/test_window_status_content.py:130
(Diff revision 3)
> +              Components.utils.import("resource://gre/modules/AppConstants.jsm");
> +              Components.utils.import("resource:///modules/RecentWindow.jsm");
> +
> +              let win = RecentWindow.getMostRecentBrowserWindow();
> +
> +              win.gBrowser.discardBrowser(win.gBrowser.selectedBrowser);

Lets figure out the right code in parallel. As you can see by my latest comment on the bug there are still some problems left.

::: testing/marionette/harness/marionette_harness/tests/unit/test_window_status_content.py:133
(Diff revision 3)
> +              let win = RecentWindow.getMostRecentBrowserWindow();
> +
> +              win.gBrowser.discardBrowser(win.gBrowser.selectedBrowser);
> +            """)
> +
> +        self.marionette.close()

Before we close the tab, we would have to switch back to the other tab first. But, we cannot bring this tab into focus. As such the `switch_to_window` method has a `focus` parameter.
Attachment #8925482 - Flags: review?(hskupin) → review-
(In reply to Henrik Skupin (:whimboo) from comment #33)
> Is there something wrong with the code,

No.

> or how can I really get the content
> brower to become null?

Not sure what exactly "content browser" means, but the <xul:browser/> element referenced as linkedBrowser can't be null. discardBrowser only removes it from the document. This is the same as what happens when restoring a session.
Flags: needinfo?(dao+bmo)
(In reply to Dão Gottwald [::dao] from comment #35)
> > or how can I really get the content
> > brower to become null?
> 
> Not sure what exactly "content browser" means, but the <xul:browser/>

contentBrowser is just the `linkedBrowser` in Firefox or `browser` in Fennec of the given tab:

http://searchfox.org/mozilla-central/rev/c99d035f00dd894feff38e4ad28a73fb679c63a6/testing/marionette/browser.js#171
http://searchfox.org/mozilla-central/rev/c99d035f00dd894feff38e4ad28a73fb679c63a6/testing/marionette/browser.js#78-81

And the assertion as failed is here:

http://searchfox.org/mozilla-central/rev/c99d035f00dd894feff38e4ad28a73fb679c63a6/testing/marionette/assert.js#140-144

Which basically means that the gBrowser.selectedTab.linkedBrowser is `null` or `undefined`.

> element referenced as linkedBrowser can't be null. discardBrowser only
> removes it from the document. This is the same as what happens when
> restoring a session.

Ok, so maybe this failure is really happening at the exact time when a remoteness change takes place, and the browser is not ready yet. Similar to bug 1363368?
Flags: needinfo?(dao+bmo)
I don't know.
Flags: needinfo?(dao+bmo)
Given that the situation under which we hit this failure is unclear, we cannot easily write a test for it right now. I would suggest that we go ahead and take the change in driver.js, and finish the test to cover the tab close scenario for a discarded browser.
Comment on attachment 8925482 [details]
Bug 1381459 - Allow closing of browserless tabs.

https://reviewboard.mozilla.org/r/196616/#review204990

Functionality wise the patch looks fine. Only the test needs some changes so that it really tests what it should do. Otherwise the comments I added are mostly improvements. It would be good if you can apply those.

With the updated version of the patch I will trigger a try build.

Thanks!

::: commit-message-dc45e:1
(Diff revision 7)
> +Bug 1381459 - Resolve issue with closing window without content browser.

The first line of the commit should indicate what has been fixed. So lets say the following:

`Bug 1381459 - Allow closing of browserless tabs.`

The next lines can contain details about the changes and why those have been done:

`Closing a tab should always be allowed, even if the current content browser doesn't exist. This can be the case when a tab gets moved to a different process.`

::: testing/marionette/driver.js:411
(Diff revision 7)
>  
>      case Context.Content:
>        if (this.curFrame !== null) {
>          win = this.curFrame;
> -      } else if (this.curBrowser !== null && this.curBrowser.contentBrowser) {
> +      } else if (this.curBrowser !== null
> +        && this.curBrowser.window.document.documentElement.getAttribute("windowtype") == "navigator:browser") {

Please move the `&&` to the end of the previous line, and do not directly use the retrieval of the window type.

Instead we should do a small refactoring, and change that line to a property on the GeckoDriver class. See the property definitions like `windowHandles` and `chromeWindowHandles` in that same file. Just create another `defineProperty` block before `windowHandles`, which contains only that single line, and returns the window type. Then `getWindowType()` can also be updated, to use this property.

::: testing/marionette/driver.js:414
(Diff revision 7)
>          win = this.curFrame;
> -      } else if (this.curBrowser !== null && this.curBrowser.contentBrowser) {
> +      } else if (this.curBrowser !== null
> +        && this.curBrowser.window.document.documentElement.getAttribute("windowtype") == "navigator:browser") {
> +        // Given that curBrowser currently covers all types of chrome
> +        // windows, make sure to only return window handles for
> +        // 'navigator:browser'windows.

Please add the comment before the `else if`.

Also you can remove `windows` from the last line.

::: testing/marionette/harness/marionette_harness/tests/unit/test_window_close_content.py:85
(Diff revision 7)
>          self.assertListEqual([self.start_tab], self.marionette.window_handles)
>          self.assertListEqual([self.start_window], self.marionette.chrome_window_handles)
>          self.assertIsNotNone(self.marionette.session)
> +
> +    @skip_if_mobile("discardBrowser is only available in Firefox")
> +    def test_closed_browserless_tab(self):

nit: we use present in those tests: "test_close_browserless_tab".

::: testing/marionette/harness/marionette_harness/tests/unit/test_window_close_content.py:88
(Diff revision 7)
> +
> +    @skip_if_mobile("discardBrowser is only available in Firefox")
> +    def test_closed_browserless_tab(self):
> +        test_page = self.marionette.absolute_url("windowHandles.html")
> +
> +        self.assertEqual(1, len(self.marionette.window_handles))

Thinking more about it, I would propose that you call `self.close_all_tabs()` instead, which will leave a single tab open. So similar to `test_close_window_for_last_open_tab`. Also move the line to the beginning of the test, so that some unnecessary empty lines can be removed too.

::: testing/marionette/harness/marionette_harness/tests/unit/test_window_close_content.py:107
(Diff revision 7)
> +                marionetteScriptFinished(true);
> +              }, { once: true});
> +              win.gBrowser.discardBrowser(win.gBrowser.tabs[1].linkedBrowser);
> +            """)
> +
> +        self.marionette.switch_to_window(self.marionette.window_handles.pop(), False)

This will not always work because the handles as returned by `window_handles` are not sorted by creation time. So you really have to remove the `start_tab`. It means `remove()` has to be used here.

For the additional option please use the keyword syntax, so that it is clear what `False` refers to.

Also please do both steps in separate lines.

::: testing/marionette/harness/marionette_harness/tests/unit/test_window_close_content.py:109
(Diff revision 7)
> +              win.gBrowser.discardBrowser(win.gBrowser.tabs[1].linkedBrowser);
> +            """)
> +
> +        self.marionette.switch_to_window(self.marionette.window_handles.pop(), False)
> +        self.marionette.close()
> +        self.assertListEqual([self.start_tab], self.marionette.window_handles)

Please check `test_close_window_for_browser_tab` and what it does after closing the window. The same logic we need here too.

I might want to do some negative tests, just to see how the test behaves. You could comment out the call to `close`, and check the failure as thrown.
Attachment #8925482 - Flags: review?(hskupin) → review-
Comment on attachment 8925482 [details]
Bug 1381459 - Allow closing of browserless tabs.

https://reviewboard.mozilla.org/r/196616/#review205174

::: testing/marionette/driver.js:414
(Diff revision 7)
>          win = this.curFrame;
> -      } else if (this.curBrowser !== null && this.curBrowser.contentBrowser) {
> +      } else if (this.curBrowser !== null
> +        && this.curBrowser.window.document.documentElement.getAttribute("windowtype") == "navigator:browser") {
> +        // Given that curBrowser currently covers all types of chrome
> +        // windows, make sure to only return window handles for
> +        // 'navigator:browser'windows.

```
if (this.curFrame !== null) {
        win = this.curFrame;
      }
      // Given that curBrowser currently covers all types of chrome
      // windows, make sure to only return window handles for
      // 'navigator:browser'.
      else if (this.curBrowser !== null && this.windowType == "navigator:browser") {
        win = this.curBrowser.window;
      }
      break;
``` Using this throws an error in the linter, so I stuck it in the block. The linter looks for the subsequent else if to be on the same line as the closing bracket of the initial if. Any idea on how to get around this?

Error:
```
error  Closing curly brace does not appear on the same line as the subsequent block.  brace-style (eslint)
```

One way around this is to make the subsequent block and if, but that feels dirty. I don't think the comment belongs in the comment documenting the method as that is for API documentation.

::: testing/marionette/harness/marionette_harness/tests/unit/test_window_close_content.py:109
(Diff revision 7)
> +              win.gBrowser.discardBrowser(win.gBrowser.tabs[1].linkedBrowser);
> +            """)
> +
> +        self.marionette.switch_to_window(self.marionette.window_handles.pop(), False)
> +        self.marionette.close()
> +        self.assertListEqual([self.start_tab], self.marionette.window_handles)

A failure is thrown is I comment out the close. I don't think a negative test is needed, but that could be done if you think it would be optimal.
Comment on attachment 8925482 [details]
Bug 1381459 - Allow closing of browserless tabs.

https://reviewboard.mozilla.org/r/196616/#review205174

> ```
> if (this.curFrame !== null) {
>         win = this.curFrame;
>       }
>       // Given that curBrowser currently covers all types of chrome
>       // windows, make sure to only return window handles for
>       // 'navigator:browser'.
>       else if (this.curBrowser !== null && this.windowType == "navigator:browser") {
>         win = this.curBrowser.window;
>       }
>       break;
> ``` Using this throws an error in the linter, so I stuck it in the block. The linter looks for the subsequent else if to be on the same line as the closing bracket of the initial if. Any idea on how to get around this?
> 
> Error:
> ```
> error  Closing curly brace does not appear on the same line as the subsequent block.  brace-style (eslint)
> ```
> 
> One way around this is to make the subsequent block and if, but that feels dirty. I don't think the comment belongs in the comment documenting the method as that is for API documentation.

Right, that won't work. So something like that with an empty line before the comment would make it clear:

      if (this.curFrame !== null) {
        win = this.curFrame;

      // Given that curBrowser currently covers all types of chrome
      // windows, make sure to only return window handles for
      // 'navigator:browser'.
      } else if (this.curBrowser !== null && this.windowType == "navigator:browser") {

> A failure is thrown is I comment out the close. I don't think a negative test is needed, but that could be done if you think it would be optimal.

I meant just for testing your patch. In some cases a test could hit an unforseen situation and as result some other kind of exception gets raised, which then can interfer with clean-up and following tests can be broken.
Comment on attachment 8925482 [details]
Bug 1381459 - Allow closing of browserless tabs.

https://reviewboard.mozilla.org/r/196616/#review205310

::: testing/marionette/driver.js:414
(Diff revision 7)
>          win = this.curFrame;
> -      } else if (this.curBrowser !== null && this.curBrowser.contentBrowser) {
> +      } else if (this.curBrowser !== null
> +        && this.curBrowser.window.document.documentElement.getAttribute("windowtype") == "navigator:browser") {
> +        // Given that curBrowser currently covers all types of chrome
> +        // windows, make sure to only return window handles for
> +        // 'navigator:browser'windows.

I should have mentioned that I also tried an extra line between the end of the initial if statement and the beginning of the comment. The linter still shows the same error. The linter is very instistant.
(In reply to Nathaniel Nebel (:nathanielnebel) from comment #46)
> Comment on attachment 8925482 [details]
> Bug 1381459 - Resolve issue with closing window without content browser.
> 
> https://reviewboard.mozilla.org/r/196616/#review205310
> 
> ::: testing/marionette/driver.js:414
> (Diff revision 7)
> >          win = this.curFrame;
> > -      } else if (this.curBrowser !== null && this.curBrowser.contentBrowser) {
> > +      } else if (this.curBrowser !== null
> > +        && this.curBrowser.window.document.documentElement.getAttribute("windowtype") == "navigator:browser") {
> > +        // Given that curBrowser currently covers all types of chrome
> > +        // windows, make sure to only return window handles for
> > +        // 'navigator:browser'windows.
> 
> I should have mentioned that I also tried an extra line between the end of
> the initial if statement and the beginning of the comment. The linter still
> shows the same error. The linter is very instistant.

I misread what you intended, that will work.
Comment on attachment 8925482 [details]
Bug 1381459 - Allow closing of browserless tabs.

https://reviewboard.mozilla.org/r/196616/#review205340

::: testing/marionette/driver.js:414
(Diff revision 7)
>          win = this.curFrame;
> -      } else if (this.curBrowser !== null && this.curBrowser.contentBrowser) {
> +      } else if (this.curBrowser !== null
> +        && this.curBrowser.window.document.documentElement.getAttribute("windowtype") == "navigator:browser") {
> +        // Given that curBrowser currently covers all types of chrome
> +        // windows, make sure to only return window handles for
> +        // 'navigator:browser'windows.

Misinterpreted what you intended (misread your example block).
Comment on attachment 8925482 [details]
Bug 1381459 - Allow closing of browserless tabs.

https://reviewboard.mozilla.org/r/196616/#review205870

::: testing/marionette/driver.js:1178
(Diff revisions 7 - 8)
>    return this.title;
>  };
>  
>  /** Gets the current type of the window. */
>  GeckoDriver.prototype.getWindowType = function(cmd, resp) {
> -  let win = assert.window(this.getCurrentWindow());
> +  resp.body.value = this.windowType;

You have to leave the assert in place here. Only chagne the line which retrieves the window type string.

::: testing/marionette/harness/marionette_harness/tests/unit/test_window_close_content.py:107
(Diff revisions 7 - 8)
>                win.gBrowser.discardBrowser(win.gBrowser.tabs[1].linkedBrowser);
>              """)
>  
> -        self.marionette.switch_to_window(self.marionette.window_handles.pop(), False)
> -        self.marionette.close()
> -        self.assertListEqual([self.start_tab], self.marionette.window_handles)
> +        tab_handle = self.marionette.window_handles
> +        tab_handle.remove(self.start_tab)
> +        self.marionette.switch_to_window(tab_handle, focus=False)

Did  you test this? You are now passing in a list of handles to `switch_to_window` which should clearly fail.
Attachment #8925482 - Flags: review?(hskupin) → review-
(In reply to Henrik Skupin (:whimboo) from comment #50)
> Comment on attachment 8925482 [details]
> Bug 1381459 - Allow closing of browserless tabs.
> 
> https://reviewboard.mozilla.org/r/196616/#review205870
> 
> ::: testing/marionette/driver.js:1178
> (Diff revisions 7 - 8)
> >    return this.title;
> >  };
> >  
> >  /** Gets the current type of the window. */
> >  GeckoDriver.prototype.getWindowType = function(cmd, resp) {
> > -  let win = assert.window(this.getCurrentWindow());
> > +  resp.body.value = this.windowType;
> 
> You have to leave the assert in place here. Only chagne the line which
> retrieves the window type string.
> 
> :::
> testing/marionette/harness/marionette_harness/tests/unit/
> test_window_close_content.py:107
> (Diff revisions 7 - 8)
> >                win.gBrowser.discardBrowser(win.gBrowser.tabs[1].linkedBrowser);
> >              """)
> >  
> > -        self.marionette.switch_to_window(self.marionette.window_handles.pop(), False)
> > -        self.marionette.close()
> > -        self.assertListEqual([self.start_tab], self.marionette.window_handles)
> > +        tab_handle = self.marionette.window_handles
> > +        tab_handle.remove(self.start_tab)
> > +        self.marionette.switch_to_window(tab_handle, focus=False)
> 
> Did  you test this? You are now passing in a list of handles to
> `switch_to_window` which should clearly fail.

I added the assertion back on driver.js as requested. In regard to the actual test I did test it to ensure that it worked as intended. The reason I did that was because <list>.remove(item) doesn't return a copy of the list with the removed item (kind of frustrating, that would be pretty useful). It shouldn't work in most cases, but this list has 1 index.
(In reply to Nathaniel Nebel (:nathanielnebel) from comment #51)
> (In reply to Henrik Skupin (:whimboo) from comment #50)
> > Comment on attachment 8925482 [details]
> > Bug 1381459 - Allow closing of browserless tabs.
> > 
> > https://reviewboard.mozilla.org/r/196616/#review205870
> > 
> > ::: testing/marionette/driver.js:1178
> > (Diff revisions 7 - 8)
> > >    return this.title;
> > >  };
> > >  
> > >  /** Gets the current type of the window. */
> > >  GeckoDriver.prototype.getWindowType = function(cmd, resp) {
> > > -  let win = assert.window(this.getCurrentWindow());
> > > +  resp.body.value = this.windowType;
> > 
> > You have to leave the assert in place here. Only chagne the line which
> > retrieves the window type string.
> > 
> > :::
> > testing/marionette/harness/marionette_harness/tests/unit/
> > test_window_close_content.py:107
> > (Diff revisions 7 - 8)
> > >                win.gBrowser.discardBrowser(win.gBrowser.tabs[1].linkedBrowser);
> > >              """)
> > >  
> > > -        self.marionette.switch_to_window(self.marionette.window_handles.pop(), False)
> > > -        self.marionette.close()
> > > -        self.assertListEqual([self.start_tab], self.marionette.window_handles)
> > > +        tab_handle = self.marionette.window_handles
> > > +        tab_handle.remove(self.start_tab)
> > > +        self.marionette.switch_to_window(tab_handle, focus=False)
> > 
> > Did  you test this? You are now passing in a list of handles to
> > `switch_to_window` which should clearly fail.
> 
> I added the assertion back on driver.js as requested. In regard to the
> actual test I did test it to ensure that it worked as intended. The reason I
> did that was because <list>.remove(item) doesn't return a copy of the list
> with the removed item (kind of frustrating, that would be pretty useful). It
> shouldn't work in most cases, but this list has 1 index.

If it would be more readable I could explicitly grab the 0th index from the list (that is the only item in the list now).
Flags: needinfo?(hskupin)
Flags: needinfo?(hskupin)
Comment on attachment 8925482 [details]
Bug 1381459 - Allow closing of browserless tabs.

https://reviewboard.mozilla.org/r/196616/#review205918

Patch-wise this looks fine now for Marionette. But as the try build shows there are permanent failures in web-platform-reftests now. Maybe the wptrunner is making wrong assumptions, and would also need an update.

Nathaniel and myself will look into this beginning of next week.
Attachment #8925482 - Flags: review?(hskupin)
Comment on attachment 8925482 [details]
Bug 1381459 - Allow closing of browserless tabs.

https://reviewboard.mozilla.org/r/196616/#review208048

The try build looks fine. I have two nits to address for you, then we can get your patch landed! Thank you a lot!

::: testing/marionette/harness/marionette_harness/tests/unit/test_window_close_content.py:109
(Diff revision 12)
> +
> +        tab_handles = self.marionette.window_handles
> +        tab_handles.remove(self.start_tab)
> +        self.assertEqual(1, len(tab_handles))
> +        self.marionette.switch_to_window(tab_handles[0], focus=False)
> +        window_handles = self.marionette.close()

To keep the style of handles in this test file, please rename `tab_handles` to `window_handles`.

::: testing/marionette/harness/marionette_harness/tests/unit/test_window_close_content.py:111
(Diff revision 12)
> +        tab_handles.remove(self.start_tab)
> +        self.assertEqual(1, len(tab_handles))
> +        self.marionette.switch_to_window(tab_handles[0], focus=False)
> +        window_handles = self.marionette.close()
> +        self.assertNotIn(tab_handles, window_handles)
> +        self.assertListEqual(self.start_tabs, window_handles)

Replace this line with what other tests are doing in that file after closing a window, which is 

`self.assertListEqual([self.start_tab], self.marionette.window_handles)`
Attachment #8925482 - Flags: review?(hskupin) → review+
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 3a5e8f6470db -d 143968ef88f8: rebasing 435683:3a5e8f6470db "Bug 1381459 - Allow closing of browserless tabs. r=whimboo" (tip)
merging testing/marionette/driver.js
warning: conflicts while merging testing/marionette/driver.js! (edit, then use 'hg resolve --mark')
unresolved conflicts (see hg resolve, then hg rebase --continue)
Comment on attachment 8925482 [details]
Bug 1381459 - Allow closing of browserless tabs.

https://reviewboard.mozilla.org/r/196616/#review208090

::: testing/marionette/driver.js:2809
(Diff revision 15)
>   *     Top-level browsing context has been discarded.
>   * @throws {UnexpectedAlertOpenError}
>   *     A modal dialog is open, blocking this operation.
>   */
>  GeckoDriver.prototype.close = async function() {
> +  assert.window(this.getCurrentWindow(Context.Content));

You did not correctly rebase those lines, which basically restores the behavior without this patch. Please fix that.
Attachment #8925482 - Flags: review+ → review-
Comment on attachment 8925482 [details]
Bug 1381459 - Allow closing of browserless tabs.

https://reviewboard.mozilla.org/r/196616/#review208098

::: testing/marionette/driver.js:2810
(Diff revision 16)
>   * @throws {UnexpectedAlertOpenError}
>   *     A modal dialog is open, blocking this operation.
>   */
>  GeckoDriver.prototype.close = async function() {
> -  assert.contentBrowser(this.curBrowser);
> +  assert.window(this.getCurrentWindow(Context.Content));
> +  assert.noUserPrompt(this.dialog);

This line is part of `_assertAndDismissModal()`, and can also be removed.
Attachment #8925482 - Flags: review?(hskupin) → review-
Comment on attachment 8925482 [details]
Bug 1381459 - Allow closing of browserless tabs.

https://reviewboard.mozilla.org/r/196616/#review208102
Attachment #8925482 - Flags: review?(hskupin) → review+
Pushed by hskupin@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/705facc285e5
Allow closing of browserless tabs. r=whimboo
https://hg.mozilla.org/mozilla-central/rev/705facc285e5
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
status-firefox59: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla59
Product: Testing → Remote Protocol
Moving bug to Testing::Marionette Client and Harness component per bug 1815831.
Component: Marionette → Marionette Client and Harness
Product: Remote Protocol → Testing
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: