Closed
Bug 1358964
Opened 5 years ago
Closed 5 years ago
Temp folder is not deleted on exit
Categories
(Core :: Security: Process Sandboxing, defect)
Tracking
()
RESOLVED
FIXED
mozilla55
Tracking | Status | |
---|---|---|
firefox-esr52 | --- | unaffected |
firefox53 | --- | wontfix |
firefox54 | --- | fixed |
firefox55 | --- | fixed |
People
(Reporter: euthanasia_waltz, Assigned: bobowen)
References
Details
(Keywords: regression, Whiteboard: sb+)
Attachments
(1 file, 1 obsolete file)
2.01 KB,
patch
|
benjamin
:
review+
jcristau
:
approval-mozilla-beta+
|
Details | Diff | Splinter Review |
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.
![]() |
||
Updated•5 years ago
|
![]() |
||
Updated•5 years ago
|
status-firefox54:
--- → unaffected
status-firefox55:
--- → affected
Updated•5 years ago
|
status-firefox53:
--- → unaffected
Assignee | ||
Comment 1•5 years ago
|
||
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?
Flags: needinfo?(benjamin)
Updated•5 years ago
|
Assignee: nobody → davidp99
![]() |
||
Updated•5 years ago
|
Flags: needinfo?(davidp99)
Whiteboard: sbwc2, sbmc2, sblc3
Comment 2•5 years ago
|
||
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?
Flags: needinfo?(benjamin)
Assignee | ||
Comment 3•5 years ago
|
||
(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.
Flags: needinfo?(benjamin)
![]() |
||
Updated•5 years ago
|
Whiteboard: sbwc2, sbmc2, sblc3 → sb+
![]() |
||
Updated•5 years ago
|
Flags: needinfo?(davidp99)
Comment 4•5 years ago
|
||
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?
Flags: needinfo?(benjamin)
Assignee | ||
Comment 6•5 years ago
|
||
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.
Assignee: davidp99 → bobowencode
Flags: needinfo?(bobowencode)
Assignee | ||
Comment 7•5 years ago
|
||
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.
Attachment #8871358 -
Flags: review?(benjamin)
Assignee | ||
Comment 8•5 years ago
|
||
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.
Attachment #8871700 -
Flags: review?(benjamin)
Assignee | ||
Updated•5 years ago
|
Attachment #8871358 -
Attachment is obsolete: true
Attachment #8871358 -
Flags: review?(benjamin)
Updated•5 years ago
|
Attachment #8871700 -
Flags: review?(benjamin) → review+
Pushed by bobowencode@gmail.com: 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•5 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/5e1cd577fb33
Status: NEW → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla55
Assignee | ||
Comment 11•5 years ago
|
||
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
Attachment #8871700 -
Flags: approval-mozilla-beta?
Updated•5 years ago
|
status-firefox-esr52:
--- → unaffected
Comment 12•5 years ago
|
||
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•5 years ago
|
||
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
Attachment #8871700 -
Flags: approval-mozilla-beta? → approval-mozilla-beta+
Comment 14•5 years ago
|
||
bugherderuplift |
https://hg.mozilla.org/releases/mozilla-beta/rev/b8920ed27258
Comment 15•5 years ago
|
||
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?
Flags: needinfo?(bobowencode)
Assignee | ||
Comment 16•5 years ago
|
||
(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.
Flags: needinfo?(bobowencode)
Comment 17•5 years ago
|
||
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?
Flags: needinfo?(bobowencode)
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.
Assignee | ||
Comment 19•5 years ago
|
||
(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.
Flags: needinfo?(bobowencode) → needinfo?(dvander)
Comment 20•5 years ago
|
||
New process always happens after the profile dialog.
If acceleration was disabled, that could explain why it stopped starting - software compositor process is (was) nightly only.
Flags: needinfo?(dvander)
Comment 22•5 years ago
|
||
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.
You need to log in
before you can comment on or make changes to this bug.
Description
•