765598 - Remove newly created MozUpdater folders in $TEMP on post update

Status: RESOLVED FIXED

(Toolkit :: Application Update, defect)

Description

[Jo Hermans]

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 ?

Comment 1

[hannu.nyman]

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 :-(

Comment 2

[Robert Strong (they/them - no direct email)]

Brian, could you take a look at this when you have the time?

Comment 3

[Brian R. Bondy [:bbondy]]

Sure, will do.

Comment 4

[Brian R. Bondy [:bbondy]]

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.

Comment 5

[Bill Gianopoulos [:WG9s]]

Changing to OS --> same issue occurs in Linux.

Comment 6

[Bill Gianopoulos [:WG9s]]

Meant to say OS --> ALL

Comment 7

[Brian R. Bondy [:bbondy]]

> 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.

Comment 8

[hannu.nyman]

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  :-(

Comment 9

[Brian R. Bondy [:bbondy]]

I'll get this done for mozilla18

Comment 10

[Brian R. Bondy [:bbondy]]

Attachment: Patch v1.

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).

Comment 11

[Brian R. Bondy [:bbondy]]

Attachment: Patch v2.

Same as before just added a comment before the new function.

Comment 12

[Robert Strong (they/them - no direct email)]

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.

Comment 13

[Robert Strong (they/them - no direct email)]

btw: I'm thinking of B2G regarding platforms.

Comment 14

[Robert Strong (they/them - no direct email)]

Also, do we have reports of this happening on Mac as well?

Comment 15

[Brian R. Bondy [:bbondy]]

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.

Comment 16

[Robert Strong (they/them - no direct email)]

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.

Comment 17

[(no longer active)]

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

Comment 18

[Brian R. Bondy [:bbondy]]

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

Comment 19

[Robert Strong (they/them - no direct email)]

I'm convinced and thanks!

Comment 20

[Brian R. Bondy [:bbondy]]

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 :)

Comment 21

[(no longer active)]

(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.

Comment 22

[Brian R. Bondy [:bbondy]]

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.

Comment 23

[Brian R. Bondy [:bbondy]]

Attachment: Patch v3.

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).

Comment 24

[Brian R. Bondy [:bbondy]]

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 :)

Comment 25

[(no longer active)]

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.

Comment 26

[Brian R. Bondy [:bbondy]]

Good idea, and thanks for the review.

Comment 27

[Brian R. Bondy [:bbondy]]

Attachment: Patch v4.

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+.

Comment 28

[Brian R. Bondy [:bbondy]]

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?

Comment 29

[Brian R. Bondy [:bbondy]]

https://tbpl.mozilla.org/?tree=Oak&rev=474fab403fcb

Comment 30

[Robert Strong (they/them - no direct email)]

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

Comment 31

[(no longer active)]

(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?

Comment 32

[Robert Strong (they/them - no direct email)]

Oh, you copied over the old bug description and not the log message. You could also increase the timeout then.

Comment 33

[Robert Strong (they/them - no direct email)]

(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.

Comment 34

[Brian R. Bondy [:bbondy]]

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.

Comment 35

[Brian R. Bondy [:bbondy]]

Attachment: Patch v1 - Increase test timeout to give time for mass MozUpdater delete

This seems like the fastest way to stop getting the errors.  We can re-decrease it later if we want to.

Comment 36

[Robert Strong (they/them - no direct email)]

Definitely decrease it after this has made the rounds across the build system.

Comment 37

[(no longer active)]

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.

Comment 38

[(no longer active)]

Comment on attachment 674685 [details] [diff] [review]
Patch v4.

See above.

Comment 39

[Brian R. Bondy [:bbondy]]

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?

Comment 40

[Brian R. Bondy [:bbondy]]

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).

Comment 41

[(no longer active)]

(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!

Comment 42

[(no longer active)]

(You can land the first part of this patch, r=me)

Comment 43

[Brian R. Bondy [:bbondy]]

Attachment: Patch v5.

Comment 44

[(no longer active)]

(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.

Comment 45

[Brian R. Bondy [:bbondy]]

doh!

Comment 46

[Brian R. Bondy [:bbondy]]

http://hg.mozilla.org/integration/mozilla-inbound/rev/3a68d4377cd8

Comment 47

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/3a68d4377cd8

Comment 48

[Satya Nemana]

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

Comment 49

[Satya Nemana]

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

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