Opened 4 years ago

Closed 4 years ago

#16236 closed defect (fixed)

Windows updater: avoid writing to the Windows registry for "in use" files

Reported by: mcs Owned by: mcs
Priority: Medium Milestone:
Component: Applications/Tor Browser Version:
Severity: Keywords: tbb-5.0a4, tbb-disk-leak, TorBrowserTeam201507R
Cc: brade, gk, mikeperry Actual Points:
Parent ID: Points:
Reviewer: Sponsor:

Description

This is a spinoff from ticket #16014.

On Windows, when the Mozilla updater encounters a file that is in use (which means it cannot be deleted), it moves the file to a "tobedeleted" directory and then uses a call like the following to arrange for it to be deleted the next time Windows is restarted:

MoveFileEx(path, nullptr, MOVEFILE_DELAY_UNTIL_REBOOT);

This works fine, but it would be better to avoid writing to the registry. We could fix this by adding code to nsUpdateService.js to make a "best effort" attempt to delete all files in the "tobedeleted" directory each time the browser is restarted.

Let's consider doing this for Tor Browser 5.

Child Tickets

Change History (7)

comment:1 Changed 4 years ago by mikeperry

Keywords: tbb-disk-leak added

comment:2 Changed 4 years ago by mikeperry

Keywords: tbb-5.0a4 added; tbb-5.0-alpha removed

Putting this in 5.0a4 for now. We can re-evaluate later in the week.

comment:4 Changed 4 years ago by mcs

Keywords: TorBrowserTeam201507R added

comment:5 Changed 4 years ago by mikeperry

This looks OK, I think, though I'm not sure it does exactly what the commit message says. Apparently there already was code to try to delete this directory at startup (based on the comment and surrounding code)? Doesn't that indicate that the extra deletion attempt in ProcessUpdates() is probably unnecessary?

How risky is this in general? Is the most likely failure mode that users end up with a pile of stuff in tobedeleted? It looks like we're safe against namespace collision due to the use of random temporary directory names... Should we take any precautions with respect to not rolling this out in 5.0-stable until we've had an additional alpha or two?

comment:6 in reply to:  5 Changed 4 years ago by mcs

Replying to mikeperry:

This looks OK, I think, though I'm not sure it does exactly what the commit message says. Apparently there already was code to try to delete this directory at startup (based on the comment and surrounding code)? Doesn't that indicate that the extra deletion attempt in ProcessUpdates() is probably unnecessary?

The existing deletion code (which our patch removes via #ifdef's) is in the updater executable, and that code only runs after an update is applied. There is also some deletion/clean up code in the Firefox Windows installer which we do not use. I do not think there is any existing deletion code that runs at startup.

How risky is this in general? Is the most likely failure mode that users end up with a pile of stuff in tobedeleted? It looks like we're safe against namespace collision due to the use of random temporary directory names... Should we take any precautions with respect to not rolling this out in 5.0-stable until we've had an additional alpha or two?

Kathy and I think it is safe. The worst case scenario is that files are not removed. But we did test it and it seems to work.

comment:7 Changed 4 years ago by mikeperry

Resolution: fixed
Status: needs_reviewclosed

Ok, merged for 5.0a4 then. Thanks!

Note: See TracTickets for help on using tickets.