Closed Bug 765598 Opened 8 years ago Closed 7 years ago

Remove newly created MozUpdater folders in $TEMP on post update

Categories

(Toolkit :: Application Update, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla19

People

(Reporter: jo.hermans, Assigned: bbondy)

References

Details

Attachments

(2 files, 5 obsolete files)

I noticed that there's a $TEMP/MozUpdater folder, which contains updater.exe. However, MozUpdater is readonly, so every day I get a new file (MozUpdater-1, MozUpdater-2, etc ...). Shouldn't this folder be removed after the update ?
Bug is still there.
Mozilla/5.0 (Windows NT 6.1; WOW64; rv:16.0) Gecko/16.0 Firefox/16.0 ID:20120802042009

If removing the directory is not suitable for the update logic, there might be two such directories  e.g. mozupdater-1 and mozupdater-2, and the update logic could rotate between them. Or something like that. 

The current approach of endlessy creating new directories sounds a bit like the growing newtab thumbnail directory size in bug 754671. Mozilla should not knowingly fill user's disk with temp files :-(
Brian, could you take a look at this when you have the time?
Sure, will do.
QA Contact: netzen
Target Milestone: --- → mozilla17
I looked into this.

It's not related to the read only attribute on the directory. The subfiles don't have that attribute by the way.  
The problem is that no logic was ever added to delete this directory.

Why we do this copy in the first place is because with background updates, when we want to 'replace' the old dir with the updated dir, we have to use updater.exe to do that.  Since we're moving the installdir and installdir/udpated dirs, we can't use the one in those locations.  So a new one is copied to the OS temp directory.   We then execute updater.exe from that new location to do the replace operation.  Possibly running through the service on Windows if needed.
 
The best thing to do to fix this is probably after a successful update, once we are back in firefox.exe, to clear out such directories.
We could clear out all but the current dir from updater.exe in the applied operation, but then we'd have to duplicate the logic in GetSpecialSystemDirectory in updater code.  

So I think it's best to do it in firefox.exe
Since people might have a bunch of these already we should probably do this with a low IO priority thread in the background off the main thread.
Blocks: bgupdates
Changing to OS --> same issue occurs in Linux.
OS: Windows XP → All
Hardware: x86 → All
Meant to say OS --> ALL
> So I think it's best to do it in firefox.exe
> Since people might have a bunch of these already we should probably do this
> with a low IO priority thread in the background off the main thread.

Talked to Rob, if people already have a lot of these folders, it will only be once that we'll have to delete them all, and they'll be on the Nightly channel, so we can just do this as part of the cleanup.  

In the future we can change to a background thread for deleting these folders and other files we cleanup after updates.  But it's not needed to fix this regression.
Now that also Thunderbird uses background updates and they are used also on the Aurora channel, two new mozupdater directories are created daily for users with Nightly/Aurora and Daily/Earlybird  :-(
I'll get this done for mozilla18
Target Milestone: mozilla17 → mozilla18
Assignee: nobody → netzen
QA Contact: netzen
Summary: multiple MozUpdater folders in $TEMP → Remove multiple MozUpdater folders in $TEMP on PostUpdate
Attached patch Patch v1. (obsolete) — Splinter Review
Tested this just by calling the new function in a js-console and it removed my 52 MozUpdater bgupdate/staged dirs.  After review I'll also push this to oak just to be sure.  (I only do the pushing to oak for updater related bugs by the way).
Attachment #673269 - Flags: review?(robert.bugzilla)
Attached patch Patch v2. (obsolete) — Splinter Review
Same as before just added a comment before the new function.
Attachment #673269 - Attachment is obsolete: true
Attachment #673269 - Flags: review?(robert.bugzilla)
Attachment #673271 - Flags: review?(robert.bugzilla)
Comment on attachment 673271 [details] [diff] [review]
Patch v2.

I don't think it makes since to call that and enumerate the tmp dir for platforms that will never have these dirs. Also, could it be made to only ever have one MozUpdater dir so that at some point in the future the tmp dir is never enumerated? In other words, I think this enumeration should only happen one time.
Attachment #673271 - Flags: review?(robert.bugzilla) → review-
btw: I'm thinking of B2G regarding platforms.
Also, do we have reports of this happening on Mac as well?
I think we have background updates on all platforms.
Regarding it being a one time operation, I think a sane way to do this would be to check for MozUpdater-1 and if it exists then likely multiple exist, do enumeration.
If it doesn't simply delete MozUpdater only.
I don't recall if it simply appends -1 but that would be sufficient if it does. I asked about other platforms because I wouldn't be surprised if it was possible to always delete the MozUpdater folder on Mac and B2G shouldn't have to take this hit if we can gaurantee to always use MozUpdater.
This happens on all platforms, including Mac:

 ls /Users/ehsanakhgari/Library/Caches/TemporaryItems/346337_test1.file                    clipboardcache-1
346337_test2.file                    clipboardcache-2
466937_test.file                     clipboardcache-3
AdobeIDataSync-CDataSync_AddressBook clipboardcache-4
MozUpdater                           clipboardcache-5
MozUpdater-1                         clipboardcache-6
MozUpdater-2                         clipboardcache-7
MozUpdater-3                         nsemail.eml
MozUpdater-4                         plugtmp
MozUpdater-5                         plugtmp-1
Outlook Temp                         tmp-y46.xpi
clipboardcache
It uses MozUpdater-i where i is the first unused index.
http://dxr.mozilla.org/mozilla-central/toolkit/xre/nsUpdateDriver.cpp.html#l380

By the way I think the implementation of CreateUnique will fail after there are 1000, so it's good we're fixing this now. I think it would make updates completely fail at 1000.
http://dxr.mozilla.org/mozilla-central/xpcom/io/nsLocalFileCommon.cpp.html#l138
Actually I think it would just use MozUpdater in the worst case again.
http://dxr.mozilla.org/mozilla-central/toolkit/xre/nsUpdateDriver.cpp.html#l280
But we have to fix anyway :)
(In reply to comment #16)
> I don't recall if it simply appends -1 but that would be sufficient if it does.
> I asked about other platforms because I wouldn't be surprised if it was
> possible to always delete the MozUpdater folder on Mac and B2G shouldn't have
> to take this hit if we can gaurantee to always use MozUpdater.

The prefixes will only kick in if the MozUpdater folder already exists.  It's just done as a way to make sure that we get a unique file name.  Theoretically we can't rely on being able to create the MozUpdater directory if another application (perhaps Thunderbird) creates it under our nose.
So maybe the best thing to do is create a subfolder that we own, and then have a unique subfolder underneath that.  Then we can just recursively delete that base folder we own.

I guess we could prefix the product name and brand name in case we have 2 things doing updates at the same time.  If we even care about that case.
Attached patch Patch v3. (obsolete) — Splinter Review
I don't think we need to worry about the edge case of 2 updates going at the same time and one of them deleting the folders of the other one, but let me know if you disagree.

I kept the code for a unique folders in case there's an old folder that can't be removed with a file in use for example.
With the new patch, we only enumerate the temp folder if MozUpdater-1 exists.  If that exists, that means that we haven't cleared multiple directories yet.
Otherwise we'll just recursively delete only the MozUpdater folder (which is the new location of the unique subfolders).
Attachment #673271 - Attachment is obsolete: true
Attachment #673501 - Flags: review?(robert.bugzilla)
Comment on attachment 673501 [details] [diff] [review]
Patch v3.

Moving this review to ehsan since I added a different B2G basecamp blocker updater bug, and time is limited :)
Attachment #673501 - Flags: review?(robert.bugzilla) → review?(ehsan)
Comment on attachment 673501 [details] [diff] [review]
Patch v3.

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

::: toolkit/mozapps/update/nsUpdateService.js
@@ +753,5 @@
>  
>  /**
> + * Removes the MozUpdater folders that bgupdates/staged updates creates.
> + */
> +function cleanUpMozUpdaterDirs() {

Please wrap the stuff here in a try/catch block, and log any possible exception thrown, but don't let them propagate to the callers.
Attachment #673501 - Flags: review?(ehsan) → review+
Good idea, and thanks for the review.
Attached patch Patch v4. (obsolete) — Splinter Review
Added surrounding try/catch block and log the exception if an exception is thrown.  I do not let the exception propagate since this is a non-fatal error.

Carrying forward r+.
Attachment #673501 - Attachment is obsolete: true
Attachment #674685 - Flags: review+
I'm seeing this come back on a couple of OSX tests:

Bug 645607 - Intermittent failure in test_0072_notify_verifyFailComplete_noPartial.xul or test_0081_error_patchApplyFailure_partial_only.xul | Test timed out. Maximum time allowed is 20 seconds

I'm thinking it's just because of extra time spent deleting those folders on talos machines though. Ehsan, thoughts?
You could add a log statement for each directory removed in nsUpdateService.js and then flip the following to true to get more info.
http://mxr.mozilla.org/mozilla-central/source/toolkit/mozapps/update/test/chrome/utils.js#184

Also, I'm not sure why that is showing a 20 second timeout... it should be 25 seconds.
http://mxr.mozilla.org/mozilla-central/source/toolkit/mozapps/update/test/chrome/utils.js#158
(In reply to comment #28)
> I'm seeing this come back on a couple of OSX tests:
> 
> Bug 645607 - Intermittent failure in
> test_0072_notify_verifyFailComplete_noPartial.xul or
> test_0081_error_patchApplyFailure_partial_only.xul | Test timed out. Maximum
> time allowed is 20 seconds
> 
> I'm thinking it's just because of extra time spent deleting those folders on
> talos machines though. Ehsan, thoughts?

That's quite likely.  Those machines have probably gathered thousands of those directories... :(

Perhaps we can add something to the boot scripts which deletes these folders on those machines?
Oh, you copied over the old bug description and not the log message. You could also increase the timeout then.
(In reply to Ehsan Akhgari [:ehsan] from comment #31)
> (In reply to comment #28)
> > I'm seeing this come back on a couple of OSX tests:
> > 
> > Bug 645607 - Intermittent failure in
> > test_0072_notify_verifyFailComplete_noPartial.xul or
> > test_0081_error_patchApplyFailure_partial_only.xul | Test timed out. Maximum
> > time allowed is 20 seconds
> > 
> > I'm thinking it's just because of extra time spent deleting those folders on
> > talos machines though. Ehsan, thoughts?
> 
> That's quite likely.  Those machines have probably gathered thousands of
> those directories... :(
> 
> Perhaps we can add something to the boot scripts which deletes these folders
> on those machines?
You could also add a test that does nothing but remove those directories that increases the timeout until it has removed all of these directories.
Re Comment 32: Ya it was just the bug title, not the actual log. 
I'll opt for increasing the timeout for now and we can re-decrease it later if we want to.
This seems like the fastest way to stop getting the errors.  We can re-decrease it later if we want to.
Attachment #674938 - Flags: review?(ehsan)
Definitely decrease it after this has made the rounds across the build system.
Blocks: 805313
Attachment #674938 - Flags: review?(ehsan) → review+
Comment on attachment 674938 [details] [diff] [review]
Patch v1 - Increase test timeout to give time for mass MozUpdater delete

Actually, I changed my mind.

This can lead to a bad user experience for Nightly users who might have quite a few of these folders.  Perhaps we should change the code to limit the number of directories that we attempt to delete each time.  We can pick an arbitrary number, like 10, I suppose.
Attachment #674938 - Flags: review+ → review-
Comment on attachment 674685 [details] [diff] [review]
Patch v4.

See above.
Attachment #674685 - Flags: review+ → review-
One problem with the approach of deleting 10 is that we are forcing users to enumerate most of the temp directory multiple times instead of one. 
I think we should split this bug into 2. 

The first bug would fix the problem moving forward and can land right away.
The second bug would find a solution for deleting the old MozUpdater folders efficiently.

Sound good?
So here's what I think is best.
1. Read Comment 39 
2. In that follow up bug we do the delete of MozUpdater-i inside NSIS in the /PostUpdate which runs async unless specified otherwise in updater.ini (which I don't think we ever set).
(In reply to comment #39)
> One problem with the approach of deleting 10 is that we are forcing users to
> enumerate most of the temp directory multiple times instead of one. 
> I think we should split this bug into 2. 
> 
> The first bug would fix the problem moving forward and can land right away.
> The second bug would find a solution for deleting the old MozUpdater folders
> efficiently.
> 
> Sound good?

Yes!
(You can land the first part of this patch, r=me)
Attachment #674938 - Attachment is obsolete: true
Attachment #674938 - Flags: review-
Summary: Remove multiple MozUpdater folders in $TEMP on PostUpdate → Remove newly created MozUpdater folders in $TEMP on post update
Attached patch Patch v5.Splinter Review
Attachment #674685 - Attachment is obsolete: true
Attachment #675155 - Flags: review?(ehsan)
Depends on: 805466
No longer blocks: 805313
(In reply to Brian R. Bondy [:bbondy] from comment #40)
> 2. In that follow up bug we do the delete of MozUpdater-i inside NSIS in the
> /PostUpdate which runs async unless specified otherwise in updater.ini
> (which I don't think we ever set).

That's not good enough, since this bug is not Windows only.
doh!
Attachment #675155 - Flags: review?(ehsan) → review+
http://hg.mozilla.org/integration/mozilla-inbound/rev/3a68d4377cd8
Target Milestone: mozilla18 → mozilla19
https://hg.mozilla.org/mozilla-central/rev/3a68d4377cd8
Status: NEW → RESOLVED
Closed: 7 years ago
Flags: in-testsuite-
Resolution: --- → FIXED
Norton File Insight display all unknown for MozUpdater\bgupdate\updater.exe

Thanks for fixing this. I just want to share my recent observations when my Norton popped up a File Insight alert. I captured this from the popup window and copied its text below. My local c:\Users\satya\AppData\Local\Temp\MozUpdater folder with updater.exe disappeared, after I rebooted my laptop, as prompted by my Norton tool. It means this bug fix is working well. Just verifying & sharing.

Following is the Norton File Insight Popup dialogue box report details. It is better if Norton report this updater.exe with proper identification as Mozilla Development team in stead of leaving all as "unknown" to raise confidence to the Mozilla User community. By seeing "unknown" in Norton, it raises curiosity and further research like I did now and finally landed into this BugZilla thread.
 
Filename: updater.exe
Full Path: c:\Users\satya\AppData\Local\Temp\MozUpdater\bgupdate\updater.exe

____________________________

Details
Stability Unknown,  Unknown Community Usage,  Unknown Age,  Unproven

Origin
Downloaded from Unknown

Activity
Actions performed: Suspicious actions performed: None

____________________________


Developers Not Available
Version Not Available
Identified Not Available
Last Used Not Available
Startup Item No

____________________________


Unknown
This program crash history is not known.

Unknown
It is unknown how many users in the Norton Community have used this file.

Unknown
This file release is currently not known.

Unproven
There is not enough information about this file to recommend it.


____________________________


____________________________


File Thumbprint - SHA:
0273d10497d0caba9ef71ca17cab2ace972363a343c537d60a818b171f04ec16
File Thumbprint - MD5:
Not available

-Satya Nemana
This is related to my comment after verifying another bug fix 765598 which I copied below. Perhaps this fix for this could be to add proper identification as the Mozilla Developer to let Norton know when they pop up this File Insight details to avoid further curiosity or suspicions, when it pops up desktop alert.

Norton File Insight display all "unknown" for MozUpdater\bgupdate\updater.exe

Check for more details and comments under Bug 765598 - Remove newly created MozUpdater folders in $TEMP on post update. My comments are copied below.

"Thanks for fixing this. I just want to share my recent observations when my Norton popped up a File Insight alert. I captured this from the popup window and copied its text below. My local c:\Users\satya\AppData\Local\Temp\MozUpdater folder with updater.exe disappeared, after I rebooted my laptop, as prompted by my Norton tool. It means this bug fix is working well. 

Following is the Norton File Insight Popup dialogue box report details. It is better if Norton report this updater.exe with proper identification as Mozilla Development team in stead of leaving all as "unknown" to raise confidence to the Mozilla User community. By seeing "unknown" in Norton, it raises curiosity and further research like I did now and finally landed into this BugZilla thread.
 
Filename: updater.exe
Full Path: c:\Users\satya\AppData\Local\Temp\MozUpdater\bgupdate\updater.exe

____________________________

Details
Stability Unknown,  Unknown Community Usage,  Unknown Age,  Unproven

Origin
Downloaded from Unknown

Activity
Actions performed: Suspicious actions performed: None

____________________________


Developers Not Available
Version Not Available
Identified Not Available
Last Used Not Available
Startup Item No

____________________________


Unknown
This program crash history is not known.

Unknown
It is unknown how many users in the Norton Community have used this file.

Unknown
This file release is currently not known.

Unproven
There is not enough information about this file to recommend it.


____________________________


____________________________


File Thumbprint - SHA:
0273d10497d0caba9ef71ca17cab2ace972363a343c537d60a818b171f04ec16
File Thumbprint - MD5:
Not available"

-Satya Nemana
You need to log in before you can comment on or make changes to this bug.