Implement the WindowsLogoffRace finch experiment.

chrome/browser/profiles/profile_browsertest.cc

 // Copyright (c) 2012 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #include "chrome/browser/profiles/profile.h"
 
 #include "base/command_line.h"
 #include "base/file_util.h"
 #include "base/files/scoped_temp_dir.h"
 #include "base/json/json_reader.h"
+#include "base/metrics/field_trial.h"
 #include "base/prefs/pref_service.h"
 #include "base/sequenced_task_runner.h"
 #include "base/synchronization/waitable_event.h"
 #include "base/values.h"
 #include "base/version.h"
 #include "chrome/browser/browser_process.h"
 #include "chrome/browser/chrome_notification_types.h"
 #include "chrome/browser/profiles/chrome_version_service.h"
 #include "chrome/browser/profiles/profile_impl.h"
 #include "chrome/browser/profiles/profile_manager.h"
@@ skipping 323 matching lines @@
 IN_PROC_BROWSER_TEST_F(ProfileBrowserTest,
                        WritesProfilesSynchronouslyOnEndSession) {
   base::ScopedTempDir temp_dir;
   ASSERT_TRUE(temp_dir.CreateUniqueTempDir());
 
   ProfileManager* profile_manager = g_browser_process->profile_manager();
   ASSERT_TRUE(profile_manager);
   std::vector<Profile*> loaded_profiles = profile_manager->GetLoadedProfiles();
 
   ASSERT_NE(loaded_profiles.size(), 0UL);
-  for (size_t i = 0; i < loaded_profiles.size(); ++i) {
-    Profile* profile = loaded_profiles[i];
+  Profile* profile = loaded_profiles[0];
 
+  // Flush the profile data to disk for all loaded profiles.
+  profile->SetExitType(Profile::EXIT_CRASHED);
+  FlushTaskRunner(profile->GetIOTaskRunner());
+
+  // Make sure that the prefs file was written with the expected key/value.
+  ASSERT_EQ(GetExitTypePreferenceFromDisk(profile), "Crashed");
+
+  // This must not return until the profile data has been written to disk.
+  // If this test flakes, then logoff on Windows has broken again.
+  g_browser_process->EndSession();
+
+  // Make sure that the prefs file was written with the expected key/value.
+  ASSERT_EQ(GetExitTypePreferenceFromDisk(profile), "SessionEnded");
+}
+
+IN_PROC_BROWSER_TEST_F(ProfileBrowserTest,
+                       EndSessionBrokenSynchronizationExperiment) {
+  base::ScopedTempDir temp_dir;
+  ASSERT_TRUE(temp_dir.CreateUniqueTempDir());
+
+  // Select into the field trial group.
+  base::FieldTrialList::CreateFieldTrial("WindowsLogoffRace",
+                                         "BrokenSynchronization");
+
+  ProfileManager* profile_manager = g_browser_process->profile_manager();
+  ASSERT_TRUE(profile_manager);
+  std::vector<Profile*> loaded_profiles = profile_manager->GetLoadedProfiles();
+
+  ASSERT_NE(loaded_profiles.size(), 0UL);
+  Profile* profile = loaded_profiles[0];
+
+  bool mis_wrote = false;
+  // This retry loop reduces flakiness due to the fact that this ultimately
+  // tests whether or not a code path hits a timed wait.
+  for (size_t retries = 0; retries < 3; ++retries) {
     // Flush the profile data to disk for all loaded profiles.
     profile->SetExitType(Profile::EXIT_CRASHED);
     FlushTaskRunner(profile->GetIOTaskRunner());
 
     // Make sure that the prefs file was written with the expected key/value.
     ASSERT_EQ(GetExitTypePreferenceFromDisk(profile), "Crashed");
+
+    base::WaitableEvent is_blocked(false, false);
+    base::WaitableEvent* unblock = new base::WaitableEvent(false, false);
+
+    // Block the profile's IO thread.
+    profile->GetIOTaskRunner()->PostTask(FROM_HERE,
+        base::Bind(&BlockThread, &is_blocked, base::Owned(unblock)));

[gab, 2014/07/11 17:13:44]

Actually base::Owned here makes it such that |unblock| is deleted after running
BlockThread()... so |unblock->Signal()| at the bottom is a UAF.

What you want is to put |unblock| on the stack and use base::Unretained() for
this callback IMO.

[Sigurður Ásgeirsson, 2014/07/14 17:04:14]

As we discussed, you've read this backwards. The code is good as-is.

+    // Wait for the IO thread to actually be blocked.
+    is_blocked.Wait();
+
+    // The blocking wait in EndSession has a timeout, so a non-write can only be
+    // concluded if it happens in less time than the timeout.
+    base::Time start = base::Time::Now();
+
+    // This must not return until the profile data has been written to disk.
+    // If this test flakes, then logoff on Windows has broken again.
+    g_browser_process->EndSession();
+
+    base::Time end = base::Time::Now();
+
+    // The EndSession timeout is 10 seconds, we take a 5 second run-through as
+    // sufficient proof that we didn't hit the timed wait.
+    if (end - start < base::TimeDelta::FromSeconds(5)) {

[gab, 2014/07/11 17:13:44]

Since you're adding this here, I think it makes to assert in your non-experiment
test that despite the wait happening, EndSession() finished before the timeout
expired (i.e. end - start < 10).

[Sigurður Ásgeirsson, 2014/07/14 17:04:14]

Done.

+      // Make sure that the prefs file is unmodified with the expected
+      // key/value.
+      EXPECT_EQ(GetExitTypePreferenceFromDisk(profile), "Crashed");
+      mis_wrote = true;
+    }
+
+    // Release the IO thread thread.
+    unblock->Signal();
   }
 
-  // This must not return until the profile data has been written to disk.
-  // If this test flakes, then logoff on Windows has broken again.
-  g_browser_process->EndSession();
+  // Reset the field trial registration.
+  // TODO(siggi): is this necessary? Maybe the browser test fixture resets
+  //    the field trial list?
+  base::FieldTrialList::CreateFieldTrial("WindowsLogoffRace",
+                                         "BrokenSynchronization");

[gab, 2014/07/11 17:13:44]

Each field trial should be unique to a given browser process IMO and each
browser test runs in its own browser process so no need to clean this up IMO.

[Sigurður Ásgeirsson, 2014/07/14 17:04:14]

Done.

 
-  // Verify that the setting was indeed written.
-  for (size_t i = 0; i < loaded_profiles.size(); ++i) {
-    Profile* profile = loaded_profiles[i];
-    // Make sure that the prefs file was written with the expected key/value.
-    ASSERT_EQ(GetExitTypePreferenceFromDisk(profile), "SessionEnded");
-  }
+  ASSERT_TRUE(mis_wrote);
 }
+
 #endif  // defined(USE_X11) || defined(OS_WIN)