Deflake PluginMetricsProviderTest.RecordCurrentStateWithDelay.

This test is flaky on platforms that where Sleep(delta) isn't
guaranteed to be in TimeTicks (e.g. on Mac, it's in base::Time).
Use base::test::ScopedTaskEnvironment::MainThreadType::MOCK_TIME
instead.

Also, cleans up some APIs and adds a comment to the Sleep() API.

Bug: 915672
Change-Id: Ib539c0d06a217dfdb9ee6f7411e885d6975438a9
Reviewed-on: https://chromium-review.googlesource.com/c/1427927
Commit-Queue: Alexei Svitkine <asvitkine@chromium.org>
Reviewed-by: Gabriel Charette <gab@chromium.org>
Cr-Commit-Position: refs/heads/master@{#625387}

base/threading/platform_thread.h

@@ -145,7 +145,10 @@ class BASE_EXPORT PlatformThread {
   // Yield the current thread so another thread can be scheduled.
   static void YieldCurrentThread();
 
-  // Sleeps for the specified duration.
+  // Sleeps for the specified duration. Note: The sleep duration may be in
+  // base::Time or base::TimeTicks, depending on platform. If you're looking to
+  // use this in unit tests testing delayed tasks, this will be unreliable -
+  // instead, use base::test::ScopedTaskEnvironment with MOCK_TIME mode.
   static void Sleep(base::TimeDelta duration);
 
   // Sets the thread name visible to debuggers/tools. This will try to

chrome/browser/metrics/plugin_metrics_provider.cc

@@ -30,7 +30,7 @@
 namespace {
 
 // Delay for RecordCurrentState execution.
-const int kRecordStateDelayMs = 15 * base::Time::kMillisecondsPerSecond;
+constexpr base::TimeDelta kRecordStateDelay = base::TimeDelta::FromSeconds(15);
 
 // Returns the plugin preferences corresponding for this user, if available.
 // If multiple user profiles are loaded, returns the preferences corresponding
@@ -303,7 +303,7 @@ void PluginMetricsProvider::LogPluginLoadingError(
     DCHECK(IsPluginProcess(stats.process_type));
   }
   stats.loading_errors++;
-  RecordCurrentStateWithDelay(kRecordStateDelayMs);
+  RecordCurrentStateWithDelay();
 }
 
 void PluginMetricsProvider::SetPluginsForTesting(
@@ -343,14 +343,14 @@ PluginMetricsProvider::GetChildProcessStats(
 void PluginMetricsProvider::BrowserChildProcessHostConnected(
     const content::ChildProcessData& data) {
   GetChildProcessStats(data).process_launches++;
-  RecordCurrentStateWithDelay(kRecordStateDelayMs);
+  RecordCurrentStateWithDelay();
 }
 
 void PluginMetricsProvider::BrowserChildProcessCrashed(
     const content::ChildProcessData& data,
     const content::ChildProcessTerminationInfo& info) {
   GetChildProcessStats(data).process_crashes++;
-  RecordCurrentStateWithDelay(kRecordStateDelayMs);
+  RecordCurrentStateWithDelay();
 }
 
 void PluginMetricsProvider::BrowserChildProcessKilled(
@@ -360,10 +360,10 @@ void PluginMetricsProvider::BrowserChildProcessKilled(
   // actual crashes, which is treated as a kill rather than a crash by
   // base::GetTerminationStatus
   GetChildProcessStats(data).process_crashes++;
-  RecordCurrentStateWithDelay(kRecordStateDelayMs);
+  RecordCurrentStateWithDelay();
 }
 
-bool PluginMetricsProvider::RecordCurrentStateWithDelay(int delay_sec) {
+bool PluginMetricsProvider::RecordCurrentStateWithDelay() {
   if (weak_ptr_factory_.HasWeakPtrs())
     return false;
 
@@ -371,7 +371,7 @@ bool PluginMetricsProvider::RecordCurrentStateWithDelay(int delay_sec) {
       FROM_HERE,
       base::BindOnce(&PluginMetricsProvider::RecordCurrentState,
                      weak_ptr_factory_.GetWeakPtr()),
-      base::TimeDelta::FromMilliseconds(delay_sec));
+      kRecordStateDelay);
   return true;
 }
 
@@ -383,3 +383,8 @@ bool PluginMetricsProvider::RecordCurrentStateIfPending() {
   RecordCurrentState();
   return true;
 }
+
+// static
+base::TimeDelta PluginMetricsProvider::GetRecordStateDelay() {
+  return kRecordStateDelay;
+}

chrome/browser/metrics/plugin_metrics_provider.h

@@ -13,6 +13,7 @@
 #include "base/macros.h"
 #include "base/memory/weak_ptr.h"
 #include "base/strings/string16.h"
+#include "base/time/time.h"
 #include "components/metrics/metrics_provider.h"
 #include "content/public/browser/browser_child_process_observer.h"
 
@@ -76,15 +77,6 @@ class PluginMetricsProvider : public metrics::MetricsProvider,
   // Saves plugin information to local state.
   void RecordCurrentState();
 
-  // Posts a delayed task for RecordCurrentState. Returns true if new task is
-  // posted and false if there was one already waiting for execution.
-  // The param delay_sec is for unit tests.
-  bool RecordCurrentStateWithDelay(int delay_ms);
-
-  // If a delayed RecordCurrnetState task exists then cancels it, calls
-  // RecordCurrentState immediately and returns true. Otherwise returns false.
-  bool RecordCurrentStateIfPending();
-
   // content::BrowserChildProcessObserver:
   void BrowserChildProcessHostConnected(
       const content::ChildProcessData& data) override;
@@ -95,6 +87,17 @@ class PluginMetricsProvider : public metrics::MetricsProvider,
       const content::ChildProcessData& data,
       const content::ChildProcessTerminationInfo& info) override;
 
+  // Posts a delayed task for RecordCurrentState. Returns true if new task is
+  // posted and false if there was one already waiting for execution.
+  bool RecordCurrentStateWithDelay();
+
+  // If a delayed RecordCurrnetState task exists then cancels it, calls
+  // RecordCurrentState immediately and returns true. Otherwise returns false.
+  bool RecordCurrentStateIfPending();
+
+  // Records the delay used internally by RecordCurrentStateWithDelay().
+  static base::TimeDelta GetRecordStateDelay();
+
   PrefService* local_state_;
 
   // The list of plugins which was retrieved on the file thread.

chrome/browser/metrics/plugin_metrics_provider_unittest.cc

@@ -12,6 +12,7 @@
 #include "base/macros.h"
 #include "base/run_loop.h"
 #include "base/strings/utf_string_conversions.h"
+#include "base/test/scoped_task_environment.h"
 #include "chrome/common/pref_names.h"
 #include "components/prefs/pref_service.h"
 #include "components/prefs/scoped_user_pref_update.h"
@@ -117,22 +118,23 @@ TEST_F(PluginMetricsProviderTest, Plugins) {
 }
 
 TEST_F(PluginMetricsProviderTest, RecordCurrentStateWithDelay) {
-  content::TestBrowserThreadBundle thread_bundle;
+  content::TestBrowserThreadBundle thread_bundle(
+      base::test::ScopedTaskEnvironment::MainThreadType::MOCK_TIME);
 
   PluginMetricsProvider provider(prefs());
 
-  int delay_ms = 10;
-  EXPECT_TRUE(provider.RecordCurrentStateWithDelay(delay_ms));
-  EXPECT_FALSE(provider.RecordCurrentStateWithDelay(delay_ms));
+  EXPECT_TRUE(provider.RecordCurrentStateWithDelay());
+  EXPECT_FALSE(provider.RecordCurrentStateWithDelay());
 
-  base::PlatformThread::Sleep(base::TimeDelta::FromMilliseconds(delay_ms));
+  thread_bundle.FastForwardBy(PluginMetricsProvider::GetRecordStateDelay());
   base::RunLoop().RunUntilIdle();
 
-  EXPECT_TRUE(provider.RecordCurrentStateWithDelay(delay_ms));
+  EXPECT_TRUE(provider.RecordCurrentStateWithDelay());
 }
 
 TEST_F(PluginMetricsProviderTest, RecordCurrentStateIfPending) {
-  content::TestBrowserThreadBundle thread_bundle;
+  content::TestBrowserThreadBundle thread_bundle(
+      base::test::ScopedTaskEnvironment::MainThreadType::MOCK_TIME);
 
   PluginMetricsProvider provider(prefs());
 
@@ -141,13 +143,12 @@ TEST_F(PluginMetricsProviderTest, RecordCurrentStateIfPending) {
 
   // After delayed task is posted RecordCurrentStateIfPending should return
   // true.
-  int delay_ms = 100000;
-  EXPECT_TRUE(provider.RecordCurrentStateWithDelay(delay_ms));
+  EXPECT_TRUE(provider.RecordCurrentStateWithDelay());
   EXPECT_TRUE(provider.RecordCurrentStateIfPending());
 
   // If RecordCurrentStateIfPending was successful then we should be able to
   // post a new delayed task.
-  EXPECT_TRUE(provider.RecordCurrentStateWithDelay(delay_ms));
+  EXPECT_TRUE(provider.RecordCurrentStateWithDelay());
 }
 
 TEST_F(PluginMetricsProviderTest, ProvideStabilityMetricsWhenPendingTask) {