Use stability of the browser process to decide whether or not to start...

Use stability of the browser process to decide whether or not to start background tracing on Android

We use the crash dump uploader to check if we had any browser crash
dumps from the previous sessions, or if we uploaded browser crash dumps
in previous session. Chrome tracing delegate uses this information to
make sure browser is stable before starting background tracing.

BUG=882741

Change-Id: Idc55b90f76e2739c054777e01ea39ce7ba5a131d
Reviewed-on: https://chromium-review.googlesource.com/1220191Reviewed-by: oysteine <oysteine@chromium.org>
Reviewed-by: Ilya Sherman <isherman@chromium.org>
Reviewed-by: Tommy Nyquist <nyquist@chromium.org>
Commit-Queue: Siddhartha S <ssid@chromium.org>
Cr-Commit-Position: refs/heads/master@{#593016}

chrome/android/java/src/org/chromium/chrome/browser/crash/MinidumpUploadService.java

@@ -31,6 +31,7 @@ import java.io.BufferedReader;
 import java.io.File;
 import java.io.FileReader;
 import java.io.IOException;
+import java.util.concurrent.atomic.AtomicBoolean;
 
 /**
  * Service that is responsible for uploading crash minidumps to the Google crash server.
@@ -60,6 +61,9 @@ public class MinidumpUploadService extends IntentService {
     private static final int FAILURE = 0;
     private static final int SUCCESS = 1;
 
+    private static AtomicBoolean sBrowserCrashMetricsInitialized = new AtomicBoolean();
+    private static AtomicBoolean sDidBrowserCrashRecently = new AtomicBoolean();
+
     @StringDef({ProcessType.BROWSER, ProcessType.RENDERER, ProcessType.GPU, ProcessType.OTHER})
     public @interface ProcessType {
         String BROWSER = "Browser";
@@ -112,14 +116,17 @@ public class MinidumpUploadService extends IntentService {
      * Stores the successes and failures from uploading crash to UMA,
      */
     public static void storeBreakpadUploadStatsInUma(ChromePreferenceManager pref) {
+        sBrowserCrashMetricsInitialized.set(true);
         for (String type : TYPES) {
             for (int success = pref.getCrashSuccessUploadCount(type); success > 0; success--) {
                 RecordHistogram.recordEnumeratedHistogram(
                         HISTOGRAM_NAME_PREFIX + type, SUCCESS, HISTOGRAM_MAX);
+                if (ProcessType.BROWSER.equals(type)) sDidBrowserCrashRecently.set(true);
             }
             for (int fail = pref.getCrashFailureUploadCount(type); fail > 0; fail--) {
                 RecordHistogram.recordEnumeratedHistogram(
                         HISTOGRAM_NAME_PREFIX + type, FAILURE, HISTOGRAM_MAX);
+                if (ProcessType.BROWSER.equals(type)) sDidBrowserCrashRecently.set(true);
             }
 
             pref.setCrashSuccessUploadCount(type, 0);
@@ -127,6 +134,31 @@ public class MinidumpUploadService extends IntentService {
         }
     }
 
+    /**
+     * Returns true if the initial breakpad upload stats have been recorded.
+     */
+    @CalledByNative
+    private static boolean browserCrashMetricsInitialized() {
+        return sBrowserCrashMetricsInitialized.get();
+    }
+
+    /**
+     * Returns if browser crash dumps were found for recent browser crashes.
+     *
+     * We detect if the browser crash dump was uploaded in last session (for a previous session) or
+     * if a crash dump was seen in current session. Detection of a crash from earlier session is
+     * valid right from the point where minidump service was initialized
+     * (sBrowserCrashMetricsInitialized() returns true). But, the detection of a crash in previous
+     * session is only valid after background minidump upload job is finished, depending on the job
+     * scheduler. So, calling this function at startup can return false even if browser crashed in
+     * previous session.
+     */
+    @CalledByNative
+    private static boolean didBrowserCrashRecently() {
+        assert browserCrashMetricsInitialized();
+        return sDidBrowserCrashRecently.get();
+    }
+
     @Override
     protected void onHandleIntent(Intent intent) {
         if (intent == null) return;
@@ -244,8 +276,12 @@ public class MinidumpUploadService extends IntentService {
      * @param originalFilename The name of the successfully uploaded minidump, *prior* to uploading.
      */
     public static void incrementCrashSuccessUploadCount(String originalFilename) {
-        ChromePreferenceManager.getInstance().incrementCrashSuccessUploadCount(
-                getCrashType(getNewNameAfterSuccessfulUpload(originalFilename)));
+        final @ProcessType String process_type =
+                getCrashType(getNewNameAfterSuccessfulUpload(originalFilename));
+        if (ProcessType.BROWSER.equals(process_type)) {
+            sDidBrowserCrashRecently.set(true);
+        }
+        ChromePreferenceManager.getInstance().incrementCrashSuccessUploadCount(process_type);
     }
 
     /**
@@ -257,8 +293,11 @@ public class MinidumpUploadService extends IntentService {
      * @param originalFilename The name of the successfully uploaded minidump, *prior* to uploading.
      */
     public static void incrementCrashFailureUploadCount(String originalFilename) {
-        ChromePreferenceManager.getInstance().incrementCrashFailureUploadCount(
-                getCrashType(originalFilename));
+        final @ProcessType String process_type = getCrashType(originalFilename);
+        if (ProcessType.BROWSER.equals(process_type)) {
+            sDidBrowserCrashRecently.set(true);
+        }
+        ChromePreferenceManager.getInstance().incrementCrashFailureUploadCount(process_type);
     }
 
     /**

chrome/browser/crash_upload_list/crash_upload_list_android.cc

@@ -45,6 +45,18 @@ CrashUploadListAndroid::CrashUploadListAndroid(
 
 CrashUploadListAndroid::~CrashUploadListAndroid() {}
 
+// static
+bool CrashUploadListAndroid::BrowserCrashMetricsInitialized() {
+  JNIEnv* env = base::android::AttachCurrentThread();
+  return Java_MinidumpUploadService_browserCrashMetricsInitialized(env);
+}
+
+// static
+bool CrashUploadListAndroid::DidBrowserCrashRecently() {
+  JNIEnv* env = base::android::AttachCurrentThread();
+  return Java_MinidumpUploadService_didBrowserCrashRecently(env);
+}
+
 std::vector<UploadList::UploadInfo> CrashUploadListAndroid::LoadUploadList() {
   // First load the list of successfully uploaded logs.
   std::vector<UploadInfo> uploads = TextLogUploadList::LoadUploadList();

chrome/browser/crash_upload_list/crash_upload_list_android.h

@@ -17,7 +17,15 @@ class FilePath;
 // the un-uploaded Minidump directory, managed by the MinidumpUploadService.
 class CrashUploadListAndroid : public TextLogUploadList {
  public:
-  CrashUploadListAndroid(const base::FilePath& upload_log_path);
+  explicit CrashUploadListAndroid(const base::FilePath& upload_log_path);
+
+  // Returns true if the browser crash metrics were initialized, only happens
+  // when minidump service is started.
+  static bool BrowserCrashMetricsInitialized();
+
+  // Returns true if a browser crash was observed in recent sessions. Can be
+  // called only when BrowserCrashMetricsInitialized() returns true.
+  static bool DidBrowserCrashRecently();
 
  protected:
   ~CrashUploadListAndroid() override;

chrome/browser/tracing/chrome_tracing_delegate.cc

@@ -30,6 +30,7 @@
 #include "content/public/browser/browser_thread.h"
 
 #if defined(OS_ANDROID)
+#include "chrome/browser/crash_upload_list/crash_upload_list_android.h"
 #include "chrome/browser/ui/android/tab_model/tab_model.h"
 #include "chrome/browser/ui/android/tab_model/tab_model_list.h"
 #endif
@@ -105,22 +106,21 @@ bool ProfileAllowsScenario(const content::BackgroundTracingConfig& config,
   // If the profile hasn't loaded or been created yet, we allow the scenario
   // to start up, but not be finalized.
   Profile* profile = GetProfile();
-  if (!profile) {
-    if (profile_permission == PROFILE_REQUIRED)
-      return false;
-    else
-      return true;
-  }
+  if (!profile)
+    return profile_permission != PROFILE_REQUIRED;
 
+// Safeguard, in case background tracing is responsible for a crash on
+// startup.
 #if !defined(OS_ANDROID)
-  // Safeguard, in case background tracing is responsible for a crash on
-  // startup.
   if (profile->GetLastSessionExitType() == Profile::EXIT_CRASHED)
     return false;
 #else
-  // In case of Android the exit state is always set as EXIT_CRASHED. So,
-  // preemptive mode cannot be used safely.
-  if (config.tracing_mode() == content::BackgroundTracingConfig::PREEMPTIVE)
+  // If the metrics haven't loaded, we allow the scenario to start up, but not
+  // be finalized.
+  if (!CrashUploadListAndroid::BrowserCrashMetricsInitialized())
+    return profile_permission != PROFILE_REQUIRED;
+
+  if (CrashUploadListAndroid::DidBrowserCrashRecently())
     return false;
 #endif