Unfortunately I can't reproduce this. I was getting a profile using the Gecko profiler addon, which opens a tab with cleopatra in it and janks the parent process a bit while it's getting the profile. While it was doing the profile symbolication, I switched between tabs, and suddenly ended up in the cleopatra tab (which I think was the one I clicked on last) while the tab I had clicked just before was still visuallyselected. So both the real active tab (the cleopatra one) and the previous one looked selected in the tab bar.

I think the specific tab switching steps I did were something like: 1. Switch away from cleopatra to tab A. 2. Click back on the cleopatra tab, which is hanging. 3. Click on some other tab (tab B) before the spinner for the cleopatra tab appears. 4. Click on the cleopatra tab again. And now both the cleopatra tab and tab B were visually selected.

Look like gBrowser.moveTabTo is messing with visually selected by always setting the visuallySelected attribute on current tab, without checking if the selected tab was visuallySelected before the move Run this code twice and you get 2 visuallySelected tabs function addNewTab() { let pos = gBrowser.selectedTab._tPos; let newTab = gBrowser.addTab("about:blank") gBrowser.moveTabTo(newTab, pos + 1); gBrowser.selectedTab = newTab; } addNewTab(); addNewTab();

Transferring needinfo from bug 1239533. From blassey in https://bugzilla.mozilla.org/show_bug.cgi?id=1239533#c3: "Jeff, do you think this should block?"

(In reply to Mike Conley (:mconley) - Needinfo me! from comment #4) ... > From blassey in https://bugzilla.mozilla.org/show_bug.cgi?id=1239533#c3: > > "Jeff, do you think this should block?" I'm yes, but could be convinced otherwise if we think this repro is sufficiently rare.

Re-nomming based on comment 5.

Thanks tabmix.onemen! I used your patch as a starting point to fix this - very helpful!

If we're in the midst of an async tab switch while calling moveTabTo, we can get into a case where _visuallySelected is set to true on two different tabs. What we want to do in moveTabTo is to remove logical selection from all tabs, and then re-add logical selection to mCurrentTab (and visual selection as well if we're not running with e10s). If we're running with e10s, then the visual selection will not be changed, which is fine, since if we weren't in the midst of a tab switch, the previously visually selected tab should still be correct, and if we are in the midst of a tab switch, then the async tab switcher will set the visually selected tab once the tab switch has completed. Review commit: https://reviewboard.mozilla.org/r/32641/diff/#index_header See other reviews: https://reviewboard.mozilla.org/r/32641/

Comment on attachment 8712777 [details] Bug 1157404 - Don't visuallyselect remote tabs during moveTabTo. https://reviewboard.mozilla.org/r/32641/#review29677 It seems to me that this problem would be better fixed by just relying on gBrowser.selectedTab everywhere (and having tabs query their gBrowser to know if they are selected). That would remove possibilities of multiple tabs being selected at once, right? If so, maybe a follow-up should be filed for this. r=me with the logicallySelected property removed and the two questions answered. ::: browser/base/content/tabbrowser.xml (Diff revision 1) > - this.mCurrentTab._logicallySelected = false; The logicallySelected property is now unused and should be removed. ::: browser/base/content/tabbrowser.xml (Diff revision 1) > - this.mCurrentTab._logicallySelected = false; > - this.mCurrentTab._visuallySelected = false; > > // invalidate caches > this._browsers = null; > this._visibleTabs = null; > > // use .item() instead of [] because dragging to the end of the strip goes out of > // bounds: .item() returns null (so it acts like appendChild), but [] throws > this.tabContainer.insertBefore(aTab, this.tabs.item(aIndex)); > > for (let i = 0; i < this.tabs.length; i++) { > this.tabs[i]._tPos = i; > - this.tabs[i]._logicallySelected = false; > + this.tabs[i]._selected = false; > - this.tabs[i]._visuallySelected = false; > } The old code here looked like it set these properties to false on the current tab, then went through all the tabs and set the same properties to false. It doesn't seem that the setting the properties to false on the current tab first was necessary. The code then set the properties to true on the current tab. Your new code only changes this by using ._selected instead of the two properties, and removed the unnecessary setting of the properties on the current tab. Can you confirm that setting those properties prior to entering the loop was unnecessary?

Currently it's not only possible to get 2 visuallyselected="true", but also to get 0 selected tabs > screenshot: https://dl.dropboxusercontent.com/s/mr50ob5bjry2tqu/bug%201157404%20comment%2010.png

(In reply to arni2033 from comment #10) > Currently it's not only possible to get 2 visuallyselected="true", but also > to get 0 selected tabs > > screenshot: https://dl.dropboxusercontent.com/s/mr50ob5bjry2tqu/bug%201157404%20comment%2010.png Was that using the same STR somehow? If not, this should perhaps be filed as a separate bug.

(In reply to Mike Conley (:mconley) - Needinfo me! from comment #13) > This sounds separate to me. Can you please file separately? Done. Initially, my plan was: to wait until this bug is resolved, then try to reproduce comment 12, and if it still happens, investigate if there's an easy way to reproduce. Then file it separately.

Just an update, I could not reproduce this issue on the latest Aurora 46.0a2 when using the steps from Comment 1, but I reproduced it when following the steps and the test case from the duplicate bug report (Bug 1239533). Reproducible only when e10s is enabled. Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:46.0) Gecko/20100101 Firefox/46.0 Build ID: 20160210004006

Not blocking, still P1 though.

It appears that my testcase is only reliable when you have at least 1 tab after the tab with testcase I've also found another 100% scenario which leads to wrong tab being visuallyselected, but I won't file it as separate bug yet for several reasons. I'll check it once this bug is fixed.

Apparently this hasn't landed yet.

mconley, is there an update here? When should this land?

Milan, can you ask around to see if this should still land?

Man, I'm so sorry - I'm the bottleneck here. I'll get to this today.

https://reviewboard.mozilla.org/r/32641/#review29677 > The old code here looked like it set these properties to false on the current tab, then went through all the tabs and set the same properties to false. It doesn't seem that the setting the properties to false on the current tab first was necessary. > > The code then set the properties to true on the current tab. > > Your new code only changes this by using ._selected instead of the two properties, and removed the unnecessary setting of the properties on the current tab. > > Can you confirm that setting those properties prior to entering the loop was unnecessary? Yeah, looks like it was a minor oversight in bug 554005.

Comment on attachment 8712777 [details] Bug 1157404 - Don't visuallyselect remote tabs during moveTabTo. Review request updated; see interdiff: https://reviewboard.mozilla.org/r/32641/diff/1-2/

Sorry that took so long!

Pushed by mconley@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/fa8ec70570e6 Don't visuallyselect remote tabs during moveTabTo. r=jaws

I managed to reproduce the issue following the steps and the test case provided from the duplicate bug report (Bug 1239533) as from comment 15. For this, I used Firefox Nightly 48.0a1 (2016-03-14) on Windows 10 Pro x64. The fix is now verified on Firefox Beta 50.0b4 across platforms: Windows 10 Pro x64, Ubuntu 16.04 LTS x64 and Mac OS Sierra 10.12.1