From b0c8a887299a7f462c41408edda6043202143233 Mon Sep 17 00:00:00 2001 From: Amith Yamasani Date: Mon, 28 Aug 2017 09:36:42 -0700 Subject: [PATCH] Retry crashed bound foreground service with some delay After quick crashes, back-off for 30 minutes and try again. Keep backing-off up to 10 times and then give up. Bug: 63075467 Test: runtest -x frameworks/base/tests/ServiceCrashTest Change-Id: I3819aefac5fd48b49a70b1765e07696f2ad61328 --- .../com/android/server/am/ActiveServices.java | 8 +- .../server/am/ActivityManagerConstants.java | 18 +++++ .../com/android/server/am/AppErrorDialog.java | 6 +- .../java/com/android/server/am/AppErrors.java | 56 ++++++++------ tests/ServiceCrashTest/Android.mk | 19 +++++ tests/ServiceCrashTest/AndroidManifest.xml | 23 ++++++ .../servicecrashtest/CrashingService.java | 66 ++++++++++++++++ .../tests/servicecrashtest/MainActivity.java | 70 +++++++++++++++++ .../servicecrashtest/ServiceCrashTest.java | 76 +++++++++++++++++++ 9 files changed, 314 insertions(+), 28 deletions(-) create mode 100644 tests/ServiceCrashTest/Android.mk create mode 100644 tests/ServiceCrashTest/AndroidManifest.xml create mode 100644 tests/ServiceCrashTest/src/com/android/tests/servicecrashtest/CrashingService.java create mode 100644 tests/ServiceCrashTest/src/com/android/tests/servicecrashtest/MainActivity.java create mode 100644 tests/ServiceCrashTest/src/com/android/tests/servicecrashtest/ServiceCrashTest.java diff --git a/services/core/java/com/android/server/am/ActiveServices.java b/services/core/java/com/android/server/am/ActiveServices.java index 756e2748b8cb5..e0cde72ffff28 100644 --- a/services/core/java/com/android/server/am/ActiveServices.java +++ b/services/core/java/com/android/server/am/ActiveServices.java @@ -1926,6 +1926,9 @@ public final class ActiveServices { if (r.restartDelay == 0) { r.restartCount++; r.restartDelay = minDuration; + } else if (r.crashCount > 1) { + r.restartDelay = mAm.mConstants.BOUND_SERVICE_CRASH_RESTART_DURATION + * (r.crashCount - 1); } else { // If it has been a "reasonably long time" since the service // was started, then reset our restart duration back to @@ -3129,8 +3132,9 @@ public final class ActiveServices { // Any services running in the application may need to be placed // back in the pending list. - if (allowRestart && sr.crashCount >= 2 && (sr.serviceInfo.applicationInfo.flags - &ApplicationInfo.FLAG_PERSISTENT) == 0) { + if (allowRestart && sr.crashCount >= mAm.mConstants.BOUND_SERVICE_MAX_CRASH_RETRY + && (sr.serviceInfo.applicationInfo.flags + &ApplicationInfo.FLAG_PERSISTENT) == 0) { Slog.w(TAG, "Service crashed " + sr.crashCount + " times, stopping: " + sr); EventLog.writeEvent(EventLogTags.AM_SERVICE_CRASHED_TOO_MUCH, diff --git a/services/core/java/com/android/server/am/ActivityManagerConstants.java b/services/core/java/com/android/server/am/ActivityManagerConstants.java index 2cd2603bf554e..b52db872f9786 100644 --- a/services/core/java/com/android/server/am/ActivityManagerConstants.java +++ b/services/core/java/com/android/server/am/ActivityManagerConstants.java @@ -32,6 +32,7 @@ import static com.android.server.am.ActivityManagerDebugConfig.DEBUG_POWER_QUICK * Settings constants that can modify the activity manager's behavior. */ final class ActivityManagerConstants extends ContentObserver { + // Key names stored in the settings value. private static final String KEY_MAX_CACHED_PROCESSES = "max_cached_processes"; private static final String KEY_BACKGROUND_SETTLE_TIME = "background_settle_time"; @@ -63,6 +64,8 @@ final class ActivityManagerConstants extends ContentObserver { static final String KEY_SERVICE_MIN_RESTART_TIME_BETWEEN = "service_min_restart_time_between"; static final String KEY_MAX_SERVICE_INACTIVITY = "service_max_inactivity"; static final String KEY_BG_START_TIMEOUT = "service_bg_start_timeout"; + static final String KEY_BOUND_SERVICE_CRASH_RESTART_DURATION = "service_crash_restart_duration"; + static final String KEY_BOUND_SERVICE_CRASH_MAX_RETRY = "service_crash_max_retry"; private static final int DEFAULT_MAX_CACHED_PROCESSES = 32; private static final long DEFAULT_BACKGROUND_SETTLE_TIME = 60*1000; @@ -88,6 +91,9 @@ final class ActivityManagerConstants extends ContentObserver { private static final long DEFAULT_SERVICE_MIN_RESTART_TIME_BETWEEN = 10*1000; private static final long DEFAULT_MAX_SERVICE_INACTIVITY = 30*60*1000; private static final long DEFAULT_BG_START_TIMEOUT = 15*1000; + private static final long DEFAULT_BOUND_SERVICE_CRASH_RESTART_DURATION = 30*60_000; + private static final int DEFAULT_BOUND_SERVICE_CRASH_MAX_RETRY = 16; + // Maximum number of cached processes we will allow. public int MAX_CACHED_PROCESSES = DEFAULT_MAX_CACHED_PROCESSES; @@ -190,6 +196,12 @@ final class ActivityManagerConstants extends ContentObserver { // allowing the next pending start to run. public long BG_START_TIMEOUT = DEFAULT_BG_START_TIMEOUT; + // Initial backoff delay for retrying bound foreground services + public long BOUND_SERVICE_CRASH_RESTART_DURATION = DEFAULT_BOUND_SERVICE_CRASH_RESTART_DURATION; + + // Maximum number of retries for bound foreground services that crash soon after start + public long BOUND_SERVICE_MAX_CRASH_RETRY = DEFAULT_BOUND_SERVICE_CRASH_MAX_RETRY; + private final ActivityManagerService mService; private ContentResolver mResolver; private final KeyValueListParser mParser = new KeyValueListParser(','); @@ -308,6 +320,12 @@ final class ActivityManagerConstants extends ContentObserver { DEFAULT_MAX_SERVICE_INACTIVITY); BG_START_TIMEOUT = mParser.getLong(KEY_BG_START_TIMEOUT, DEFAULT_BG_START_TIMEOUT); + BOUND_SERVICE_CRASH_RESTART_DURATION = mParser.getLong( + KEY_BOUND_SERVICE_CRASH_RESTART_DURATION, + DEFAULT_BOUND_SERVICE_CRASH_RESTART_DURATION); + BOUND_SERVICE_MAX_CRASH_RETRY = mParser.getInt(KEY_BOUND_SERVICE_CRASH_MAX_RETRY, + DEFAULT_BOUND_SERVICE_CRASH_MAX_RETRY); + updateMaxCachedProcesses(); } } diff --git a/services/core/java/com/android/server/am/AppErrorDialog.java b/services/core/java/com/android/server/am/AppErrorDialog.java index 51ce30ef08f32..541226682bac3 100644 --- a/services/core/java/com/android/server/am/AppErrorDialog.java +++ b/services/core/java/com/android/server/am/AppErrorDialog.java @@ -40,7 +40,6 @@ final class AppErrorDialog extends BaseErrorDialog implements View.OnClickListen private final ProcessRecord mProc; private final boolean mRepeating; private final boolean mIsRestartable; - private CharSequence mName; static int CANT_SHOW = -1; @@ -110,17 +109,16 @@ final class AppErrorDialog extends BaseErrorDialog implements View.OnClickListen LayoutInflater.from(context).inflate( com.android.internal.R.layout.app_error_dialog, frame, true); - boolean hasRestart = !mRepeating && mIsRestartable; final boolean hasReceiver = mProc.errorReportReceiver != null; final TextView restart = findViewById(com.android.internal.R.id.aerr_restart); restart.setOnClickListener(this); - restart.setVisibility(hasRestart ? View.VISIBLE : View.GONE); + restart.setVisibility(mIsRestartable ? View.VISIBLE : View.GONE); final TextView report = findViewById(com.android.internal.R.id.aerr_report); report.setOnClickListener(this); report.setVisibility(hasReceiver ? View.VISIBLE : View.GONE); final TextView close = findViewById(com.android.internal.R.id.aerr_close); - close.setVisibility(!hasRestart ? View.VISIBLE : View.GONE); + close.setVisibility(mRepeating ? View.VISIBLE : View.GONE); close.setOnClickListener(this); boolean showMute = !Build.IS_USER && Settings.Global.getInt(context.getContentResolver(), diff --git a/services/core/java/com/android/server/am/AppErrors.java b/services/core/java/com/android/server/am/AppErrors.java index fc03db1203bd2..a842724c31778 100644 --- a/services/core/java/com/android/server/am/AppErrors.java +++ b/services/core/java/com/android/server/am/AppErrors.java @@ -55,7 +55,6 @@ import java.io.PrintWriter; import java.util.ArrayList; import java.util.Collections; import java.util.HashMap; -import java.util.List; import java.util.Set; import static com.android.server.Watchdog.NATIVE_STACKS_OF_INTEREST; @@ -593,20 +592,46 @@ class AppErrors { boolean handleAppCrashLocked(ProcessRecord app, String reason, String shortMsg, String longMsg, String stackTrace, AppErrorDialog.Data data) { - long now = SystemClock.uptimeMillis(); - boolean showBackground = Settings.Secure.getInt(mContext.getContentResolver(), + final long now = SystemClock.uptimeMillis(); + final boolean showBackground = Settings.Secure.getInt(mContext.getContentResolver(), Settings.Secure.ANR_SHOW_BACKGROUND, 0) != 0; + final boolean procIsBoundForeground = + (app.curProcState == ActivityManager.PROCESS_STATE_BOUND_FOREGROUND_SERVICE); + Long crashTime; Long crashTimePersistent; + boolean tryAgain = false; + if (!app.isolated) { crashTime = mProcessCrashTimes.get(app.info.processName, app.uid); crashTimePersistent = mProcessCrashTimesPersistent.get(app.info.processName, app.uid); } else { crashTime = crashTimePersistent = null; } - if (crashTime != null && now < crashTime+ProcessList.MIN_CRASH_INTERVAL) { - // This process loses! + + // Bump up the crash count of any services currently running in the proc. + for (int i = app.services.size() - 1; i >= 0; i--) { + // Any services running in the application need to be placed + // back in the pending list. + ServiceRecord sr = app.services.valueAt(i); + // If the service was restarted a while ago, then reset crash count, else increment it. + if (now > sr.restartTime + ProcessList.MIN_CRASH_INTERVAL) { + sr.crashCount = 1; + } else { + sr.crashCount++; + } + // Allow restarting for started or bound foreground services that are crashing. + // This includes wallpapers. + if (sr.crashCount < mService.mConstants.BOUND_SERVICE_MAX_CRASH_RETRY + && (sr.isForeground || procIsBoundForeground)) { + tryAgain = true; + } + } + + if (crashTime != null && now < crashTime + ProcessList.MIN_CRASH_INTERVAL) { + // The process crashed again very quickly. If it was a bound foreground service, let's + // try to restart again in a while, otherwise the process loses! Slog.w(TAG, "Process " + app.info.processName + " has crashed too many times: killing!"); EventLog.writeEvent(EventLogTags.AM_PROCESS_CRASHED_TOO_MUCH, @@ -631,7 +656,7 @@ class AppErrors { // Don't let services in this process be restarted and potentially // annoy the user repeatedly. Unless it is persistent, since those // processes run critical code. - mService.removeProcessLocked(app, false, false, "crash"); + mService.removeProcessLocked(app, false, tryAgain, "crash"); mService.mStackSupervisor.resumeFocusedStackTopActivityLocked(); if (!showBackground) { return false; @@ -650,21 +675,8 @@ class AppErrors { } } - boolean procIsBoundForeground = - (app.curProcState == ActivityManager.PROCESS_STATE_BOUND_FOREGROUND_SERVICE); - // Bump up the crash count of any services currently running in the proc. - for (int i=app.services.size()-1; i>=0; i--) { - // Any services running in the application need to be placed - // back in the pending list. - ServiceRecord sr = app.services.valueAt(i); - sr.crashCount++; - - // Allow restarting for started or bound foreground services that are crashing the - // first time. This includes wallpapers. - if ((data != null) && (sr.crashCount <= 1) - && (sr.isForeground || procIsBoundForeground)) { - data.isRestartableForService = true; - } + if (data != null && tryAgain) { + data.isRestartableForService = true; } // If the crashing process is what we consider to be the "home process" and it has been @@ -690,7 +702,7 @@ class AppErrors { if (!app.isolated) { // XXX Can't keep track of crash times for isolated processes, - // because they don't have a perisistent identity. + // because they don't have a persistent identity. mProcessCrashTimes.put(app.info.processName, app.uid, now); mProcessCrashTimesPersistent.put(app.info.processName, app.uid, now); } diff --git a/tests/ServiceCrashTest/Android.mk b/tests/ServiceCrashTest/Android.mk new file mode 100644 index 0000000000000..d1f845623be8a --- /dev/null +++ b/tests/ServiceCrashTest/Android.mk @@ -0,0 +1,19 @@ +LOCAL_PATH:= $(call my-dir) +include $(CLEAR_VARS) + +LOCAL_MODULE_TAGS := tests + +# Only compile source java files in this apk. +LOCAL_SRC_FILES := $(call all-java-files-under, src) + +LOCAL_PACKAGE_NAME := ServiceCrashTest + +LOCAL_CERTIFICATE := platform +LOCAL_JAVA_LIBRARIES := legacy-android-test + +LOCAL_STATIC_JAVA_LIBRARIES := compatibility-device-util android-support-test + +include $(BUILD_PACKAGE) + +# Use the following include to make our test apk. +include $(call all-makefiles-under,$(LOCAL_PATH)) diff --git a/tests/ServiceCrashTest/AndroidManifest.xml b/tests/ServiceCrashTest/AndroidManifest.xml new file mode 100644 index 0000000000000..387c8b8ae36b3 --- /dev/null +++ b/tests/ServiceCrashTest/AndroidManifest.xml @@ -0,0 +1,23 @@ + + + + + + + + + + + + + + + + + + + diff --git a/tests/ServiceCrashTest/src/com/android/tests/servicecrashtest/CrashingService.java b/tests/ServiceCrashTest/src/com/android/tests/servicecrashtest/CrashingService.java new file mode 100644 index 0000000000000..8593bcea9c3d0 --- /dev/null +++ b/tests/ServiceCrashTest/src/com/android/tests/servicecrashtest/CrashingService.java @@ -0,0 +1,66 @@ +/* + * Copyright (C) 2017 The Android Open Source Project + * + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +package com.android.tests.servicecrashtest; + +import android.app.Service; +import android.content.Intent; +import android.os.Handler; +import android.os.HandlerThread; +import android.os.IBinder; +import android.os.Looper; +import android.os.Message; +import android.os.Process; +import android.widget.Toast; + +public class CrashingService extends Service { + private ServiceHandler mServiceHandler; + + static long CRASH_DELAY = 1000; + + // Handler that receives messages from the thread + private final class ServiceHandler extends Handler { + public ServiceHandler(Looper looper) { + super(looper); + } + + @Override + public void handleMessage(Message msg) { + throw new RuntimeException("Crashing!"); + } + } + + @Override + public void onCreate() { + mServiceHandler = new ServiceHandler(Looper.getMainLooper()); + Toast.makeText(this, "service starting", Toast.LENGTH_SHORT).show(); + + Message msg = mServiceHandler.obtainMessage(); + mServiceHandler.sendMessageDelayed(msg, CRASH_DELAY); + } + + @Override + public int onStartCommand(Intent intent, int flags, int startId) { + // If we get killed, after returning from here, restart + return START_STICKY; + } + + @Override + public IBinder onBind(Intent intent) { + // We don't provide binding, so return null + return null; + } +} \ No newline at end of file diff --git a/tests/ServiceCrashTest/src/com/android/tests/servicecrashtest/MainActivity.java b/tests/ServiceCrashTest/src/com/android/tests/servicecrashtest/MainActivity.java new file mode 100644 index 0000000000000..8fffecc41dbd5 --- /dev/null +++ b/tests/ServiceCrashTest/src/com/android/tests/servicecrashtest/MainActivity.java @@ -0,0 +1,70 @@ +/* + * Copyright (C) 2017 The Android Open Source Project + * + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +package com.android.tests.servicecrashtest; + +import android.app.Activity; +import android.app.Service; +import android.content.ComponentName; +import android.content.Intent; +import android.content.ServiceConnection; +import android.os.Bundle; +import android.os.IBinder; +import android.util.Log; +import android.widget.TextView; + +import java.util.concurrent.CountDownLatch; + +public class MainActivity extends Activity { + + private static final String TAG = "ServiceCrashTest"; + + static final CountDownLatch sBindingDiedLatch = new CountDownLatch(1); + + private ServiceConnection mServiceConnection = new ServiceConnection() { + + @Override + public void onServiceConnected(ComponentName name, IBinder service) { + Log.i(TAG, "Service connected"); + } + + @Override + public void onServiceDisconnected(ComponentName name) { + Log.i(TAG, "Service disconnected"); + } + + @Override + public void onBindingDied(ComponentName componentName) { + Log.i(TAG, "Binding died"); + sBindingDiedLatch.countDown(); + } + }; + + @Override + public void onCreate(Bundle savedInstance) { + super.onCreate(savedInstance); + + setContentView(new TextView(this)); + } + + public void onResume() { + Intent intent = new Intent(); + intent.setClass(this, CrashingService.class); + bindService(intent, mServiceConnection, Service.BIND_AUTO_CREATE); + + super.onResume(); + } +} diff --git a/tests/ServiceCrashTest/src/com/android/tests/servicecrashtest/ServiceCrashTest.java b/tests/ServiceCrashTest/src/com/android/tests/servicecrashtest/ServiceCrashTest.java new file mode 100644 index 0000000000000..fb0fa4b86185b --- /dev/null +++ b/tests/ServiceCrashTest/src/com/android/tests/servicecrashtest/ServiceCrashTest.java @@ -0,0 +1,76 @@ +/* + * Copyright (C) 2017 The Android Open Source Project + * + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +package com.android.tests.servicecrashtest; + +import android.app.UiAutomation; +import android.content.Context; +import android.content.Intent; +import android.os.RemoteException; +import android.provider.Settings; +import android.test.InstrumentationTestCase; + +import com.android.compatibility.common.util.SystemUtil; + +import java.io.IOException; +import java.util.concurrent.TimeUnit; + +public class ServiceCrashTest extends InstrumentationTestCase { + + private static final String TAG = ServiceCrashTest.class.getSimpleName(); + + private String mResetConstants = "foo=bar"; + + @Override + protected void setUp() throws Exception { + super.setUp(); + mResetConstants = Settings.Global.getString( + getInstrumentation().getContext().getContentResolver(), + Settings.Global.ACTIVITY_MANAGER_CONSTANTS); + setAMConstants("service_crash_restart_duration=5000,service_crash_max_retry=4"); + } + + @Override + protected void tearDown() throws Exception { + // Reset the activity manager constants + setAMConstants(mResetConstants); + super.tearDown(); + } + + private void setAMConstants(String value) throws IOException { + // Set the activity manager constants + if (value == null) { + SystemUtil.runShellCommand(getInstrumentation(), + "settings delete global activity_manager_constants"); + } else { + SystemUtil.runShellCommand(getInstrumentation(), "settings put global " + + "activity_manager_constants " + value); + } + } + + public void testCrashQuickly() throws RemoteException { + Context ctx = getInstrumentation().getContext(); + // Start the activity, which will bind the crashing service + Intent intent = new Intent(); + intent.setAction(Intent.ACTION_MAIN); + intent.setClass(ctx, MainActivity.class); + ctx.startActivity(intent); + try { + assertTrue(MainActivity.sBindingDiedLatch.await(200, TimeUnit.SECONDS)); + } catch (InterruptedException ie) { + } + } +}