Fix a race condition in ~RegistryHashStoreContentsWin

services/preferences/tracked/registry_hash_store_contents_win_unittest.cc

Index: services/preferences/tracked/registry_hash_store_contents_win_unittest.cc
diff --git a/services/preferences/tracked/registry_hash_store_contents_win_unittest.cc b/services/preferences/tracked/registry_hash_store_contents_win_unittest.cc
index 468843437fb8ebfa64327206d963c3cd0d980065..8577879aa97e6923cff5ee51d0e22597b1e41f05 100644
--- a/services/preferences/tracked/registry_hash_store_contents_win_unittest.cc
+++ b/services/preferences/tracked/registry_hash_store_contents_win_unittest.cc
@@ -4,6 +4,7 @@
 
 #include "services/preferences/tracked/registry_hash_store_contents_win.h"
 
+#include "base/files/scoped_temp_dir.h"
 #include "base/strings/string16.h"
 #include "base/strings/utf_string_conversions.h"
 #include "base/test/test_reg_util_win.h"
@@ -33,7 +34,8 @@ class RegistryHashStoreContentsWinTest : public testing::Test {
     ASSERT_NO_FATAL_FAILURE(
         registry_override_manager_.OverrideRegistry(HKEY_CURRENT_USER));
 
-    contents.reset(new RegistryHashStoreContentsWin(kRegistryPath, kStoreKey));
+    contents.reset(
+        new RegistryHashStoreContentsWin(kRegistryPath, kStoreKey, nullptr));
   }
 
   std::unique_ptr<HashStoreContents> contents;
@@ -118,3 +120,35 @@ TEST_F(RegistryHashStoreContentsWinTest, TestReset) {
   EXPECT_FALSE(contents->GetSplitMacs(kSplitPrefPath, &split_macs));
   EXPECT_EQ(0U, split_macs.size());
 }
+
+TEST(RegistryHashStoreContentsWinScopedTest, TestScopedDirsCleared) {
+  RegistryHashStoreContentsWin verifying_contents(registry_path, kStoreKey,
+                                                  nullptr);
+  std::string stored_mac;
+
+  base::ScopedTempDir temp_dir;
+  ASSERT_TRUE(temp_dir.CreateUniqueTempDir());
+  const base::string16 registry_path =
+      temp_dir.GetPath().DirName().BaseName().LossyDisplayName();
+
+  scoped_refptr<TempScopedDirRegistryCleaner> temp_scoped_dir_cleaner =
+      new TempScopedDirRegistryCleaner();

[gab, 2017/06/12 15:52:16]

MakeRefCounted<TempScopedDirRegistryCleaner>()

[proberge, 2017/06/12 18:46:02]

Done. Just out of curiosity, is this by convention or for performance?

[gab, 2017/06/12 18:52:24]

We try to avoid "new" as much as possible these days, using smart types + moving
them around allows to ensure we don't lose a pointer (and moving them around
ensures we don't add counting overhead).

+  RegistryHashStoreContentsWin* contentsA = new RegistryHashStoreContentsWin(

[gab, 2017/06/12 15:52:16]

Use unique_ptr, MakeUnique, and unique_ptr::reset() below

[proberge, 2017/06/12 18:46:02]

Done.

+      registry_path, kStoreKey, temp_scoped_dir_cleaner);
+  RegistryHashStoreContentsWin* contentsB = new RegistryHashStoreContentsWin(
+      registry_path, kStoreKey, temp_scoped_dir_cleaner);
+
+  contentsA.SetMac(kAtomicPrefPath, kTestStringA);
+  contentsB.SetMac(kAtomicPrefPath, kTestStringB);
+
+  temp_scoped_dir_cleaner = nullptr;
+  EXPECT_TRUE(verifying_contents.GetMac(kAtomicPrefPath, &stored_mac));
+  EXPECT_EQ(kTestStringB, stored_mac);
+
+  delete contentsB;
+  EXPECT_TRUE(verifying_contents.GetMac(kAtomicPrefPath, &stored_mac));
+  EXPECT_EQ(kTestStringB, stored_mac);
+
+  delete contentsA;
+  EXPECT_FALSE(verifying_contents.GetMac(kAtomicPrefPath, &stored_mac));
+}

[gab, 2017/06/12 15:52:16]

Also make a test with a Copy() that runs on another physical base::Thread (to
ensure TSAN doesn't complain about any racy bits)

[proberge, 2017/06/12 18:46:02]

Done (not sure what you mean by "with a Copy()", however). 

[gab, 2017/06/12 18:52:24]

I meant using HashStoreContents::MakeCopy() just like the initial bug.

[proberge, 2017/06/12 19:30:24]

Ah, gotcha. Done.