1523946 - Opening a new tab sometimes causes both the new tab and the old tab to show up as selected in win10 tablet mode

:Gijs (he/him)

Reporter

Description

•

5 years ago

(Requires a win10 touch-enabled device)

STR:

open firefox
switch to tablet mode
touch 'new tab' button on the tabstrip

ER:
only 1 tab ever looks selected

AR:
sometimes 2 tabs show up as selected.

I'm fairy sure this is a regression. Not sure what from.

:Gijs (he/him)

Reporter

Comment 1

•

5 years ago

Not arm64-specific but given how many of these devices (as currently available) are touch-enabled and/or support use as a tablet/tent (without keyboard), let's track this for the arm64 stuff.

Blocks: arm64-windows-bugs

Dão Gottwald [:dao]

Comment 2

•

5 years ago

Could you please post a screenshot?

Flags: needinfo?(gijskruitbosch+bugs)

:Gijs (he/him)

Reporter

Comment 3

•

5 years ago

Attached image Screenshot — Details

Flags: needinfo?(gijskruitbosch+bugs)

Dão Gottwald [:dao]

Updated

•

5 years ago

Priority: -- → P2

:Gijs (he/him)

Reporter

Comment 4

•

5 years ago

This also happens when using the keyboard (which, granted, is a bit of an edgecase when using tablet mode) and mouse.

Seems specific to tablet mode though, no idea why atm.

Summary: Opening a new tab using touch sometimes causes both the new tab and the old tab to show up as selected in win10 tablet mode → Opening a new tab sometimes causes both the new tab and the old tab to show up as selected in win10 tablet mode

Dão Gottwald [:dao]

Comment 5

•

5 years ago

It really shouldn't be possible for two tab to be selected at the same time. I suspect there's a bug somewhere in the async tab switcher at least contributing to whatever is going on here.

Blocks: async-tab-switcher

:Gijs (he/him)

Reporter

Comment 6

•

5 years ago

mozregression suggests this broke between 2018-08-20 and 2018-08-21 (specifically, https://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=d0d2e0f4b33c&tochange=7c96ad3ab673 ) which looks like it's probably bug 1478112...

Blocks: 1478112

Dão Gottwald [:dao]

Updated

•

5 years ago

status-firefox65: ? → wontfix

status-firefox-esr60: ? → unaffected

Keywords: regressionwindow-wanted

:Gijs (he/him)

Reporter

Comment 7

•

5 years ago

OK, I think I've figured out what's going on here and why it's intermittent.

Basically, the tab switch code calls focusAndSelectUrlBar, which calls select() in the URL bar, which apparently still, in some cases (maybe depending on tablet mode because of the touch keyboard / IME integration?) causes some kind of layout flush. This seems to sometimes send MozAfterPaint events (unsure why, tbh), which causes the tab switcher to call postActions, which calls updateDisplay, which alters lastVisibleTab to the now visible tab, which means that when we then try to remove visuallySelected from the previously visible tab, we remove it from the now-visible tab (shortly before re-setting it), and never end up removing it from the actually previously selected tab.

I think these types of re-entrancy are supposed to not happen anymore. Mike, any idea what the best fix here would be?

Flags: needinfo?(mconley)

Dão Gottwald [:dao]

Updated

•

5 years ago

Comment 8

•

5 years ago

Yeah, this code is supposed to prevent the kind of re-entrancy you're describing:

https://searchfox.org/mozilla-central/rev/03ebbdab952409640c6857d835d3040bf6f9e2db/browser/modules/AsyncTabSwitcher.jsm#1029-1068

So if updateDisplay somehow causes a MozAfterPaint to fire, I would expect it to not enter updateDisplay again until the original updateDisplay exits (and we process the event off of the setTimeout(0) timer).

(In reply to :Gijs (he/him) from comment #7)

OK, I think I've figured out what's going on here and why it's intermittent.

The re-entrancy guard looks solid to me... did you see re-entrancy by logging, or is this supposition?

Flags: needinfo?(mconley) → needinfo?(gijskruitbosch+bugs)

:Gijs (he/him)

Reporter

Comment 9

•

5 years ago

•

Edited

(In reply to Mike Conley (:mconley) (:⚙️) from comment #8)

Yeah, this code is supposed to prevent the kind of re-entrancy you're describing:

https://searchfox.org/mozilla-central/rev/03ebbdab952409640c6857d835d3040bf6f9e2db/browser/modules/AsyncTabSwitcher.jsm#1029-1068

So if updateDisplay somehow causes a MozAfterPaint to fire, I would expect it to not enter updateDisplay again until the original updateDisplay exits (and we process the event off of the setTimeout(0) timer).

(In reply to :Gijs (he/him) from comment #7)

OK, I think I've figured out what's going on here and why it's intermittent.

The re-entrancy guard looks solid to me... did you see re-entrancy by logging, or is this supposition?

Logging:

START
Last at contruction: xkcd: Error Bars AsyncTabSwitcher.jsm:87
Initial tab is loaded?: true
requestTab 1(about:newtab) 0:VR(AR)(+) 1:(R)(+) cached: 0
Tab should be blank: false
Requested tab is remote?: true
Switch to tab 1 - 1(about:newtab)
MozLayerTreeReady AsyncTabSwitcher.jsm:1047
onLayersReady(1, true) 0:V(AR)(+) 1:R(R)(+) cached: 0
Tab should be blank: false
Requested tab is remote?: true
Last now: xkcd: Error Bars AsyncTabSwitcher.jsm:457
DEBUG: tab switch time = 117
done 0:(AR)(+) 1:VR(R)(+) cached: 0
MozAfterPaint AsyncTabSwitcher.jsm:1047
DEBUG: tab switch time including compositing = 130
Tab should be blank: false
Requested tab is remote?: true
Last now: New Tab AsyncTabSwitcher.jsm:457
    updateDisplay resource:///modules/AsyncTabSwitcher.jsm:457
    postActions resource:///modules/AsyncTabSwitcher.jsm:623
    handleEvent resource:///modules/AsyncTabSwitcher.jsm:1073
    select chrome://global/content/bindings/textbox.xml:109
    focusAndSelectUrlBar chrome://browser/content/browser.js:2251
    _adjustFocusAfterTabSwitch chrome://browser/content/tabbrowser.js:1186
    updateDisplay resource:///modules/AsyncTabSwitcher.jsm:443
    postActions resource:///modules/AsyncTabSwitcher.jsm:623
    queueUnload resource:///modules/AsyncTabSwitcher.jsm:1031
    requestTab resource:///modules/AsyncTabSwitcher.jsm:1020
    updateCurrentBrowser chrome://browser/content/tabbrowser.js:917
    _setupEventListeners chrome://browser/content/tabbrowser.js:4551
    set selectedIndex chrome://global/content/elements/tabbox.js:201
    set selectedPanel chrome://global/content/elements/tabbox.js:215
    set_selectedIndex chrome://global/content/bindings/tabbox.xml:176
    set_selectedItem chrome://global/content/bindings/tabbox.xml:201
    set selectedTab chrome://global/content/elements/tabbox.js:80
    set selectedTab chrome://browser/content/tabbrowser.js:267
    loadOneTab chrome://browser/content/tabbrowser.js:1471
    openLinkIn chrome://browser/content/utilityOverlay.js:535
    openUILinkIn chrome://browser/content/utilityOverlay.js:260
    openTrustedLinkIn chrome://browser/content/utilityOverlay.js:190
    wrappedJSObject chrome://browser/content/browser.js:2306
    BrowserOpenTab chrome://browser/content/browser.js:2305
    oncommand chrome://browser/content/browser.xul:1
done 0:(AR)(+) 1:VR(R)(+) cached: 0
Set requested tab docshell to active and preserveLayers to false 0:(AR)(+) 1:VR(AR)(+) cached: 0
last visible tab: New Tab AsyncTabSwitcher.jsm:449
Removing visuallyselected from tab New Tab tabbrowser.xml:1974
Setting visuallyselected from tab New Tab tabbrowser.xml:1974

Looks to me like, while we prevented re-entrancy through handleEvent, we didn't prevent it if updateDisplay is called through postActions from somewhere other than handleEvent (in this case, queueUnload).

Does that make more sense?

Flags: needinfo?(gijskruitbosch+bugs) → needinfo?(mconley)

Mike Conley (:mconley) (:⚙️)

Assignee

Comment 10

•

5 years ago

It makes perfect sense, thanks.

I wonder then if the handleEvent re-entrancy guard should actually be set inside of preActions, and reset in postActions.

I can try that.

Assignee: nobody → mconley

Flags: needinfo?(mconley)

Mike Conley (:mconley) (:⚙️)

Assignee

Comment 11

•

5 years ago

Try push: https://treeherder.mozilla.org/#/jobs?repo=try&revision=52d66928259513c23883a532e646a0a0a05196c7

Mike Conley (:mconley) (:⚙️)

Assignee

Comment 12

•

5 years ago

Attached file Bug 1523946 - More carefully avoid re-entrancy in AsyncTabSwitcher. r?dthayer — Details

Mike Conley (:mconley) (:⚙️)

Assignee

Comment 13

•

5 years ago

Hey Gijs, does this patch fix the bug for you?

Flags: needinfo?(gijskruitbosch+bugs)

:Gijs (he/him)

Reporter

Comment 14

•

5 years ago

(In reply to Mike Conley (:mconley) (:⚙️) from comment #13)

Hey Gijs, does this patch fix the bug for you?

It's made it harder to reproduce, but I still see it occasionally. I also see no tabs being selected now... will try and debug this some tomorrow.

:Gijs (he/him)

Reporter

Comment 15

•

5 years ago

(In reply to :Gijs (he/him) from comment #14)

(In reply to Mike Conley (:mconley) (:⚙️) from comment #13)

Hey Gijs, does this patch fix the bug for you?

It's made it harder to reproduce, but I still see it occasionally. I also see no tabs being selected now... will try and debug this some tomorrow.

I'm struggling to come up with clear STR here. What I can say is that I still see some kind of re-entrancy (see below).

Error: TelemetryStopwatch: key "FX_TAB_SWITCH_UPDATE_MS" was already initialized tabbrowser.js:913:26
    updateCurrentBrowser chrome://browser/content/tabbrowser.js:913
    _setupEventListeners chrome://browser/content/tabbrowser.js:4573
    set selectedIndex chrome://global/content/elements/tabbox.js:202
    set selectedPanel chrome://global/content/elements/tabbox.js:216
    set_selectedIndex chrome://global/content/bindings/tabbox.xml:176
    set_selectedItem chrome://global/content/bindings/tabbox.xml:201
    set selectedTab chrome://global/content/elements/tabbox.js:81
    set selectedTab chrome://browser/content/tabbrowser.js:266
    loadOneTab chrome://browser/content/tabbrowser.js:1470
    openLinkIn chrome://browser/content/utilityOverlay.js:538
    openUILinkIn chrome://browser/content/utilityOverlay.js:260
    openTrustedLinkIn chrome://browser/content/utilityOverlay.js:190
    wrappedJSObject chrome://browser/content/browser.js:2337
    BrowserOpenTab chrome://browser/content/browser.js:2336
    oncommand chrome://browser/content/browser.xul:1
    select chrome://global/content/bindings/textbox.xml:109
    focusAndSelectUrlBar chrome://browser/content/browser.js:2282
    _adjustFocusAfterTabSwitch chrome://browser/content/tabbrowser.js:1185
    updateDisplay resource:///modules/AsyncTabSwitcher.jsm:440
    postActions resource:///modules/AsyncTabSwitcher.jsm:621
    queueUnload resource:///modules/AsyncTabSwitcher.jsm:1030
    requestTab resource:///modules/AsyncTabSwitcher.jsm:1019
    updateCurrentBrowser chrome://browser/content/tabbrowser.js:916
    _setupEventListeners chrome://browser/content/tabbrowser.js:4573
    set selectedIndex chrome://global/content/elements/tabbox.js:202
    set selectedPanel chrome://global/content/elements/tabbox.js:216
    set_selectedIndex chrome://global/content/bindings/tabbox.xml:176
    set_selectedItem chrome://global/content/bindings/tabbox.xml:201
    set selectedTab chrome://global/content/elements/tabbox.js:81
    set selectedTab chrome://browser/content/tabbrowser.js:266
    loadOneTab chrome://browser/content/tabbrowser.js:1470
    openLinkIn chrome://browser/content/utilityOverlay.js:538
    openUILinkIn chrome://browser/content/utilityOverlay.js:260
    openTrustedLinkIn chrome://browser/content/utilityOverlay.js:190
    wrappedJSObject chrome://browser/content/browser.js:2337
    BrowserOpenTab chrome://browser/content/browser.js:2336
    oncommand chrome://browser/content/browser.xul:1
Error: TelemetryStopwatch: requesting elapsed time for nonexisting stopwatch. Histogram: "FX_TAB_SWITCH_UPDATE_MS", key: "" tabbrowser.js:1110:26
    updateCurrentBrowser chrome://browser/content/tabbrowser.js:1110
    _setupEventListeners chrome://browser/content/tabbrowser.js:4573
    set selectedIndex chrome://global/content/elements/tabbox.js:202
    set selectedPanel chrome://global/content/elements/tabbox.js:216
    set_selectedIndex chrome://global/content/bindings/tabbox.xml:176
    set_selectedItem chrome://global/content/bindings/tabbox.xml:201
    set selectedTab chrome://global/content/elements/tabbox.js:81
    set selectedTab chrome://browser/content/tabbrowser.js:266
    loadOneTab chrome://browser/content/tabbrowser.js:1470
    openLinkIn chrome://browser/content/utilityOverlay.js:538
    openUILinkIn chrome://browser/content/utilityOverlay.js:260
    openTrustedLinkIn chrome://browser/content/utilityOverlay.js:190
    wrappedJSObject chrome://browser/content/browser.js:2337
    BrowserOpenTab chrome://browser/content/browser.js:2336
    oncommand chrome://browser/content/browser.xul:1
    select chrome://global/content/bindings/textbox.xml:109
    focusAndSelectUrlBar chrome://browser/content/browser.js:2282
    _adjustFocusAfterTabSwitch chrome://browser/content/tabbrowser.js:1185
    updateDisplay resource:///modules/AsyncTabSwitcher.jsm:440
    postActions resource:///modules/AsyncTabSwitcher.jsm:621
    queueUnload resource:///modules/AsyncTabSwitcher.jsm:1030
    requestTab resource:///modules/AsyncTabSwitcher.jsm:1019
    updateCurrentBrowser chrome://browser/content/tabbrowser.js:916
    _setupEventListeners chrome://browser/content/tabbrowser.js:4573
    set selectedIndex chrome://global/content/elements/tabbox.js:202
    set selectedPanel chrome://global/content/elements/tabbox.js:216
    set_selectedIndex chrome://global/content/bindings/tabbox.xml:176
    set_selectedItem chrome://global/content/bindings/tabbox.xml:201
    set selectedTab chrome://global/content/elements/tabbox.js:81
    set selectedTab chrome://browser/content/tabbrowser.js:266
    loadOneTab chrome://browser/content/tabbrowser.js:1470
    openLinkIn chrome://browser/content/utilityOverlay.js:538
    openUILinkIn chrome://browser/content/utilityOverlay.js:260
    openTrustedLinkIn chrome://browser/content/utilityOverlay.js:190
    wrappedJSObject chrome://browser/content/browser.js:2337
    BrowserOpenTab chrome://browser/content/browser.js:2336
    oncommand chrome://browser/content/browser.xul:1
Error: TelemetryStopwatch: requesting elapsed time for nonexisting stopwatch. Histogram: "FX_TAB_SWITCH_UPDATE_MS", key: "" tabbrowser.js:1110:26
    updateCurrentBrowser chrome://browser/content/tabbrowser.js:1110
    _setupEventListeners chrome://browser/content/tabbrowser.js:4573
    set selectedIndex chrome://global/content/elements/tabbox.js:202
    set selectedPanel chrome://global/content/elements/tabbox.js:216
    set_selectedIndex chrome://global/content/bindings/tabbox.xml:176
    set_selectedItem chrome://global/content/bindings/tabbox.xml:201
    set selectedTab chrome://global/content/elements/tabbox.js:81
    set selectedTab chrome://browser/content/tabbrowser.js:266
    loadOneTab chrome://browser/content/tabbrowser.js:1470
    openLinkIn chrome://browser/content/utilityOverlay.js:538
    openUILinkIn chrome://browser/content/utilityOverlay.js:260
    openTrustedLinkIn chrome://browser/content/utilityOverlay.js:190
    wrappedJSObject chrome://browser/content/browser.js:2337
    BrowserOpenTab chrome://browser/content/browser.js:2336
    oncommand chrome://browser/content/browser.xul:1

It also seems to now be possible for the switcher's requestedTab to have no linkedBrowser and/or other properties are missing, causing exceptions in various bits of code:

TypeError: initialBrowser is null AsyncTabSwitcher.jsm:151:23
    AsyncTabSwitcher resource:///modules/AsyncTabSwitcher.jsm:151
    _getSwitcher chrome://browser/content/tabbrowser.js:4205
    warmupTab chrome://browser/content/tabbrowser.js:4212
    _mouseenter chrome://browser/content/tabbrowser.xml:2193
    onxblmouseover chrome://browser/content/tabbrowser.xml:2328

15:36:46.930 TypeError: aURI is null
tabbrowser.js:770:9
    isLocalAboutURI chrome://browser/content/tabbrowser.js:770
    updateDisplay resource:///modules/AsyncTabSwitcher.jsm:355
    postActions resource:///modules/AsyncTabSwitcher.jsm:621
    onTabRemoved resource:///modules/AsyncTabSwitcher.jsm:800
    _endRemoveTab chrome://browser/content/tabbrowser.js:3176
    onxbltransitionend chrome://browser/content/tabbrowser.xml:1262
15:37:10.591
TypeError: oldTab.updateLastAccessed is not a function tabbrowser.js:990:14
    updateCurrentBrowser chrome://browser/content/tabbrowser.js:990
    _setupEventListeners chrome://browser/content/tabbrowser.js:4573
    set selectedIndex chrome://global/content/elements/tabbox.js:202
    set selectedPanel chrome://global/content/elements/tabbox.js:216
    set_selectedIndex chrome://global/content/bindings/tabbox.xml:176
    set_selectedItem chrome://global/content/bindings/tabbox.xml:201
    set selectedTab chrome://global/content/elements/tabbox.js:81
    set selectedTab chrome://browser/content/tabbrowser.js:266
    loadOneTab chrome://browser/content/tabbrowser.js:1470
    openLinkIn chrome://browser/content/utilityOverlay.js:538
    openUILinkIn chrome://browser/content/utilityOverlay.js:260
    openTrustedLinkIn chrome://browser/content/utilityOverlay.js:190
    wrappedJSObject chrome://browser/content/browser.js:2337
    BrowserOpenTab chrome://browser/content/browser.js:2336
    oncommand chrome://browser/content/browser.xul:1

For the first issue, this seems to happen if I press ctrl-t in quick succession. It might not be related to your patch.

The second issue is trickier; it's to do with a mixture of opening/closing tabs quickly, but I haven't found reliable STR yet.

Are these logs any use? I'll see if I can reproduce these issues some more while the tab switcher's logs are also turned on.

Flags: needinfo?(gijskruitbosch+bugs) → needinfo?(mconley)

:Gijs (he/him)

Reporter

Comment 16

•

5 years ago

Attached file More logging — Details

So I found semi-reliable STR on my yoga:

switch to tablet mode using the windows sidebar, but keep the keyboard available
open firefox
open xkcd.com in initial tab.
once loaded, hit ctrl-t 3 times in quick succession, followed by ctrl-w

ER:

the last opened tab is closed

AR:
the first newly opened (so second of 4 total) tabs is closed. Subsequent attempts to close something with the keyboard are no-ops; nothing happens. Trying to use the mouse to close the middle tab trips warmup, which trips the initialBrowser is null error.

Liz Henry (:lizzard) (relman/hg->git project)

Updated

•

5 years ago

status-firefox66: affected → wontfix

Liz Henry (:lizzard) (relman/hg->git project)

Updated

•

5 years ago

No longer blocks: 1478112

status-firefox67: affected → fix-optional

status-firefox68: --- → affected

Regressed by: 1478112

Liz Henry (:lizzard) (relman/hg->git project)

Updated

•

5 years ago

status-firefox67: fix-optional → wontfix

status-firefox68: affected → fix-optional

status-firefox69: --- → affected

Liz Henry (:lizzard) (relman/hg->git project)

Comment 18

•

5 years ago

This is P2 and assigned, so I'm removing it from weekly regression triage by marking it fix-optional.
Happy to take a patch for 70 or in future.

status-firefox68: fix-optional → wontfix

status-firefox69: affected → fix-optional

status-firefox70: --- → fix-optional

BugBot [:suhaib / :marco/ :calixte]

Updated

•

2 years ago

Has Regression Range: --- → yes

BMO Automation

Updated

•

2 years ago

Severity: normal → S3

Screenshot 5 years ago :Gijs (he/him) 11.23 KB, image/png		Details
Bug 1523946 - More carefully avoid re-entrancy in AsyncTabSwitcher. r?dthayer 5 years ago Mike Conley (:mconley) (:⚙️) 47 bytes, text/x-phabricator-request		Details \| Review
More logging 5 years ago :Gijs (he/him) 6.25 KB, text/plain		Details