Closed Bug 1020743 Opened 6 years ago Closed 6 years ago

The text from the find bar is overwriting the system clipboard

Categories

(Core :: Widget: Cocoa, defect, major)

All
macOS
defect
Not set
major

Tracking

()

VERIFIED FIXED
mozilla33
Tracking Status
firefox30 - wontfix
firefox31 + verified
firefox32 + verified
firefox33 --- verified

People

(Reporter: MattN, Assigned: jaas)

References

Details

(Keywords: regression, reproducible)

Attachments

(1 file)

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.
If anyone has any initial clue, it's likely Mike....
Flags: needinfo?(mdeboer)
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)
(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)
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.
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.
Duplicate of this bug: 1024481
(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)
Here you go: accessibility.typeaheadfind.prefillwithselection;false
Flags: needinfo?(warpdag)
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".
And in particular the prefilled text in step 5 is whatever got stuck in the clipboard in step 6 of the previous browser run(!).
Per previous comments, 30 and 31 are also affected.
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
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.
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.
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)
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
Assignee: nobody → joshmoz
QA Contact: joshmoz
Attached patch Fix v1.0Splinter Review
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 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
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)
https://hg.mozilla.org/mozilla-central/rev/0246b0d10ea9
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla33
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?
Attachment #8445573 - Flags: approval-mozilla-beta?
Attachment #8445573 - Flags: approval-mozilla-beta+
Attachment #8445573 - Flags: approval-mozilla-aurora?
Attachment #8445573 - Flags: approval-mozilla-aurora+
Thanks, this is great!
Duplicate of this bug: 1032012
Duplicate of this bug: 1033527
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.
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.
(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. :-\
You need to log in before you can comment on or make changes to this bug.