Introduce ChromeStabilityMetricsProvider

Introduce ChromeStabilityMetricsProvider

This class provides Chrome-specific stability metrics, enabling moving code
related to these metrics out of MetricsLog and MetricsService. It also
eliminates all notification listening from MetricsService. As part of the
latter change, ThreadWatcherObserver now registers explicitly for the
notifications it is concerned with rather than going through MetricsService for
this purpose.

BUG=375237
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=272426

[Alexei Svitkine (slow), 2014-05-21 15:02:29]

https://codereview.chromium.org/289283011/diff/1/chrome/browser/metrics/chrom...
File chrome/browser/metrics/chrome_stability_metrics_provider.cc (right):

https://codereview.chromium.org/289283011/diff/1/chrome/browser/metrics/chrom...
chrome/browser/metrics/chrome_stability_metrics_provider.cc:13: #include
"base/strings/string16.h"
Nit: Is this needed?

https://codereview.chromium.org/289283011/diff/1/chrome/browser/metrics/chrom...
chrome/browser/metrics/chrome_stability_metrics_provider.cc:14: #include
"base/strings/string_util.h"
Nit: ditto

https://codereview.chromium.org/289283011/diff/1/chrome/browser/metrics/chrom...
chrome/browser/metrics/chrome_stability_metrics_provider.cc:60: //
TODO(blundell): Rename this class.
Remove.

https://codereview.chromium.org/289283011/diff/1/chrome/browser/metrics/chrom...
File chrome/browser/metrics/chrome_stability_metrics_provider.h (right):

https://codereview.chromium.org/289283011/diff/1/chrome/browser/metrics/chrom...
chrome/browser/metrics/chrome_stability_metrics_provider.h:56: //
TODO(blundell): Is this needed?
I don't think it is in this case.

[blundell, 2014-05-21 15:14:16]

https://codereview.chromium.org/289283011/diff/1/chrome/browser/metrics/chrom...
File chrome/browser/metrics/chrome_stability_metrics_provider.cc (right):

https://codereview.chromium.org/289283011/diff/1/chrome/browser/metrics/chrom...
chrome/browser/metrics/chrome_stability_metrics_provider.cc:13: #include
"base/strings/string16.h"
On 2014/05/21 15:02:29, Alexei Svitkine wrote:
> Nit: Is this needed?

Done.

https://codereview.chromium.org/289283011/diff/1/chrome/browser/metrics/chrom...
chrome/browser/metrics/chrome_stability_metrics_provider.cc:14: #include
"base/strings/string_util.h"
On 2014/05/21 15:02:29, Alexei Svitkine wrote:
> Nit: ditto

Done.

https://codereview.chromium.org/289283011/diff/1/chrome/browser/metrics/chrom...
chrome/browser/metrics/chrome_stability_metrics_provider.cc:60: //
TODO(blundell): Rename this class.
On 2014/05/21 15:02:29, Alexei Svitkine wrote:
> Remove.

Done.

https://codereview.chromium.org/289283011/diff/1/chrome/browser/metrics/chrom...
File chrome/browser/metrics/chrome_stability_metrics_provider.h (right):

https://codereview.chromium.org/289283011/diff/1/chrome/browser/metrics/chrom...
chrome/browser/metrics/chrome_stability_metrics_provider.h:56: //
TODO(blundell): Is this needed?
On 2014/05/21 15:02:29, Alexei Svitkine wrote:
> I don't think it is in this case.

Done.

[Alexei Svitkine (slow), 2014-05-21 15:18:53]

LGTM % comments

Thanks!

https://codereview.chromium.org/289283011/diff/1/chrome/browser/metrics/chrom...
File chrome/browser/metrics/chrome_stability_metrics_provider.cc (right):

https://codereview.chromium.org/289283011/diff/1/chrome/browser/metrics/chrom...
chrome/browser/metrics/chrome_stability_metrics_provider.cc:14: #include
"base/strings/string_util.h"
On 2014/05/21 15:14:17, blundell wrote:
> On 2014/05/21 15:02:29, Alexei Svitkine wrote:
> > Nit: ditto
> 
> Done.

Do you mean it is actually needed? (Just making sure.)

https://codereview.chromium.org/289283011/diff/40001/chrome/browser/metrics/c...
File chrome/browser/metrics/chrome_stability_metrics_provider.h (right):

https://codereview.chromium.org/289283011/diff/40001/chrome/browser/metrics/c...
chrome/browser/metrics/chrome_stability_metrics_provider.h:12: #include
"components/metrics/proto/chrome_user_metrics_extension.pb.h"
Nit: I think this can be removed and
"components/metrics/proto/system_profile.pb.h" included from the .cc file now.

[blundell, 2014-05-21 15:23:17]

https://codereview.chromium.org/289283011/diff/1/chrome/browser/metrics/chrom...
File chrome/browser/metrics/chrome_stability_metrics_provider.cc (right):

https://codereview.chromium.org/289283011/diff/1/chrome/browser/metrics/chrom...
chrome/browser/metrics/chrome_stability_metrics_provider.cc:14: #include
"base/strings/string_util.h"
oops, removed both but forgot to save the file. Removed now.

On 2014/05/21 15:18:54, Alexei Svitkine wrote:
> On 2014/05/21 15:14:17, blundell wrote:
> > On 2014/05/21 15:02:29, Alexei Svitkine wrote:
> > > Nit: ditto
> > 
> > Done.
> 
> Do you mean it is actually needed? (Just making sure.)

https://codereview.chromium.org/289283011/diff/40001/chrome/browser/metrics/c...
File chrome/browser/metrics/chrome_stability_metrics_provider.h (right):

https://codereview.chromium.org/289283011/diff/40001/chrome/browser/metrics/c...
chrome/browser/metrics/chrome_stability_metrics_provider.h:12: #include
"components/metrics/proto/chrome_user_metrics_extension.pb.h"
On 2014/05/21 15:18:54, Alexei Svitkine wrote:
> Nit: I think this can be removed and
> "components/metrics/proto/system_profile.pb.h" included from the .cc file now.

Done.

[blundell, 2014-05-22 09:23:51]

Alexei,

Could you take another look? I've made a bunch of changes around notifications
after rebasing onto Ilya's CL moving the idle->not idle transition handling out
of MetricsService. I believe that CL actually introduced a bug wherein
ThreadWatcherObserver was no longer listening for all the notifications it
should have been.

[Alexei Svitkine (slow), 2014-05-22 09:32:23]

LGTM, good catch!

[blundell, 2014-05-22 09:35:38]

The CQ bit was checked by blundell@chromium.org

[commit-bot: I haz the power, 2014-05-22 09:37:42]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/blundell@chromium.org/289283011/80001

[blundell, 2014-05-22 14:06:39]

The CQ bit was checked by blundell@chromium.org

[commit-bot: I haz the power, 2014-05-22 14:07:55]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/blundell@chromium.org/289283011/100001

[commit-bot: I haz the power, 2014-05-22 18:28:54]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-05-22 18:28:54]

Failed to apply patch for chrome/browser/metrics/metrics_service.cc:
While running patch -p1 --forward --force --no-backup-if-mismatch;
  patching file chrome/browser/metrics/metrics_service.cc
  Hunk #1 FAILED at 185.
  Hunk #2 FAILED at 205.
  Hunk #3 succeeded at 234 (offset -4 lines).
  Hunk #4 succeeded at 292 (offset -8 lines).
  Hunk #5 succeeded at 442 (offset -27 lines).
  Hunk #6 succeeded at 532 (offset -27 lines).
  Hunk #7 succeeded at 546 (offset -27 lines).
  Hunk #8 succeeded at 564 (offset -27 lines).
  Hunk #9 succeeded at 583 (offset -27 lines).
  Hunk #10 succeeded at 1428 (offset -106 lines).
  2 out of 10 hunks FAILED -- saving rejects to file
chrome/browser/metrics/metrics_service.cc.rej

Patch:       chrome/browser/metrics/metrics_service.cc
Index: chrome/browser/metrics/metrics_service.cc
diff --git a/chrome/browser/metrics/metrics_service.cc
b/chrome/browser/metrics/metrics_service.cc
index
568cb32d3ccb31704a03f0839b41888f40504b22..cf11dd677ec08b6f3bf0702d3876427e1054ce60
100644
--- a/chrome/browser/metrics/metrics_service.cc
+++ b/chrome/browser/metrics/metrics_service.cc
@@ -185,6 +185,7 @@
 #include "chrome/browser/chrome_notification_types.h"
 #include "chrome/browser/io_thread.h"
 #include "chrome/browser/memory_details.h"
+#include "chrome/browser/metrics/chrome_stability_metrics_provider.h"
 #include "chrome/browser/metrics/compression_utils.h"
 #include "chrome/browser/metrics/metrics_log.h"
 #include "chrome/browser/metrics/metrics_state_manager.h"
@@ -205,15 +206,11 @@
 #include "components/variations/metrics_util.h"
 #include "content/public/browser/child_process_data.h"
 #include "content/public/browser/histogram_fetcher.h"
-#include "content/public/browser/load_notification_details.h"
-#include "content/public/browser/notification_service.h"
 #include "content/public/browser/plugin_service.h"
 #include "content/public/browser/render_process_host.h"
 #include "content/public/browser/user_metrics.h"
-#include "content/public/browser/web_contents.h"
 #include "content/public/common/process_type.h"
 #include "content/public/common/webplugininfo.h"
-#include "extensions/browser/process_map.h"
 #include "net/base/load_flags.h"
 #include "net/url_request/url_fetcher.h"
 
@@ -238,7 +235,6 @@
 using base::Time;
 using content::BrowserThread;
 using content::ChildProcessData;
-using content::LoadNotificationDetails;
 using content::PluginService;
 using metrics::MetricsLogManager;
 
@@ -301,20 +297,6 @@ ResponseStatus ResponseCodeToStatus(int response_code) {
   }
 }
 
-// Converts an exit code into something that can be inserted into our
-// histograms (which expect non-negative numbers less than MAX_INT).
-int MapCrashExitCodeForHistogram(int exit_code) {
-#if defined(OS_WIN)
-  // Since |abs(STATUS_GUARD_PAGE_VIOLATION) == MAX_INT| it causes problems in
-  // histograms.cc. Solve this by remapping it to a smaller value, which
-  // hopefully doesn't conflict with other codes.
-  if (exit_code == STATUS_GUARD_PAGE_VIOLATION)
-    return 0x1FCF7EC3;  // Randomly picked number.
-#endif
-
-  return std::abs(exit_code);
-}
-
 void MarkAppCleanShutdownAndCommit() {
   PrefService* pref = g_browser_process->local_state();
   pref->SetBoolean(prefs::kStabilityExitedCleanly, true);
@@ -484,9 +466,11 @@
MetricsService::MetricsService(metrics::MetricsStateManager* state_manager,
           g_browser_process->local_state())));
 #endif  // defined(OS_ANDROID)
 
-  // TODO(asvitkine): Move this out of MetricsService.
+  // TODO(asvitkine): Move these out of MetricsService.
   RegisterMetricsProvider(
       scoped_ptr<metrics::MetricsProvider>(new OmniboxMetricsProvider));
+  RegisterMetricsProvider(
+      scoped_ptr<metrics::MetricsProvider>(new
ChromeStabilityMetricsProvider));
 
 #if defined(OS_WIN)
   google_update_metrics_provider_ = new GoogleUpdateMetricsProviderWin;
@@ -572,7 +556,6 @@ void MetricsService::EnableRecording() {
   for (size_t i = 0; i < metrics_providers_.size(); ++i)
     metrics_providers_[i]->OnRecordingEnabled();
 
-  SetUpNotifications(&registrar_, this);
   base::RemoveActionCallback(action_callback_);
   action_callback_ = base::Bind(&MetricsService::OnUserAction,
                                 base::Unretained(this));
@@ -587,7 +570,6 @@ void MetricsService::DisableRecording() {
   recording_active_ = false;
 
   base::RemoveActionCallback(action_callback_);
-  registrar_.RemoveAll();
 
   for (size_t i = 0; i < metrics_providers_.size(); ++i)
     metrics_providers_[i]->OnRecordingDisabled();
@@ -606,18 +588,6 @@ bool MetricsService::reporting_active() const {
   return reporting_active_;
 }
 
-// static
-void MetricsService::SetUpNotifications(
-    content::NotificationRegistrar* registrar,
-    content::NotificationObserver* observer) {
-  registrar->Add(observer, content::NOTIFICATION_LOAD_START,
-                 content::NotificationService::AllSources());
-  registrar->Add(observer, content::NOTIFICATION_RENDERER_PROCESS_CLOSED,
-                 content::NotificationService::AllSources());
-  registrar->Add(observer, content::NOTIFICATION_RENDER_WIDGET_HOST_HANG,
-                 content::NotificationService::AllSources());
-}
-
 void MetricsService::BrowserChildProcessHostConnected(
     const content::ChildProcessData& data) {
   GetChildProcessStats(data).process_launches++;
@@ -637,42 +607,6 @@ void MetricsService::BrowserChildProcessInstanceCreated(
   GetChildProcessStats(data).instances++;
 }
 
-void MetricsService::Observe(int type,
-                             const content::NotificationSource& source,
-                             const content::NotificationDetails& details) {
-  DCHECK(log_manager_.current_log());
-  DCHECK(IsSingleThreaded());
-
-  switch (type) {
-    case content::NOTIFICATION_LOAD_START: {
-      content::NavigationController* controller =
-          content::Source<content::NavigationController>(source).ptr();
-      content::WebContents* web_contents = controller->GetWebContents();
-      LogLoadStarted(web_contents);
-      break;
-    }
-
-    case content::NOTIFICATION_RENDERER_PROCESS_CLOSED: {
-      content::RenderProcessHost::RendererClosedDetails* process_details =
-          content::Details<
-              content::RenderProcessHost::RendererClosedDetails>(
-                  details).ptr();
-      content::RenderProcessHost* host =
-          content::Source<content::RenderProcessHost>(source).ptr();
-      LogRendererCrash(
-          host, process_details->status, process_details->exit_code);
-      break;
-    }
-
-    case content::NOTIFICATION_RENDER_WIDGET_HOST_HANG:
-      LogRendererHang();
-      break;
-
-    default:
-      NOTREACHED();
-  }
-}
-
 void MetricsService::HandleIdleSinceLastTransmission(bool in_idle) {
   // If there wasn't a lot of action, maybe the computer was asleep, in which
   // case, the log transmissions should have stopped.  Here we start them up
@@ -1597,50 +1531,6 @@ void MetricsService::IncrementLongPrefsValue(const char*
path) {
   pref->SetInt64(path, value + 1);
 }
 
-void MetricsService::LogLoadStarted(content::WebContents* web_contents) {
-  content::RecordAction(base::UserMetricsAction("PageLoad"));
-  HISTOGRAM_ENUMERATION("Chrome.UmaPageloadCounter", 1, 2);
-  IncrementPrefValue(prefs::kStabilityPageLoadCount);
-  IncrementLongPrefsValue(prefs::kUninstallMetricsPageLoadCount);
-  // We need to save the prefs, as page load count is a critical stat, and it
-  // might be lost due to a crash :-(.
-}
-
-void MetricsService::LogRendererCrash(content::RenderProcessHost* host,
-                                      base::TerminationStatus status,
-                                      int exit_code) {
-  bool was_extension_process =
-      extensions::ProcessMap::Get(host->GetBrowserContext())
-          ->Contains(host->GetID());
-  if (status == base::TERMINATION_STATUS_PROCESS_CRASHED ||
-      status == base::TERMINATION_STATUS_ABNORMAL_TERMINATION) {
-    if (was_extension_process) {
-      IncrementPrefValue(prefs::kStabilityExtensionRendererCrashCount);
-
-      UMA_HISTOGRAM_SPARSE_SLOWLY("CrashExitCodes.Extension",
-                                  MapCrashExitCodeForHistogram(exit_code));
-    } else {
-      IncrementPrefValue(prefs::kStabilityRendererCrashCount);
-
-      UMA_HISTOGRAM_SPARSE_SLOWLY("CrashExitCodes.Renderer",
-                                  MapCrashExitCodeForHistogram(exit_code));
-    }
-
-    UMA_HISTOGRAM_PERCENTAGE("BrowserRenderProcessHost.ChildCrashes",
-                             was_extension_process ? 2 : 1);
-  } else if (status == base::TERMINATION_STATUS_PROCESS_WAS_KILLED) {
-    UMA_HISTOGRAM_PERCENTAGE("BrowserRenderProcessHost.ChildKills",
-                             was_extension_process ? 2 : 1);
-  } else if (status == base::TERMINATION_STATUS_STILL_RUNNING) {
-    UMA_HISTOGRAM_PERCENTAGE("BrowserRenderProcessHost.DisconnectedAlive",
-                               was_extension_process ? 2 : 1);
-  }
-}
-
-void MetricsService::LogRendererHang() {
-  IncrementPrefValue(prefs::kStabilityRendererHangCount);
-}
-
 bool MetricsService::UmaMetricsProperlyShutdown() {
   CHECK(clean_shutdown_status_ == CLEANLY_SHUTDOWN ||
         clean_shutdown_status_ == NEED_TO_SHUTDOWN);

[blundell, 2014-05-22 19:07:08]

The CQ bit was checked by blundell@chromium.org

[commit-bot: I haz the power, 2014-05-22 19:11:27]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/blundell@chromium.org/289283011/120001

[commit-bot: I haz the power, 2014-05-22 23:52:10]

FYI, CQ is re-trying this CL (attempt #1).
Please consider checking whether the failures are real,
and report flakes to chrome-troopers@google.com.
The failing builders are:
  mac_chromium_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/mac_chromium_rel/buil...)

[commit-bot: I haz the power, 2014-05-23 07:16:43]

Change committed as 272426