Closed Bug 1386828 Opened 2 years ago Closed 2 years ago

Intermittent awsy gBrowser.removeCurrentTab() Unexpected error: <class 'marionette_driver.errors.NoSuchWindowException'>

Categories

(Testing :: AWSY, defect, P5)

Version 3
defect

Tracking

(firefox56 fixed, firefox57 fixed)

RESOLVED FIXED
mozilla57
Tracking Status
firefox56 --- fixed
firefox57 --- fixed

People

(Reporter: intermittent-bug-filer, Assigned: erahm)

References

Details

(Keywords: intermittent-failure)

Attachments

(1 file)

There is not that much information available yet, so here just an excerpt:

13:49:57     INFO - areweslimyet run by 100 pages, 3 iterations, 10 perTabPause, 30 settleWaitTime
13:49:57     INFO - closing window
13:49:57     INFO - None
13:49:57     INFO - closing window
13:49:58     INFO - None
13:49:58     INFO - closing window
13:49:58    ERROR - gBrowser.removeCurrentTab() Unexpected error: <class 'marionette_driver.errors.NoSuchWindowException'>
13:49:58     INFO - done setting up!

The call to `gBrowser.removeCurrentTab()` is done in AWSY. It's clearly not called by Marionette. So checking the code there is clearly a race condition. I have not idea why it's not using `self.marionette.close()` which would be synchronous.

https://dxr.mozilla.org/mozilla-central/rev/52285ea5e54c73d3ed824544cef2ee3f195f05e6/testing/awsy/awsy/test_memory_usage.py#110-127
Component: Marionette → General
Flags: needinfo?(erahm)
Flags: needinfo?(bob)
I'll defer to erahm on this.
Flags: needinfo?(bob)
Kate, is it possible we're not handling an error condition in `removeCurrentTab`? I can just add a catch locally but wanted to make sure the error is due to the preloaded browser already being gone (vs attempting to close the wrong browser).
Flags: needinfo?(erahm) → needinfo?(khudson)
Eric, mind answering why you are not using `self.marionette.close()` to close the current tab?
(In reply to Henrik Skupin (:whimboo) from comment #5)
> Eric, mind answering why you are not using `self.marionette.close()` to
> close the current tab?

See bug 1382889 and friends.
Well, I don't see on those bugs why you have chosen to use execute_script in combination with `gBrowser.removeCurrentTab()`.

Marionette's close() method also triggers closing the tab, and waits until the tab is gone before it returns. In your case there is a race when the tab closing takes longer, and then happens while you are cleaning up the tabs. So if you want to continue to use `removeCurrentTab` you may want to use `execute_async_script` and return when the `TabClose` event has been caught.
(In reply to Henrik Skupin (:whimboo) from comment #7)
> Well, I don't see on those bugs why you have chosen to use execute_script in
> combination with `gBrowser.removeCurrentTab()`.
> 
> Marionette's close() method also triggers closing the tab, and waits until
> the tab is gone before it returns. In your case there is a race when the tab
> closing takes longer, and then happens while you are cleaning up the tabs.
> So if you want to continue to use `removeCurrentTab` you may want to use
> `execute_async_script` and return when the `TabClose` event has been caught.

Sorry I thought you were referring to `removePreloadedBrowser` :( I think I assumed `close` would close the session by closing the entire top level window, not just the tab. I guess we could flesh out the docs. I'll `self.marionette.close` a try.
Flags: needinfo?(khudson)
This switches over to using `marionette.close()`. I have no idea if this will fix the intermittent.

MozReview-Commit-ID: AL5yTSbX5Mn
Attachment #8895050 - Flags: review?(hskupin)
Assignee: nobody → erahm
Status: NEW → ASSIGNED
Comment on attachment 8895050 [details] [diff] [review]
Use marionette.close to close tabs

Review of attachment 8895050 [details] [diff] [review]:
-----------------------------------------------------------------

Looks fine to me. I also added some additional notes in case you want to improve it more. But feel free to move this out to another bug.

::: testing/awsy/awsy/test_memory_usage.py
@@ +108,5 @@
>          self._pages_loaded = 0
>  
>          # Close all tabs except one
>          for x in range(len(self.marionette.window_handles) - 1):
>              self.logger.info("closing window")

If you want you may also want to print the window handle which you are closing. It would help a lot for debugging purposes later.

@@ +111,5 @@
>          for x in range(len(self.marionette.window_handles) - 1):
>              self.logger.info("closing window")
> +            self.marionette.close()
> +            self._tabs = self.marionette.window_handles
> +            self.marionette.switch_to_window(self._tabs[-1])

I don't see the need to request the window handles each time.

Have a look at our WindowManager mixin for Marionette in how we close the open tabs:

https://dxr.mozilla.org/mozilla-central/source/testing/marionette/harness/marionette_harness/runner/mixins/window_manager.py#32

The wait.until() is not needed anymore and I'm going to remove that too via bug 1388627.
Attachment #8895050 - Flags: review?(hskupin) → review+
(In reply to Henrik Skupin (:whimboo) from comment #10)
> Comment on attachment 8895050 [details] [diff] [review]
> Use marionette.close to close tabs
> 
> Review of attachment 8895050 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Looks fine to me. I also added some additional notes in case you want to
> improve it more. But feel free to move this out to another bug.
> 
> ::: testing/awsy/awsy/test_memory_usage.py
> @@ +108,5 @@
> >          self._pages_loaded = 0
> >  
> >          # Close all tabs except one
> >          for x in range(len(self.marionette.window_handles) - 1):
> >              self.logger.info("closing window")
> 
> If you want you may also want to print the window handle which you are
> closing. It would help a lot for debugging purposes later.

Good idea, I'll add a debug line.

> @@ +111,5 @@
> >          for x in range(len(self.marionette.window_handles) - 1):
> >              self.logger.info("closing window")
> > +            self.marionette.close()
> > +            self._tabs = self.marionette.window_handles
> > +            self.marionette.switch_to_window(self._tabs[-1])
> 
> I don't see the need to request the window handles each time.

My understanding was that window handles aren't stable after mutating a window, but maybe that's just the ordering?
(In reply to Eric Rahm [:erahm] (please no mozreview requests) from comment #11)
> > >          for x in range(len(self.marionette.window_handles) - 1):
> > >              self.logger.info("closing window")
> > > +            self.marionette.close()
> > > +            self._tabs = self.marionette.window_handles
> > > +            self.marionette.switch_to_window(self._tabs[-1])
> > 
> > I don't see the need to request the window handles each time.
> 
> My understanding was that window handles aren't stable after mutating a
> window, but maybe that's just the ordering?

Window handles only change if remoteness changes, or if the content process holding the tab is changed. You aren't doing anything of that here. So retrieving it once will be all fine.
https://hg.mozilla.org/mozilla-central/rev/058ba1b60293
Status: ASSIGNED → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla57
Component: General → AWSY
You need to log in before you can comment on or make changes to this bug.