Loosen/improve AtomicFlag semantics.

base/synchronization/atomic_flag_unittest.cc

Index: base/synchronization/atomic_flag_unittest.cc
diff --git a/base/synchronization/atomic_flag_unittest.cc b/base/synchronization/atomic_flag_unittest.cc
index b0dfd47e4653970789739e24126946bd5c790a1f..7bcc680ff6b474cebbc552ee56d6efeea2eb5047 100644
--- a/base/synchronization/atomic_flag_unittest.cc
+++ b/base/synchronization/atomic_flag_unittest.cc
@@ -31,9 +31,14 @@ void ExpectSetFlagDeath(AtomicFlag* flag) {
   EXPECT_DCHECK_DEATH(flag->Set(), "");
 }
 
-void BusyWaitUntilFlagIsSet(AtomicFlag* flag) {
+// Busy waits (to explicitly avoid using synchronization constructs that would
+// defeat the purpose of testing atomics) until |flag| is set and then invokes
+// |callback|.
+void BusyWaitUntilFlagIsSet(AtomicFlag* flag, const base::Closure& callback) {

[danakj, 2016/07/28 19:57:22]

nit: this would be easier to read if you just passed the bool* and flag* here
instead of a callback.

[gab, 2016/08/01 17:46:45]

Done.

   while (!flag->IsSet())
     PlatformThread::YieldCurrentThread();
+
+  callback.Run();
 }
 
 }  // namespace
@@ -55,32 +60,79 @@ TEST(AtomicFlagTest, DoubleSetTest) {
 }
 
 TEST(AtomicFlagTest, ReadFromDifferentThread) {
-  AtomicFlag flag;
+  // |tested_flag| is the one being tested below.
+  AtomicFlag tested_flag;
+  // |expected_after_flag| is used to confirm that sequential consistency is
+  // obtained around |tested_flag|.
+  bool expected_after_flag = false;
+  // |reset_flag| is used to confirm the test flows as intended without using
+  // synchronization constructs which would defeat the purpose of exercising
+  // atomics.
+  AtomicFlag reset_flag;
+
+  const base::Closure verify_and_signal = Bind(
+      [](bool* expected_after_flag, AtomicFlag* reset_flag) {
+        EXPECT_TRUE(*expected_after_flag);
+        reset_flag->Set();
+      },
+      &expected_after_flag, &reset_flag);
 
   Thread thread("AtomicFlagTest.ReadFromDifferentThread");
   ASSERT_TRUE(thread.Start());
-  thread.task_runner()->PostTask(FROM_HERE,
-                                 Bind(&BusyWaitUntilFlagIsSet, &flag));
+  thread.task_runner()->PostTask(
+      FROM_HERE,
+      Bind(&BusyWaitUntilFlagIsSet, &tested_flag, verify_and_signal));
 
   // To verify that IsSet() fetches the flag's value from memory every time it
   // is called (not just the first time that it is called on a thread), sleep
   // before setting the flag.
-  PlatformThread::Sleep(TimeDelta::FromMilliseconds(25));
+  PlatformThread::Sleep(TimeDelta::FromMilliseconds(20));
 
-  flag.Set();
+  // |expected_after_flag| is used to verify that all memory operations
+  // performed before |tested_flag| is Set() are visible to threads that can see
+  // IsSet().
+  expected_after_flag = true;
+  tested_flag.Set();
+
+  // Sleep again to give the busy loop time to observe the flag and verify
+  // expectations.
+  PlatformThread::Sleep(TimeDelta::FromMilliseconds(20));
+
+  // Use |reset_flag| to confirm that the above completed (which the rest of
+  // this test assumes).
+  ASSERT_TRUE(reset_flag.IsSet());
+
+  tested_flag.UnsafeReset();
+  EXPECT_FALSE(tested_flag.IsSet());
+  expected_after_flag = false;
+
+  // Perform the same test again after the controlled UnsafeReset(), |thread| is
+  // guaranteed to be synchronized past the |UnsafeReset()| call when the task
+  // runs per the implicit synchronization in the post task mechanism.
+  thread.task_runner()->PostTask(
+      FROM_HERE,
+      Bind(&BusyWaitUntilFlagIsSet, &tested_flag, verify_and_signal));
+
+  PlatformThread::Sleep(TimeDelta::FromMilliseconds(20));
+
+  expected_after_flag = true;
+  tested_flag.Set();
 
   // The |thread|'s destructor will block until the posted task completes, so
   // the test will time out if it fails to see the flag be set.
 }
 
-TEST(AtomicFlagTest, SetOnDifferentThreadDeathTest) {
-  // Checks that Set() can't be called from any other thread. AtomicFlag should
-  // die on a DCHECK if Set() is called from other thread.
+TEST(AtomicFlagTest, SetOnDifferentSequenceDeathTest) {
+  // Checks that Set() can't be called from another sequence after being called
+  // on this one. AtomicFlag should die on a DCHECK if Set() is called again
+  // from another sequence.
   ::testing::FLAGS_gtest_death_test_style = "threadsafe";
   Thread t("AtomicFlagTest.SetOnDifferentThreadDeathTest");
   ASSERT_TRUE(t.Start());
+  EXPECT_TRUE(t.WaitUntilThreadStarted());
 
   AtomicFlag flag;
+  flag.Set();
   t.task_runner()->PostTask(FROM_HERE, Bind(&ExpectSetFlagDeath, &flag));
 }