Closed
Bug 1386828
Opened 7 years ago
Closed 7 years ago
Intermittent awsy gBrowser.removeCurrentTab() Unexpected error: <class 'marionette_driver.errors.NoSuchWindowException'>
Categories
(Testing :: AWSY, defect, P5)
Tracking
(firefox56 fixed, firefox57 fixed)
RESOLVED
FIXED
mozilla57
People
(Reporter: intermittent-bug-filer, Assigned: erahm)
References
Details
(Keywords: intermittent-failure)
Attachments
(1 file)
1.83 KB,
patch
|
whimboo
:
review+
|
Details | Diff | Splinter Review |
Filed by: wkocher [at] mozilla.com https://treeherder.mozilla.org/logviewer.html#?job_id=120384180&repo=autoland https://queue.taskcluster.net/v1/task/RGAx0_-qTMq5u8FmBEfUUA/runs/0/artifacts/public/logs/live_backing.log
Comment 1•7 years ago
|
||
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)
Comment hidden (Intermittent Failures Robot) |
Assignee | ||
Comment 4•7 years ago
|
||
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)
Comment 5•7 years ago
|
||
Eric, mind answering why you are not using `self.marionette.close()` to close the current tab?
Assignee | ||
Comment 6•7 years ago
|
||
(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.
Comment 7•7 years ago
|
||
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.
Assignee | ||
Comment 8•7 years ago
|
||
(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)
Assignee | ||
Comment 9•7 years ago
|
||
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 | ||
Updated•7 years ago
|
Assignee: nobody → erahm
Status: NEW → ASSIGNED
Comment 10•7 years ago
|
||
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+
Assignee | ||
Comment 11•7 years ago
|
||
(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?
Assignee | ||
Comment 12•7 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/058ba1b60293e7058689d901cf5d8efe4ef92b25 Bug 1386828 - Use marionette.close to close tabs. r=whimboo
Comment 13•7 years ago
|
||
(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.
Comment 14•7 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/058ba1b60293
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
status-firefox57:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla57
Comment 15•7 years ago
|
||
bugherder uplift |
https://hg.mozilla.org/releases/mozilla-beta/rev/2592fec10324
status-firefox56:
--- → fixed
Updated•7 years ago
|
Component: General → AWSY
You need to log in
before you can comment on or make changes to this bug.
Description
•