Closed Bug 335545 Opened 18 years ago Closed 6 years ago

clipboardcache files are not deleted when fx exits

Categories

(Core :: Widget, defect, P1)

defect

Tracking

()

RESOLVED FIXED
mozilla60
Tracking Status
firefox60 --- fixed

People

(Reporter: joi, Assigned: robwu)

References

(Depends on 1 open bug, Blocks 1 open bug)

Details

(Whiteboard: tpi:+)

Attachments

(6 files, 3 obsolete files)

User-Agent:       Mozilla/5.0 (X11; U; Linux x86_64; pl-PL; rv:1.8.0.2) Gecko/20060415 Firefox/1.5.0.2
Build Identifier: Mozilla/5.0 (X11; U; Linux x86_64; pl-PL; rv:1.8.0.2) Gecko/20060415 Firefox/1.5.0.2

when copying text bigger than 500kB fx creates new file in /tmp - clipboardcache[-X] which is 2x bigger than selection and never deleted


Reproducible: Always

Steps to Reproduce:
1.open file bigger than 500kB
2.select all, copy to clipboard (creates file clipboardcache-X in /tmp)
3.close fx


Actual Results:  
file clipboardcache is still present in /tmp

Expected Results:  
delete that file at exit or when it's not needed
(but i would prefer no to create that file at all...)


this bug was first reported in bug 289897 and confirmed od windows
Confirmed, Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.8) Gecko/20060425 BonEcho/2.0a1 ID:2006042512

Following my description on bug 289897, I've just created 40 clipboardcache files (2mb each) within less than a minute, by flipping between two tabs. At least the memory leak is fixed.

Still unanswered, but related: Why does the clipboard content get read on page loading and tab flipping at all?
Status: UNCONFIRMED → NEW
Ever confirmed: true
QA Contact: general
This bug still seems to exist in current Firefox.

I agree with the original reporter that ideally Firefox should not create this file at all.  Barring that, Firefox should at least clean it up afterward.
(In reply to Josh Triplett from comment #2)
> This bug still seems to exist in current Firefox.

To clarify: this bug still seems to exist in Firefox 11.
I just discovered some files /tmp/clipboardcache* and stumbled upon this bug, so it's still alive with firefox 16.
And the file has 00 between each byte, which explain the double size.
still exists in firefox 30 beta 9 (I had to delete the file manually, good thing my %temp% is on a ramdrive)
 Mozilla/5.0 (Windows NT 6.1; WOW64; rv:30.0) Gecko/20100101 Firefox/30.0
Still happening in Firefox 35.0 release: after selecting and copying some text, several  clipboardcache-x files are created in /tmp, up to 2M in size each.
They are not deleted after closing Firefox .
Firefox became non responsive and I had to end task from Task Manager. it's still happening in Firefox 37.0.1 on Windows 8.1, in user's TEMP folder there are 500s files created each with size of 6 MB. So total size was around 3GBs.
Firefox 41.0.1 started creating continuously clipboardcache, clipboardcache-1, clipboardcache-2... files in the %APPDATA%\local\temp directory; these files were all about 24000 Kb big. It filled up the remaining 16Gb of free space on the disk with that.

The behaviour was not present in "Safe" mode. Starting from there I managed to trace the culprit down: the Pushbullet addon. After disabling it, the problem hasn't reoccurred.
Priority: -- → P1
Whiteboard: tpi:+
Blocks: 1254094
It would be great if we never wrote this out, but if we do per bug 1254094 it would be good if we didn't write it to tmp.
Seeing the same problem with Firefox 50.0.2 on Fedora 25. The files are created by just selecting a large body of text, without having to use Ctrl-C/copy.

For example,

1. Clear all clipboardcache* files in /tmp.
2. Go to http://eel.is/c++draft/full (warning: 1.5 MiB).
3. Select all text.
4. The files clipboardcache (18 MiB) and clipboardcache-1 (7.1 MiB) are created in /tmp with only rw permissions for the owner.

Both files seem to be encoded in UTF-16, as you can view them with the command "strings -e l clipboardcache". clipboardcache looks like the full HTML for the page, and clipboardcache-1 looks like the copied text.
Forgot to mention, after selecting the text and using Ctrl-C/copy, the nothing is seen when trying to paste somewhere else with either Ctrl-P or the middle mouse button.
This bug still exists in FF 52.0. Additionally, clearing all context via ctrl/shift/del doesn't delete the files, either.

I have attached a POC that creates a 16MB clipboardcache file in the running account's |temp| folder. Use the POC by selecting, dragging, and dropping the "select and drag" text onto the "to here" text, then examining the |temp| folder. Close FF, examine the folder again, and notice that the file is still there. Restart FF, examine the folder again, and notice that the file is still there. Now press ctrl/shift/del, select all the checkboxes and "everything" for "time range...", then click "clear now". Now examine the folder again and notice that the file is still there.
See Also: → 1396224
At first I just tried a patch that removed the cache file in the destructor. That naive approach does not help, because the destructor is not invoked upon shutdown.

I'll submit a patch that is more reliable.

When trying to verify my patch, do NOT look for "clipboardcache-N" files. You won't find them, since the cache file is created and then immediately deleted (while keeping a file descriptor around). This file descriptor can be located using

 $ lsof +L1 | grep mozilla-temp


(verified on macOS, see commit message in patch for more details)
Assignee: general → rob
Attachment #8903914 - Flags: review?(vladimir) → review?(mstange)
Attachment #8903975 - Flags: review?(vladimir) → review?(mstange)
Markus, vladv (listed as owner at https://wiki.mozilla.org/Modules/All#Core) hasn't responded in the last two weeks (last activity is more than half a year ago).

I looked at the history of nsClipboard.mm, and you show up a couple of times. Could you review my patches?
Comment on attachment 8903975 [details]
Bug 335545 - Store clipboard data in memory XOR file

https://reviewboard.mozilla.org/r/175732/#review187244

::: widget/nsTransferable.cpp:93
(Diff revision 1)
>  {
>    // check here to see if the data is cached on disk
> -  if ( !mData && mCacheFD ) {
> +  if (mCacheFD) {
>      // if so, read it in and pass it back
>      // ReadCache creates memory and copies the data into it.
>      if ( NS_SUCCEEDED(ReadCache(aData, aDataLen)) )

Maybe you want to PR_Close here, just before the return?

::: widget/nsTransferable.cpp:105
(Diff revision 1)
> +      PR_Close(mCacheFD);
> +      mCacheFD = nullptr;
>        return;
>      }
>    }
> +  if (mCacheFD) {

I don't think this branch gets entered. Both of the branches inside the previous if (mCacheFD) return.
Comment on attachment 8903975 [details]
Bug 335545 - Store clipboard data in memory XOR file

https://reviewboard.mozilla.org/r/175732/#review187244

> Maybe you want to PR_Close here, just before the return?

No, closing the file descriptor will cause the data to be released. Calling GetData multiple times should be fine.
PR_Close will be called upon destruction of DataStruct (see the first patch).

> I don't think this branch gets entered. Both of the branches inside the previous if (mCacheFD) return.

You're right, fixed that.
Comment on attachment 8903914 [details]
Bug 335545 - Use nsAnonymousTemporaryFile for clipboard cache

https://reviewboard.mozilla.org/r/175688/#review187242

This seems like a great improvement. I'm not familiar with nsAnonymousTemporaryFile but this looks very straightforward.
Attachment #8903914 - Flags: review?(mstange) → review+
Comment on attachment 8903975 [details]
Bug 335545 - Store clipboard data in memory XOR file

https://reviewboard.mozilla.org/r/175732/#review187252
Attachment #8903975 - Flags: review?(mstange) → review+
Pushed by rob@robwu.nl:
https://hg.mozilla.org/integration/autoland/rev/dff1e1774422
Use nsAnonymousTemporaryFile for clipboard cache r=mstange
https://hg.mozilla.org/integration/autoland/rev/e96494792b66
Store clipboard data in memory XOR file r=mstange
Backed out for failing clipboard mochitest widget/tests/test_bug1123480.xul on Linux x64 asan:

https://hg.mozilla.org/integration/autoland/rev/6694f4b93826511dd254dd36a7044891329beaf2
https://hg.mozilla.org/integration/autoland/rev/e8f4ef8801fbe8013a7cb0eeb5a7fae23db19edf

Push with failures: https://treeherder.mozilla.org/#/jobs?repo=autoland&revision=e96494792b66b2b7d8199d532164a3d23f66b004&filter-resultStatus=testfailed&filter-resultStatus=busted&filter-resultStatus=exception&filter-resultStatus=retry&filter-resultStatus=usercancel&filter-resultStatus=runnable
Failure log: https://treeherder.mozilla.org/logviewer.html#?job_id=132310823&repo=autoland

[task 2017-09-20T21:37:16.577Z] 21:37:16     INFO - TEST-START | widget/tests/test_bug1123480.xul
[task 2017-09-20T21:37:17.923Z] 21:37:17     INFO - TEST-INFO | started process screentopng
[task 2017-09-20T21:37:18.327Z] 21:37:18     INFO - TEST-INFO | screentopng: exit 0
[task 2017-09-20T21:37:18.328Z] 21:37:18     INFO - Buffered messages logged at 21:37:17
[task 2017-09-20T21:37:18.328Z] 21:37:18     INFO - TEST-PASS | widget/tests/test_bug1123480.xul | failed to presanitize the environment 
[task 2017-09-20T21:37:18.329Z] 21:37:18     INFO - Buffered messages finished
[task 2017-09-20T21:37:18.329Z] 21:37:18     INFO - TEST-UNEXPECTED-FAIL | widget/tests/test_bug1123480.xul | correctly saved memory by caching to disk 
[task 2017-09-20T21:37:18.330Z] 21:37:18     INFO - RunTest@chrome://mochitests/content/chrome/widget/tests/test_bug1123480.xul:40:5
[task 2017-09-20T21:37:18.330Z] 21:37:18     INFO - onload@chrome://mochitests/content/chrome/widget/tests/test_bug1123480.xul:1:1
[task 2017-09-20T21:37:18.330Z] 21:37:18     INFO - rval@chrome://mochikit/content/tests/SimpleTest/SimpleTest.js:146:17
[task 2017-09-20T21:37:18.331Z] 21:37:18     INFO - EventHandlerNonNull*this.addLoadEvent@chrome://mochikit/content/tests/SimpleTest/SimpleTest.js:171:13
[task 2017-09-20T21:37:18.331Z] 21:37:18     INFO - @chrome://mochikit/content/tests/SimpleTest/SimpleTest.js:1441:5
Flags: needinfo?(rob)
Rebased (no change), and updated the test to count file descriptors instead of looking for the "clipboardcache" file.

The test now passes locally, let's see whether that is also the case on try servers.
https://treeherder.mozilla.org/#/jobs?repo=try&revision=c8f9839f5c83
Flags: needinfo?(rob)
Comment on attachment 8910714 [details]
Bug 335545 - Count FD instead of looking for clipboardcache in test_bug1123480.xul

https://reviewboard.mozilla.org/r/182174/#review188940

Sorry for the delay, I missed this review request.

::: widget/tests/test_bug1123480.xul:22
(Diff revision 1)
> +  // Get a list of open file descriptors, and ASSUME that any mutations in file
> +  // descriptor counts are caused by our test. This is a bold assumption,
> +  // but better than the alternatives (spawning lsof as a subprocess and parsing
> +  // its output, or adding test-only code to nsAnonymousTemporaryFile).

Yes, this is quite a bold assumption. It would not surprise me if this test starts failing intermittently due to other things that happen on the test machine. Adding test-only code to nsAnonymousTemporaryFile doesn't sound all that bad to me... the glue to access it from JavaScript land would probably be the most annoying bit.

So let's try this out, and if we see too many failures, we'll need to revisit it.
Attachment #8910714 - Flags: review?(mstange) → review+
Pushed by rob@robwu.nl:
https://hg.mozilla.org/integration/autoland/rev/ea69ee15ed90
Use nsAnonymousTemporaryFile for clipboard cache r=mstange
https://hg.mozilla.org/integration/autoland/rev/b83ddb70c8b5
Store clipboard data in memory XOR file r=mstange
https://hg.mozilla.org/integration/autoland/rev/25a686779a94
Count FD instead of looking for clipboardcache in test_bug1123480.xul r=mstange
I had to back these out for failures like https://treeherder.mozilla.org/logviewer.html#?job_id=133442244&repo=autoland on at least asan linux.
Flags: needinfo?(rob)
Backout by kwierso@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/ffcfc5b57722
Backed out 3 changesets for asan failures in test_bug1123480.xul a=backout
The Linux test failed because on Linux the number of extra FDs is expected to be 2, not 1 because copyString copies to two clipboards instead of one.

I've updated the test, it's a trivial change so if the try run passes I'll re-land.
Flags: needinfo?(rob)
(In reply to Rob Wu [:robwu] from comment #38)
> The Linux test failed because on Linux the number of extra FDs is expected
> to be 2, not 1 because copyString copies to two clipboards instead of one.
> 
> I've updated the test, it's a trivial change so if the try run passes I'll
> re-land.

Hi Rob, did this change get lost? :-)
Flags: needinfo?(rob)
Yes it did, literally. My Macbook with the WIP was stolen and I haven't had the opportunity yet to recover it from a backup from Linux.

I have just started to revive the patch based on the previously uploaded patches and will submit a new review request when they are ready.

I am rewriting the tests so that it is less prone to intermittents, by only counting file descriptors that look related to the test rather than all file descriptors of the application.

The tests fail, and one of the reasons for that is that the clipboard cache is never used because of an undesired side effect from the patch for bug 1433030 (see https://bugzilla.mozilla.org/show_bug.cgi?id=1433030#c7 ).
Depends on: 1433030
Flags: needinfo?(rob)
Hi Markus,

could you review the patches again?
I modified the previously approved patches as follows (diff): https://pastebin.mozilla.org/9078615 and added two new patches on top.

The easiest way to review is to review the C++ changes in the diff at https://pastebin.mozilla.org/9078615,
and then the last three patches separately.

Summary of changes:
- (amended patch 1) Add PR_Seek64 to ensure that we read/write from the start of the file (instead of the end). Otherwise when the FD is re-used, the data is not correctly read/written.

- (amended patch 1): Set aDataLen when reading from a file.

- (amended patch 2): Clear mCacheFd if the transferable is reused, after switching from a large value to a small value. Previously the mCacheFd and mData pointer were both kept.

- (amended patch 3): Modify test to not count any file descriptor, but file descriptors that point to files of a specific size. Also re-enable the test for Windows, by relying on the fact that nsAnonymousTemporaryFile stores the temp files in a specific directory.

- (new patch 4): Mark DataStruct as non-copyable because that is unsafe.

- (new patch 5): Improve test coverage by adding unit test to cover all bugs that are solved by the amended patch 1 & 2.
Comment on attachment 8953931 [details]
Bug 335545 - Store clipboard data in memory XOR file

https://reviewboard.mozilla.org/r/223076/#review230018
Attachment #8953931 - Flags: review?(mstange) → review+
Comment on attachment 8953933 [details]
Bug 335545 - Make DataStruct non-copyable

https://reviewboard.mozilla.org/r/223080/#review230020
Attachment #8953933 - Flags: review?(mstange) → review+
Comment on attachment 8953930 [details]
Bug 335545 - Use nsAnonymousTemporaryFile for clipboard cache

https://reviewboard.mozilla.org/r/223074/#review230024
Attachment #8953930 - Flags: review?(mstange) → review+
Comment on attachment 8953932 [details]
Bug 335545 - Count FD instead of looking for clipboardcache in test_bug1123480.xul

https://reviewboard.mozilla.org/r/223078/#review230026
Attachment #8953932 - Flags: review?(mstange) → review+
Comment on attachment 8953934 [details]
Bug 335545 - Add test to verify that overflowing clipboard data survives

https://reviewboard.mozilla.org/r/223082/#review230032
Attachment #8953934 - Flags: review?(mstange) → review+
Looks good! Thanks for the extended tests and the explanations of the diff.

One way to make the code look nicer might be to have two structs for the two different backing methods, and then use a mozilla::Variant of them as a member of DataStruct. Each of the structs could have their own destructor, and then you wouldn't need to do manual cleanup in DataStruct whenever you switch backing methods. Just as an idea... but let's go with what you have now.
We're sorry, Autoland could not rebase your commits for you automatically. Please manually rebase your commits and try again.

hg error in cmd: hg rebase -s eb6354ecf9d3b89e28b91cbaaa6a4af354710108 -d 39df0ca8a664: rebasing 450191:eb6354ecf9d3 "Bug 335545 - Use nsAnonymousTemporaryFile for clipboard cache r=mstange"
rebasing 450192:e8e192442fd2 "Bug 335545 - Store clipboard data in memory XOR file r=mstange"
rebasing 450193:7bb095fc7095 "Bug 335545 - Count FD instead of looking for clipboardcache in test_bug1123480.xul r=mstange"
merging widget/tests/test_bug1123480.xul
warning: conflicts while merging widget/tests/test_bug1123480.xul! (edit, then use 'hg resolve --mark')
unresolved conflicts (see hg resolve, then hg rebase --continue)
Pushed by rob@robwu.nl:
https://hg.mozilla.org/integration/autoland/rev/07e72c13b476
Use nsAnonymousTemporaryFile for clipboard cache r=mstange
https://hg.mozilla.org/integration/autoland/rev/28a5bbda5231
Store clipboard data in memory XOR file r=mstange
https://hg.mozilla.org/integration/autoland/rev/a9eca77bd638
Count FD instead of looking for clipboardcache in test_bug1123480.xul r=mstange
https://hg.mozilla.org/integration/autoland/rev/5469af7e5a36
Make DataStruct non-copyable r=mstange
https://hg.mozilla.org/integration/autoland/rev/46c607ac246a
Add test to verify that overflowing clipboard data survives r=mstange
Attachment #8903914 - Attachment is obsolete: true
Attachment #8903975 - Attachment is obsolete: true
Attachment #8910714 - Attachment is obsolete: true
Depends on: 1482540
No longer depends on: 1482540
Regressions: 1482540
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: