As noticed while working on bug 1323169, Marionette cannot handle the current tab count of Fennec. It always returns a single handle. Not sure about windows but I doubt that this is supported in Fennec at all. But anyway we should check both. Snippet: with self.marionette.using_context("chrome"): self.marionette.execute_script(""" if (window.BrowserApp) { BrowserApp.addTab('about:config', { selected: true, parentId: BrowserApp.selectedTab.id }); } """) The tab will be opened but self.marionette.window_handles will continue to return a single entry only.

Henrik: this is not the place to discuss this for real, but it'll do for now. In unpublished work I have been adding Marionette support to GeckoView. GeckoView is nascent "let's do embedding right on Android" work that the Android platform team (principally snorp and jchen) and myself have been pushing forward. Myk wrote about it at https://mykzilla.org/2016/09/14/the-once-and-future-geckoview/. While adding support to GeckoView for Marionette and the remote debugger, I have noticed that the remote debugger interface is significantly easier to work with and more versatile. The main difference is that the consuming App supplies to the remote debugger various actors that provide the required counts and objects, where-as Marionette switches on the App and tries to do various things itself. It's a classic inversion of control, and it's a great solution to this problem. Are there plans to generalize Marionette? Has this been considered and rejected? Could we try to achieve this, so that the *next* Marionette consumer (GeckoView?) has a better API to work with? Link a ticket or tickets, or I can file a new one and try to make my case for real.

(In reply to Nick Alexander :nalexander (leave until January 2017) from comment #1) > Henrik: this is not the place to discuss this for real, but it'll do for > now. Oh! I should say that I bring this up here because GeckoView and Fennec have very different models of windows and tabs. Each WebView-like GeckoView corresponds to a XUL window, containing exactly one <browser>. (I believe his is because GeckoView widgets can be different sizes, and this is best modeled in Gecko by an nsWindow.) Fennec, of course, has exactly one XUL window containing many <browser> elements. I got the "one GeckoView" connection to Marionette working but haven't tried the "multiple GeckoView" connections yet.

(In reply to Nick Alexander :nalexander (leave until January 2017) from comment #1) > While adding support to GeckoView for Marionette and the remote debugger, I > have noticed that the remote debugger interface is significantly easier to > work with and more versatile. The main difference is that the consuming App > supplies to the remote debugger various actors that provide the required > counts and objects, where-as Marionette switches on the App and tries to do > various things itself. It's a classic inversion of control, and it's a > great solution to this problem. This is great to hear Nick! I assume there is even not a bug filed for this work? If there is, I would love to see something, at least when it is showable to the world. :) Regarding the remote debugger, it sounds great and I would like to see how this works. Maybe you have a simple example for getting the eg. chrome windows? > Are there plans to generalize Marionette? Has this been considered and > rejected? Could we try to achieve this, so that the *next* Marionette > consumer (GeckoView?) has a better API to work with? Link a ticket or > tickets, or I can file a new one and try to make my case for real. We definitely want support for different platforms and products. So having a way to generalize all sounds interesting. But its something we don't have priorities for. This bug has only been filed to get the immediate needs implemented so we can run at least on Fennec. For a broader discussion I would propose that you start a new thread in the mozilla.tools.marionette newsgroup/list so we can all participate. I for myself would support such a simplification.

Nick, so for the current issue I assume that we always have a single "chrome window" only. There is no way to raise other dialogs or windows. And the tabbrowser which we should be able to access via "window.BrowserApp" should give us the correct window handles for tabs.

Details for the BrowserApp object can be found here: https://developer.mozilla.org/en-US/Add-ons/Firefox_for_Android/API/BrowserApp#Manipulating_browser_tabs Listing tabs (getWindowHandles): https://developer.mozilla.org/en-US/Add-ons/Firefox_for_Android/API/BrowserApp/tabs Selected tab (getWindowHandle): https://developer.mozilla.org/en-US/Add-ons/Firefox_for_Android/API/BrowserApp/selectTab Change active tab (switchToWindow): https://developer.mozilla.org/en-US/Add-ons/Firefox_for_Android/API/BrowserApp/selectTab Close a tab (close): https://developer.mozilla.org/en-US/Add-ons/Firefox_for_Android/API/BrowserApp/closeTab

We might want to wait for bug 1311041 being landed first which changes how we handle content windows.

(In reply to Henrik Skupin (:whimboo) from comment #4) > Nick, so for the current issue I assume that we always have a single "chrome > window" only. There is no way to raise other dialogs or windows. And the > tabbrowser which we should be able to access via "window.BrowserApp" should > give us the correct window handles for tabs. Sorry for the delayed reply. I think your understanding correct: there is only a single chrome XUL window for the Fennec lifetime. Dialogs are handled on the Java side (as modal Java overlays), and there is no way to open a second chrome XUL window. And yes, `window.BrowserApp` can map App-domain tabs to XUL <browser> elements. I hesitate only because I'm not 100% certain there aren't edge cases. For example, it is possible some part of the code uses a hidden window in some circumstance. But I would expect just the one.

Thanks Nick! Given that I want to see this working in Fennec to proof my upcoming changes for bug 1124604, I created a patch for this bug. It wasn't that hard to get all the pieces working. It will add the retrieval of all open tabs, the current tab, switching to a tab, and closing a tab.

Comment on attachment 8827477 [details] Bug 1323185 - Skip unit tests which should not be run with Fennec. https://reviewboard.mozilla.org/r/105150/#review106076

Comment on attachment 8827557 [details] Bug 1323185 - Fix test_close_not_selected_tab for correctly closing a background tab. https://reviewboard.mozilla.org/r/105202/#review106078

Comment on attachment 8827478 [details] Bug 1323185 - Add window (tab) handling support for Fennec. https://reviewboard.mozilla.org/r/105152/#review106336 ::: testing/marionette/browser.js:34 (Diff revision 4) > + let browser = null; > + > + if (tab.browser) { // Fennec > + browser = tab.browser; > + } else if (tab.linkedBrowser) { // Firefox > + browser = tab.linkedBrowser; > + } > + > + return browser; I’m not a big fan of functions that return null because of a couple of different reasons: 1. Consumers are unlikely to check the return value before using. This a common cause of runtime errors in JavaScript. 2. The common way to handle invalid input to functions is to raise `TypeError` and this breaks the expected pattern. Is it not possible for consumers to check that the input to `browser.getBrowserForTab` is valid before calling it? ```js let br; if (tab) { br = browser.getBrowserForTab(tab); } ``` ::: testing/marionette/browser.js:199 (Diff revision 4) > - if (!this.browser || !this.tab || this.browser.browsers.length == 1) { > + if (!this.tabBrowser || this.tabBrowser.tabs.length == 1 || !this.tab) { > return this.closeWindow(); > } > > return new Promise((resolve, reject) => { > - if (this.browser.removeTab) { > + if (this.tabBrowser.closeTab) { // Fennec) Put comments about what is to come above. ::: testing/marionette/browser.js:204 (Diff revision 4) > - if (this.browser.removeTab) { > + if (this.tabBrowser.closeTab) { // Fennec) > + this.tabBrowser.deck.addEventListener("TabClose", ev => { > + resolve(); > + }, {once: true}); > + this.tabBrowser.closeTab(this.tab); > + } else if (this.tabBrowser.removeTab) { // Firefox Same here. You might want to consider separating these two conditions by an addition newline. ::: testing/marionette/browser.js:238 (Diff revision 4) > - this.browser.selectTabAtIndex(ind); > - this.tab = this.browser.selectedTab; > - this._browserWasRemote = this.browserForTab.isRemoteBrowser; > } > - else { > - this.tab = this.browser.selectedTab; > + > + if (this.tabBrowser) { Make this negatory and return, and point out this behaviour in the docstring. I want it to be very clear that when calling `switchToTab(n)`, without `win` and when `this.tabBrowser` isn’t set, that it acts as a no-op. ::: testing/marionette/browser.js:239 (Diff revision 4) > + if (index === undefined) { > + this.tab = this.tabBrowser.selectedTab; > + } else { > + this.tab = this.tabBrowser.tabs[index]; > - } > + } Mention in docstring that if `index` is also not defined, that it switches to the selected tab. You could mark both the `index` and `win` arugmens optional arguments. ::: testing/marionette/browser.js:239 (Diff revision 4) > - this.tab = this.browser.selectedTab; > - this._browserWasRemote = this.browserForTab.isRemoteBrowser; > } > - else { > - this.tab = this.browser.selectedTab; > + > + if (this.tabBrowser) { > + if (index === undefined) { Please use `typeof index == "undefined"`. ::: testing/marionette/browser.js:242 (Diff revision 4) > - else { > - this.tab = this.browser.selectedTab; > + > + if (this.tabBrowser) { > + if (index === undefined) { > + this.tab = this.tabBrowser.selectedTab; > + } else { > + this.tab = this.tabBrowser.tabs[index]; Does this actually switch to a tab? I see that you are no longer calling `this.browser.selectTabAtIndex`. ::: testing/marionette/driver.js:195 (Diff revision 4) > let winEn = Services.wm.getEnumerator(null); > > while (winEn.hasMoreElements()) { > let win = winEn.getNext(); > - if (win.gBrowser) { > - let tabbrowser = win.gBrowser; > + > + if (win.BrowserApp) { // Fennec Please comments about what is to come before it actually happens. ::: testing/marionette/driver.js:196 (Diff revision 4) > - for (let i = 0; i < tabbrowser.browsers.length; ++i) { > - let winId = this.getIdForBrowser(tabbrowser.getBrowserAtIndex(i)); > + win.BrowserApp.tabs.forEach(tab => { > + let winId = this.getIdForBrowser(tab.browser); > if (winId !== null) { > hs.push(winId); > } > + }); This is fine, but you could replace it with this: ```js return win.BrowserApp.tabs .map(tab => this.getIdForBrowser(tab.browser)) .filter(id => id !== null); ```

Comment on attachment 8827478 [details] Bug 1323185 - Add window (tab) handling support for Fennec. https://reviewboard.mozilla.org/r/105152/#review106336 > I’m not a big fan of functions that return null because of a couple of different reasons: > > 1. Consumers are unlikely to check the return value before using. This a common cause of runtime errors in JavaScript. > > 2. The common way to handle invalid input to functions is to raise `TypeError` and this breaks the expected pattern. > > Is it not possible for consumers to check that the input to `browser.getBrowserForTab` is valid before calling it? > > ```js > let br; > if (tab) { > br = browser.getBrowserForTab(tab); > } > ``` It might fix this single case if tab is undefined. But what shall we return if we cannot figure out a browser? Shall we then raise? I think it would be a valid case. > Put comments about what is to come above. I did it that way because putting it above makes at least the else case a bit awkward to look at. Anyway, I can change it if that is what you want. > Mention in docstring that if `index` is also not defined, that it switches to the selected tab. > > You could mark both the `index` and `win` arugmens optional arguments. Arguments are by default optional and have the value "undefined". As such it's not necessary to explicitly define it in the function argument list unless you want a different default value. I will mark them optional in the docstring. > Does this actually switch to a tab? I see that you are no longer calling `this.browser.selectTabAtIndex`. Ups, good catch! I had those changes first as part of my commit series for bug 1124604, but then decided to move it out to this one. So I missed to add this back. Interesting that no tests are failing, which means we have poor test coverage here. I already wrote some additional tests with focus checks for bug 1124604, so I will add those over there. > Please comments about what is to come before it actually happens. I removed this code with the changes as described below. > This is fine, but you could replace it with this: > > ```js > return win.BrowserApp.tabs > .map(tab => this.getIdForBrowser(tab.browser)) > .filter(id => id !== null); > ``` This won't work for Firefox because tabs is a nodeList and doesn't implement `map()`. I would say I combine the both if conditions by using the browser specific wrappers instead.

Comment on attachment 8827478 [details] Bug 1323185 - Add window (tab) handling support for Fennec. https://reviewboard.mozilla.org/r/105152/#review106336 > It might fix this single case if tab is undefined. But what shall we return if we cannot figure out a browser? Shall we then raise? I think it would be a valid case. Generally the problem is that the Context class in browser.js is used for every kind of window and not only the browser window. I will have a look what's best here to do.

Comment on attachment 8827478 [details] Bug 1323185 - Add window (tab) handling support for Fennec. https://reviewboard.mozilla.org/r/105152/#review106336 > Generally the problem is that the Context class in browser.js is used for every kind of window and not only the browser window. I will have a look what's best here to do. Andreas, please have a look at the latest changes. I've made some updates and tested those also with the Firefox ui tests which make use of non-browser windows. And it works fine. Please also let me know about the last remaining quesiton from above. Maybe we can address that in a different bug? I would suggest a wrapper module for each application to have an abstract layer for accessing tab related methods. Thanks.

Comment on attachment 8827478 [details] Bug 1323185 - Add window (tab) handling support for Fennec. https://reviewboard.mozilla.org/r/105152/#review106336 > I did it that way because putting it above makes at least the else case a bit awkward to look at. Anyway, I can change it if that is what you want. I subscribe to Pike’s ideas on comments, which you are free to disagree with, but I think the the “post-fix” comments are only acceptable when explaining the metric of some hard-to-understand number, such as milliseconds: ```js let pageLoadTimeout = 300000; // 5 minutes ``` See the ‘Comments’ chapter: https://www.lysator.liu.se/c/pikestyle.html > This won't work for Firefox because tabs is a nodeList and doesn't implement `map()`. I would say I combine the both if conditions by using the browser specific wrappers instead. Ugh, right. Blasted `NodeList`. It would be nice if we could generalise this somehow so we wouldn’t need a null check. Perhaps a function literal iterator. But we can address this at some later point.

Comment on attachment 8827478 [details] Bug 1323185 - Add window (tab) handling support for Fennec. https://reviewboard.mozilla.org/r/105152/#review106864 ::: testing/marionette/browser.js:26 (Diff revision 6) > + * @return {<xul:browser>} > + * Linked browser or null if it's not a valid tab. Might return null, and this should be reflected in the return type declaration. ::: testing/marionette/browser.js:50 (Diff revision 6) > + * @return {<xul:tabbrowser>} > + * Tab browser or null if it's not a browser window. Might return null, and this should be reflected in the return type declaration. ::: testing/marionette/browser.js:196 (Diff revision 6) > * A promise which is resolved when the current tab has been closed. > */ > closeTab() { > // If the current window is not a browser then close it directly. Do the > // same if only one remaining tab is open, or no tab selected at all. > - if (!this.browser || !this.tab || this.browser.browsers.length == 1) { > + if (!this.tabBrowser || this.tabBrowser.tabs.length == 1 || !this.tab) { Should probably use `===` here. ::: testing/marionette/browser.js:201 (Diff revision 6) > - if (this.browser.removeTab) { > + if (this.tabBrowser.closeTab) { // Fennec) > + this.tabBrowser.deck.addEventListener("TabClose", ev => { > + resolve(); > + }, {once: true}); > + this.tabBrowser.closeTab(this.tab); > + } else if (this.tabBrowser.removeTab) { // Firefox > this.tab.addEventListener("TabClose", ev => { > resolve(); > }, {once: true}); > - this.browser.removeTab(this.tab); > + this.tabBrowser.removeTab(this.tab); > } else { > reject(new UnsupportedOperationError( > `closeTab() not supported in ${this.driver.appName}`)); > } Same thing about comments here. ::: testing/marionette/browser.js:231 (Diff revision 6) > - * If a window is provided, the internal reference is updated before > - * proceeding. > + * @param {number} index > + * Optional, tab index to switch to. If not defined, > + * the currently selected tab will be used. Mark with suffix `=`. ::: testing/marionette/browser.js:234 (Diff revision 6) > + * @param {nsIDOMWindow} win > + * Optional, if specified, switch to that window before selecting the tab. Optionals are marked with the suffix `=`. ::: testing/marionette/browser.js:237 (Diff revision 6) > - * proceeding. > + * Optional, tab index to switch to. If not defined, > + * the currently selected tab will be used. > + * @param {nsIDOMWindow} win > + * Optional, if specified, switch to that window before selecting the tab. > */ > - switchToTab(ind, win) { > + switchToTab(index, win) { ```js switchToTab (index = undefined, win = undefined) { … } ``` ::: testing/marionette/browser.js:247 (Diff revision 6) > + > + if (!this.tabBrowser) { > + return; > } > - if (this.browser.selectTabAtIndex) { > - this.browser.selectTabAtIndex(ind); > + > + if (typeof index === "undefined") { Two `==` is sufficient here, since `typeof`’s return type is guaranteed to be a string. ::: testing/marionette/browser.js:262 (Diff revision 6) > + if (this.driver.appName == "Firefox") { > + this._browserWasRemote = browser.getBrowserForTab(this.tab).isRemoteBrowser; > - this._hasRemotenessChange = false; > + this._hasRemotenessChange = false; > - } > + } This can be moved up to the Firefox block above. ::: testing/marionette/browser.js:307 (Diff revision 6) > hasRemotenessChange() { > // None of these checks are relevant on b2g or if we don't have a tab yet, > // and may not apply on Fennec. > if (this.driver.appName != "Firefox" || > this.tab === null || > - this.browserForTab === null) { > + browser.getBrowserForTab(this.tab) === null) { > return false; > } > > if (this._hasRemotenessChange) { > return true; > } > > - let currentIsRemote = this.browserForTab.isRemoteBrowser; > + let currentIsRemote = browser.getBrowserForTab(this.tab).isRemoteBrowser; > this._hasRemotenessChange = this._browserWasRemote !== currentIsRemote; > this._browserWasRemote = currentIsRemote; > return this._hasRemotenessChange; > } Assign `browser.getBrowserForTab(this.tab)` at the top of the function because it’s used twice and will make the code nicer to read. ::: testing/marionette/driver.js (Diff revision 6) > "Marionette:pollForReadyState" + this.curBrowser.curFrameId, > cmd.parameters); > }); > > yield get; > - this.curBrowser.browserForTab.focus(); Why remove this?

Comment on attachment 8827478 [details] Bug 1323185 - Add window (tab) handling support for Fennec. https://reviewboard.mozilla.org/r/105152/#review106336 > Andreas, please have a look at the latest changes. I've made some updates and tested those also with the Firefox ui tests which make use of non-browser windows. And it works fine. > > Please also let me know about the last remaining quesiton from above. Maybe we can address that in a different bug? I would suggest a wrapper module for each application to have an abstract layer for accessing tab related methods. > > Thanks. OK.

Comment on attachment 8827478 [details] Bug 1323185 - Add window (tab) handling support for Fennec. https://reviewboard.mozilla.org/r/105152/#review106864 > Might return null, and this should be reflected in the return type declaration. After our conversation on IRC I think that I will change it to raise an exception instead. So similar to what we already do in closeTab(). > ```js > switchToTab (index = undefined, win = undefined) { > … > } > ``` It's not something we have used in our code anywhere else. If you want this style we should get everything updated at once, which is a perfect good first bug. I hope you are fine when I skip this for now. > This can be moved up to the Firefox block above. I don't think so, because we would miss the index === undefined case, and wouldn't initialize the variables correctly. > Assign `browser.getBrowserForTab(this.tab)` at the top of the function because it’s used twice and will make the code nicer to read. Please note that moving this code up would mean we need an extra if condition for the tab first. Also we should do this check after the Firefox one. All in all I don't see an improvement of the code here, it's only getting more complicated to read. > Why remove this? I can add this back for now, but will remove it in the focus patch on the other bug. If a discussion is necessary we should do it over there once the patch is up for review.

Comment on attachment 8827478 [details] Bug 1323185 - Add window (tab) handling support for Fennec. https://reviewboard.mozilla.org/r/105152/#review106336 > I subscribe to Pike’s ideas on comments, which you are free to disagree with, but I think the the “post-fix” comments are only acceptable when explaining the metric of some hard-to-understand number, such as milliseconds: > > ```js > let pageLoadTimeout = 300000; // 5 minutes > ``` > > See the ‘Comments’ chapter: https://www.lysator.liu.se/c/pikestyle.html Fine with me. But I will update the patch to move the comments at least inside the conditional blocks. It makes it way better readable and is also consistent with the Mozilla like JS coding style.

Comment on attachment 8827478 [details] Bug 1323185 - Add window (tab) handling support for Fennec. https://reviewboard.mozilla.org/r/105152/#review107176 ::: testing/marionette/browser.js:33 (Diff revision 6) > + */ > +browser.getBrowserForTab = function (tab) { > + let browser = null; > + > + // Fennec > + if (tab.browser) { Oh, wow. Those lines are purely wrong! For properties we really have to use `.hasOwnProperty(prop)`. Otherwise we will fail if the object.prop is null or undefined.

This all sounds good. r+.

Sadly the latest try builds shows failures on Linux and OS X which I cannot reproduce locally. I will hvae to do more investigation on Monday, and maybe revert throwing an exception if needed. https://treeherder.mozilla.org/#/jobs?repo=try&revision=78b16b879081&selectedJob=70853272

Looks like everything was related to inconsistent window handles, which happens after loading a remote uri in a tab. I pushed another try build. So lets see if that fixes the problems: https://treeherder.mozilla.org/#/jobs?repo=try&revision=7fe0008918124e9934dbb8d7423c435b156ab9d9

The remaining problem with try builds is that we are ending up with "about:blankhttp://localhost:port/windowHandles.html". Not sure why navigating to "about:blank" leaves this string in the urlbar. Maybe it's a timing issue, and for safety we should reset the urlbar value first.

Try build with the fixes in place: https://treeherder.mozilla.org/#/jobs?repo=try&revision=0804cb5bc694c376394a43b4900a9122b0febfdc&filter-tier=1&filter-tier=2&filter-tier=3 Everything looks fine now except for the test_about_pages.py for Fennec. I'm going to skip those again with the next patch. Try results will come up here: https://treeherder.mozilla.org/#/jobs?repo=try&revision=06becb6dee4298454dacf690eed98ae6eef2d585 Andreas, can you please have again a look over the latest changes? If all look fine we should be able to push the patch now. Thanks.

Comment on attachment 8827478 [details] Bug 1323185 - Add window (tab) handling support for Fennec. https://reviewboard.mozilla.org/r/105152/#review106864 > I don't think so, because we would miss the index === undefined case, and wouldn't initialize the variables correctly. Right, OK. > Please note that moving this code up would mean we need an extra if condition for the tab first. Also we should do this check after the Firefox one. All in all I don't see an improvement of the code here, it's only getting more complicated to read. No, that makes sense. Thanks for pointing out the null check.

Comment on attachment 8827478 [details] Bug 1323185 - Add window (tab) handling support for Fennec. https://reviewboard.mozilla.org/r/105152/#review107176 > Oh, wow. Those lines are purely wrong! For properties we really have to use `.hasOwnProperty(prop)`. Otherwise we will fail if the object.prop is null or undefined. I can’t close this issue.

The changes look OK to me. Please ship it when you’re confident.

Pushed by hskupin@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/ae6657776fe0 Fix test_close_not_selected_tab for correctly closing a background tab. r=maja_zf https://hg.mozilla.org/integration/autoland/rev/5b6ffae33504 Skip unit tests which should not be run with Fennec. r=maja_zf https://hg.mozilla.org/integration/autoland/rev/2aa7604fbaaf Add window (tab) handling support for Fennec. r=ato

Backout by kwierso@gmail.com: https://hg.mozilla.org/integration/autoland/rev/b772206648a0 Backed out 3 changesets for Mn bustage a=backout CLOSED TREE

I had to back these out for frequent marionette failures like https://treeherder.mozilla.org/logviewer.html#?job_id=71557655&repo=autoland https://hg.mozilla.org/integration/autoland/rev/b772206648a0

In all the cases the failing test is TestAboutPages.test_type_to_non_remote_tab, and only in e10s mode. There is a hang in getCurrentUrl() whereby Marionette doesn't send a response at all. As result Firefox gets killed. Maybe a remoteness change is not correctly detected. I will have a look at it tomorrow.

(In reply to Henrik Skupin (:whimboo) from comment #52) > In all the cases the failing test is > TestAboutPages.test_type_to_non_remote_tab, and only in e10s mode. There is > a hang in getCurrentUrl() whereby Marionette doesn't send a response at all. > As result Firefox gets killed. > > Maybe a remoteness change is not correctly detected. I will have a look at > it tomorrow. That is entirely likely. It would be similar to the errors I’ve encountered previously surrounding remoteness changes. If we can work around the issue for now, I hope to provide a fix for remoteness changes soon by using <xul:browser>’s permanentKey. I was hoping these patches would land before tackling that, as extensive modifications to testing/marionette/browser.js are necessary in order to not do it in a hacky way.

Oh, and it is a Linux and Windows 7 VM only issue. So I will have to use one click loaners to nail this problem down.

Here a link for all the test failures on autoland while the patch was checked-in: https://treeherder.mozilla.org/#/jobs?repo=autoland&filter-searchStr=mn&fromchange=96d0d062d97829e5e981ba7ca9fd1fe6fd0996e3&filter-resultStatus=testfailed&filter-resultStatus=busted&filter-resultStatus=exception&filter-resultStatus=retry&filter-resultStatus=usercancel&filter-resultStatus=runnable&tochange=b772206648a0

So the last changes I did and where I thought that those would fix the issue, actually caused complete bustages for e10s jobs: https://treeherder.mozilla.org/#/jobs?repo=try&revision=ab0dfbc81e6a585f861cb82e8dbf27350cae65df&filter-tier=1&filter-tier=2&filter-tier=3&selectedJob=71946745 So I think that we have some weird and hard to reproduce race conditions here. Several tries with one-click loaners weren't even successful. I think that I will add a lot of debug() calls to get an idea under which situation the hang is happening. Andreas, I wish we wouldn't have those infinite hangs of the proxy in case of a remoteness change happened unexpectedly. Maybe we should use pending commands at every time?

Btw. in all the cases we hang in getCurrentUrl(): > 1485283683541 Marionette TRACE conn611 -> [0,30,"setContext",{"value":"content"}] > 1485283683544 Marionette TRACE conn611 <- [1,30,null,{}] > 1485283683799 Marionette TRACE conn611 -> [0,31,"getCurrentUrl",null] > 1485283683911 Marionette DEBUG loaded listener.js > 1485283743669 Marionette DEBUG Closed connection conn611 Andreas, I wonder if we should simply disable this test for now. It looks like my patch didn't cause an issue for any other test. Also because of the test seem to be wrong. It's name is "test_type_to_non_remote_tab" but when I check the logs I see: > 1485283682715 Marionette TRACE conn611 <- [1,22,null,["2147483827","2147483838"]] > 1485283682718 Marionette TRACE conn611 -> [0,23,"switchToWindow",{"name":"2147483838"}] This means the tab has actually the remote flag set from right after its opening.

And when the test passes the remote flags of both tabs are not set: > 1485283769725 Marionette TRACE conn610 -> [0,19,"getWindowHandles",null] > 1485283769726 Marionette TRACE conn610 <- [1,19,null,["249","268"]] > 1485283769736 Marionette TRACE conn610 -> [0,20,"switchToWindow",{"name":"268"}] > 1485283769738 Marionette TRACE conn610 <- [1,20,null,{}]

(In reply to Henrik Skupin (:whimboo) from comment #56) > Andreas, I wish we wouldn't have those infinite hangs of the proxy in case > of a remoteness change happened unexpectedly. Maybe we should use pending > commands at every time? I don’t know how to fix that a promise doesn’t resolve in a timely manner. Unfortunately using pending commands would pretty much dismantle how Marionette works currently.

(In reply to Henrik Skupin (:whimboo) from comment #57) > Andreas, I wonder if we should simply disable this test for now. It looks > like my patch didn't cause an issue for any other test. Also because of the > test seem to be wrong. That’s fine with me.

Pushed by hskupin@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/f919fbf070ac Fix test_close_not_selected_tab for correctly closing a background tab. r=maja_zf https://hg.mozilla.org/integration/autoland/rev/d4e1988f25df Skip unit tests which should not be run with Fennec. r=maja_zf https://hg.mozilla.org/integration/autoland/rev/56cf0d1016b8 Add window (tab) handling support for Fennec. r=ato

https://hg.mozilla.org/mozilla-central/rev/f919fbf070ac https://hg.mozilla.org/mozilla-central/rev/d4e1988f25df https://hg.mozilla.org/mozilla-central/rev/56cf0d1016b8

It's a feature we want to have for the next ESR. So please uplift this test-only patch to aurora and beta. Thanks.

https://hg.mozilla.org/releases/mozilla-aurora/rev/23a648cc280c https://hg.mozilla.org/releases/mozilla-aurora/rev/53f4b1138f26 https://hg.mozilla.org/releases/mozilla-aurora/rev/addfb6ec7605

https://hg.mozilla.org/releases/mozilla-beta/rev/615cff70df60 https://hg.mozilla.org/releases/mozilla-beta/rev/9cc076bce62f https://hg.mozilla.org/releases/mozilla-beta/rev/88d8b4f8a2df

Marking bug as spec related, because it helps us with other bugs, and also allows us to see the webdriver spec implementation status for Fennec.