Open Bug 1523946 Opened 6 years ago Updated 2 years ago

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

Categories

(Firefox :: Tabbed Browser, defect, P2)

Product:
Firefox
Component:
Tabbed Browser
Platform:
Desktop
Windows 10
Type:
defect

Tracking

()

Tracking Flags:
Tracking Status
firefox-esr60 --- unaffected
firefox65 --- wontfix
firefox66 --- wontfix
firefox67 --- wontfix
firefox68 --- wontfix
firefox69 --- fix-optional
firefox70 --- fix-optional

People

(Reporter: Gijs, Assigned: mconley, NeedInfo)

References

(Blocks 3 open bugs, Regression)

Details

(Keywords: regression)

Attachments

(3 files)

Screenshot
(deleted), image/png
Details
Bug 1523946 - More carefully avoid re-entrancy in AsyncTabSwitcher. r?dthayer
(deleted), text/x-phabricator-request
Details
More logging
(deleted), text/plain
Details

(Requires a win10 touch-enabled device)

STR:

  1. open firefox
  2. switch to tablet mode
  3. 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.

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

Could you please post a screenshot?

Flags: needinfo?(gijskruitbosch+bugs)
Attached image Screenshot (deleted) —
Flags: needinfo?(gijskruitbosch+bugs)
Priority: -- → P2

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

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

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

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)

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)

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

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)

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

Flags: needinfo?(gijskruitbosch+bugs)

(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.

(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)
Attached file More logging (deleted) —

So I found semi-reliable STR on my yoga:

  1. switch to tablet mode using the windows sidebar, but keep the keyboard available
  2. open firefox
  3. open xkcd.com in initial tab.
  4. 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.

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
Has Regression Range: --- → yes
Severity: normal → S3
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: