924069 - completion in urlbar is copied to X primary selection

Status: VERIFIED FIXED

(Firefox :: Address Bar, defect, P1)

Description

[Sebastian Freundt]

User Agent: Mozilla/5.0 (X11; Linux x86_64; rv:27.0) Gecko/20100101 Firefox/27.0 (Beta/Release)
Build ID: 20131006030201

Steps to reproduce:

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

E.g. say http://www.google.com is completeable, type http://www.goo RET


Actual results:

The completed bit of the URL ("gle.com") is now in the X clipboard.


Expected results:

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

Comment 1

[[:Aleksej]]

WFM 2013-09-18-03-02-02-mozilla-central-firefox-27.0a1.en-US.linux-x86_64
bug 2013-09-19-03-02-02-mozilla-central-firefox-27.0a1.en-US.linux-x86_64
bug 2013-10-06-03-02-01-mozilla-central-firefox-27.0a1.en-US.linux-x86_64

https://hg.mozilla.org/mozilla-central/pushloghtml?startdate=2013-09-18&enddate=2013-09-19+03%3A03

Comment 2

[Alice0775 White]

Regression window(m-c)
Good:
http://hg.mozilla.org/mozilla-central/rev/e8bb95435a54
Mozilla/5.0 (X11; Linux i686; rv:27.0) Gecko/20100101 Firefox/27.0 ID:20130918105418
Bad:
http://hg.mozilla.org/mozilla-central/rev/dc09d922d41f
Mozilla/5.0 (X11; Linux i686; rv:27.0) Gecko/20100101 Firefox/27.0 ID:20130918164934
Pushlog:
http://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=e8bb95435a54&tochange=dc09d922d41f


Regression window(m-i)
Good:
http://hg.mozilla.org/integration/mozilla-inbound/rev/9892615a9776
Mozilla/5.0 (X11; Linux i686; rv:27.0) Gecko/20100101 Firefox/27.0 ID:20130917221353
Bad:
http://hg.mozilla.org/integration/mozilla-inbound/rev/ce65d48e182e
Mozilla/5.0 (X11; Linux i686; rv:27.0) Gecko/20100101 Firefox/27.0 ID:20130917223022
Pushlog:
http://hg.mozilla.org/integration/mozilla-inbound/pushloghtml?fromchange=9892615a9776&tochange=ce65d48e182e
Regressed by: 
ce65d48e182e	Andrew Quartey — Bug 850364 - Implement setRangeText for HTMLInputElement. r=ehsan

Comment 3

[Andrew Quartey [:drexler]]

The location bar is a XUL textbox which is an HTML input element underneath. The cause of this is: http://mxr.mozilla.org/mozilla-central/source/content/html/content/src/HTMLInputElement.cpp#4527. When the user presses RET to complete the suggested url, the remainder of the appended url is interpreted as a selection. This is in line with what the spec says about HTMLInputElement.setSelectionRange()...see: http://www.whatwg.org/specs/web-apps/current-work/multipage/association-of-controls-and-forms.html#dom-textarea/input-setselectionrange. 

Although for every "successful" range selection made, a select event is to be fired, are there other cases besides this one where we don't want the select event? In such cases, then a solution might be to check |SetSelectionRange| for success at the call sites and fire the select event if necessary but then again this will be going against the spec. Ehsan?

Comment 4

[(no longer active)]

I don't understand where this bug is coming from quite yet.  Is it because we dispatch the select event?

Comment 5

[Andrew Quartey [:drexler]]

(In reply to :Ehsan Akhgari (needinfo? me!) from comment #4)
> I don't understand where this bug is coming from quite yet.  Is it because
> we dispatch the select event?

Yes.

Comment 7

[Andrew Quartey [:drexler]]

Attachment: fix

This prevents the select event being dispatched if the caller is a chrome element, such as a XUL textbox in this instance. Perhaps, a similar amendment is needed for HTMLTextAreaElement::SetSelectionRange too.

Also, it is interesting to note that Opera and Chrome do not fire select events for |setRangeText| and |setSelectionRange|. A simple adjustment to dump the test results from http://mxr.mozilla.org/mozilla-central/source/content/html/content/test/forms/test_set_range_text.html?force=1 to the console will show this.

Comment 8

[:Gavin Sharp [email: [e-mail]]]

Comment on attachment 815606 [details] [diff] [review]
fix

I don't think you want IsCallerChrome here - you probably rather want to check nsContentUtils::IsChromeDoc on the element's ownerDocument, or something like that?

Comment 10

[Karl Tomlinson (:karlt)]

clipboard is unaffected (unless there is a utility running to set the clipboard to the primary selection).

Comment 11

[Andrew Quartey [:drexler]]

Attachment: fix v2

switched nsContentUtils::IsCallerChrome to nsContentUtils::IsChromeDoc, on Gavin's suggestion.

Comment 12

[Karl Tomlinson (:karlt)]

I'm not clear why Chrome should be special here.  If a web page were to implement something similar, wouldn't it also use similar methods?  And we still don't want the X primary to be changed.

I don't know the purpose of the "select" event from setSelectionRange().
I wonder whether that is conflicting with an existing event, with a listener that sets the primary?

Comment 14

[(no longer active)]

Comment on attachment 815686 [details] [diff] [review]
fix v2

Review of attachment 815686 [details] [diff] [review]:
-----------------------------------------------------------------

We should definitely not special case chrome documents like this.

Can you please attach a C++ and Javascript call stack to the SetSelectionRange call which triggers this bug?  I'd like to understand the conditions of this bug a bit better.

Comment 15

[Andrew Quartey [:drexler]]

> Can you please attach a C++ and Javascript call stack to the
> SetSelectionRange call which triggers this bug?  I'd like to understand the
> conditions of this bug a bit better.

I'm currently reconfiguring my system so will take day or two to get to this.

Comment 16

[Andrew Quartey [:drexler]]

Attachment: Call Stack

Comment 17

[(no longer active)]

Can you please provide a js callstack as well?  I basically want to know what our XBL bindings are doing to trigger this...

Comment 18

[Blake Kaplan (:mrbkap) (inactive)]

Attachment: JS backtrace

This should be the JS stack that results in the C++ backtrace found in attachment 818850 [details].

Comment 19

[(no longer active)]

OK, I finally found where this string is copied to the clipboard: <http://mxr.mozilla.org/mozilla-central/source/browser/base/content/urlbarBindings.xml#757>

It is indeed done from a "select" event handler.  Now, my ideal fix would be to get rid of this "select" handler altogether, and let the selection controller code takes care of this.  This code was added back in bug 440075, but the reasons why that was done may not exist any more.

Dao, what do you think?

Comment 20

[:Gavin Sharp [email: [e-mail]]]

(In reply to :Ehsan Akhgari (needinfo? me!) from comment #19)
> This code was added back in bug 440075, but the reasons why that was done may not
> exist any more.

They still exist - the location bar has custom copying behavior. We don't want the selection copy in the common case to use the default behavior of copying the selected text.

Comment 21

[(no longer active)]

(In reply to comment #20)
> (In reply to :Ehsan Akhgari (needinfo? me!) from comment #19)
> > This code was added back in bug 440075, but the reasons why that was done may not
> > exist any more.
> 
> They still exist - the location bar has custom copying behavior. We don't want
> the selection copy in the common case to use the default behavior of copying
> the selected text.

What are the required behaviors when copying?  I tested things such as escape sequences in URLs and internationalized URLs and they all seemed to work fine with the "select" event handler removed.

Comment 22

[:Gavin Sharp [email: [e-mail]]]

The set of things described at http://hg.mozilla.org/mozilla-central/annotate/c6fc35c53c37/browser/base/content/urlbarBindings.xml#l475

Comment 23

[(no longer active)]

(In reply to comment #22)
> The set of things described at
> http://hg.mozilla.org/mozilla-central/annotate/c6fc35c53c37/browser/base/content/urlbarBindings.xml#l475

OK, can you suggest an alternative solution please?

Comment 24

[Dão Gottwald [:dao]]

Why was this moved from Core to Firefox? I don't see an answer to this question:

(In reply to Karl Tomlinson (:karlt) from comment #12)
> If a web page were to
> implement something similar, wouldn't it also use similar methods?  And we
> still don't want the X primary to be changed.

Comment 25

[(no longer active)]

(In reply to Dão Gottwald [:dao] from comment #24)
> Why was this moved from Core to Firefox?

Because it's a bug in the front-end code in that it incorrectly assumes that the "select" event is only dispatched for user initiated selections.

> I don't see an answer to this
> question:
> 
> (In reply to Karl Tomlinson (:karlt) from comment #12)
> > If a web page were to
> > implement something similar, wouldn't it also use similar methods?  And we
> > still don't want the X primary to be changed.

Define "something similar".  The problem here is that the front-end code calls input.setSelectionRange(), and somewhere else it has a "select" handler which, unaware of that call, tries to copy the selection to the primary selection clipboard.  Now, if a web page relies on the "select" event only coming from the user, then it will break in similar ways.  If and when we find such a web page, we either need to evangelize to the website, or take this discussion back to the spec and try to get everyone to agree to not dispatch the select event for programatic selection changes.  But for now I see no good reason to not dispatch the select event instead of fixing the front-end code.

Comment 26

[:Gavin Sharp [email: [e-mail]]]

(In reply to :Ehsan Akhgari (needinfo? me!) [Away 10/29-11/6] from comment #25)
> But for now I see no good reason to not dispatch the select event instead of fixing
> the front-end code.

What's the advantage to dispatching the select event? I.e. what motivated the change in behavior introduced by bug 850364? "Adhere to the spec" on its own isn't a great answer in the face of bugs like these.

Comment 27

[(no longer active)]

(In reply to comment #26)
> (In reply to :Ehsan Akhgari (needinfo? me!) [Away 10/29-11/6] from comment #25)
> > But for now I see no good reason to not dispatch the select event instead of fixing
> > the front-end code.
> 
> What's the advantage to dispatching the select event? I.e. what motivated the
> change in behavior introduced by bug 850364? "Adhere to the spec" on its own
> isn't a great answer in the face of bugs like these.

Being more consistent with the rest of the HTMLInputEleement APIs for selection.  The old behavior of dispatching the select event only some of the time was indeed very weird, and this is an attempt at fixing that.  Now if we hit a web compat issue with this, then the conversation would be different, but a Firefox front-end compat issue is just not going to be a convincing argument to change the spec here.  ;-)

I agree that this bug is serious, but let's not lose sight of the fact that the front-end code is making incorrect assumptions here.  Can we just fix the front-end code to not do that?

Comment 28

[:Gavin Sharp [email: [e-mail]]]

Fair enough. It really boils down to whether the front end code is likely to be the only code making this assumption. It sounds likely, given that other browsers apparently have this same behavior. I don't have any great ideas, short of a flag set in the binding across calls to setSelectionRange, which is a bit gross...

Comment 29

[(no longer active)]

(In reply to comment #28)
> Fair enough. It really boils down to whether the front end code is likely to be
> the only code making this assumption. It sounds likely, given that other
> browsers apparently have this same behavior.

Chrome and Safari don't currently dispatch the "select" event when you call setSelectionRange(), so I would expect us to be the discoverer of the potential web compat issues here (but that's off topic for this bug.)

> I don't have any great ideas,
> short of a flag set in the binding across calls to setSelectionRange, which is
> a bit gross...

Yeah, I was hoping for something better, but maybe there is no better solution.  (Note that I did take a quick stab at writing such a fix but part of the code lives in autocomplete and the other lives in urlbarBindings.xml, and it's not clear to me how they're supposed to interact.)

Comment 30

[Dão Gottwald [:dao]]

(In reply to :Ehsan Akhgari (needinfo? me!) [Away 10/29-11/6] from comment #27)
> (In reply to comment #26)
> > (In reply to :Ehsan Akhgari (needinfo? me!) [Away 10/29-11/6] from comment #25)
> > > But for now I see no good reason to not dispatch the select event instead of fixing
> > > the front-end code.
> > 
> > What's the advantage to dispatching the select event? I.e. what motivated the
> > change in behavior introduced by bug 850364? "Adhere to the spec" on its own
> > isn't a great answer in the face of bugs like these.
> 
> Being more consistent with the rest of the HTMLInputEleement APIs for
> selection.  The old behavior of dispatching the select event only some of
> the time was indeed very weird, and this is an attempt at fixing that.  Now
> if we hit a web compat issue with this, then the conversation would be
> different, but a Firefox front-end compat issue is just not going to be a
> convincing argument to change the spec here.  ;-)

Well, if parts of the Firefox UI can be regarded as role models for desktop-like applications using web technology, and if the goal is to enable all kinds of desktop-like applications on the web, then us running into such issues seems relevant.

> I agree that this bug is serious, but let's not lose sight of the fact that
> the front-end code is making incorrect assumptions here.

Not quite. It makes assumptions that used to be true, until bug 850364 changed behavior.

Comment 31

[(no longer active)]

(In reply to comment #30)
> (In reply to :Ehsan Akhgari (needinfo? me!) [Away 10/29-11/6] from comment #27)
> > (In reply to comment #26)
> > > (In reply to :Ehsan Akhgari (needinfo? me!) [Away 10/29-11/6] from comment #25)
> > > > But for now I see no good reason to not dispatch the select event instead of fixing
> > > > the front-end code.
> > > 
> > > What's the advantage to dispatching the select event? I.e. what motivated the
> > > change in behavior introduced by bug 850364? "Adhere to the spec" on its own
> > > isn't a great answer in the face of bugs like these.
> > 
> > Being more consistent with the rest of the HTMLInputEleement APIs for
> > selection.  The old behavior of dispatching the select event only some of
> > the time was indeed very weird, and this is an attempt at fixing that.  Now
> > if we hit a web compat issue with this, then the conversation would be
> > different, but a Firefox front-end compat issue is just not going to be a
> > convincing argument to change the spec here.  ;-)
> 
> Well, if parts of the Firefox UI can be regarded as role models for
> desktop-like applications using web technology, and if the goal is to enable
> all kinds of desktop-like applications on the web, then us running into such
> issues seems relevant.

There is nothing here which prevents people from writing desktop-like applications on the web.  Can we please stay on topic here?

> > I agree that this bug is serious, but let's not lose sight of the fact that
> > the front-end code is making incorrect assumptions here.
> 
> Not quite. It makes assumptions that used to be true, until bug 850364 changed
> behavior.

I consider the fact that we were not dispatching "select" before a bug, but sure, I'm more interested in a fix here.  :-)

Comment 32

[Dão Gottwald [:dao]]

(In reply to :Ehsan Akhgari (needinfo? me!) [Away 10/29-11/6] from comment #31)
> There is nothing here which prevents people from writing desktop-like
> applications on the web.

I didn't say "prevent". It's an issue we could work around (as discussed in comment 28 and comment 29) and presumably web content could work around it similarly. However this misses the point that it might be better if the issue wouldn't exist in the first place.

> Can we please stay on topic here?

I'm confused. You said a Firefox front-end compat issue wouldn't matter, I argued against that, and now you're telling me that argument is off-topic?

Comment 33

[(no longer active)]

(In reply to comment #32)
> (In reply to :Ehsan Akhgari (needinfo? me!) [Away 10/29-11/6] from comment #31)
> > There is nothing here which prevents people from writing desktop-like
> > applications on the web.
> 
> I didn't say "prevent". It's an issue we could work around (as discussed in
> comment 28 and comment 29) and presumably web content could work around it
> similarly. However this misses the point that it might be better if the issue
> wouldn't exist in the first place.

Yes, in an ideal world it wouldn't exist in the first place.

> > Can we please stay on topic here?
> 
> I'm confused. You said a Firefox front-end compat issue wouldn't matter, I
> argued against that, and now you're telling me that argument is off-topic?

I did not say that it doesn't matter, I said it's not good enough of a reason to change the HTML spec.  We may reconsider dispatching this event in case we find lots of web compat issue caused by it, but we still have no evidence that such issues exist in practice.

What's the next step here?

Comment 34

[Mats Palmgren (inactive)]

This is an extremely annoying bug that disrupts my workflow several times per day.
Can we please backout the offending patch while this bug is being worked on?  Thanks.

Comment 35

[Andrew Quartey [:drexler]]

The only reason we're having this lengthy discussion is that |input.setRangeText| had to dispatch the select event and we duly moved it to setSelectionRange. Currently, every |input.selection{Start|End} = someValue;| will also dispatch the event in the present form since they depend on setSelectionRange for implementation. The question here then becomes: is this what the spec intended? I suspect there's a valid reason why other browsers skipped this part of the spec.

Also, others have reservations about removing the location bar selection handler and i agree - we might be playing whack-a-mole with future issues because of that. I'd suggest we instead remove the dispatching of the select event temporarily to satisfy users and in the interim remove the handler and move over its responsibilities to the selection controller, in a way the dispatching future events doesnt interfere with it (easier-said-than-done but doable). Is this agreeable, Ehsan?

Comment 36

[(no longer active)]

I still haven't seen any evidence that this is a bigger problem than just this one bug, which should be easy to work around, but I was expecting that the front-end folks can provide a quick workaround which hasn't happened yet.  I'll write a patch to change where the select event is being dispatched for now, since I'm going away and want to resolve this before that.

Comment 37

[(no longer active)]

Attachment: Patch (v1)

I'd like to land this today, so fast reviews are highly appreciated!

Comment 38

[Andrew Quartey [:drexler]]

Comment on attachment 823344 [details] [diff] [review]
Patch (v1)

Review of attachment 823344 [details] [diff] [review]:
-----------------------------------------------------------------

This will need to be changed too: http://mxr.mozilla.org/mozilla-central/source/content/html/content/test/forms/test_set_range_text.html?force=1

Comment 39

[(no longer active)]

(In reply to comment #38)
> Comment on attachment 823344 [details] [diff] [review]
>   --> https://bugzilla.mozilla.org/attachment.cgi?id=823344
> Patch (v1)
> 
> Review of attachment 823344 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> This will need to be changed too:
> http://mxr.mozilla.org/mozilla-central/source/content/html/content/test/forms/test_set_range_text.html?force=1

Why?

Comment 40

[Andrew Quartey [:drexler]]

> Why?

Since SetSelectionRange no longer dispatches the select event, it shouldn't be counted in places like this and others:

 //subcase: selection{Start|End} < end
165       elem.value = "0123456789";
166       elem.setSelectionRange(4, 5);
167       elem.setRangeText("QRST", 2, 9, "preserve");
168       is(elem.value, "01QRST9", msg + ".value == \"01QRST9\"");
169       is(elem.selectionStart, 2, msg + ".selectionStart == 2, with \"preserve\"");
170       is(elem.selectionEnd, 6, msg + ".selectionEnd == 6, with \"preserve\"");
171       expectedNumOfSelectCalls += 2;

Comment 41

[(no longer active)]

You're right!

Comment 42

[(no longer active)]

Attachment: Patch (v2)

Comment 43

[Andrew Quartey [:drexler]]

Comment on attachment 823366 [details] [diff] [review]
Patch (v2)

Review of attachment 823366 [details] [diff] [review]:
-----------------------------------------------------------------

::: content/html/content/test/forms/test_set_range_text.html
@@ +96,5 @@
>        is(elem.selectionStart, 1, msg + ".selectionStart == 1");
>        is(elem.selectionEnd, 4, msg + ".selectionEnd == 4");
>        elem.setRangeText("mnk");
>        is(elem.value, "0mnk6789ABCDEF", msg + ".value == \"0mnk6789ABCDEF\"");
> +      expectedNumOfSelectCalls += 1;

Should be expectedNumOfSelectCalls += 2; here.

Comment 44

[(no longer active)]

Attachment: Patch (v3)

Yeah, the test wasn't resilient to the case where too many "select" events are dispatched.  This patch fixes that too.

Comment 45

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 823388 [details] [diff] [review]
Patch (v3)

So this is explicitly against what HTML spec says.
I think we need to test this on IE too. I'd prefer keeping the consistency
HTML spec has for select events.

Comment 46

[(no longer active)]

FWIW, IE seems to dispatch select on this test case: <http://people.mozilla.org/~eakhgari/selectevent.html>

Comment 47

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 823388 [details] [diff] [review]
Patch (v3)

Ok, so we added the "select" event during FF27, but don't have
time to fix browser chrome... So this is effectively a backout. Sounds ok.
But please file a bug to re-enable select event and fix browser chrome.

Comment 48

[(no longer active)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/0af6a04d5831

Filed bug 931904 as a follow-up to work around this in the front-end code.

Comment 49

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/0af6a04d5831

Comment 50

[bhavana bajaj [:bajaj]]

(In reply to :Ehsan Akhgari (needinfo? me!) [Away 10/29-11/6] from comment #48)
> https://hg.mozilla.org/integration/mozilla-inbound/rev/0af6a04d5831
> 
> Filed bug 931904 as a follow-up to work around this in the front-end code.

Is this ready to be uplifted given its tracking Firefox 27 ?

Comment 52

[(no longer active)]

Comment on attachment 823388 [details] [diff] [review]
Patch (v3)

[Approval Request Comment]
Bug caused by (feature/regressing bug #): Bug 850364
User impact if declined: See comment 0
Testing completed (on m-c, etc.): m-c/locally
Risk to taking this patch (and alternatives if risky): Low risk
String or IDL/UUID changes made by this patch: none

Comment 53

[(no longer active)]

(In reply to comment #50)
> (In reply to :Ehsan Akhgari (needinfo? me!) [Away 10/29-11/6] from comment #48)
> > https://hg.mozilla.org/integration/mozilla-inbound/rev/0af6a04d5831
> > 
> > Filed bug 931904 as a follow-up to work around this in the front-end code.
> 
> Is this ready to be uplifted given its tracking Firefox 27 ?

Yes, sorry this slipped when I was on vacation.

Comment 54

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/40dd183c2a20

Comment 55

[Cornel Ionce [:noni] [Hubs QA]]

Mozilla/5.0 (X11; Linux x86_64; rv:27.0) Gecko/20100101 Firefox/27.0

Verified as fixed on Firefox 27 beta 4 (build ID: 20140109165205).

Comment 56

[u279076]

Cornel, please also verify this in the latest Aurora build.

Comment 57

[Cornel Ionce [:noni] [Hubs QA]]

Mozilla/5.0 (X11; Linux x86_64; rv:27.0) Gecko/20100101 Firefox/27.0

Confirming this is fixed on latest Audora (build ID: 20140115004003).
Marking as verified.