Sync plugin list to local state after plugin change with delayed tasks

BUG=379148

Review URL: https://codereview.chromium.org/441013002

git-svn-id: svn://svn.chromium.org/chrome/trunk/src@287920 0039d316-1c4b-4281-b951-d872f2087c98

chrome/browser/metrics/plugin_metrics_provider.cc

@@ -11,6 +11,7 @@
 #include "base/prefs/scoped_user_pref_update.h"
 #include "base/stl_util.h"
 #include "base/strings/utf_string_conversions.h"
+#include "base/time/time.h"
 #include "chrome/browser/browser_process.h"
 #include "chrome/browser/plugins/plugin_prefs.h"
 #include "chrome/browser/profiles/profile_manager.h"
@@ -23,6 +24,9 @@
 
 namespace {
 
+// Delay for RecordCurrentState execution.
+const int kRecordStateDelayMs = 15 * base::Time::kMillisecondsPerSecond;
+
 // Returns the plugin preferences corresponding for this user, if available.
 // If multiple user profiles are loaded, returns the preferences corresponding
 // to an arbitrary one of the profiles.
@@ -126,6 +130,7 @@ void PluginMetricsProvider::ProvideSystemProfileMetrics(
 
 void PluginMetricsProvider::ProvideStabilityMetrics(
     metrics::SystemProfileProto* system_profile_proto) {
+  RecordCurrentStateIfPending();
   const base::ListValue* plugin_stats_list = local_state_->GetList(
       prefs::kStabilityPluginStats);
   if (!plugin_stats_list)
@@ -299,6 +304,7 @@ void PluginMetricsProvider::LogPluginLoadingError(
     DCHECK(IsPluginProcess(stats.process_type));
   }
   stats.loading_errors++;
+  RecordCurrentStateWithDelay(kRecordStateDelayMs);
 }
 
 void PluginMetricsProvider::SetPluginsForTesting(
@@ -339,14 +345,38 @@ PluginMetricsProvider::GetChildProcessStats(
 void PluginMetricsProvider::BrowserChildProcessHostConnected(
     const content::ChildProcessData& data) {
   GetChildProcessStats(data).process_launches++;
+  RecordCurrentStateWithDelay(kRecordStateDelayMs);
 }
 
 void PluginMetricsProvider::BrowserChildProcessCrashed(
     const content::ChildProcessData& data) {
   GetChildProcessStats(data).process_crashes++;
+  RecordCurrentStateWithDelay(kRecordStateDelayMs);
 }
 
 void PluginMetricsProvider::BrowserChildProcessInstanceCreated(
     const content::ChildProcessData& data) {
   GetChildProcessStats(data).instances++;
+  RecordCurrentStateWithDelay(kRecordStateDelayMs);
+}
+
+bool PluginMetricsProvider::RecordCurrentStateWithDelay(int delay_sec) {
+  if (weak_ptr_factory_.HasWeakPtrs())
+    return false;
+
+  base::MessageLoopProxy::current()->PostDelayedTask(
+      FROM_HERE,
+      base::Bind(&PluginMetricsProvider::RecordCurrentState,
+                weak_ptr_factory_.GetWeakPtr()),
+                base::TimeDelta::FromMilliseconds(delay_sec));
+  return true;
+}
+
+bool PluginMetricsProvider::RecordCurrentStateIfPending() {
+  if (!weak_ptr_factory_.HasWeakPtrs())
+    return false;
+
+  weak_ptr_factory_.InvalidateWeakPtrs();
+  RecordCurrentState();
+  return true;
 }

chrome/browser/metrics/plugin_metrics_provider.h

@@ -9,6 +9,7 @@
 #include <vector>
 
 #include "base/basictypes.h"
+#include "base/gtest_prod_util.h"
 #include "base/memory/weak_ptr.h"
 #include "base/strings/string16.h"
 #include "components/metrics/metrics_provider.h"
@@ -42,7 +43,6 @@ class PluginMetricsProvider : public metrics::MetricsProvider,
       metrics::SystemProfileProto* system_profile_proto) OVERRIDE;
   virtual void ProvideStabilityMetrics(
       metrics::SystemProfileProto* system_profile_proto) OVERRIDE;
-  virtual void RecordCurrentState() OVERRIDE;
 
   // Notifies the provider about an error loading the plugin at |plugin_path|.
   void LogPluginLoadingError(const base::FilePath& plugin_path);
@@ -58,6 +58,12 @@ class PluginMetricsProvider : public metrics::MetricsProvider,
   static void RegisterPrefs(PrefRegistrySimple* registry);
 
  private:
+  FRIEND_TEST_ALL_PREFIXES(PluginMetricsProviderTest,
+                           RecordCurrentStateWithDelay);
+  FRIEND_TEST_ALL_PREFIXES(PluginMetricsProviderTest,
+                           RecordCurrentStateIfPending);
+  FRIEND_TEST_ALL_PREFIXES(PluginMetricsProviderTest,
+                           ProvideStabilityMetricsWhenPendingTask);
   struct ChildProcessStats;
 
   // Receives the plugin list from the PluginService and calls |done_callback|.
@@ -68,6 +74,18 @@ class PluginMetricsProvider : public metrics::MetricsProvider,
   ChildProcessStats& GetChildProcessStats(
       const content::ChildProcessData& data);
 
+  // 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:
   virtual void BrowserChildProcessHostConnected(
       const content::ChildProcessData& data) OVERRIDE;

chrome/browser/metrics/plugin_metrics_provider_unittest.cc

@@ -10,9 +10,11 @@
 #include "base/prefs/pref_service.h"
 #include "base/prefs/scoped_user_pref_update.h"
 #include "base/prefs/testing_pref_service.h"
+#include "base/run_loop.h"
 #include "base/strings/utf_string_conversions.h"
 #include "chrome/common/pref_names.h"
 #include "components/metrics/proto/system_profile.pb.h"
+#include "content/public/browser/child_process_data.h"
 #include "content/public/common/process_type.h"
 #include "content/public/common/webplugininfo.h"
 #include "content/public/test/test_browser_thread_bundle.h"
@@ -36,9 +38,27 @@ content::WebPluginInfo CreateFakePluginInfo(
   return plugin;
 }
 
+class PluginMetricsProviderTest : public ::testing::Test {
+ protected:
+  PluginMetricsProviderTest()
+    : prefs_(new TestingPrefServiceSimple) {
+      PluginMetricsProvider::RegisterPrefs(prefs()->registry());
+    }
+
+  TestingPrefServiceSimple* prefs() {
+    return prefs_.get();
+  }
+
+ private:
+  scoped_ptr<TestingPrefServiceSimple> prefs_;
+
+  DISALLOW_COPY_AND_ASSIGN(PluginMetricsProviderTest);
+
+};
+
 }  // namespace
 
-TEST(PluginMetricsProviderTest, IsPluginProcess) {
+TEST_F(PluginMetricsProviderTest, IsPluginProcess) {
   EXPECT_TRUE(PluginMetricsProvider::IsPluginProcess(
       content::PROCESS_TYPE_PLUGIN));
   EXPECT_TRUE(PluginMetricsProvider::IsPluginProcess(
@@ -47,12 +67,10 @@ TEST(PluginMetricsProviderTest, IsPluginProcess) {
       content::PROCESS_TYPE_GPU));
 }
 
-TEST(PluginMetricsProviderTest, Plugins) {
+TEST_F(PluginMetricsProviderTest, Plugins) {
   content::TestBrowserThreadBundle thread_bundle;
 
-  TestingPrefServiceSimple prefs;
-  PluginMetricsProvider::RegisterPrefs(prefs.registry());
-  PluginMetricsProvider provider(&prefs);
+  PluginMetricsProvider provider(prefs());
 
   std::vector<content::WebPluginInfo> plugins;
   plugins.push_back(CreateFakePluginInfo("p1", FILE_PATH_LITERAL("p1.plugin"),
@@ -82,7 +100,7 @@ TEST(PluginMetricsProviderTest, Plugins) {
   plugin_dict->SetInteger(prefs::kStabilityPluginInstances, 3);
   plugin_dict->SetInteger(prefs::kStabilityPluginLoadingErrors, 4);
   {
-    ListPrefUpdate update(&prefs, prefs::kStabilityPluginStats);
+    ListPrefUpdate update(prefs(), prefs::kStabilityPluginStats);
     update.Get()->Append(plugin_dict.release());
   }
 
@@ -100,3 +118,66 @@ TEST(PluginMetricsProviderTest, Plugins) {
   EXPECT_EQ(3, stability.plugin_stability(0).instance_count());
   EXPECT_EQ(4, stability.plugin_stability(0).loading_error_count());
 }
+
+TEST_F(PluginMetricsProviderTest, RecordCurrentStateWithDelay) {
+  content::TestBrowserThreadBundle thread_bundle;
+
+  PluginMetricsProvider provider(prefs());
+
+  int delay_ms = 10;
+  EXPECT_TRUE(provider.RecordCurrentStateWithDelay(delay_ms));
+  EXPECT_FALSE(provider.RecordCurrentStateWithDelay(delay_ms));
+
+  base::PlatformThread::Sleep(base::TimeDelta::FromMilliseconds(delay_ms));
+  base::RunLoop().RunUntilIdle();
+
+  EXPECT_TRUE(provider.RecordCurrentStateWithDelay(delay_ms));
+}
+
+TEST_F(PluginMetricsProviderTest, RecordCurrentStateIfPending) {
+  content::TestBrowserThreadBundle thread_bundle;
+
+  PluginMetricsProvider provider(prefs());
+
+  // First there should be no need to force RecordCurrentState.
+  EXPECT_FALSE(provider.RecordCurrentStateIfPending());
+
+  // After delayed task is posted RecordCurrentStateIfPending should return
+  // true.
+  int delay_ms = 100000;
+  EXPECT_TRUE(provider.RecordCurrentStateWithDelay(delay_ms));
+  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));
+}
+
+TEST_F(PluginMetricsProviderTest, ProvideStabilityMetricsWhenPendingTask) {
+  content::TestBrowserThreadBundle thread_bundle;
+
+  PluginMetricsProvider provider(prefs());
+
+  // Create plugin information for testing.
+  std::vector<content::WebPluginInfo> plugins;
+  plugins.push_back(CreateFakePluginInfo("p1", FILE_PATH_LITERAL("p1.plugin"),
+                                         "1.5", true));
+  provider.SetPluginsForTesting(plugins);
+  metrics::SystemProfileProto system_profile;
+  provider.ProvideSystemProfileMetrics(&system_profile);
+
+  // Increase number of created instances which should also start a delayed
+  // task.
+  content::ChildProcessData child_process_data(content::PROCESS_TYPE_PLUGIN);
+  child_process_data.name = base::UTF8ToUTF16("p1");
+  provider.BrowserChildProcessInstanceCreated(child_process_data);
+
+  // Call ProvideStabilityMetrics to check that it will force pending tasks to
+  // be executed immediately.
+  provider.ProvideStabilityMetrics(&system_profile);
+
+  // Check current number of instances created.
+  const metrics::SystemProfileProto_Stability& stability =
+      system_profile.stability();
+  EXPECT_EQ(1, stability.plugin_stability(0).instance_count());
+}

components/metrics/metrics_provider.h

@@ -44,9 +44,6 @@ class MetricsProvider {
   virtual void ProvideGeneralMetrics(
       ChromeUserMetricsExtension* uma_proto) {}
 
-  // TODO(asvitkine): Remove this method. http://crbug.com/379148
-  virtual void RecordCurrentState() {}
-
  private:
   DISALLOW_COPY_AND_ASSIGN(MetricsProvider);
 };

components/metrics/metrics_service.cc

@@ -1194,7 +1194,4 @@ void MetricsService::RecordBooleanPrefValue(const char* path, bool value) {
 void MetricsService::RecordCurrentState(PrefService* pref) {
   pref->SetInt64(metrics::prefs::kStabilityLastTimestampSec,
                  Time::Now().ToTimeT());
-
-  for (size_t i = 0; i < metrics_providers_.size(); ++i)
-    metrics_providers_[i]->RecordCurrentState();
 }