Bug 1562831 - Stop the Crash Service cleanly to prevent it's restart; r=VladBaicu
authorPetru Lingurar <petru.lingurar@softvision.ro>
Wed, 03 Jul 2019 13:13:10 +0000
changeset 543947 d9c61c4e5b1dde53a85002aa0ca74afebf608c80
parent 543946 9fd54a07823002bc12c3fe361dc87041c39ed828
child 543948 8fa127ef3d047608fdee9de59a263d797bb7c5b3
push id2131
push userffxbld-merge
push dateMon, 26 Aug 2019 18:30:20 +0000
treeherdermozilla-release@b19ffb3ca153 [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewersVladBaicu
bugs1562831
milestone69.0a1
first release with
nightly linux32
nightly linux64
nightly mac
nightly win32
nightly win64
last release without
nightly linux32
nightly linux64
nightly mac
nightly win32
nightly win64
Bug 1562831 - Stop the Crash Service cleanly to prevent it's restart; r=VladBaicu The service would be restarted after System.exit(0) which would show the crash feedback form again to the user. That System.exit(0) was initially used to prevent a silent ANR because of the Service being started from background on Android Oreo+ without a foreground notification. To overcome all this we'll also use a foreground notification on Android Oreo+ but with NotificationManager.IMPORTANCE_LOW to be non-intrusive. Differential Revision: https://phabricator.services.mozilla.com/D36746
mobile/android/base/AppConstants.java.in
mobile/android/base/java/org/mozilla/gecko/CrashHandlerService.java
mobile/android/base/java/org/mozilla/gecko/notifications/NotificationHelper.java
--- a/mobile/android/base/AppConstants.java.in
+++ b/mobile/android/base/AppConstants.java.in
@@ -68,16 +68,17 @@ public class AppConstants {
         public static final boolean preLollipopMR1 = MAX_SDK_VERSION < 22 || (MIN_SDK_VERSION < 22 && Build.VERSION.SDK_INT < 22);
         public static final boolean preLollipop = MAX_SDK_VERSION < 21 || (MIN_SDK_VERSION < 21 && Build.VERSION.SDK_INT < 21);
         public static final boolean preJBMR2 = MAX_SDK_VERSION < 18 || (MIN_SDK_VERSION < 18 && Build.VERSION.SDK_INT < 18);
         public static final boolean preJBMR1 = MAX_SDK_VERSION < 17 || (MIN_SDK_VERSION < 17 && Build.VERSION.SDK_INT < 17);
         public static final boolean preJB = MAX_SDK_VERSION < 16 || (MIN_SDK_VERSION < 16 && Build.VERSION.SDK_INT < 16);
         public static final boolean preN = MAX_SDK_VERSION < 24 || (MIN_SDK_VERSION < 24 && Build.VERSION.SDK_INT < 24);
         public static final boolean N = Build.VERSION.SDK_INT == 24 || Build.VERSION.SDK_INT == 25;
         public static final boolean preO = MAX_SDK_VERSION < 26 || (MIN_SDK_VERSION < 26 && Build.VERSION.SDK_INT < 26);
+        public static final boolean preQ = MAX_SDK_VERSION < 29 || (MIN_SDK_VERSION < 29 && Build.VERSION.SDK_INT < 29);
     }
 
     /**
      * The name of the Java class that represents the android application.
      */
     public static final String MOZ_ANDROID_APPLICATION_CLASS = "@MOZ_ANDROID_APPLICATION_CLASS@";
     /**
      * The name of the Java class that launches the browser activity.
--- a/mobile/android/base/java/org/mozilla/gecko/CrashHandlerService.java
+++ b/mobile/android/base/java/org/mozilla/gecko/CrashHandlerService.java
@@ -1,31 +1,30 @@
 package org.mozilla.gecko;
 
 import android.annotation.TargetApi;
 import android.app.Notification;
 import android.app.PendingIntent;
 import android.app.Service;
 import android.content.Context;
 import android.content.Intent;
+import android.os.Build;
 import android.os.IBinder;
 import android.provider.Settings;
 import android.support.annotation.Nullable;
 import android.util.Log;
 
 import org.mozilla.gecko.notifications.NotificationHelper;
 
 import java.io.File;
 import java.io.IOException;
 
 public class CrashHandlerService extends Service {
     private static final String LOGTAG = "CrashHandlerService";
     private static final String ACTION_STOP = "action_stop";
-    // Build.VERSION_CODES.Q placeholder. While Android Q is in Beta it shares API 28 with Android P.
-    private static final int ANDROID_Q = 29;
 
     @Override
     public int onStartCommand(@Nullable Intent intent, int flags, int startId) {
         if (intent == null) {
             // no-op
             // The Intent may be null if the service is being restarted after its process has gone away,
             // and it had previously returned anything except START_STICKY_COMPATIBILITY.
         } else if (ACTION_STOP.equals(intent.getAction())) {
@@ -37,46 +36,44 @@ public class CrashHandlerService extends
                 crashFlag.createNewFile();
             } catch (GeckoProfileDirectories.NoMozillaDirectoryException | IOException e) {
                 Log.e(LOGTAG, "Cannot set crash flag: ", e);
             }
 
             intent.setClass(this, CrashReporterActivity.class);
             intent.addFlags(Intent.FLAG_ACTIVITY_NEW_TASK);
 
-            if (AppConstants.Versions.feature29Plus) {
+            if (AppConstants.Versions.feature26Plus) {
                 startCrashHandling(intent);
             } else {
                 startActivity(intent);
-
-                // Avoid ANR due to background limitations on Oreo+
-                System.exit(0);
+                stopSelf();
             }
         }
 
         return Service.START_NOT_STICKY;
     }
 
     @Override
     public IBinder onBind(Intent intent) {
         return null;
     }
 
     /**
      * Call this for any necessary cleanup like removing the foreground notification shown on Android Q+.
      */
     public static void reportingStarted(final Context context) {
-        if (AppConstants.Versions.feature29Plus) {
+        if (AppConstants.Versions.feature26Plus) {
             final Intent intent = new Intent(context, CrashHandlerService.class);
             intent.setAction(ACTION_STOP);
             context.startService(intent);
         }
     }
 
-    @TargetApi(ANDROID_Q)
+    @TargetApi(Build.VERSION_CODES.O)
     private Notification getActivityNotification(final Context context, final Intent activityIntent) {
         final Intent dismissNotificationIntent = new Intent(ACTION_STOP, null, this, this.getClass());
         final PendingIntent dismissNotification = PendingIntent.getService(this, 0, dismissNotificationIntent, PendingIntent.FLAG_CANCEL_CURRENT);
         final PendingIntent startReporterActivity = PendingIntent.getActivity(this, 0, activityIntent, 0);
         final String notificationChannelId = NotificationHelper.getInstance(context)
                 .getNotificationChannel(NotificationHelper.Channel.CRASH_HANDLER).getId();
 
         return new Notification.Builder(this, notificationChannelId)
@@ -84,27 +81,36 @@ public class CrashHandlerService extends
                 .setContentTitle(getString(R.string.crash_notification_title))
                 .setContentText(getString(R.string.crash_notification_message))
                 .setDefaults(Notification.DEFAULT_ALL)
                 .setContentIntent(startReporterActivity)
                 .addAction(0, getString(R.string.crash_notification_negative_button_text), dismissNotification)
                 .build();
     }
 
-    @TargetApi(ANDROID_Q)
+    @TargetApi(Build.VERSION_CODES.O)
     private void dismissNotification() {
         stopForeground(Service.STOP_FOREGROUND_REMOVE);
+        stopSelf();
     }
 
-    @TargetApi(ANDROID_Q)
+    @TargetApi(Build.VERSION_CODES.O)
     private void startCrashHandling(final Intent activityIntent) {
         // Piggy-back the SYSTEM_ALERT_WINDOW permission given by the user for the Tab Queue functionality.
         // Otherwise fallback to display a foreground notification, this being the only way we can
         // start an activity from background.
         // https://developer.android.com/preview/privacy/background-activity-starts#conditions-allow-activity-starts
         if (Settings.canDrawOverlays(this)) {
             startActivity(activityIntent);
         } else {
             final Notification notification = getActivityNotification(this, activityIntent);
+
+            // Android O+ will show a non-intrusive notification (needed for a foreground service)
+            // and then show the crash reporter immediately
+            // Android Q+ will show an intrusive notification which when acted upon by the user will
+            // allow starting the crash reporter. See http://g.co/dev/bgblock
             startForeground(R.id.mediaControlNotification, notification);
+            if (AppConstants.Versions.feature26Plus && AppConstants.Versions.preQ) {
+                startActivity(activityIntent);
+            }
         }
     }
 }
--- a/mobile/android/base/java/org/mozilla/gecko/notifications/NotificationHelper.java
+++ b/mobile/android/base/java/org/mozilla/gecko/notifications/NotificationHelper.java
@@ -280,17 +280,19 @@ public final class NotificationHelper im
                             mContext.getString(R.string.site_notifications_channel),
                             NotificationManager.IMPORTANCE_DEFAULT);
                 }
                 break;
 
                 case CRASH_HANDLER: {
                     channel = new NotificationChannel(mDefinedNotificationChannels.get(definedChannel),
                             mContext.getString(R.string.crash_handler_notifications_channel),
-                            NotificationManager.IMPORTANCE_HIGH);
+                            AppConstants.Versions.feature29Plus ?
+                                    NotificationManager.IMPORTANCE_HIGH :
+                                    NotificationManager.IMPORTANCE_LOW);
                 }
                 break;
 
                 case DEFAULT:
                 default: {
                     channel = new NotificationChannel(mDefinedNotificationChannels.get(definedChannel),
                             mContext.getString(R.string.default_notification_channel2),
                             NotificationManager.IMPORTANCE_LOW);