1555115 - Visiting fast.com leaves temp files in app_tmpdir

Status: RESOLVED DUPLICATE of bug 1685556

(Core :: DOM: File, defect, P3)

Description

[qnc0dz]

WARNING - May cause large amounts of hard-to-remove cruft; don't try this unless you're prepared to completely wipe the app's data

Fennec 68.0b5 build 20190527103257, clean profile

STR:

1. Start the browser
2. Navigate to fast.com. It will perform a download speed test, let it finish
3. Navigate to file:///data/data/org.mozilla.<id>/app_tmpdir (replace <id> with firefox for release, firefox_beta for beta, fennec_aurora for nightly)
4. Notice the newly-created mozilla-temp-<randomnumber> files

Expected:

These temp files will eventually be deleted.

Actual:

They seem to stay there forever.

Also see bug 1532446 related to cleaning up app_tmpdir in general

Comment 1

[Kevin Brosnan [Ex-Mozilla]]

This happens in GeckoView apps as well. Moving it to them.

Comment 2

[Kevin Brosnan [Ex-Mozilla]]

User data grows by 30-40 MB each time you reload fast.com reproduced in Fenix and Reference Browser.

Comment 3

[Chris Peterson [:cpeterson]]

Is this a duplicate of bug 1532446?

Comment 4

[Jan]

I wouldn't call it a duplicate. This bug identifies (or at least provides a repro case) for one specific bug that generates orphaned temp files. Bug 1532446 is about the general problem of orphan temp files, and I've proposed a generic clean-up method to at least mitigate the impact.

I expect multiple bugs to add such stale temp files, so fixing this bug alone will likely not resolve the problem, while adding a garbage collector as suggested in bug 1532446 just mitigates the impact without addressing the root causes (so this bug, as well as all the other sources of stale temp files, should also be fixed).

However, the fact that it is so easy to generate the cruft shows the need for a clean-up routine: Without it, users who have used fast.com (or any other site that triggers this, of which there probably are many) in the past have the choice between wiping all their app data, or leaving the cruft that occupies hundreds of MB on their phones.

Comment 5

[Rob Wu [:robwu]]

Confirmed. Even the "Clear private data" option does not clear the data.

This does not happen in private browsing mode.

To distinguish this bug from bug 1532446, I connected the debugger via about:debugging with Fennec, and ran the following snippet in the main process:

for (let f of FileUtils.getDir('TmpD', [], false).directoryEntries) {
  if(f.path.includes('tmpaddon') || f.path.includes('/tmp-')) continue;
  console.log(f.path, f.fileSize);
}

Example of output:

/data/user/0/org.mozilla.fennec_aurora/app_tmpdir/mozilla-temp-586798990 5144576
/data/user/0/org.mozilla.fennec_aurora/app_tmpdir/mozilla-temp-352247709 6242304
/data/user/0/org.mozilla.fennec_aurora/app_tmpdir/mozilla-temp-178472751 4489216
/data/user/0/org.mozilla.fennec_aurora/app_tmpdir/mozilla-temp-1198755214 5373952
/data/user/0/org.mozilla.fennec_aurora/app_tmpdir/mozilla-temp-892424442 1392640

I printed the content of one such file using the following snippet. Its content looks like garbage (not surprising, since fast.com appears to be creating Blob objects with randomly generated values - source: https://pastebin.com/S15HUtaS):

OS.File.read('/data/user/0/org.mozilla.fennec_aurora/app_tmpdir/mozilla-temp-892424442', {encoding:'utf-8'}).then(console.log)

Comment 6

[Chris Peterson [:cpeterson]]

Eugen, is app_tmpdir a temp directory for file downloads or for Gecko's own file cache?

How often should GV clean up the temp dir?

Even the "Clear private data" option does not clear the data.

We should also probably clear the app_tmpdir directory when the user/app asks GV to clear user data.

Comment 7

[Eugen Sawin [:esawin]]

(In reply to Chris Peterson [:cpeterson] from comment #6)

Eugen, is app_tmpdir a temp directory for file downloads or for Gecko's own file cache?

It's a Gecko DOM file cache, also used by devtools for XPI downloads.

How often should GV clean up the temp dir?

A DOM peer should be better suited to answer this, we can figure out the Android details based on that.
I would assume that deleting on each runtime startup/shutdown would be the way to go.

Even the "Clear private data" option does not clear the data.

We should also probably clear the app_tmpdir directory when the user/app asks GV to clear user data.

app_tmpdir isn't really a user-facing cache, I don't think it would make sense exposing that through the API. Deletion should be automatic.

Comment 8

[Chris Peterson [:cpeterson]]

(In reply to Eugen Sawin [:esawin] from comment #7)

(In reply to Chris Peterson [:cpeterson] from comment #6)

Eugen, is app_tmpdir a temp directory for file downloads or for Gecko's own file cache?

It's a Gecko DOM file cache, also used by devtools for XPI downloads.

How often should GV clean up the temp dir?

A DOM peer should be better suited to answer this, we can figure out the Android details based on that.
I would assume that deleting on each runtime startup/shutdown would be the way to go.

Dragana, the GeckoView team has some questions about Gecko's file cache on Android. Is the file cache owned by the Networking team? Who should we talk with?

Comment 9

[Dragana Damjanovic [:dragana]]

I think this is in DOM. I will ask there, but keep my needinfo.
Andrew, looking shortly at the code this seems to be in DOM, can you take a look?

Comment 10

[Andrew Sutherland [:asuth] (he/him)]

Files of the form "mozilla-temp-" are created by https://dev.searchfox.org/mozilla-central/source/xpcom/io/nsAnonymousTemporaryFile.h which is XPCOM. It looks like it tries to perform general cleanup when it receives idle service notifications[1] and will perform a one-off cleanup at startup if it couldn't register with the idle service[2].

It seems like most callers are using the DELETE_ON_CLOSE NS_OpenAnonymousTemporaryFile variant. But there is https://searchfox.org/mozilla-central/rev/f8b11433159cbc9cc80500b3e579d767473fa539/dom/file/ipc/TemporaryIPCBlobParent.cpp#30 which intentionally uses a more complex state machine. Per https://bugzilla.mozilla.org/show_bug.cgi?id=1403706#c18 it's expected that deletion will happen in an orderly cleanup case. In the event of unclean shutdown (which seems likely on Android), I think the cleanup mentioned in the first paragraph should happen at least when Firefox is next started.

NI'ing :froydnj as owner of XPCOM where the anon temp file stuff lives. :baku also knows a lot about what's going on in this area of code, but is trying to not own so much of it, so it's likely preferable to not ask him until we need his particular expertise. (This might also be something to involve the DOM performance team in?)

1: https://dev.searchfox.org/mozilla-central/rev/eb09749a72763685d7909c0c3c9da4e886434f13/xpcom/io/nsAnonymousTemporaryFile.cpp#205
2: https://dev.searchfox.org/mozilla-central/rev/eb09749a72763685d7909c0c3c9da4e886434f13/xpcom/io/nsAnonymousTemporaryFile.cpp#202

Comment 11

[Chris Peterson [:cpeterson]]

In the event of unclean shutdown (which seems likely on Android), I think the cleanup mentioned in the first paragraph should happen at least when Firefox is next started.

Yes. We should not depend (solely) on some cleanup code running on app shutdown. We've had recent Android bugs related to code trying to save cached font files or submit telemetry on app shutdown.

Comment 12

[Nathan Froyd [:froydnj]]

(In reply to Andrew Sutherland [:asuth] (he/him) from comment #10)

Files of the form "mozilla-temp-" are created by https://dev.searchfox.org/mozilla-central/source/xpcom/io/nsAnonymousTemporaryFile.h which is XPCOM. It looks like it tries to perform general cleanup when it receives idle service notifications[1] and will perform a one-off cleanup at startup if it couldn't register with the idle service[2].

It seems like most callers are using the DELETE_ON_CLOSE NS_OpenAnonymousTemporaryFile variant. But there is https://searchfox.org/mozilla-central/rev/f8b11433159cbc9cc80500b3e579d767473fa539/dom/file/ipc/TemporaryIPCBlobParent.cpp#30 which intentionally uses a more complex state machine. Per https://bugzilla.mozilla.org/show_bug.cgi?id=1403706#c18 it's expected that deletion will happen in an orderly cleanup case. In the event of unclean shutdown (which seems likely on Android), I think the cleanup mentioned in the first paragraph should happen at least when Firefox is next started.

NI'ing :froydnj as owner of XPCOM where the anon temp file stuff lives. :baku also knows a lot about what's going on in this area of code, but is trying to not own so much of it, so it's likely preferable to not ask him until we need his particular expertise. (This might also be something to involve the DOM performance team in?)

1: https://dev.searchfox.org/mozilla-central/rev/eb09749a72763685d7909c0c3c9da4e886434f13/xpcom/io/nsAnonymousTemporaryFile.cpp#205
2: https://dev.searchfox.org/mozilla-central/rev/eb09749a72763685d7909c0c3c9da4e886434f13/xpcom/io/nsAnonymousTemporaryFile.cpp#202

I have never touched this code or reviewed it, though I nominally own it. Since Eric is in the same boat, he would be a reasonable person to answer these questions in my absence.

Comment 13

[Eric Rahm [:erahm]]

nsAnonTempFileRemover is created in CreateAnonTempFileRemover which is only setup on windows in NS_InitXPCOM. It's possible we should be enabling this for GeckoView as well. It looks like cpearce is the "expert" here (this was all added 7-9 years ago for media related things and has sat untouched since).

Chris, do you have any thoughts on if enabling the temp file remover is safe and makes sense for GeckoView?

Comment 14

[Andrew Sutherland [:asuth] (he/him)]

I think we probably need to be enabling it everywhere. I presume the original rationale was "on linux and OS X we can delete files but keep the fd open, so there's never any need to perform a cleanup sweep there, but windows is dumb and we can't delete the file until we close the fd, so we will need to do cleanup sweeps." But since we're now explicitly deleting files on all platforms, we need to cleanup on all platforms.

Comment 15

[Chris Pearce [:cpearce (Not reading bugmail)]]

I think the relevant detail is here: https://bugzilla.mozilla.org/show_bug.cgi?id=785662#c2

From memory, the temp file remover was needed on Windows XP for the case where we create an anonymous temporary file with nsIFile::DELETE_ON_CLOSE, and then under some circumstances the OS doesn't clean up the file. IIRC, the only way I could repro the OS failing to delete the file was to switch off the power at the wall for my desktop, and then the anonymous temporary files wouldn't be deleted. I don't recall whether later versions of Windows had this problem.

I don't think we need to enable the anonymous temporary file remover on GV, unless we are supporting GV on Windows versions affected by this.

Comment 16

[Chris Pearce [:cpearce (Not reading bugmail)]]

Though, reading the scrollback, it sounds like it may actually be required on Android. I'm surprised that Android isn't automatically deleting temporary files on close... Is this a bug in nsIFile::DELETE_ON_CLOSE on Android?

Comment 17

[Jan]

Since there seem to be multiple sources that generate and leave temp files (see bug 1532446 for examples of other file name patterns found), and there are existing users with temp files that will need to be cleaned up retroactively unless we want to leave those users stuck with large unremovable files, enabling the temp file remover seems like a good idea.

Comment 18

[Chris Peterson [:cpeterson]]

Deferring this bug from Fenix's M7 (July) milestone to the M8 backlog for later in Q3.

Comment 19

[Andrea Marchesini [:baku]]

Though, reading the scrollback, it sounds like it may actually be required on Android. I'm surprised that Android isn't automatically deleting temporary files on close... Is this a bug in nsIFile::DELETE_ON_CLOSE on Android?

DELETE_ON_CLOSE is implemented only in nsLocalFileUnix:
https://searchfox.org/mozilla-central/rev/1097f59d0bc17f6f8f805325c2f607e60cf0d26b/xpcom/io/nsLocalFileUnix.cpp#399-401

and, in theory, android file behavior should follow the POSIX standards: a file descriptor can be deleted, the file should disappear from the disk, even if other apps have a opened file descriptions for the same file (those FDs remain valid and fully funtional).

We should check if:
a. nsLocalFileUnix is actually used on Android.
b. if the flag is correctly set.

Comment 20

[Simon Giesecke [:sg] [he/him]]

FWIW, there is a test for this behaviour at https://searchfox.org/mozilla-central/rev/35b97af64a55d1d30caa4d6e9fabc1a7fbabc509/xpcom/tests/gtest/TestFile.cpp#165-201 As far as I can tell, this should be run on all platforms, though I can't verify this using the coverage report, as we don't seem to gather coverage on Android.

Comment 21

[Kevin Brosnan [Ex-Mozilla]]

Confirmed that bug 1685556 has these files being put in the user clearable cache folder.