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

VERIFIED FIXED in Firefox 50

Status

()

Firefox
Tabbed Browser
P1
normal
VERIFIED FIXED
2 years ago
2 months ago

People

(Reporter: mstange, Assigned: mconley)

Tracking

(Depends on: 2 bugs)

Trunk
Firefox 50
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(e10s+, firefox40 affected, firefox50 verified)

Details

MozReview Requests

()

Submitter Diff Changes Open Issues Last Updated
Loading...
Error loading review requests:

Attachments

(3 attachments)

(Reporter)

Description

2 years ago
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.
(Reporter)

Comment 1

2 years ago
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.
tracking-e10s: --- → ?
tracking-e10s: ? → +

Comment 2

2 years ago
Created attachment 8634669 [details]
Proposed patch, changes in gBrowser.moveTabTo

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();
(Assignee)

Updated

a year ago
Duplicate of this bug: 1239533
(Assignee)

Comment 4

a year ago
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)
(Assignee)

Comment 6

a year ago
Re-nomming based on comment 5.
tracking-e10s: + → ?
(Assignee)

Updated

a year ago
tracking-e10s: ? → +
Flags: needinfo?(mconley)
(Assignee)

Comment 7

a year ago
Thanks tabmix.onemen! I used your patch as a starting point to fix this - very helpful!
Flags: needinfo?(mconley)
(Assignee)

Comment 8

a year ago
Created attachment 8712777 [details]
Bug 1157404 - Don't visuallyselect remote tabs during moveTabTo.

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+

Comment 10

a year ago
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
(Assignee)

Comment 11

a year ago
(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)
Comment hidden (obsolete)
Comment hidden (obsolete)
(Assignee)

Updated

a year ago
Flags: needinfo?(arni2033)

Comment 14

a year ago
(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: → bug 1244317
Created attachment 8717896 [details]
testcase 1 - [e10s] It's possible to get 2 visually selected tabs.html

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
tracking-e10s: --- → +

Comment 17

a year ago
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.
(Assignee)

Comment 18

a year ago
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)
(Assignee)

Comment 21

10 months ago
Man, I'm so sorry - I'm the bottleneck here. I'll get to this today.
Flags: needinfo?(milan)
(Assignee)

Comment 22

10 months ago
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.
(Assignee)

Comment 23

10 months ago
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.
(Assignee)

Comment 24

10 months ago
Sorry that took so long!
Flags: needinfo?(mconley)

Comment 25

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

Comment 26

10 months ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/fa8ec70570e6
Status: ASSIGNED → RESOLVED
Last Resolved: 10 months ago
status-firefox50: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 50
Flags: qe-verify+

Comment 27

8 months ago
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+

Updated

7 months ago
Duplicate of this bug: 1244317

Updated

5 months ago
Depends on: 1327968

Updated

2 months ago
Depends on: 1353769
You need to log in before you can comment on or make changes to this bug.