Closed
Bug 1020743
Opened 10 years ago
Closed 10 years ago
The text from the find bar is overwriting the system clipboard
Categories
(Core :: Widget: Cocoa, defect)
Tracking
()
VERIFIED
FIXED
mozilla33
People
(Reporter: MattN, Assigned: jaas)
References
Details
(Keywords: regression, reproducible)
Attachments
(1 file)
(deleted),
patch
|
bzbarsky
:
review+
Sylvestre
:
approval-mozilla-aurora+
Sylvestre
:
approval-mozilla-beta+
|
Details | Diff | Splinter Review |
Nightly 32
The normal system clipboard seems to get overwritten by what I use or have used in the find toolbar. I'm tentatively blaming bug 326743 but it could have also been the follow-up from that (bug 978861) or something else.
I haven't found exact STR but this has happened to me dozens of times in the last few days (I think I've been using find-in-page more often recently) but it has been happening for at least a few weeks I think. On the other hand, Paul Rouget thinks it only started in the last few days.
Comment 1•10 years ago
|
||
If anyone has any initial clue, it's likely Mike....
Flags: needinfo?(mdeboer)
Comment 2•10 years ago
|
||
Matt, do you have `accessibility.typeaheadfind.prefillwithselection` set to true by any chance?
Regardless, It should _never_ populate the regular clipboard. Ever.
I did not find any conspicuous change in the clipboard and findbar files recently. It seems I did the most recent ones.
An STR would be awesome, so I can start finding a regression range... because this bug worries me a lot.
Flags: needinfo?(mdeboer) → needinfo?(MattN+bmo)
Reporter | ||
Comment 3•10 years ago
|
||
(In reply to Mike de Boer [:mikedeboer] from comment #2)
> Matt, do you have `accessibility.typeaheadfind.prefillwithselection` set to
> true by any chance?
No, it's at its default value of false.
> Regardless, It should _never_ populate the regular clipboard. Ever.
That's what I thought but both Paul and I think that's what we've been seeing.
> I did not find any conspicuous change in the clipboard and findbar files
> recently. It seems I did the most recent ones.
>
> An STR would be awesome, so I can start finding a regression range...
> because this bug worries me a lot.
I haven't encountered it in the hours since opening this bug so I don't have any STR yet :(
I'm running 10.9.3 btw.
Flags: needinfo?(MattN+bmo)
Comment 4•10 years ago
|
||
To reproduce:
1) Open a web page
2) Highlight and copy some text from that page (command-C)
3) Hit command-F, type whatever you want in the find box
=> The text you initially copied in the clipboard will be erased/replaced by whatever you typed in the find box
Using OS X 10.9.3 and FF 30.0.
Note that in previous versions of FF, I noticed that the find bar was sometimes erased/replaced by the content of the clipboard (without copying the content of the clipboard in there, just by hitting Command-F), so it looks like the behavior is now reversed.
Comment 5•10 years ago
|
||
Actually, I can also reproduce the "reverse" behavior, it's also impacting 30.0. First, make sure the find bar is closed. Highlight some text on a page, hit command-c, then hit command-f, the find bar is already/automatically populated with the content of the clipboard. Could be "normal/expected" behavior, but a bit odd in my opinion.
Comment 7•10 years ago
|
||
(In reply to David Girard from comment #5)
> Actually, I can also reproduce the "reverse" behavior, it's also impacting
> 30.0. First, make sure the find bar is closed. Highlight some text on a
> page, hit command-c, then hit command-f, the find bar is
> already/automatically populated with the content of the clipboard. Could be
> "normal/expected" behavior, but a bit odd in my opinion.
David, can you tell me the value of the `accessibility.typeaheadfind.prefillwithselection` preference, which you can find in the `about:config` page?
Flags: needinfo?(warpdag)
Comment 8•10 years ago
|
||
Here you go: accessibility.typeaheadfind.prefillwithselection;false
Flags: needinfo?(warpdag)
This issue is surfacing on SuMo:
https://support.mozilla.org/questions/1006605
https://support.mozilla.org/questions/1006098
Comment 10•10 years ago
|
||
I have str that seems to reproduce quite reliably:
1) Start a nightly with a clean profile.
2) Load about:buildconfig
3) Select the "Build Machine" text.
4) Cmd-C
5) Hit the '/' key to open typeahead find. Note that this is coming up prefilled with
some text that is NOT "Build Machine" for me.
6) Hit 'x'
7) Cmd-L
8) Cmd-V
In step 8, I see "x" pasted, not "Build Machine".
Comment 11•10 years ago
|
||
And in particular the prefilled text in step 5 is whatever got stuck in the clipboard in step 6 of the previous browser run(!).
Comment 12•10 years ago
|
||
Regression range on nightlies: http://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=d275eebfae04&tochange=b80f7eece913
That's the initial (later backed out) landing of bug 326743.
Comment 13•10 years ago
|
||
Per previous comments, 30 and 31 are also affected.
Reporter | ||
Comment 14•10 years ago
|
||
Mike, I can also reproduce with STR from comment 10 (including Command-F instead of typeahead find for #5). Do you have enough information now?
Flags: needinfo?(mdeboer)
Keywords: reproducible
Comment 15•10 years ago
|
||
OK, so that patch presumably did this bit:
ClipboardHelper.copyStringToClipboard(aSearchString,
Ci.nsIClipboard.kFindClipboard,
this._getWindow().document);
and then we end up in nsClipboard::SetNativeClipboardData. I do see aWhichClipboard == 2 == kFindClipboard, and we set
cocoaPasteboard = [NSPasteboard pasteboardWithName:NSFindPboard];
and so forth. I do not see us entering SetNativeClipboardData again. But doing a Cmd-V after all that ends up pasting the text that the findbar highlighted, not whatever was on the system clipboard before.
Comment 16•10 years ago
|
||
So I did another experiment. I changed the
if (currentKey == NSStringPboardType)
condition in SetNativeClipboardData in the kFindClipboard to "false". This prevents the value from ending up on the system find clipboard (verified by testing what happens when I Cmd-F in Safari), but does NOT change the behavior of my STR.
Gijs' hypothesis is that the problem is on the paste end, not the copy end, and that we're grabbing the wrong value on paste.
Comment 17•10 years ago
|
||
Ah, here we go. nsClipboard::GetNativeClipboardData has a cache. This cache is used to serve up data requests. Its use is guarded on the native pasteboard not having changed since we last changed it, and this is implemented by caching change counts in mChangeCountFind and mChangeCountGeneral.
The problem is that we're comparing our cached count to the actual count of the native pasteboard, like so:
int changeCount = (aWhichClipboard == kFindClipboard) ? mChangeCountFind : mChangeCountGeneral;
if (changeCount == [cocoaPasteboard changeCount]) {
Now consider the sequence of events in my STR:
* Step 4 puts "Build Machine" in our cache and on the general pasteboard, caches
mChangeCountGeneral.
* Step 6 puts "x" in our cache and on the find pasteboard, caches mChangeCountFind.
* Step 8 goes to get data from the general pasteboard, checks that mChangeCountGeneral matches
the change count on that pasteboard (which it does), and decides it can use the cache.
We should either have separate cached transferables for the two pasteboards or we should store which clipboard our cached transferable is caching.
Flags: needinfo?(joshmoz)
Assignee | ||
Comment 18•10 years ago
|
||
Lets just keep a second cached transferable for the find clipboard. I can write this up today I think.
Flags: needinfo?(joshmoz)
QA Contact: joshmoz
Updated•10 years ago
|
Assignee: nobody → joshmoz
QA Contact: joshmoz
Assignee | ||
Comment 19•10 years ago
|
||
Requesting review from Boris since he already took the time to understand the problem here.
It turns out to be much easier to fix this by keeping track of which clipboard we cached. I think it would be better to cache for multiple clipboards, but there is no nice way to do that without re-structuring a lot of code due to the fact that we subclass nsBaseClipboard.
If I were going to re-do all of this I'd just not subclass nsBaseClipboard and implement nsIClipboard directly.
Attachment #8445573 -
Flags: review?(bzbarsky)
Comment 20•10 years ago
|
||
Comment on attachment 8445573 [details] [diff] [review]
Fix v1.0
r=me. Thank you!
Attachment #8445573 -
Flags: review?(bzbarsky) → review+
Component: Find Toolbar → Widget: Cocoa
Product: Toolkit → Core
Comment 21•10 years ago
|
||
Boris, Josh: wow, this went fast! I should sleep more often! Thanks so much for the STR and this fix, it is excellent.
Too bad there's no metadata in the patch file (author info and commit message), otherwise I would've pushed this to inbound for you.
Status: NEW → ASSIGNED
Flags: needinfo?(mdeboer)
Comment 22•10 years ago
|
||
Regression, tracking.
Assignee | ||
Comment 23•10 years ago
|
||
pushed to mozilla-inbound
https://hg.mozilla.org/integration/mozilla-inbound/rev/0246b0d10ea9
Comment 24•10 years ago
|
||
Status: ASSIGNED → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla33
Comment 25•10 years ago
|
||
Comment on attachment 8445573 [details] [diff] [review]
Fix v1.0
Approval Request Comment
[Feature/regressing bug #]: 326743
[User impact if declined]: A user will have their system main clipboard filled with the find string entered in the find toolbar on OSX.
[Describe test coverage new/current, TBPL]: no extra coverage. TBPL runs are green.
[Risks and why]: minor.
[String/UUID change made/needed]: n/a.
Attachment #8445573 -
Flags: approval-mozilla-beta?
Attachment #8445573 -
Flags: approval-mozilla-aurora?
Updated•10 years ago
|
status-firefox33:
--- → fixed
Updated•10 years ago
|
Attachment #8445573 -
Flags: approval-mozilla-beta?
Attachment #8445573 -
Flags: approval-mozilla-beta+
Attachment #8445573 -
Flags: approval-mozilla-aurora?
Attachment #8445573 -
Flags: approval-mozilla-aurora+
Comment 26•10 years ago
|
||
Pushed to Aurora as: https://hg.mozilla.org/releases/mozilla-aurora/rev/577c1dd3fc02
...and Beta as: https://hg.mozilla.org/releases/mozilla-beta/rev/05942c665c7e
Thanks again, Josh!
Updated•10 years ago
|
Comment 27•10 years ago
|
||
Thanks, this is great!
Comment 30•10 years ago
|
||
Reproduced the initial issue using Nightly from 2014-06-04, verified that the issue is fixed on Mac OS X 10.9.4 and Mac OS X 10.8.5 using Firefox 31 beta 7, latest Aurora and latest Nightly.
Status: RESOLVED → VERIFIED
Keywords: verifyme
Comment 31•10 years ago
|
||
Can we reconsider taking this in any v30 point release we may put out? I've had a second person tell me today that they've moved to Chrome because this makes search and replace utterly impossible. I imagine this is pissing off a lot of users.
Updated•10 years ago
|
tracking-firefox30:
--- → ?
Comment 32•10 years ago
|
||
(In reply to Jonathan Watt [:jwatt] from comment #31)
> Can we reconsider taking this in any v30 point release we may put out? I've
> had a second person tell me today that they've moved to Chrome because this
> makes search and replace utterly impossible. I imagine this is pissing off a
> lot of users.
31 will be released in 2 weeks... This is also mac-only, so I'm not sure this is still worth spinning a 30.0.1 for. :-\
Updated•10 years ago
|
You need to log in
before you can comment on or make changes to this bug.
Description
•