1457355 - New tab overwrites X11/Linux primary selection when location bar text is highlighted

Status: RESOLVED FIXED

(Firefox :: Address Bar, defect, P3)

Description

[Karl Tomlinson (:karlt back Feb 10)]

1. Run Firefox 60.0b14 in a new profile and load (for example)
   https://www.mozilla.org/en-US/firefox/nightly/firstrun/
2. Select "mozilla" in URL bar.
3. Click "Open a new tab" button.
4. Close new tab.
   Location bar text is highlighted (a variation of bug 1393059).
5. Click "Open a new tab" button.
6. Click middle mouse button to insert primary selection,
   in location bar or elsewhere.

Expected new text: "mozilla".
Actual new text: "https://www.mozilla.org/en-US/firefox/nightly/firstrun/"

Regression from https://hg.mozilla.org/integration/autoland/rev/562966f195b0

Comment 1

[Karl Tomlinson (:karlt back Feb 10)]

Slightly different steps are required to reproduce on recent Nightlies, due to
a different variation of bug 1393059.  These steps reproduce on both Nightly
and 60.0b14:

1. Run browser in a new profile and load (if not already shown)
   https://www.mozilla.org/en-US/firefox/nightly/firstrun/
2. Select "mozilla" in URL bar.
3. (not required)
4. Select text in another app. e.g. terminal, or another web browser.
5. Click "Open a new tab" button.
6. Click middle mouse button to insert primary selection,
   in location bar or elsewhere.

Expected new text: (from other app in step 3)
Actual new text: "mozilla".

Comment 2

[Dão Gottwald [:dao]]

No clue how bug 1442651 would have caused this, but this sounds like an Address Bar bug. FWIW, we have code that explicitly writes to the primary selection here:

https://searchfox.org/mozilla-central/rev/78dbe34925f04975f16cb9a5d4938be714d41897/browser/base/content/urlbarBindings.xml#1732-1750

Maybe the isHandlingUserInput check is broken?

Comment 5

[Karl Tomlinson (:karlt back Feb 10)]

This reproduces reliably on new tab.
It sometimes reproduces even when switching to an existing new tab.
It doesn't seem to happen when opening about:preferences or about:addons in a new tab.

The unexpected modification to the primary selection on new tab happens with this call stack on revision adc59d50adf8:

0 _on_select(event = [object Event]) ["resource:///modules/UrlbarInput.jsm":1425:20]
this = [object Object]
1 handleEvent(event = [object Event]) ["resource:///modules/UrlbarInput.jsm":333:22]
this = [object Object]
2 select() ["chrome://global/content/bindings/textbox.xml":107:26]
this = [object XULElement]
3 UrlbarInput/thismethod ["resource:///modules/UrlbarInput.jsm":115:35]
4 focusAndSelectUrlBar() ["chrome://browser/content/browser.js":2428:10]
this = [object ChromeWindow]
5 _adjustFocusAfterTabSwitch(newTab = [object XULElement]) ["chrome://browser/content/tabbrowser.js":1186:8]
this = [object Object]
6 updateDisplay() ["resource:///modules/AsyncTabSwitcher.jsm":440:26]
this = [object Object]
7 postActions() ["resource:///modules/AsyncTabSwitcher.jsm":618:9]
this = [object Object]
8 queueUnload(unloadTimeout = 300) ["resource:///modules/AsyncTabSwitcher.jsm":1026:9]
this = [object Object]
9 requestTab(tab = [object XULElement]) ["resource:///modules/AsyncTabSwitcher.jsm":1015:9]
this = [object Object]
10 updateCurrentBrowser() ["chrome://browser/content/tabbrowser.js":917:28]
this = [object Object]
11 _setupEventListeners/<(event = [object Event]) ["chrome://browser/content/tabbrowser.js":4509:13]
12 set selectedIndex(val = 1) ["chrome://global/content/elements/tabbox.js":193:11]
this = [object XULElement]
13 set selectedPanel(val = [object XULElement]) ["chrome://global/content/elements/tabbox.js":207:4]
this = [object XULElement]
14 set selectedIndex(val = 1) ["chrome://global/content/elements/tabbox.js":532:8]
this = [object XULElement]
15 set selectedItem(val = [object XULElement]) ["chrome://global/content/elements/tabbox.js":551:32]
this = [object XULElement]
16 set selectedTab(val = [object XULElement]) ["chrome://global/content/elements/tabbox.js":86:8]
this = [object XULElement]
17 set selectedTab(val = [object XULElement]) ["chrome://browser/content/tabbrowser.js":261:4]
this = [object Object]
18 loadOneTab(aURI = "about:newtab"

_gBrowser.updateCurrentBrowser calls requestTab before setting _selectedTab and "Update the URL bar".

The location bar contents are subsequently updated at

0 _setValue(val = "", allowTrim = true) ["resource:///modules/UrlbarInput.jsm":767:4]
<failed to get 'this' value>
1 set value(val = "") ["resource:///modules/UrlbarInput.jsm":749:16]
<failed to get 'this' value>
2 URLBarSetURI( <Failed to get argument while inspecting stack frame>
updatePopupNotifications = true) ["chrome://browser/content/browser.js":2847:2]
<failed to get 'this' value>
3 onLocationChange( <Failed to get argument while inspecting stack frame>
aRequest = null <Failed to get argument while inspecting stack frame>
, aFlags = 0, aIsSimulated = true) ["chrome://browser/content/browser.js":4895:6]
<failed to get 'this' value>
4 callListeners( <Failed to get argument while inspecting stack frame>
<Failed to get argument while inspecting stack frame>
) ["chrome://browser/content/tabbrowser.js":742:28]
<failed to get 'this' value>
5 _callProgressListeners(aBrowser = null, aMethod = "onLocationChange" <Failed to get argument while inspecting stack frame>
, aCallGlobalListeners = true, aCallTabsListeners = false) ["chrome://browser/content/tabbrowser.js":755:6]
<failed to get 'this' value>
6 updateCurrentBrowser() ["chrome://browser/content/tabbrowser.js":965:9]
<failed to get 'this' value>

This ordering dates back to at least "Tab switch refactoring".
https://hg.mozilla.org/mozilla-central/rev/564a9e11d9ec89b0d9a6e2ea7e93cd531c9a468d
I don't know whether this was the ordering at the time of https://hg.mozilla.org/integration/autoland/rev/562966f195b0 triggering this regression. I haven't identified how that triggered this regression.

Comment 6

[Karl Tomlinson (:karlt back Feb 10)]

The intention of UrlbarInput._on_select() is to adjust the primary after it has been set (differently) by the editor. The editor does not set the primary during HTMLInputElement::Select() as used by focusAndSelectUrlBar(), and similarly _on_select() should not adjust the primary in this situation.

I also investigated changing focusAndSelectUrlBar() to merely focus and not select the urlbar, but selection is expected at least on keyboard shortcuts to directly focus the location bar.

Comment 7

[Karl Tomlinson (:karlt back Feb 10)]

Attachment: bug 1457355 don't change primary selection during UrlbarInput.select() r?dao

This suppresses modification of the primary selection when opening a new tab
as well as when a keyboard shortcut is used to directly focus the location
bar.

Comment 8

[Karl Tomlinson (:karlt back Feb 10)]

Attachment: Bug 1457355 test that primary selection is unaffected by opening new tab r?dao

Depends on D37353

Comment 9

[Karl Tomlinson (:karlt back Feb 10)]

https://treeherder.mozilla.org/#/jobs?repo=try&tier=1&revision=db89e8caa917fb96e97a8eaf019ed585007afba3

Comment 10

[int-e]

@karlt thanks so much for working on this! I'll be much happier once this is fixed.

Comment 11

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/435cbd9fb896
don't change primary selection during UrlbarInput.select() r=dao
https://hg.mozilla.org/integration/autoland/rev/3cf34ed0ac83
test that primary selection is unaffected by opening new tab r=dao

Comment 12

[Andreea Pavel [:apavel]]

https://hg.mozilla.org/mozilla-central/rev/435cbd9fb896
https://hg.mozilla.org/mozilla-central/rev/3cf34ed0ac83

Comment 13

[Ryan VanderMeulen [:RyanVM]]

Is this something you wanted to nominate for Beta approval or can this ride the trains?

Comment 14

[Karl Tomlinson (:karlt back Feb 10)]

This has existed long enough that I think there is no urgency to uplift, thanks. 69 is also not an ESR so no benefit there.
There would be a small risk to uplift with some of the refactoring that has been happening, and so safer to ride trains.