New issue
Advanced search Search tips

Issue 668120 link

Starred by 1 user

Issue metadata

Status: Closed
Owner:
Closed: Nov 23
Cc:
Components:
EstimatedDays: ----
NextAction: ----
OS: Windows
Pri: 2
Type: Bug



Sign in to add a comment

Make deleting of files from the unistaller more robust

Project Member Reported by pastarmovj@chromium.org, Nov 23 2016

Issue description

Files that are kept open by some process other than chrome will cause the uninstall to stop deleting files altogether.

Come up with way to make this process more robust and make sure the installation directory is empty at least after the next reboot.

See https://cs.chromium.org/chromium/src/chrome/installer/setup/uninstall.cc?rcl=0&l=573 for reference.

Concrete issue that exhibits the problem is the eventlog_provider.dll that is being kept open by the Event Viewer.
 
Status: Started (was: Assigned)
Greg, I was thinking about the solution to this issue and here is my analysis:

Currently we do the following :

I. For each file in the installation location:
  1. Try to delete file. If successful go to next file.
  2. Try to close all Chrome processes.
  3. Try to delete file once more. If NOT successful break out of the loop and return DELETE_FAILED.
II. Return DELETE_SUCCEEDED
and here is what I propose we implement instead:

  1. Try to delete file. If successful go to next file.
  2. Try to close all Chrome processes (remember we tries it makes no sense to try at each file).
  3. Try to delete file once more. If successful go to next file.
  4. If not running as admin continue to next file.
  5. Rename file to a new name e.g. <original_name>.todel. If NOT successful go to next file. For the reason why see: https://marc.durdin.net/2011/09/why-you-should-not-use-movefileex-with-movefile_delay_until_reboot-2/
  6. Use MoveFileEx e.g. base::DeleteFileAfterReboot to schedule it for deletion after reboot.
II. If at least one file failed to delete and could not be scheduled for removal return DELETE_FAILED.
III. If at least one file was scheduled for delete on reboot return DELETE_REBOOT_REQURED.
IV. Return DELETE_SUCCEEDED.

It is important to rename files before deleting them (Even with unique names) to allow for installing Chrome again without restarting the computer. Alternatively renaming the containing top folder might be sufficient.

Wdyt?

Comment 2 by grt@chromium.org, Jan 9 2017

Regarding Marc's blog post, we have code in delete_after_reboot_helper.cc to handle the "re-install before reboot" case. I think we're on okay ground to use MoveFileEx so long as we remember to clear relevant delete values out during install. I hope we're doing that today. :-)

Renaming the top-level directory will fail if any file within it is open, so it's a non-starter to do that (setup.exe is within it). Also, I think the special handling of W.X.Y.Z\Installer is still needed.

When moving files to a new name, they may as well be moved into %TEMP% (or some temp dir on the same volume) so that if all delete attempts fail, they're at least in a dir where they will eventually be reclaimed. https://codereview.chromium.org/2545283002/diff/200001/base/files/file_util_win.cc has some code for finding a writeable same-volume temp dir.

I think this could be a fairly hairy thing to get correct in all cases, and seems to duplicate some of what i'm working on in https://codereview.chromium.org/2545283002/. Since the immediate problem is the event viewer preventing Chrome's uninstall from working, does it make sense to focus on that one file for now?

Can you check to see if event viewer opens the provider dll with FILE_SHARE_DELETE? If yes, we can likely move it to a temp dir and then mark it for delete on close.
Yes it is opened with FILE_SHARE_DELETE (if this is what Sharing: Read, Delete means in procmon.

I see you are already onto the full solution anyways so do you want to merge this issue into 571906? 

Provided that XP is not in the picture anymore there should be nothing in the way anymore :) 

Comment 4 by grt@chromium.org, Jan 10 2017

The robust delete plus some refactoring in uninstall.cc may be enough. I wonder if we should change uninstall so that it moves setup into %TEMP% first and then deletes the install dir as aggressively as possible. Is there any benefit to the current approach in DeleteChromeFilesAndFolders? Maybe all of that deletion can be simplified.
Status: Closed (was: Started)
I am going to close this bug. I think we have not heard much in terms of complaints about the installer recently.

Sign in to add a comment