Delete old files after an update.

Delete old files after an update.

This CL introduces a WorkItem which deletes old executables and version
directories from the Chrome installation directory.

For now, the WorkItem is used at the end of an update and at the
end of a new_chrome.exe->chrome.exe rename. Eventually, it will also
be invoked repetitively from a cleanup process launched when the
WorkItem fails to delete everything the first time.

BUG=451546
Committed: https://crrev.com/1b50c70773051b24925b46bab8b006e606633c5c
Cr-Commit-Position: refs/heads/master@{#378802}

[fdoray, 2016-02-04 22:07:07]

fdoray@chromium.org changed reviewers:
+ grt@chromium.org

[fdoray, 2016-02-04 22:07:08]

Can you review this CL?

I think that having a "bool success" is simpler than removing paths from the
executable/directory collections during the iterations, like we planned in the
design doc. Let me know if you disagree.

I support the case where there is multiple old_chrome*.exe with the same
version. Let me know if you don't think it's necessary.

Thanks!

[fdoray, 2016-02-05 22:56:41]

https://codereview.chromium.org/1666363002/diff/1/chrome/installer/setup/inst...
File chrome/installer/setup/install_worker.cc (right):

https://codereview.chromium.org/1666363002/diff/1/chrome/installer/setup/inst...
chrome/installer/setup/install_worker.cc:1023: &DeleteOldVersionsCallback,
installer_state.target_path()));
Note to myself: I've just seen that  InstallerState::RemoveOldVersionDirectories
exists. Make sure that my code interacts correctly with this method.

[grt (UTC plus 2), 2016-02-10 20:02:04]

Can the old RemoveOldVersionDirectories simply be removed? What you're building
here seems to be a more robust replacement. First round of comments below.

https://codereview.chromium.org/1666363002/diff/1/chrome/installer/util/delet...
File chrome/installer/util/delete_old_versions.cc (right):

https://codereview.chromium.org/1666363002/diff/1/chrome/installer/util/delet...
chrome/installer/util/delete_old_versions.cc:46: const base::string16
directory_name = directory_path.BaseName().value();
nit: make this and the container use base::FilePath rather than string16.

https://codereview.chromium.org/1666363002/diff/1/chrome/installer/util/delet...
chrome/installer/util/delete_old_versions.cc:49:
directories.insert(directory_name);
for the sake of caution, perhaps this should only consider directories that are
named W.X.Y.Z. wdyt?

https://codereview.chromium.org/1666363002/diff/1/chrome/installer/util/delet...
chrome/installer/util/delete_old_versions.cc:57: using ExecutableVector =
std::vector<base::FilePath>;
nit: change this type to PathVector and use it on line 122

https://codereview.chromium.org/1666363002/diff/1/chrome/installer/util/delet...
chrome/installer/util/delete_old_versions.cc:58: using ExecutableMap =
std::map<base::string16, ExecutableVector>;
maybe this should also map a FilePath to a vector of FilePaths.
GetExecutableVersion would need to return a FilePath, so it could be changed to
GetExecutableVersionDirName or somesuch

https://codereview.chromium.org/1666363002/diff/1/chrome/installer/util/delet...
chrome/installer/util/delete_old_versions.cc:80: if
(executables.find(directory_name) == executables.end())
nit:
#include "base/stl_util.h"
    if (!ContainsValue(executables, directory_name))

https://codereview.chromium.org/1666363002/diff/1/chrome/installer/util/delet...
chrome/installer/util/delete_old_versions.cc:81: success &=
base::DeleteFile(install_dir.Append(directory_name), true);
we expect that this will never happen, so i think it makes sense to LOG(WARNING)
that it's about to happen, and PLOG(ERROR) if the delete fails.

https://codereview.chromium.org/1666363002/diff/1/chrome/installer/util/delet...
chrome/installer/util/delete_old_versions.cc:97: if (directories.find(version)
!= directories.end())
    if (ContainsValue(directories, version))

https://codereview.chromium.org/1666363002/diff/1/chrome/installer/util/delet...
chrome/installer/util/delete_old_versions.cc:102: success &=
base::DeleteFile(executable_path, false);
we expect that this will never happen, so i think it makes sense to LOG(WARNING)
that it's about to happen, and PLOG(ERROR) if the delete fails.

https://codereview.chromium.org/1666363002/diff/1/chrome/installer/util/delet...
chrome/installer/util/delete_old_versions.cc:129: if
(path.MatchesExtension(L".exe") || path.MatchesExtension(L".dll")) {
nit: use FILE_PATH_LITERAL("") rather than L"" for constants like these

https://codereview.chromium.org/1666363002/diff/1/chrome/installer/util/delet...
chrome/installer/util/delete_old_versions.cc:129: if
(path.MatchesExtension(L".exe") || path.MatchesExtension(L".dll")) {
nit: reverse the logic here to "continue" when not either one to reduce
indentation below

https://codereview.chromium.org/1666363002/diff/1/chrome/installer/util/delet...
chrome/installer/util/delete_old_versions.cc:130: base::File
lock(GetFileLock(path));
wdyt of doing away with the local here and on line 140?
       locks.push_back(GetFileLock(path));
       if (!locks.back().IsValid())
         return false;

https://codereview.chromium.org/1666363002/diff/1/chrome/installer/util/delet...
chrome/installer/util/delete_old_versions.cc:132: return false;
It makes sense to use LOG(WARNING) to indicate what file(s) are in use so we can
see from logs why the deletion fails.

https://codereview.chromium.org/1666363002/diff/1/chrome/installer/util/delet...
chrome/installer/util/delete_old_versions.cc:133:
locks.push_back(std::move(lock));
#include <utility> if you keep this

https://codereview.chromium.org/1666363002/diff/1/chrome/installer/util/delet...
chrome/installer/util/delete_old_versions.cc:142: return false;
same comment here about logging

https://codereview.chromium.org/1666363002/diff/1/chrome/installer/util/delet...
chrome/installer/util/delete_old_versions.cc:152: success &=
base::DeleteFile(locked_file_path, false);
this isn't ever expected to fail, right? PLOG(ERROR) in case it does.

https://codereview.chromium.org/1666363002/diff/1/chrome/installer/util/delet...
chrome/installer/util/delete_old_versions.cc:159: success &=
base::DeleteFile(version_directory, true);
PLOG(ERROR) when this fails so we understand why

https://codereview.chromium.org/1666363002/diff/1/chrome/installer/util/delet...
chrome/installer/util/delete_old_versions.cc:173: for (const auto directory_name
: directories) {
nit: const auto&

https://codereview.chromium.org/1666363002/diff/1/chrome/installer/util/delet...
chrome/installer/util/delete_old_versions.cc:195: success &=
DeleteDirectoriesWithoutMatchingExecutable(
I'm not a fan of using bitwise operations on bools like this. Is there another
way? Is it important for this function to return a bool (it's currently
ignored)?

https://codereview.chromium.org/1666363002/diff/1/chrome/installer/util/delet...
File chrome/installer/util/delete_old_versions.h (right):

https://codereview.chromium.org/1666363002/diff/1/chrome/installer/util/delet...
chrome/installer/util/delete_old_versions.h:1: // Copyright (c) 2016 The
Chromium Authors. All rights reserved.
nit: no "(c)" in new file headers
(http://www.chromium.org/developers/coding-style#TOC-File-headers)

[fdoray, 2016-02-11 21:17:12]

Description was changed from

==========
Delete old files at the end of a regular update.

This CL introduces the DeleteOldVersions() function which deletes old
executables and version directories from the Chrome installation
directory. For now, the function is only called at the end of a regular
update (not in use).

BUG=451546
==========

to

==========
Delete old files after an update.

This CL introduces the DeleteOldVersions() function which deletes old
executables and version directories from the Chrome installation
directory.

For now, the function is called once at the end of an update and at the
end of a new_chrome.exe->chrome.exe rename. Eventually, it will also
be called repetitively from a cleanup process launched when the
function fails to delete all old files the first time.

BUG=451546
==========

[fdoray, 2016-02-11 21:18:11]

Description was changed from

==========
Delete old files after an update.

This CL introduces the DeleteOldVersions() function which deletes old
executables and version directories from the Chrome installation
directory.

For now, the function is called once at the end of an update and at the
end of a new_chrome.exe->chrome.exe rename. Eventually, it will also
be called repetitively from a cleanup process launched when the
function fails to delete all old files the first time.

BUG=451546
==========

to

==========
Delete old files after an update.

This CL introduces the DeleteOldVersions() function which deletes old
executables and version directories from the Chrome installation
directory.

For now, the function is called once at the end of an update and at the
end of a new_chrome.exe->chrome.exe rename. Eventually, it will also
be called repetitively from a cleanup process launched when the
function fails to delete everything the first time.

BUG=451546
==========

[fdoray, 2016-02-18 17:59:21]

grt@: Can you take another look? Thanks.

https://codereview.chromium.org/1666363002/diff/1/chrome/installer/util/delet...
File chrome/installer/util/delete_old_versions.cc (right):

https://codereview.chromium.org/1666363002/diff/1/chrome/installer/util/delet...
chrome/installer/util/delete_old_versions.cc:46: const base::string16
directory_name = directory_path.BaseName().value();
On 2016/02/10 20:02:04, grt wrote:
> nit: make this and the container use base::FilePath rather than string16.

Done.

https://codereview.chromium.org/1666363002/diff/1/chrome/installer/util/delet...
chrome/installer/util/delete_old_versions.cc:49:
directories.insert(directory_name);
On 2016/02/10 20:02:04, grt wrote:
> for the sake of caution, perhaps this should only consider directories that
are
> named W.X.Y.Z. wdyt?

Done.

https://codereview.chromium.org/1666363002/diff/1/chrome/installer/util/delet...
chrome/installer/util/delete_old_versions.cc:57: using ExecutableVector =
std::vector<base::FilePath>;
On 2016/02/10 20:02:04, grt wrote:
> nit: change this type to PathVector and use it on line 122

Done.

https://codereview.chromium.org/1666363002/diff/1/chrome/installer/util/delet...
chrome/installer/util/delete_old_versions.cc:58: using ExecutableMap =
std::map<base::string16, ExecutableVector>;
On 2016/02/10 20:02:03, grt wrote:
> maybe this should also map a FilePath to a vector of FilePaths.
> GetExecutableVersion would need to return a FilePath, so it could be changed
to
> GetExecutableVersionDirName or somesuch

Done.

https://codereview.chromium.org/1666363002/diff/1/chrome/installer/util/delet...
chrome/installer/util/delete_old_versions.cc:80: if
(executables.find(directory_name) == executables.end())
On 2016/02/10 20:02:04, grt wrote:
> nit:
> #include "base/stl_util.h"
>     if (!ContainsValue(executables, directory_name))

Done.

https://codereview.chromium.org/1666363002/diff/1/chrome/installer/util/delet...
chrome/installer/util/delete_old_versions.cc:81: success &=
base::DeleteFile(install_dir.Append(directory_name), true);
On 2016/02/10 20:02:03, grt wrote:
> we expect that this will never happen, so i think it makes sense to
LOG(WARNING)
> that it's about to happen, and PLOG(ERROR) if the delete fails.

Done.

https://codereview.chromium.org/1666363002/diff/1/chrome/installer/util/delet...
chrome/installer/util/delete_old_versions.cc:97: if (directories.find(version)
!= directories.end())
On 2016/02/10 20:02:03, grt wrote:
>     if (ContainsValue(directories, version))

Done.

https://codereview.chromium.org/1666363002/diff/1/chrome/installer/util/delet...
chrome/installer/util/delete_old_versions.cc:102: success &=
base::DeleteFile(executable_path, false);
On 2016/02/10 20:02:04, grt wrote:
> we expect that this will never happen, so i think it makes sense to
LOG(WARNING)
> that it's about to happen, and PLOG(ERROR) if the delete fails.

Done.

https://codereview.chromium.org/1666363002/diff/1/chrome/installer/util/delet...
chrome/installer/util/delete_old_versions.cc:129: if
(path.MatchesExtension(L".exe") || path.MatchesExtension(L".dll")) {
On 2016/02/10 20:02:04, grt wrote:
> nit: use FILE_PATH_LITERAL("") rather than L"" for constants like these

Done.

https://codereview.chromium.org/1666363002/diff/1/chrome/installer/util/delet...
chrome/installer/util/delete_old_versions.cc:129: if
(path.MatchesExtension(L".exe") || path.MatchesExtension(L".dll")) {
On 2016/02/10 20:02:04, grt wrote:
> nit: reverse the logic here to "continue" when not either one to reduce
> indentation below

Done.

https://codereview.chromium.org/1666363002/diff/1/chrome/installer/util/delet...
chrome/installer/util/delete_old_versions.cc:130: base::File
lock(GetFileLock(path));
On 2016/02/10 20:02:04, grt wrote:
> wdyt of doing away with the local here and on line 140?
>        locks.push_back(GetFileLock(path));
>        if (!locks.back().IsValid())
>          return false;

Done.

https://codereview.chromium.org/1666363002/diff/1/chrome/installer/util/delet...
chrome/installer/util/delete_old_versions.cc:132: return false;
On 2016/02/10 20:02:03, grt wrote:
> It makes sense to use LOG(WARNING) to indicate what file(s) are in use so we
can
> see from logs why the deletion fails.

Done.

https://codereview.chromium.org/1666363002/diff/1/chrome/installer/util/delet...
chrome/installer/util/delete_old_versions.cc:133:
locks.push_back(std::move(lock));
On 2016/02/10 20:02:03, grt wrote:
> #include <utility> if you keep this

I no longer use std::move.

https://codereview.chromium.org/1666363002/diff/1/chrome/installer/util/delet...
chrome/installer/util/delete_old_versions.cc:142: return false;
On 2016/02/10 20:02:03, grt wrote:
> same comment here about logging

Done.

https://codereview.chromium.org/1666363002/diff/1/chrome/installer/util/delet...
chrome/installer/util/delete_old_versions.cc:152: success &=
base::DeleteFile(locked_file_path, false);
On 2016/02/10 20:02:04, grt wrote:
> this isn't ever expected to fail, right? PLOG(ERROR) in case it does.

Done.

https://codereview.chromium.org/1666363002/diff/1/chrome/installer/util/delet...
chrome/installer/util/delete_old_versions.cc:159: success &=
base::DeleteFile(version_directory, true);
On 2016/02/10 20:02:04, grt wrote:
> PLOG(ERROR) when this fails so we understand why

Done.

https://codereview.chromium.org/1666363002/diff/1/chrome/installer/util/delet...
chrome/installer/util/delete_old_versions.cc:173: for (const auto directory_name
: directories) {
On 2016/02/10 20:02:04, grt wrote:
> nit: const auto&

Done.

https://codereview.chromium.org/1666363002/diff/1/chrome/installer/util/delet...
chrome/installer/util/delete_old_versions.cc:195: success &=
DeleteDirectoriesWithoutMatchingExecutable(
On 2016/02/10 20:02:04, grt wrote:
> I'm not a fan of using bitwise operations on bools like this. Is there another
> way? Is it important for this function to return a bool (it's currently
> ignored)?

I want the function to return a bool so that we know when to launch/keep alive a
cleanup process. I didn't write if (!DeleteDirectories... ||
!DeleteExecutables... | ...) because of short-circuit evaluation. I could write:

if (!DeleteDirectories...)
  success = false;
if (!DeleteExecutables...)
  success = false;
if (!DeleteMatching...)
  success = false;

Or I could get rid of all the |success| variables and check after the fact if
there is remaining old files.

https://codereview.chromium.org/1666363002/diff/1/chrome/installer/util/delet...
File chrome/installer/util/delete_old_versions.h (right):

https://codereview.chromium.org/1666363002/diff/1/chrome/installer/util/delet...
chrome/installer/util/delete_old_versions.h:1: // Copyright (c) 2016 The
Chromium Authors. All rights reserved.
On 2016/02/10 20:02:04, grt wrote:
> nit: no "(c)" in new file headers
> (http://www.chromium.org/developers/coding-style#TOC-File-headers)

Done.

[grt (UTC plus 2), 2016-02-24 20:06:17]

https://codereview.chromium.org/1666363002/diff/100001/chrome/installer/setup...
File chrome/installer/setup/install.cc (right):

https://codereview.chromium.org/1666363002/diff/100001/chrome/installer/setup...
chrome/installer/setup/install.cc:557:
installer_state.UpdateStage(installer::REMOVING_OLD_VERSIONS);
move this to the new place where old versions are removed

https://codereview.chromium.org/1666363002/diff/100001/chrome/installer/util/...
File chrome/installer/util/delete_old_versions.cc (right):

https://codereview.chromium.org/1666363002/diff/100001/chrome/installer/util/...
chrome/installer/util/delete_old_versions.cc:41: using DirectorySet =
std::set<base::FilePath>;
nit: move all of these aliases up to the top of the namespace

https://codereview.chromium.org/1666363002/diff/100001/chrome/installer/util/...
chrome/installer/util/delete_old_versions.cc:42: DirectorySet
GetDirectories(const base::FilePath& install_dir) {
nit: GetVersionDirectories since it is careful not to include anything that
isn't a W.X.Y.Z dir

https://codereview.chromium.org/1666363002/diff/100001/chrome/installer/util/...
chrome/installer/util/delete_old_versions.cc:54: const base::Version
version(base::UTF16ToUTF8(directory_name.value()));
base::UTF16ToUTF8(directory_name.value()) -> directory_name.AsUTF8Unsafe()

https://codereview.chromium.org/1666363002/diff/100001/chrome/installer/util/...
chrome/installer/util/delete_old_versions.cc:56: if (directory_name !=
new_chrome_exe_version_dir_name &&
nit: version.IsValid() and the size check are both cheap, so test those before
the string comparisons.

https://codereview.chromium.org/1666363002/diff/100001/chrome/installer/util/...
chrome/installer/util/delete_old_versions.cc:81: // executable in |executables|.
"Returns false if any such directories could not be deleted."

https://codereview.chromium.org/1666363002/diff/100001/chrome/installer/util/...
chrome/installer/util/delete_old_versions.cc:92: if
(!base::DeleteFile(install_dir.Append(directory_name), true)) {
nit: put install_dir.Append(directory_name) in a local var and use it for both
log messages

https://codereview.chromium.org/1666363002/diff/100001/chrome/installer/util/...
chrome/installer/util/delete_old_versions.cc:103: // directory in |directories|.
"Returns false if any such files could not be deleted."

https://codereview.chromium.org/1666363002/diff/100001/chrome/installer/util/...
chrome/installer/util/delete_old_versions.cc:116: // Delete executables for
version |version|.
version_dir_name

https://codereview.chromium.org/1666363002/diff/100001/chrome/installer/util/...
chrome/installer/util/delete_old_versions.cc:118: const base::FilePath
executable_name = executable_path.BaseName();
nit: use executable_path in the log messages

https://codereview.chromium.org/1666363002/diff/100001/chrome/installer/util/...
chrome/installer/util/delete_old_versions.cc:134: base::File GetFileLock(const
base::FilePath path) {
const base::FilePath&

https://codereview.chromium.org/1666363002/diff/100001/chrome/installer/util/...
chrome/installer/util/delete_old_versions.cc:215: ExecutableMap::const_iterator
version_executables_it =
auto?

https://codereview.chromium.org/1666363002/diff/100001/chrome/installer/util/...
chrome/installer/util/delete_old_versions.cc:218: // Don't delete the version
unless the directory has at least one matching
nit: move this comment above the call to find()

https://codereview.chromium.org/1666363002/diff/100001/chrome/installer/util/...
File chrome/installer/util/delete_old_versions.h (right):

https://codereview.chromium.org/1666363002/diff/100001/chrome/installer/util/...
chrome/installer/util/delete_old_versions.h:23: bool
DeleteOldVersionsCallback(const base::FilePath& install_dir,
how about replacing this with AddDeleteOldVersionsWorkItem(const
base::FilePath&, WorkItemList*)?

https://codereview.chromium.org/1666363002/diff/100001/chrome/installer/util/...
File chrome/installer/util/delete_old_versions_unittest.cc (right):

https://codereview.chromium.org/1666363002/diff/100001/chrome/installer/util/...
chrome/installer/util/delete_old_versions_unittest.cc:44: const char
kDummyContent[] = "dummy";
nit: static const char for string literal constants like this

https://codereview.chromium.org/1666363002/diff/100001/chrome/installer/util/...
chrome/installer/util/delete_old_versions_unittest.cc:93: };
nit: DISALLOW_COPY_AND_ASSIGN

[fdoray, 2016-02-25 18:18:22]

Description was changed from

==========
Delete old files after an update.

This CL introduces the DeleteOldVersions() function which deletes old
executables and version directories from the Chrome installation
directory.

For now, the function is called once at the end of an update and at the
end of a new_chrome.exe->chrome.exe rename. Eventually, it will also
be called repetitively from a cleanup process launched when the
function fails to delete everything the first time.

BUG=451546
==========

to

==========
Delete old files after an update.

This CL introduces a WorkItem which deletes old executables and version
directories from the Chrome installation directory.

For now, the WorkItem is used at the end of an update and at the
end of a new_chrome.exe->chrome.exe rename. Eventually, it will also
be invoked repetitively from a cleanup process launched when the
WorkItem fails to delete everything the first time.

BUG=451546
==========

[fdoray, 2016-02-25 18:26:48]

grt@: Can you take another look? Thanks.

https://codereview.chromium.org/1666363002/diff/100001/chrome/installer/setup...
File chrome/installer/setup/install.cc (right):

https://codereview.chromium.org/1666363002/diff/100001/chrome/installer/setup...
chrome/installer/setup/install.cc:557:
installer_state.UpdateStage(installer::REMOVING_OLD_VERSIONS);
On 2016/02/24 20:06:16, grt wrote:
> move this to the new place where old versions are removed

Done.

https://codereview.chromium.org/1666363002/diff/100001/chrome/installer/util/...
File chrome/installer/util/delete_old_versions.cc (right):

https://codereview.chromium.org/1666363002/diff/100001/chrome/installer/util/...
chrome/installer/util/delete_old_versions.cc:41: using DirectorySet =
std::set<base::FilePath>;
On 2016/02/24 20:06:17, grt wrote:
> nit: move all of these aliases up to the top of the namespace

Done.

https://codereview.chromium.org/1666363002/diff/100001/chrome/installer/util/...
chrome/installer/util/delete_old_versions.cc:42: DirectorySet
GetDirectories(const base::FilePath& install_dir) {
On 2016/02/24 20:06:16, grt wrote:
> nit: GetVersionDirectories since it is careful not to include anything that
> isn't a W.X.Y.Z dir

Done.

https://codereview.chromium.org/1666363002/diff/100001/chrome/installer/util/...
chrome/installer/util/delete_old_versions.cc:54: const base::Version
version(base::UTF16ToUTF8(directory_name.value()));
On 2016/02/24 20:06:16, grt wrote:
> base::UTF16ToUTF8(directory_name.value()) -> directory_name.AsUTF8Unsafe()

Done.

https://codereview.chromium.org/1666363002/diff/100001/chrome/installer/util/...
chrome/installer/util/delete_old_versions.cc:56: if (directory_name !=
new_chrome_exe_version_dir_name &&
On 2016/02/24 20:06:16, grt wrote:
> nit: version.IsValid() and the size check are both cheap, so test those before
> the string comparisons.

Done.

https://codereview.chromium.org/1666363002/diff/100001/chrome/installer/util/...
chrome/installer/util/delete_old_versions.cc:81: // executable in |executables|.
On 2016/02/24 20:06:17, grt wrote:
> "Returns false if any such directories could not be deleted."

Done.

https://codereview.chromium.org/1666363002/diff/100001/chrome/installer/util/...
chrome/installer/util/delete_old_versions.cc:92: if
(!base::DeleteFile(install_dir.Append(directory_name), true)) {
On 2016/02/24 20:06:16, grt wrote:
> nit: put install_dir.Append(directory_name) in a local var and use it for both
> log messages

Done.

https://codereview.chromium.org/1666363002/diff/100001/chrome/installer/util/...
chrome/installer/util/delete_old_versions.cc:103: // directory in |directories|.
On 2016/02/24 20:06:17, grt wrote:
> "Returns false if any such files could not be deleted."

Done.

https://codereview.chromium.org/1666363002/diff/100001/chrome/installer/util/...
chrome/installer/util/delete_old_versions.cc:116: // Delete executables for
version |version|.
On 2016/02/24 20:06:17, grt wrote:
> version_dir_name

Done.

https://codereview.chromium.org/1666363002/diff/100001/chrome/installer/util/...
chrome/installer/util/delete_old_versions.cc:118: const base::FilePath
executable_name = executable_path.BaseName();
On 2016/02/24 20:06:17, grt wrote:
> nit: use executable_path in the log messages

Done.

https://codereview.chromium.org/1666363002/diff/100001/chrome/installer/util/...
chrome/installer/util/delete_old_versions.cc:134: base::File GetFileLock(const
base::FilePath path) {
On 2016/02/24 20:06:17, grt wrote:
> const base::FilePath&

Done.

https://codereview.chromium.org/1666363002/diff/100001/chrome/installer/util/...
chrome/installer/util/delete_old_versions.cc:215: ExecutableMap::const_iterator
version_executables_it =
On 2016/02/24 20:06:17, grt wrote:
> auto?

Done.

https://codereview.chromium.org/1666363002/diff/100001/chrome/installer/util/...
chrome/installer/util/delete_old_versions.cc:218: // Don't delete the version
unless the directory has at least one matching
On 2016/02/24 20:06:17, grt wrote:
> nit: move this comment above the call to find()

Done.

https://codereview.chromium.org/1666363002/diff/100001/chrome/installer/util/...
File chrome/installer/util/delete_old_versions.h (right):

https://codereview.chromium.org/1666363002/diff/100001/chrome/installer/util/...
chrome/installer/util/delete_old_versions.h:23: bool
DeleteOldVersionsCallback(const base::FilePath& install_dir,
On 2016/02/24 20:06:17, grt wrote:
> how about replacing this with AddDeleteOldVersionsWorkItem(const
> base::FilePath&, WorkItemList*)?

Done. InstallerState instead of base::FilePath because I now call
installer_state->UpdateStage() inside the callback.

https://codereview.chromium.org/1666363002/diff/100001/chrome/installer/util/...
File chrome/installer/util/delete_old_versions_unittest.cc (right):

https://codereview.chromium.org/1666363002/diff/100001/chrome/installer/util/...
chrome/installer/util/delete_old_versions_unittest.cc:44: const char
kDummyContent[] = "dummy";
On 2016/02/24 20:06:17, grt wrote:
> nit: static const char for string literal constants like this

Done.

https://codereview.chromium.org/1666363002/diff/100001/chrome/installer/util/...
chrome/installer/util/delete_old_versions_unittest.cc:93: };
On 2016/02/24 20:06:17, grt wrote:
> nit: DISALLOW_COPY_AND_ASSIGN

Done.

[grt (UTC plus 2), 2016-02-25 19:25:40]

looks great

https://codereview.chromium.org/1666363002/diff/160001/chrome/installer/setup...
File chrome/installer/setup/install_worker.cc (right):

https://codereview.chromium.org/1666363002/diff/160001/chrome/installer/setup...
chrome/installer/setup/install_worker.cc:1020: ->set_ignore_failure(true);
is it reasonable to do this in the Add function? is there ever a case where a
caller wouldn't want failures to be ignored?

https://codereview.chromium.org/1666363002/diff/160001/chrome/installer/util/...
File chrome/installer/util/delete_old_versions.cc (right):

https://codereview.chromium.org/1666363002/diff/160001/chrome/installer/util/...
chrome/installer/util/delete_old_versions.cc:28: namespace {
nit: empty line between these

https://codereview.chromium.org/1666363002/diff/160001/chrome/installer/util/...
File chrome/installer/util/delete_old_versions.h (right):

https://codereview.chromium.org/1666363002/diff/160001/chrome/installer/util/...
chrome/installer/util/delete_old_versions.h:18: // if a .exe or .dll file for
that version is in use. |installer_state| has to
nit: "has to be" -> "must be"

https://codereview.chromium.org/1666363002/diff/160001/chrome/installer/util/...
File chrome/installer/util/delete_old_versions_unittest.cc (right):

https://codereview.chromium.org/1666363002/diff/160001/chrome/installer/util/...
chrome/installer/util/delete_old_versions_unittest.cc:29: namespace {
nit: blank line between these

[fdoray, 2016-02-26 17:00:05]

grt@: Can you take another look? Thanks.

https://codereview.chromium.org/1666363002/diff/160001/chrome/installer/setup...
File chrome/installer/setup/install_worker.cc (right):

https://codereview.chromium.org/1666363002/diff/160001/chrome/installer/setup...
chrome/installer/setup/install_worker.cc:1020: ->set_ignore_failure(true);
On 2016/02/25 19:25:40, grt wrote:
> is it reasonable to do this in the Add function? is there ever a case where a
> caller wouldn't want failures to be ignored?

Done. In my previous patch, I didn't to it inside the Add function because I
tested failure in the unit tests. Now I decided to wait until I have the code to
launch the cleanup process to test failure.

https://codereview.chromium.org/1666363002/diff/160001/chrome/installer/util/...
File chrome/installer/util/delete_old_versions.cc (right):

https://codereview.chromium.org/1666363002/diff/160001/chrome/installer/util/...
chrome/installer/util/delete_old_versions.cc:28: namespace {
On 2016/02/25 19:25:40, grt wrote:
> nit: empty line between these

Done.

https://codereview.chromium.org/1666363002/diff/160001/chrome/installer/util/...
File chrome/installer/util/delete_old_versions.h (right):

https://codereview.chromium.org/1666363002/diff/160001/chrome/installer/util/...
chrome/installer/util/delete_old_versions.h:18: // if a .exe or .dll file for
that version is in use. |installer_state| has to
On 2016/02/25 19:25:40, grt wrote:
> nit: "has to be" -> "must be"

Done.

https://codereview.chromium.org/1666363002/diff/160001/chrome/installer/util/...
File chrome/installer/util/delete_old_versions_unittest.cc (right):

https://codereview.chromium.org/1666363002/diff/160001/chrome/installer/util/...
chrome/installer/util/delete_old_versions_unittest.cc:29: namespace {
On 2016/02/25 19:25:40, grt wrote:
> nit: blank line between these

Done.

[grt (UTC plus 2), 2016-02-29 21:43:18]

lgtm w/ a nit.

https://codereview.chromium.org/1666363002/diff/180001/chrome/installer/util/...
File chrome/installer/util/delete_old_versions_unittest.cc (right):

https://codereview.chromium.org/1666363002/diff/180001/chrome/installer/util/...
chrome/installer/util/delete_old_versions_unittest.cc:50: bool
DeleteOldVersions() {
void?

[fdoray, 2016-03-02 18:44:56]

https://codereview.chromium.org/1666363002/diff/180001/chrome/installer/util/...
File chrome/installer/util/delete_old_versions_unittest.cc (right):

https://codereview.chromium.org/1666363002/diff/180001/chrome/installer/util/...
chrome/installer/util/delete_old_versions_unittest.cc:50: bool
DeleteOldVersions() {
On 2016/02/29 21:43:18, grt (no reviews Mar 2-4) wrote:
> void?

Done.

[fdoray, 2016-03-02 18:45:05]

The CQ bit was checked by fdoray@chromium.org

[fdoray, 2016-03-02 18:45:06]

The patchset sent to the CQ was uploaded after l-g-t-m from grt@chromium.org
Link to the patchset: https://codereview.chromium.org/1666363002/#ps200001
(title: "fix return type")

[commit-bot: I haz the power, 2016-03-02 18:45:43]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1666363002/200001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1666363002/200001

[commit-bot: I haz the power, 2016-03-02 19:36:02]

Description was changed from

==========
Delete old files after an update.

This CL introduces a WorkItem which deletes old executables and version
directories from the Chrome installation directory.

For now, the WorkItem is used at the end of an update and at the
end of a new_chrome.exe->chrome.exe rename. Eventually, it will also
be invoked repetitively from a cleanup process launched when the
WorkItem fails to delete everything the first time.

BUG=451546
==========

to

==========
Delete old files after an update.

This CL introduces a WorkItem which deletes old executables and version
directories from the Chrome installation directory.

For now, the WorkItem is used at the end of an update and at the
end of a new_chrome.exe->chrome.exe rename. Eventually, it will also
be invoked repetitively from a cleanup process launched when the
WorkItem fails to delete everything the first time.

BUG=451546
==========

[commit-bot: I haz the power, 2016-03-02 19:36:03]

Committed patchset #11 (id:200001)

[commit-bot: I haz the power, 2016-03-02 19:37:03]

Description was changed from

==========
Delete old files after an update.

This CL introduces a WorkItem which deletes old executables and version
directories from the Chrome installation directory.

For now, the WorkItem is used at the end of an update and at the
end of a new_chrome.exe->chrome.exe rename. Eventually, it will also
be invoked repetitively from a cleanup process launched when the
WorkItem fails to delete everything the first time.

BUG=451546
==========

to

==========
Delete old files after an update.

This CL introduces a WorkItem which deletes old executables and version
directories from the Chrome installation directory.

For now, the WorkItem is used at the end of an update and at the
end of a new_chrome.exe->chrome.exe rename. Eventually, it will also
be invoked repetitively from a cleanup process launched when the
WorkItem fails to delete everything the first time.

BUG=451546

Committed: https://crrev.com/1b50c70773051b24925b46bab8b006e606633c5c
Cr-Commit-Position: refs/heads/master@{#378802}
==========

[commit-bot: I haz the power, 2016-03-02 19:37:06]

Patchset 11 (id:??) landed as
https://crrev.com/1b50c70773051b24925b46bab8b006e606633c5c
Cr-Commit-Position: refs/heads/master@{#378802}

[benwells, 2016-03-04 05:54:35]

A revert of this CL (patchset #11 id:200001) has been created in
https://codereview.chromium.org/1764053002/ by benwells@chromium.org.

The reason for reverting is: This has caused memory errors to be reported on
DrMemory. The errors are in GenerateSpecificPEFileVersion, and relate to the
VisitResourceContext not being properly initialized.

Turns out you're just really unlucky, until now nobody was actually using that
(or GenerateAlternatePEFileVersion, which looks like it would have the same
problem).

This is the bot where it started happening:
https://build.chromium.org/p/chromium.memory.fyi/builders/Windows%20Unit%20%2...

This is some sample error output:
[----------] 10 tests from DeleteOldVersionsTest
[ RUN      ] DeleteOldVersionsTest.DeleteOldExecutableWithoutMatchingDirectory
~~Dr.M~~ 
~~Dr.M~~ Error #1: UNINITIALIZED READ: reading register cl
~~Dr.M~~ # 0 `anonymous namespace'::ReplaceAll                                  
       [chrome\installer\test\alternate_version_generator.cc:296]
~~Dr.M~~ # 1 `anonymous namespace'::VisitResource                               
       [chrome\installer\test\alternate_version_generator.cc:356]
~~Dr.M~~ # 2 `anonymous namespace'::EnumResourcesWorker                         
       [chrome\installer\test\pe_image_resources.cc:96]
~~Dr.M~~ # 3 `anonymous namespace'::EnumResourcesWorker                         
       [chrome\installer\test\pe_image_resources.cc:84]
~~Dr.M~~ # 4 `anonymous namespace'::EnumResourcesWorker                         
       [chrome\installer\test\pe_image_resources.cc:84]
~~Dr.M~~ # 5 upgrade_test::EnumResources                                        
       [chrome\installer\test\pe_image_resources.cc:122]
~~Dr.M~~ # 6 `anonymous namespace'::UpdateVersionIfMatch                        
       [chrome\installer\test\alternate_version_generator.cc:401]
~~Dr.M~~ # 7 upgrade_test::GenerateSpecificPEFileVersion                        
       [chrome\installer\test\alternate_version_generator.cc:769]
~~Dr.M~~ # 8 installer::`anonymous
namespace'::DeleteOldVersionsTest::CreateExecutable 
[chrome\installer\util\delete_old_versions_unittest.cc:66]
~~Dr.M~~ # 9
installer::DeleteOldVersionsTest_DeleteOldExecutableWithoutMatchingDirectory_Test::TestBody
[chrome\installer\util\delete_old_versions_unittest.cc:129]
~~Dr.M~~ #10 testing::internal::HandleExceptionsInMethodIfSupported<>           
       [testing\gtest\src\gtest.cc:2458]
~~Dr.M~~ Note: @0:00:21.478 in thread 5132
~~Dr.M~~ Note: instruction: cmp    %cl (%eax)
~~Dr.M~~ 
~~Dr.M~~ Error #2: UNINITIALIZED READ: reading register cl
~~Dr.M~~ # 0 `anonymous namespace'::ReplaceAll                                  
       [chrome\installer\test\alternate_version_generator.cc:296]
~~Dr.M~~ # 1 `anonymous namespace'::VisitResource                               
       [chrome\installer\test\alternate_version_generator.cc:365]
~~Dr.M~~ # 2 `anonymous namespace'::EnumResourcesWorker                         
       [chrome\installer\test\pe_image_resources.cc:96]
~~Dr.M~~ # 3 `anonymous namespace'::EnumResourcesWorker                         
       [chrome\installer\test\pe_image_resources.cc:84]
~~Dr.M~~ # 4 `anonymous namespace'::EnumResourcesWorker                         
       [chrome\installer\test\pe_image_resources.cc:84]
~~Dr.M~~ # 5 upgrade_test::EnumResources                                        
       [chrome\installer\test\pe_image_resources.cc:122]
~~Dr.M~~ # 6 `anonymous namespace'::UpdateVersionIfMatch                        
       [chrome\installer\test\alternate_version_generator.cc:401]
~~Dr.M~~ # 7 upgrade_test::GenerateSpecificPEFileVersion                        
       [chrome\installer\test\alternate_version_generator.cc:769]
~~Dr.M~~ # 8 installer::`anonymous
namespace'::DeleteOldVersionsTest::CreateExecutable 
[chrome\installer\util\delete_old_versions_unittest.cc:66]
~~Dr.M~~ # 9
installer::DeleteOldVersionsTest_DeleteOldExecutableWithoutMatchingDirectory_Test::TestBody
[chrome\installer\util\delete_old_versions_unittest.cc:129]
~~Dr.M~~ #10 testing::internal::HandleExceptionsInMethodIfSupported<>           
       [testing\gtest\src\gtest.cc:2458]
~~Dr.M~~ Note: @0:00:21.483 in thread 5132
~~Dr.M~~ Note: instruction: cmp    %cl (%eax)
.

[grt (UTC plus 2), 2016-03-07 15:08:32]

https://codereview.chromium.org/1666363002/diff/200001/chrome/installer/util/...
File chrome/installer/util/delete_old_versions.cc (right):

https://codereview.chromium.org/1666363002/diff/200001/chrome/installer/util/...
chrome/installer/util/delete_old_versions.cc:260: return
DeleteOldVersions(installer_state.target_path());
ah, this is a problem here. when a WorkItem's ignore_failure bit is set, it
should unconditionally return true. i think the right fix is to change
CallbackWorkItem::Do so that it does something like:
  return ignore_failure() ? true : result;