Closed Bug 1157404 Opened 9 years ago Closed 8 years ago

[e10s] Possible to end up with two visuallyselected="true" tabs at the same time

Categories

(Firefox :: Tabbed Browser, defect, P1)

Product:
Firefox
Component:
Tabbed Browser
Type:
defect

Tracking

()

Status:
VERIFIED FIXED
Milestone:
Firefox 50
Tracking Flags:
Tracking Status
e10s + ---
firefox40 --- affected
firefox50 --- verified

People

(Reporter: mstange, Assigned: mconley)

References

(Depends on 1 open bug)

Details

Attachments

(3 files)

Proposed patch, changes in gBrowser.moveTabTo
1.49 KB, text/plain
Details
Bug 1157404 - Don't visuallyselect remote tabs during moveTabTo.
58 bytes, text/x-review-board-request
jaws
: review+
Details
testcase 1 - [e10s] It's possible to get 2 visually selected tabs.html
265 bytes, text/html
Details 
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?"
Flags: needinfo?(jgriffiths)
(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.
Flags: needinfo?(jgriffiths)
Re-nomming based on comment 5.
tracking-e10s: + → ?
tracking-e10s: ? → +
Flags: needinfo?(mconley)
Thanks tabmix.onemen! I used your patch as a starting point to fix this - very helpful!
Flags: needinfo?(mconley)
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/
Attachment #8712777 - Flags: review?(jaws)
Assignee: nobody → mconley
Status: NEW → ASSIGNED
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?
Attachment #8712777 - Flags: review?(jaws) → review+
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.
Flags: needinfo?(arni2033)
Flags: needinfo?(arni2033)
(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.
Flags: needinfo?(arni2033)
See Also: → 1244317
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
Flags: needinfo?(jgriffiths)
Not blocking, still P1 though.
tracking-e10s: + → ---
Flags: needinfo?(jgriffiths)
Priority: -- → P1
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.
Flags: needinfo?(mconley)
mconley, is there an update here? When should this land?
Flags: needinfo?(mconley)
Flags: needinfo?(mconley)
Milan, can you ask around to see if this should still land?
Flags: needinfo?(milan)
Man, I'm so sorry - I'm the bottleneck here. I'll get to this today.
Flags: needinfo?(milan)
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/
Attachment #8712777 - Attachment description: MozReview Request: Bug 1157404 - Don't visuallyselect remote tabs during moveTabTo. r?jaws → Bug 1157404 - Don't visuallyselect remote tabs during moveTabTo.
Sorry that took so long!
Flags: needinfo?(mconley)
Pushed by mconley@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/fa8ec70570e6
Don't visuallyselect remote tabs during moveTabTo. r=jaws
https://hg.mozilla.org/mozilla-central/rev/fa8ec70570e6
Status: ASSIGNED → RESOLVED
Closed: 8 years ago
status-firefox50: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 50
Flags: qe-verify+
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
Status: RESOLVED → VERIFIED
status-firefox50: fixed → verified
Flags: qe-verify+
Depends on: 1327968
Depends on: 1353769
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: