From 6b38fce107364db2ee8939f867e7cf3375f539cf Mon Sep 17 00:00:00 2001 From: "pasko@chromium.org" Date: Wed, 17 Jul 2013 08:29:49 +0000 Subject: Revert "Make DeleteCache() recursive" This reverts commit ca37f304399948559b6fff0b1829b79d70a2b51b. Conflicts: net/disk_cache/cache_util.cc (due to r211823) Reason: race condition in base::DeleteFile() between test teardown and cache initialization in tests. TSan auto-suggested suppression: { ThreadSanitizer:Race fun:SHGetThreadRef fun:SHPathPrepareForWriteW fun:SHFileOperationW fun:SHFileOperationW fun:base::DeleteFileW fun:base::ScopedTempDir::Delete fun:base::ScopedTempDir::Delete fun:base::ScopedTempDir::~ScopedTempDir fun:DiskCacheTest::~DiskCacheTest fun:DiskCacheBackendTest_NewEvictionInvalidEntry2_Test::`scalar deleting destructor' fun:testing::Test::DeleteSelf_ fun:testing::internal::HandleExceptionsInMethodIfSupported } BUG=none TBR=rnk@chromium.org Review URL: https://codereview.chromium.org/19545002 git-svn-id: svn://svn.chromium.org/chrome/trunk/src@211997 0039d316-1c4b-4281-b951-d872f2087c98 --- net/disk_cache/cache_util.cc | 21 --------------------- net/disk_cache/cache_util_posix.cc | 20 ++++++++++++++++++++ net/disk_cache/cache_util_unittest.cc | 17 ++++++----------- net/disk_cache/cache_util_win.cc | 29 +++++++++++++++++++++++++++++ 4 files changed, 55 insertions(+), 32 deletions(-) (limited to 'net') diff --git a/net/disk_cache/cache_util.cc b/net/disk_cache/cache_util.cc index 7389960..2457553 100644 --- a/net/disk_cache/cache_util.cc +++ b/net/disk_cache/cache_util.cc @@ -5,7 +5,6 @@ #include "net/disk_cache/cache_util.h" #include "base/file_util.h" -#include "base/files/file_enumerator.h" #include "base/location.h" #include "base/strings/string_util.h" #include "base/strings/stringprintf.h" @@ -52,26 +51,6 @@ base::FilePath GetTempCacheName(const base::FilePath& path, namespace disk_cache { -void DeleteCache(const base::FilePath& path, bool remove_folder) { - if (remove_folder) { - if (!base::DeleteFile(path, /* recursive */ true)) - LOG(WARNING) << "Unable to delete cache folder."; - return; - } - - base::FileEnumerator iter( - path, - /* recursive */ false, - base::FileEnumerator::FILES | base::FileEnumerator::DIRECTORIES); - for (base::FilePath file = iter.Next(); !file.value().empty(); - file = iter.Next()) { - if (!base::DeleteFile(file, /* recursive */ true)) { - LOG(WARNING) << "Unable to delete cache."; - return; - } - } -} - // In order to process a potentially large number of files, we'll rename the // cache directory to old_ + original_name + number, (located on the same parent // directory), and use a worker thread to delete all the files on all the stale diff --git a/net/disk_cache/cache_util_posix.cc b/net/disk_cache/cache_util_posix.cc index b33c560..2b13d9e 100644 --- a/net/disk_cache/cache_util_posix.cc +++ b/net/disk_cache/cache_util_posix.cc @@ -39,6 +39,26 @@ bool MoveCache(const base::FilePath& from_path, const base::FilePath& to_path) { #endif } +void DeleteCache(const base::FilePath& path, bool remove_folder) { + base::FileEnumerator iter(path, + /* recursive */ false, + base::FileEnumerator::FILES); + for (base::FilePath file = iter.Next(); !file.value().empty(); + file = iter.Next()) { + if (!base::DeleteFile(file, /* recursive */ false)) { + LOG(WARNING) << "Unable to delete cache."; + return; + } + } + + if (remove_folder) { + if (!base::DeleteFile(path, /* recursive */ false)) { + LOG(WARNING) << "Unable to delete cache folder."; + return; + } + } +} + bool DeleteCacheFile(const base::FilePath& name) { return base::DeleteFile(name, false); } diff --git a/net/disk_cache/cache_util_unittest.cc b/net/disk_cache/cache_util_unittest.cc index d2e7605..51d0443 100644 --- a/net/disk_cache/cache_util_unittest.cc +++ b/net/disk_cache/cache_util_unittest.cc @@ -19,7 +19,6 @@ class CacheUtilTest : public PlatformTest { file1_ = base::FilePath(cache_dir_.Append(FILE_PATH_LITERAL("file01"))); file2_ = base::FilePath(cache_dir_.Append(FILE_PATH_LITERAL(".file02"))); dir1_ = base::FilePath(cache_dir_.Append(FILE_PATH_LITERAL("dir01"))); - file3_ = base::FilePath(dir1_.Append(FILE_PATH_LITERAL("file03"))); ASSERT_TRUE(file_util::CreateDirectory(cache_dir_)); FILE *fp = file_util::OpenFile(file1_, "w"); ASSERT_TRUE(fp != NULL); @@ -28,9 +27,6 @@ class CacheUtilTest : public PlatformTest { ASSERT_TRUE(fp != NULL); file_util::CloseFile(fp); ASSERT_TRUE(file_util::CreateDirectory(dir1_)); - fp = file_util::OpenFile(file3_, "w"); - ASSERT_TRUE(fp != NULL); - file_util::CloseFile(fp); dest_dir_ = tmp_dir_.path().Append(FILE_PATH_LITERAL("old_Cache_001")); dest_file1_ = base::FilePath(dest_dir_.Append(FILE_PATH_LITERAL("file01"))); dest_file2_ = @@ -44,7 +40,6 @@ class CacheUtilTest : public PlatformTest { base::FilePath file1_; base::FilePath file2_; base::FilePath dir1_; - base::FilePath file3_; base::FilePath dest_dir_; base::FilePath dest_file1_; base::FilePath dest_file2_; @@ -68,29 +63,29 @@ TEST_F(CacheUtilTest, MoveCache) { } TEST_F(CacheUtilTest, DeleteCache) { + // DeleteCache won't delete subdirs, so let's not start with this + // one around. + base::DeleteFile(dir1_, false); disk_cache::DeleteCache(cache_dir_, false); EXPECT_TRUE(base::PathExists(cache_dir_)); // cache dir stays - EXPECT_FALSE(base::PathExists(dir1_)); EXPECT_FALSE(base::PathExists(file1_)); EXPECT_FALSE(base::PathExists(file2_)); - EXPECT_FALSE(base::PathExists(file3_)); } TEST_F(CacheUtilTest, DeleteCacheAndDir) { + // DeleteCache won't delete subdirs, so let's not start with this + // one around. + base::DeleteFile(dir1_, false); disk_cache::DeleteCache(cache_dir_, true); EXPECT_FALSE(base::PathExists(cache_dir_)); // cache dir is gone - EXPECT_FALSE(base::PathExists(dir1_)); EXPECT_FALSE(base::PathExists(file1_)); EXPECT_FALSE(base::PathExists(file2_)); - EXPECT_FALSE(base::PathExists(file3_)); } TEST_F(CacheUtilTest, DeleteCacheFile) { EXPECT_TRUE(disk_cache::DeleteCacheFile(file1_)); EXPECT_FALSE(base::PathExists(file1_)); EXPECT_TRUE(base::PathExists(cache_dir_)); // cache dir stays - EXPECT_TRUE(base::PathExists(dir1_)); - EXPECT_TRUE(base::PathExists(file3_)); } } // namespace disk_cache diff --git a/net/disk_cache/cache_util_win.cc b/net/disk_cache/cache_util_win.cc index 3337511..8c54b91 100644 --- a/net/disk_cache/cache_util_win.cc +++ b/net/disk_cache/cache_util_win.cc @@ -11,6 +11,29 @@ #include "base/message_loop.h" #include "base/win/scoped_handle.h" +namespace { + +// Deletes all the files on path that match search_name pattern. +void DeleteFiles(const base::FilePath& path, const wchar_t* search_name) { + base::FilePath name(path.Append(search_name)); + + WIN32_FIND_DATA data; + HANDLE handle = FindFirstFile(name.value().c_str(), &data); + if (handle == INVALID_HANDLE_VALUE) + return; + + do { + if (data.dwFileAttributes == FILE_ATTRIBUTE_DIRECTORY || + data.dwFileAttributes == FILE_ATTRIBUTE_REPARSE_POINT) + continue; + DeleteFile(path.Append(data.cFileName).value().c_str()); + } while (FindNextFile(handle, &data)); + + FindClose(handle); +} + +} // namespace + namespace disk_cache { bool MoveCache(const base::FilePath& from_path, const base::FilePath& to_path) { @@ -23,6 +46,12 @@ bool MoveCache(const base::FilePath& from_path, const base::FilePath& to_path) { return true; } +void DeleteCache(const base::FilePath& path, bool remove_folder) { + DeleteFiles(path, L"*"); + if (remove_folder) + RemoveDirectory(path.value().c_str()); +} + bool DeleteCacheFile(const base::FilePath& name) { // We do a simple delete, without ever falling back to SHFileOperation, as the // version from base does. -- cgit v1.1