1269878 - Thunderbird tries to delete user's temp directory upon exit

Status: VERIFIED FIXED

(Thunderbird :: General, defect)

Description

[Jorg K (CEST = GMT+2)]

I've just done some debugging for another bug and added
printf ("***** |%s|\n", NS_ConvertUTF16toUTF8(mWorkingPath).get());
here:
https://dxr.mozilla.org/comm-central/source/mozilla/xpcom/io/nsLocalFileWin.cpp#2377

When closing TB, I get this:
***** |C:\Users\jorgk\AppData\Local\Temp\AdobeARM.log|
***** |C:\Users\jorgk\AppData\Local\Temp\axcrypt1\axx1F48.tmp|
***** |C:\Users\jorgk\AppData\Local\Temp\etilqs_FTiOzMufpisd0pu|
***** |C:\Users\jorgk\AppData\Local\Temp\etilqs_fuk6cPhCQbzWaH7|
***** |C:\Users\jorgk\AppData\Local\Temp\etilqs_jRkujgI1AGoMuqg|
***** |C:\Users\jorgk\AppData\Local\Temp\etilqs_w2fnXxWomwwSdzc|
***** |C:\Users\jorgk\AppData\Local\Temp\etilqs_wg9vkb3pJ9bEBeN|
***** |C:\Users\jorgk\AppData\Local\Temp\etilqs_ZFaHnbILKxxFuee|
***** |C:\Users\jorgk\AppData\Local\Temp\FXSAPIDebugLogFile.txt|
***** |C:\Users\jorgk\AppData\Local\Temp\procexp64.exe|
***** |C:\Users\jorgk\AppData\Local\Temp\~DFE5F49A15D8ED5F0E.TMP|
[7784] WARNING: Failed to delete temp directory.: file c:/mozilla-source/comm-central/mozilla/toolkitxre/nsAppRunner.cpp, line 632
http://mxr.mozilla.org/mozilla-central/source/toolkit/xre/nsAppRunner.cpp#620

So somehow, the application tried to delete the entire Temp directory, which of course fails.

Looking at bug 1237847 that landed
+  rv = tempDir->Append(NS_LITERAL_STRING("Temp-") + aTempDirSuffix);
https://hg.mozilla.org/mozilla-central/rev/5b20181a5b50#l7.47

to delete a subdirectory of the Temp directory, but this has since been removed in bug 1236108
http://hg.mozilla.org/mozilla-central/rev/19074ba43502#l2.44

Perhaps we're missing some setup, but TB cleans out the Windows Temp directory.

While it's nice to clean up the Temp directory from time to time, I don't think Mozilla software should get into this business.

Comment 1

[Jorg K (CEST = GMT+2)]

What do we need to do so TB doesn't go around deleting Temp directories?

Comment 2

[(No longer employed by Mozilla) Aaron Klotz]

That code is intended to delete a temp subdirectory that is specifically created for sandboxed content processes in e10s.

That code doesn't get built unless MOZ_CONTENT_SANDBOX is defined. Why would MOZ_CONTENT_SANDBOX be turned on for TB?

Comment 3

[Haik Aftandilian [:haik]]

(In reply to Jorg K (GMT+2) from comment #0)
> So somehow, the application tried to delete the entire Temp directory, which
> of course fails.
> 
> Looking at bug 1237847 that landed
> +  rv = tempDir->Append(NS_LITERAL_STRING("Temp-") + aTempDirSuffix);
> https://hg.mozilla.org/mozilla-central/rev/5b20181a5b50#l7.47
> 
> to delete a subdirectory of the Temp directory, but this has since been
> removed in bug 1236108
> http://hg.mozilla.org/mozilla-central/rev/19074ba43502#l2.44
> 
> Perhaps we're missing some setup, but TB cleans out the Windows Temp
> directory.
> 
> While it's nice to clean up the Temp directory from time to time, I don't
> think Mozilla software should get into this business.

The intent with that code is to delete a temporary directory that is only used for content process sandboxing. The parent process creates a directory with a unique name "Temp-<UUID>". The content process is allowed to write to that directory for stashing temporary files. Eventually the file I/O will be proxied through the parent process instead.

Maybe that code is somehow being used to delete a Thunderbird caching directory due to a bug.

Could you post the name and path of the directory that is failing to be deleted?

Comment 4

[Jorg K (CEST = GMT+2)]

Thanks for the hint. I have no idea, I'm not controlling the build infrastructure. I'll ask someone who might know.

Aleth: We really need to not define MOZ_CONTENT_SANDBOX for TB.

Comment 5

[Haik Aftandilian [:haik]]

(In reply to Haik Aftandilian [:haik] from comment #3)
> Could you post the name and path of the directory that is failing to be
> deleted?

Aaron's point above about MOZ_CONTENT_SANDBOX should supersede my question.

Comment 6

[Jorg K (CEST = GMT+2)]

(In reply to Haik Aftandilian [:haik] from comment #3)
> Could you post the name and path of the directory that is failing to be
> deleted?
You can see it in comment #0:
***** |C:\Users\jorgk\AppData\Local\Temp\AdobeARM.log|
It recursively tries to delete all the files from the directory first, and of course fails since some of them are locked bu other processes.

C:\Users\jorgk\AppData\Local\Temp is the official Windows temp directory of the user (%TEMP%).

Comment 7

[Jorg K (CEST = GMT+2)]

Surely this will also be a problem on Linux. You'd have to hook into here:
https://dxr.mozilla.org/comm-central/source/mozilla/xpcom/io/nsLocalFileUnix.cpp#1025
adding printf ("***** |%s|\n", NS_ConvertUTF16toUTF8(mWorkingPath).get());
and see what gets deleted upon application close.

Aceman, can you please take a look. Perhaps you also know where we need to unset MOZ_CONTENT_SANDBOX. You can see in comment #0 that this got landed in bug 1237847 and bug 1236108 on mozilla47/48 so we need to fix this soon in TB 49, 48 and the upcoming TB 47 beta.

Comment 8

[Bob Owen (:bobowen)]

(In reply to Jorg K (GMT+2) from comment #7)

> bug 1237847 and bug 1236108 on mozilla47/48 so we need to fix this soon in
> TB 49, 48 and the upcoming TB 47 beta.

Mistakenly clearing the %TEMP% directory with certain settings was introduced in 48.
If you're picking up gecko's config for MOZ_CONTENT_SANDBOX, then that's only defined for Nightly at the moment.

Comment 9

[aleth [:aleth]]

(In reply to Aaron Klotz [:aklotz] from comment #2)
> That code is intended to delete a temp subdirectory that is specifically
> created for sandboxed content processes in e10s.
 
> That code doesn't get built unless MOZ_CONTENT_SANDBOX is defined. Why would
> MOZ_CONTENT_SANDBOX be turned on for TB?

TB does not currently do anything special with MOZ_CONTENT_SANDBOX. It's not app-specific and set in old-configure.

It seems to be set here
https://dxr.mozilla.org/comm-central/source/mozilla/old-configure.in#4992
and should therefore only affect nightlies on OSX and Windows.

There are two separate issues here:

* Should TB override this or should, instead, this nightly-specific inheritance from MOZ_SANDBOX be moved to browser/?

* It seems to me that file deletion code which removes the entire temp dir if the subdir it's looking for doesn't exist for some reason has a bug which should be fixed.

Comment 10

[(No longer employed by Mozilla) Aaron Klotz]

(In reply to aleth [:aleth] from comment #9)
> * Should TB override this or should, instead, this nightly-specific
> inheritance from MOZ_SANDBOX be moved to browser/?

In the immediate term, TB should be configured with --disable-content-sandbox or --disable-sandbox (the former just disables content sandboxing whereas the latter completely disables all sandboxing code). I'll discuss the topic of moving sandbox config to browser/ in our weekly sandboxing meeting today.

> 
> * It seems to me that file deletion code which removes the entire temp dir
> if the subdir it's looking for doesn't exist for some reason has a bug which
> should be fixed.

Yeah that deletion code will be fixed in bug 1270018.

Comment 11

[aleth [:aleth]]

(In reply to Aaron Klotz [:aklotz] from comment #10)
> (In reply to aleth [:aleth] from comment #9)
> In the immediate term, TB should be configured with
> --disable-content-sandbox or --disable-sandbox (the former just disables
> content sandboxing whereas the latter completely disables all sandboxing
> code). I'll discuss the topic of moving sandbox config to browser/ in our
> weekly sandboxing meeting today.

I can add that to the mail/ mozconfigs, but that won't help local builds...

> > * It seems to me that file deletion code which removes the entire temp dir
> > if the subdir it's looking for doesn't exist for some reason has a bug which
> > should be fixed.
> 
> Yeah that deletion code will be fixed in bug 1270018.

Great, thanks.

Comment 12

[aleth [:aleth]]

(In reply to aleth [:aleth] from comment #11)
> (In reply to Aaron Klotz [:aklotz] from comment #10)
> > (In reply to aleth [:aleth] from comment #9)
> > In the immediate term, TB should be configured with
> > --disable-content-sandbox or --disable-sandbox (the former just disables
> > content sandboxing whereas the latter completely disables all sandboxing
> > code). I'll discuss the topic of moving sandbox config to browser/ in our
> > weekly sandboxing meeting today.
> 
> I can add that to the mail/ mozconfigs, but that won't help local builds...

At least I *think* NIGHTLY_BUILD is set for local builds?

Comment 13

[Jorg K (CEST = GMT+2)]

Just a little anecdote: I was wondering why my VS 2015 kept loading the same symbols from the MS servers over and over ... until I found out that it placed them into the %TEMP% directory where TB deleted them. Grrrr.

Comment 14

[rsx11m]

As another twist, if you start Thunderbird on Windows from within a Cygwin window, it's actually wiping out C:\Cygwin64\tmp (which is /tmp within Cygwin itself) rather than %TEMP%. Thus, it nicely bridges the POSIX barrier.

Your example in comment #13 fits well with my temptation earlier to raise the severity of this bug to "critical" (not that it matters much, but for better visibility). While I agree that /tmp is a bad location for your family photos, actual dataloss may occur if Thunderbird wipes out temporary files of another application while that one is running, thus causing it to malfunction.

Comment 15

[Jorg K (CEST = GMT+2)]

(In reply to rsx11m from comment #14)
> actual dataloss may occur if Thunderbird wipes out temporary
> files of another application while that one is running, thus causing it to
> malfunction.
My builds fail occasionally with this strange VS2013/2015 message:
  cannot read compiler command line
I googled it a bit and it seems to have to do with the temp directory.
Next time I do a big build, I will close TB before.

Comment 16

[(No longer employed by Mozilla) Aaron Klotz]

Jorg, you can add --disable-sandbox to your mozconfig to disable this on your local build. We're working on improving this so that no intervention is needed, but there's no point being frustrated in the meantime.

Comment 17

[Jorg K (CEST = GMT+2)]

(In reply to Aaron Klotz [:aklotz] from comment #16)
> Jorg, you can add --disable-sandbox to your mozconfig to disable this on
> your local build. We're working on improving this so that no intervention is
> needed, but there's no point being frustrated in the meantime.
Thanks for the reminder. You already mentioned it in comment #10 but I was lazy and left it to the people handle our build config. I will add --disable-sandbox to my mozconfig before I do a complete rebuild next which happens every couple of days (unless there is a quick way to just rebuild the affected part).

Aleth, how do you feel about adding this as per comment #11 so Daily builds stop deleting the temp directory?

Comment 18

[rsx11m]

(In reply to Jorg K (PTO during summer, NI me) from comment #17)
> Aleth, how do you feel about adding this as per comment #11 so Daily builds
> stop deleting the temp directory?

Isn't Earlybird affected as well? It says "status-thunderbird48: affected" in the Tracking section.

Comment 19

[Jorg K (CEST = GMT+2)]

This was caused by bug 1236108 which landed on mozilla48 and was uplifted to mozilla47 and mozilla46 as per bug 1236108 comment #67 on 2016-03-28.

In Earlybird 48.0a2 (2016-05-03) I don't see the problem, most likely do comment #8 (quote):
> If you're picking up gecko's config for MOZ_CONTENT_SANDBOX, then that's
> only defined for Nightly at the moment.

Anyway, bug 1270018 is on the way with a patch waiting for review, so perhaps this will land soon.

Comment 20

[:aceman]

(In reply to Jorg K (PTO during summer, NI me) from comment #7)
> Surely this will also be a problem on Linux. You'd have to hook into here:
> https://dxr.mozilla.org/comm-central/source/mozilla/xpcom/io/nsLocalFileUnix.
> cpp#1025
> adding printf ("***** |%s|\n", NS_ConvertUTF16toUTF8(mWorkingPath).get());
> and see what gets deleted upon application close.
> 
> Aceman, can you please take a look. Perhaps you also know where we need to
> unset MOZ_CONTENT_SANDBOX. You can see in comment #0 that this got landed in
> bug 1237847 and bug 1236108 on mozilla47/48 so we need to fix this soon in
> TB 49, 48 and the upcoming TB 47 beta.

I haven't noticed this would happen to me on Linux. I run debug builds of trunk and never noticed /tmp being cleared (at least the files belonging to the user).

Comment 21

[Jorg K (CEST = GMT+2)]

(In reply to Jorg K (PTO during summer, NI me) from comment #17)
> Aleth, how do you feel about adding this as per comment #11 so Daily builds
> stop deleting the temp directory?
You cleared the NI without answering the question :-(

Comment 22

[aleth [:aleth]]

Attachment: Disable content-sandbox on nightly builds

We can land this in the interim.

Comment 23

[aleth [:aleth]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=af627b120ebe

Comment 24

[(No longer employed by Mozilla) Aaron Klotz]

(In reply to :aceman from comment #20)
> I haven't noticed this would happen to me on Linux. I run debug builds of
> trunk and never noticed /tmp being cleared (at least the files belonging to
> the user).

The affected code is not compiled into linux builds.

Comment 25

[Jorg K (CEST = GMT+2)]

I don't see much progress in bug 1270018, so I suggest to review/land what Aleth suggested one day soon now.

Comment 26

[Jorg K (CEST = GMT+2)]

Comment on attachment 8753729 [details] [diff] [review]
Disable content-sandbox on nightly builds

Review of attachment 8753729 [details] [diff] [review]:
-----------------------------------------------------------------

I think it is very bad that our Dailies wipe out user's temp directories causing malfunction to other software. I'm one of those affected.
So r=jorgk.

On the issue of whether we should do --disable-content-sandbox or --disable-sandbox, I'm using the latter on my local build and I suggest to do the same here unless there are good reasons to do the former.

Comment 27

[Magnus Melin [:mkmelin]]

Yeah I suppose bug 1270018 seems stuck in limbo. Anyway, we should back this back out once that's fixed. Please add a comment to that affect in the "code" too.

Comment 28

[Jorg K (CEST = GMT+2)]

Great, so let's get rolling here ;-)

Comment 29

[aleth [:aleth]]

(In reply to Jorg K (PTO during summer, NI me) from comment #26)
> On the issue of whether we should do --disable-content-sandbox or
> --disable-sandbox, I'm using the latter on my local build and I suggest to
> do the same here unless there are good reasons to do the former.

Yes, there are good reasons ;) There are situations where TB may use the sandbox, e.g. the Flash plugins is still enabled by default.

Comment 30

[Jorg K (CEST = GMT+2)]

Good. Let's land it then with the extra comment Magnus requested.

Comment 31

[aleth [:aleth]]

https://hg.mozilla.org/comm-central/rev/25b7a4f905be8796089e771885ceba66773b6fe2
Bug 1269878 - Disable content-sandbox on nightly builds. r=mkmelin,jorgk

Comment 32

[Jorg K (CEST = GMT+2)]

This works for me. Files in %TEMP% are no longer removed.

Comment 33

[rsx11m]

Bug 1278547 attachment 8760777 [details] [diff] [review] should provide a core fix ahead of bug 1270018.

Comment 34

[Jorg K (CEST = GMT+2)]

Should we back this out now that bug 1278547 has landed?

Comment 35

[aleth [:aleth]]

(In reply to Jorg K (GMT+2, PTO during summer, NI me) from comment #34)
> Should we back this out now that bug 1278547 has landed?

We don't need to hurry with this as TB doesn't use the content sandbox afaik.

Comment 36

[Jorg K (CEST = GMT+2)]

This got backed out from TB49 since it caused problems during the beta build:
https://hg.mozilla.org/releases/comm-beta/rev/1840c4971ebe

Quoting from the drivers mailing list:
===
ERROR : found in mail/config/mozconfigs/macosx-universal/nightly but not in mail/config/mozconfigs/macosx-universal/release: ac_add_options --disable-content-sandbox
ERROR : found in mail/config/mozconfigs/win32/nightly but not in mail/config/mozconfigs/win32/release: ac_add_options --disable-content-sandbox
ERROR : found in mail/config/mozconfigs/linux64/nightly but not in mail/config/mozconfigs/linux64/release: ac_add_options --disable-content-sandbox
ERROR : found in mail/config/mozconfigs/linux32/nightly but not in mail/config/mozconfigs/linux32/release: ac_add_options --disable-content-sandbox

which appears to be from https://hg.mozilla.org/releases/comm-beta/rev/25b7a4f905be.  So --disable-content-sandbox needs to be added to the release mozconfigs ? If it genuinely should be different, then modify http://hg.mozilla.org/build/tools/file/default/buildbot-helpers/mozconfig_whitelist. Either way we don't need to do a build2.
===

Aleth, should be back this out from more repos?

Comment 37

[Jorg K (CEST = GMT+2)]

Aleth, should *we* back this out from more repos?

Comment 38

[aleth [:aleth]]

(In reply to Jorg K (GMT+2, PTO during summer) from comment #36)
> ERROR : found in mail/config/mozconfigs/macosx-universal/nightly but not in
> mail/config/mozconfigs/macosx-universal/release: ac_add_options
> --disable-content-sandbox

I don't understand why nightly and release mozconfigs are expected to match?

At any rate given comment 34 and the fact that this issue only ever affected nightlies, it seems we can either
1) backout this patch everywhere
or 
2) --disable-content-sandbox in all mozconfigs.

Comment 39

[Jorg K (CEST = GMT+2)]

So which do you prefer? I've already backed out one as a quick fix (comment #36).
Can I leave it with you, Aleth? I'm happy to do the backouts ;-)

Comment 40

[aleth [:aleth]]

(In reply to Jorg K (GMT+2, PTO during summer) from comment #39)
> So which do you prefer? I've already backed out one as a quick fix (comment
> #36).

I'd say just back it out, it's the simplest solution and the existing comment is obsolete.

If TB ever decides to disable content-sandbox for some reason, it should be done everywhere (not just in nightlies) and in a separate bug.

Comment 41

[Jorg K (CEST = GMT+2)]

Backout:
C-C: https://hg.mozilla.org/comm-central/rev/283d3bb819ea
C-A: https://hg.mozilla.org/releases/comm-aurora/rev/966855e84d61
C-B: https://hg.mozilla.org/releases/comm-beta/rev/1840c4971ebe (comment #36)