Closed
Bug 335545
Opened 18 years ago
Closed 6 years ago
clipboardcache files are not deleted when fx exits
Categories
(Core :: Widget, defect, P1)
Core
Widget
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)
766 bytes,
text/html
|
Details | |
59 bytes,
text/x-review-board-request
|
mstange
:
review+
|
Details |
59 bytes,
text/x-review-board-request
|
mstange
:
review+
|
Details |
59 bytes,
text/x-review-board-request
|
mstange
:
review+
|
Details |
59 bytes,
text/x-review-board-request
|
mstange
:
review+
|
Details |
59 bytes,
text/x-review-board-request
|
mstange
:
review+
|
Details |
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
Comment 1•18 years ago
|
||
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
Updated•15 years ago
|
QA Contact: general
Comment 2•12 years ago
|
||
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.
Comment 3•12 years ago
|
||
(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.
Comment 5•10 years ago
|
||
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.
Comment 8•9 years ago
|
||
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.
Updated•8 years ago
|
Priority: -- → P1
Updated•8 years ago
|
Whiteboard: tpi:+
Comment 11•8 years ago
|
||
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.
Comment 12•7 years ago
|
||
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.
Comment 13•7 years ago
|
||
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.
Comment 14•7 years ago
|
||
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.
Comment 15•7 years ago
|
||
Comment hidden (mozreview-request) |
Assignee | ||
Comment 17•7 years ago
|
||
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
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Assignee | ||
Updated•7 years ago
|
Attachment #8903914 -
Flags: review?(vladimir) → review?(mstange)
Attachment #8903975 -
Flags: review?(vladimir) → review?(mstange)
Assignee | ||
Comment 20•7 years ago
|
||
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 21•7 years ago
|
||
mozreview-review |
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 hidden (mozreview-request) |
Assignee | ||
Comment 23•7 years ago
|
||
mozreview-review-reply |
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 24•7 years ago
|
||
mozreview-review |
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 25•7 years ago
|
||
mozreview-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+
Comment 26•7 years ago
|
||
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
Comment 27•7 years ago
|
||
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)
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Assignee | ||
Comment 31•7 years ago
|
||
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 32•7 years ago
|
||
mozreview-review |
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+
Comment 33•7 years ago
|
||
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)
Comment 35•7 years ago
|
||
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
Comment hidden (mozreview-request) |
Assignee | ||
Comment 38•7 years ago
|
||
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)
Comment 39•6 years ago
|
||
(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)
Assignee | ||
Comment 40•6 years ago
|
||
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)
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Assignee | ||
Comment 46•6 years ago
|
||
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 47•6 years ago
|
||
mozreview-review |
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 48•6 years ago
|
||
mozreview-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 49•6 years ago
|
||
mozreview-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 50•6 years ago
|
||
mozreview-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 51•6 years ago
|
||
mozreview-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+
Comment 52•6 years ago
|
||
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.
Comment 53•6 years ago
|
||
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)
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment 59•6 years ago
|
||
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
Comment 60•6 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/07e72c13b476 https://hg.mozilla.org/mozilla-central/rev/28a5bbda5231 https://hg.mozilla.org/mozilla-central/rev/a9eca77bd638 https://hg.mozilla.org/mozilla-central/rev/5469af7e5a36 https://hg.mozilla.org/mozilla-central/rev/46c607ac246a
Status: NEW → RESOLVED
Closed: 6 years ago
status-firefox60:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla60
Assignee | ||
Updated•6 years ago
|
Attachment #8903914 -
Attachment is obsolete: true
Assignee | ||
Updated•6 years ago
|
Attachment #8903975 -
Attachment is obsolete: true
Assignee | ||
Updated•6 years ago
|
Attachment #8910714 -
Attachment is obsolete: true
Assignee | ||
Updated•5 years ago
|
You need to log in
before you can comment on or make changes to this bug.
Description
•