1358964 - Temp folder is not deleted on exit

Status: RESOLVED FIXED

(Core :: Security: Process Sandboxing, defect)

Description

[atlanto]

STR:
0. Ensure AppData\LocalLow\Mozilla is empty
1. Start firefox with -p option
2. Make sure Temp-... folder is created
3. Exit profile chooser

AR:
Temp-... folder is not deleted.

ER:
Temp-... folder is deleted.

On Windows10 1703 64bit and Firefox win64,
55.0a1 is affected.
35b4896888d3--mozilla-inbound--firefox-55.0a1.en-US.win64.zip is affected.
d7af59656111--mozilla-inbound--firefox-55.0a1.en-US.win64.zip is not affected.
53.0 is not affected. (maybe because there is no GPU process unless browser started...?)

This won't occur when started with default or some profile.

Comment 1

[Bob Owen (:bobowen)]

This is because when we are using the profile manager, we don't run nsXREDirProvider::DoShutdown, but we're creating the dir because of the GPU process.

Normally we only used to create in DoStartup, which doesn't run in this case either.

Looks like we don't really have a profile and the SavePrefFile actually fails because it doesn't exist.
Seems pretty odd that we return NS_OK in this circumstance.
bsmedberg - do you think we should change this?
Not sure what the knock-on effect might be.

Anyway, given that the content temp dir can now be created without calling DoStartup, maybe the deletion should be triggered by something other than DoShutdown.

Perhaps we could do it via the ShutdownPhase mechanism?

Comment 2

[Benjamin Smedberg]

I'm sorry Bob, I don't quite understand the question. The profile manager does run without a profile (which is one of the reasons I've long wanted to remove it). So yes, SavePrefFile should fail.

What function is returning NS_OK?

Comment 3

[Bob Owen (:bobowen)]

(In reply to Benjamin Smedberg [:bsmedberg] from comment #2)
> I'm sorry Bob, I don't quite understand the question. The profile manager
> does run without a profile (which is one of the reasons I've long wanted to
> remove it). So yes, SavePrefFile should fail.
> 
> What function is returning NS_OK?

SavePrefFile returns NS_OK, even though the save fails because aFile and mCurrentFile are both null in SavePrefFileInternal.

That seems weird and as it happens we wouldn't create the temp dir if it did fail, but that is a bit incidental.

However as I said, given that the content temp dir can now be created without calling DoStartup, maybe the deletion should be triggered by something that is guaranteed to happen, perhaps the XPCOM ShutdownPhase mechanism?
I think we'd have to create a special UniquePtr with a deleter that deletes the file or something.

Comment 4

[Benjamin Smedberg]

please let's not touch SavePrefFile, even though it's strange behavior.

I'm really fine with any solution that either doesn't create this temp file under some conditions (no profile dir) or deletes it appropriately. I'm surprised that we're not calling nsXREDirProvider::DoShutdown in this case, though: code inspection seems to indicate that we would, we'd just skip most of the body because of the mProfileNotified check. Could you move this just outside that check?

Comment 5

[Mike Taylor [:miketaylr]]

(ni? bob for bsmedberg's question)

Comment 6

[Bob Owen (:bobowen)]

I think I can steal this and pick it up next now.

(In reply to Benjamin Smedberg [:bsmedberg] from comment #4)
> please let's not touch SavePrefFile, even though it's strange behavior.
> 
> I'm really fine with any solution that either doesn't create this temp file
> under some conditions (no profile dir) or deletes it appropriately. I'm
> surprised that we're not calling nsXREDirProvider::DoShutdown in this case,
> though: code inspection seems to indicate that we would, we'd just skip most
> of the body because of the mProfileNotified check. Could you move this just
> outside that check?

You're right of course, that looks like it might be a simple fix for this case, I'll try it out.

I do think I need to rethink the deletion of these in general though, particular because of bug 1364496, but I'll file a separate bug for that.

Comment 7

[Bob Owen (:bobowen)]

Attachment: Always delete content process temp dir even when there is no profile

This fixes the issue when the profile manager is used.
As I said I'll file a follow-up to look into clearing up undeleted ones probably when we are idle similar to nsAnonymousTemporaryFile.

Comment 8

[Bob Owen (:bobowen)]

Attachment: Always delete content process temp dir even when there is no profile

Sorry about the churn here, but I just realised that we seem to be attemping to delete before the child processes have ended.
At least on my system, their shutdown happens during the rest of DoShutdown, so moving to the end gives us a much better chance of being able to delete it.

Comment 9

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/5e1cd577fb33
Always delete content process temp dir even when there is no profile. r=bsmedberg

Comment 10

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/5e1cd577fb33

Comment 11

[Bob Owen (:bobowen)]

Comment on attachment 8871700 [details] [diff] [review]
Always delete content process temp dir even when there is no profile

Approval Request Comment
[Feature/Bug causing the regression]:
Combination of GPU process and underlying potential issue with the way the content temp was deleted. 

[User impact if declined]:
People using the profile manager could get many undeleted temp directories left on disk. The fix also makes it more likely that the directory gets cleaned during normal shutdown.

[Is this code covered by automated tests?]:
Code is run during nearly all e10s tests.
There are also tests that check that we can write to content temp.

[Has the fix been verified in Nightly?]:
No but verified on local build.

[Needs manual test from QE? If yes, steps to reproduce]: 
Yes should be straight forward to test from STR in description. 

[List of other uplifts needed for the feature/fix]:
None

[Is the change risky?]:
No

[Why is the change risky/not risky?]:
Fix just moves very small piece of existing code within a function.

[String changes made/needed]:
None

Comment 12

[Julien Cristau [:jcristau]]

Bug 1329294 was uplifted to 53 and 54 so updating status.  Setting qe-verify+ based on comment 11; STR are in comment 0.

Comment 13

[Julien Cristau [:jcristau]]

Comment on attachment 8871700 [details] [diff] [review]
Always delete content process temp dir even when there is no profile

fix for temp dir deletion, beta54+, should be in 54.0b12

Comment 14

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/releases/mozilla-beta/rev/b8920ed27258

Comment 15

[Bogdan Maris [:bogdan_maris], Release Desktop QA]

Reproduced the initial issue using old Firefox from 2017-05-10, verified that the temp folder created when profile manager is opened will be deleted once one exits profile manager on latest Nightly but using Firefox 54 beta 12 I can't see any temp folder created when profile manager is opened on Windows 10. 

Is this the right behavior for Firefox 54?

Comment 16

[Bob Owen (:bobowen)]

(In reply to Bogdan Maris, QA [:bogdan_maris] from comment #15)
> Reproduced the initial issue using old Firefox from 2017-05-10, verified
> that the temp folder created when profile manager is opened will be deleted
> once one exits profile manager on latest Nightly but using Firefox 54 beta
> 12 I can't see any temp folder created when profile manager is opened on
> Windows 10. 
> 
> Is this the right behavior for Firefox 54?

Possibly, it all depends on whether the GPU process is getting started for the profile manager, perhaps it isn't in this case. I think you have to have the crash reporter enabled as well, but I think it normally should be for Beta.

Comment 17

[Bogdan Maris [:bogdan_maris], Release Desktop QA]

Hmm, the only difference I see between 55 beta and Nightly 56 is that 56 has two processes (could be the GPU process, I don't know shot to differentiate it from a normal process though) when starting the profile manager and 55 beta 12 does not so the Temp folder does not appear there so I can't mark this bug as verified fixed. I'm not sure how to enable the crash reporter (I did not modify my beta at all from when I installed it).

Do you know some way I could make the Temp folder appear for 55 as well?

Comment 18

[Milan Sreckovic [:milan] (needinfo for best results)]

We are creating the GPU process when just running the profile manager.  Don't think we do it on purpose, and have no opinion on whether we should stop. If we don't, it may get complicated to do it if the user continues by choosing a profile and running with it.

Comment 19

[Bob Owen (:bobowen)]

(In reply to Bogdan Maris, QA [:bogdan_maris] from comment #17)
> Hmm, the only difference I see between 55 beta and Nightly 56 is that 56 has
> two processes (could be the GPU process, I don't know shot to differentiate
> it from a normal process though) when starting the profile manager and 55
> beta 12 does not so the Temp folder does not appear there so I can't mark
> this bug as verified fixed. I'm not sure how to enable the crash reporter (I
> did not modify my beta at all from when I installed it).
> 
> Do you know some way I could make the Temp folder appear for 55 as well?

It was being triggered by the GPU process, not sure why that would have stopped starting for 55 when it went to Beta.
dvander might know.

(In reply to Milan Sreckovic [:milan] from comment #18)
> We are creating the GPU process when just running the profile manager. 
> Don't think we do it on purpose, and have no opinion on whether we should
> stop. If we don't, it may get complicated to do it if the user continues by
> choosing a profile and running with it.

I think a new firefox process gets launched once you've chosen a profile, not sure if that's always guaranteed to happen.

Comment 20

[Benjamin Smedberg]

New process always happens after the profile dialog.

Comment 21

[David Anderson [:dvander] - inactive, e-mail if emergency]

If acceleration was disabled, that could explain why it stopped starting - software compositor process is (was) nightly only.

Comment 22

[Bogdan Maris [:bogdan_maris], Release Desktop QA]

I can confirm that for Beta, the Temp appears only after I select a profile and start Firefox. After I close Beta the Temp file will be deleted.