1180080 - completion in urlbar is copied to X primary selection

Status: RESOLVED FIXED

(Firefox :: Address Bar, defect, P1)

Description

[Karl Tomlinson (:karlt)]

+++ This bug was initially created as a clone of Bug #924069 +++

Steps to reproduce:

Type the first bit of an URL in the urlbar for which a completion is available.  Press RET.

E.g. say bugzilla.mozilla.org is completable, type "bug"


Actual results:

The completed bit of the URL ("zilla.mozilla.org") is now in the X clipboard.


Expected results:

The X clipboard's contents should not have been tampered with.

Comment 1

[Karl Tomlinson (:karlt)]

The problem seems to be that urlbarBindings is using a "select" event handler
in an attempt to modify what gets put in the primary selection during
user-directed selection changes.  The modification is for the reasons
described in https://bugzilla.mozilla.org/show_bug.cgi?id=931904#c6 .

In user-directed selection changes, a second string is put in the primary
selection from the select event handler, which runs after
nsAutoCopyListener::NotifySelectionChanged() has put a (possibly not quite as desired) string in the primary selection.  However, the select event is async, and so is received too late to know what is triggering the selection or what was
the selection at the time the select event was queued.

What would work better is if the correct string could be put in the primary
selection during nsAutoCopyListener::NotifySelectionChanged(), but I don't
know whether there is a good way to override what string is fetched there.

What goes wrong during completion is that the same select event handling code
is run during the selection of auto url completion because Bug 981652
introduced select events for non-user-directed events.  Although I assume that
is what triggered this regression, the select event handler already wasn't
really right for the reasons described above.

Bug 931904 attempted to work around this problem using a flag set during
completion to ignore the next select event received.  However, that sometimes
doesn't work because select events are async and so there can be more than one
select event queued and I don't know of a way to identify which select events
are associated with nsAutoCopyListener::NotifySelectionChanged().

Comment 4

[Karl Tomlinson (:karlt)]

I didn't grasp this before, but we now have at least two different kinds of events called "select".

The first kind a synchronous and usually cancellable.  urlbarBindings was designed for these events and works fine with these events.  nsTextInputListener::NotifySelectionChanged() seems as good a notification as any available to override what is put into the primary selection.

Things went wrong with another kind of events added in bug 981652.  These are asynchronous and never cancellable.

Because one kind is synchronous and the other is async these events can get out of order.

However, urlbarBindings need only handle the synchronous (first) kind and only when the event is dispatched during user input.  If we can determine whether the event is received during user input then we can know whether to copyStringToClipboard().
(An event during user input will not be of the async kind.)

Comment 5

[Karl Tomlinson (:karlt)]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=92f094dfbf8d

Comment 6

[Karl Tomlinson (:karlt)]

Attachment: MozReview Request: bug 1180080 back out often ineffective urlbar _ignoreNextSelect from 86fad085d4b1 r?dao

bug 1180080 back out often ineffective urlbar _ignoreNextSelect from 86fad085d4b1 r?dao

Comment 7

[Karl Tomlinson (:karlt)]

Attachment: MozReview Request: bug 1180080 only copy selection to primary when select event received during user input r?dao

bug 1180080 only copy selection to primary when select event received during user input r?dao

Comment 8

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/39f17984c447
https://hg.mozilla.org/integration/mozilla-inbound/rev/4b1053b81975

Comment 9

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/39f17984c447
https://hg.mozilla.org/mozilla-central/rev/4b1053b81975

Comment 10

[Ryan VanderMeulen [:RyanVM]]

Is this something we should consider for ESR38 and/or Beta42 uplift?

Comment 11

[Karl Tomlinson (:karlt)]

This is an awful bug, but it has existed since version 30.
I've been using the patch with 41 and 42 without noticing any problems, but I can't say it is risk free.  The chrome has hidden features that I never use but others will notice if they break.

Particularly at this stage of beta 42, I think this would be a risky change.
If people haven't been pushed away by this bug yet, then I guess they can survive one more release.
This is not the kind of bug fix that would usually be accepted for ESR uplift.

Comment 12

[Ryan VanderMeulen [:RyanVM]]

I suggested it for ESR uplift because of the likely-dupe bug 1215964 and Linux users being more-likely to be on an ESR release.

Comment 13

[Karl Tomlinson (:karlt)]

The reporter in bug 1215964 seems to indicate that it didn't happen in 31.  I don't know what might have changed between 31 and 38 but perhaps a change in history look-ups makes it much worse in 38 than 31 qualifying for "regressions specific to the ESR".