Set BugreportProgressService to run on foreground.
BugreportProgressService do not persist the user-provided
information (like details and screenshot paths), so if it's killed by
the framework, that info is lost.
Running it as foreground mitigates the changes of it being killed.
BUG: 27431998
BUG: 28291423
Change-Id: I2f58507beb38309628f2f19d3f7f950d07eca16f
diff --git a/packages/Shell/src/com/android/shell/BugreportProgressService.java b/packages/Shell/src/com/android/shell/BugreportProgressService.java
index ec3a7fc..7023a1a 100644
--- a/packages/Shell/src/com/android/shell/BugreportProgressService.java
+++ b/packages/Shell/src/com/android/shell/BugreportProgressService.java
@@ -198,6 +198,11 @@
private File mScreenshotsDir;
/**
+ * id of the notification used to set service on foreground.
+ */
+ private int mForegroundId = -1;
+
+ /**
* Flag indicating whether a screenshot is being taken.
* <p>
* This is the only state that is shared between the 2 handlers and hence must have synchronized
@@ -257,6 +262,7 @@
writer.printf("No monitored processes");
return;
}
+ writer.printf("Foreground id: %d\n\n", mForegroundId);
writer.printf("Monitored dumpstate processes\n");
writer.printf("-----------------------------\n");
for (int i = 0; i < size; i++) {
@@ -479,10 +485,21 @@
return;
}
if (DEBUG) {
- Log.d(TAG, "Sending 'Progress' notification for id " + info.id + "(pid " + info.pid
+ Log.d(TAG, "Sending 'Progress' notification for id " + info.id + " (pid " + info.pid
+ "): " + percentageText);
}
- NotificationManager.from(mContext).notify(TAG, info.id, notification);
+ sendForegroundabledNotification(info.id, notification);
+ }
+
+ private void sendForegroundabledNotification(int id, Notification notification) {
+ if (mForegroundId >= 0) {
+ if (DEBUG) Log.d(TAG, "Already running as foreground service");
+ NotificationManager.from(mContext).notify(id, notification);
+ } else {
+ mForegroundId = id;
+ Log.d(TAG, "Start running as foreground service on id " + mForegroundId);
+ startForeground(mForegroundId, notification);
+ }
}
/**
@@ -506,8 +523,10 @@
Log.d(TAG, "Removing ID " + id);
mProcesses.remove(id);
}
- Log.v(TAG, "stopProgress(" + id + "): cancel notification");
- NotificationManager.from(mContext).cancel(TAG, id);
+ // Must stop foreground service first, otherwise notif.cancel() will fail below.
+ stopForegroundWhenDone(id);
+ Log.d(TAG, "stopProgress(" + id + "): cancel notification");
+ NotificationManager.from(mContext).cancel(id);
stopSelfWhenDone();
}
@@ -625,7 +644,7 @@
Log.w(TAG, "launchBugreportInfoDialog(): canceling notification because id " + id
+ " was not found");
// TODO: add test case to make sure notification is canceled.
- NotificationManager.from(mContext).cancel(TAG, id);
+ NotificationManager.from(mContext).cancel(id);
return;
}
@@ -648,7 +667,7 @@
Log.w(TAG, "takeScreenshot(): canceling notification because id " + id
+ " was not found");
// TODO: add test case to make sure notification is canceled.
- NotificationManager.from(mContext).cancel(TAG, id);
+ NotificationManager.from(mContext).cancel(id);
return;
}
setTakingScreenshot(true);
@@ -731,7 +750,7 @@
if (info.finished) {
Log.d(TAG, "Screenshot finished after bugreport; updating share notification");
info.renameScreenshots(mScreenshotsDir);
- sendBugreportNotification(mContext, info, mTakingScreenshot);
+ sendBugreportNotification(info, mTakingScreenshot);
}
msg = mContext.getString(R.string.bugreport_screenshot_taken);
} else {
@@ -753,6 +772,33 @@
}
/**
+ * Stop running on foreground once there is no more active bugreports being watched.
+ */
+ private void stopForegroundWhenDone(int id) {
+ if (id != mForegroundId) {
+ Log.d(TAG, "stopForegroundWhenDone(" + id + "): ignoring since foreground id is "
+ + mForegroundId);
+ return;
+ }
+
+ Log.d(TAG, "detaching foreground from id " + mForegroundId);
+ stopForeground(Service.STOP_FOREGROUND_DETACH);
+ mForegroundId = -1;
+
+ // Might need to restart foreground using a new notification id.
+ final int total = mProcesses.size();
+ if (total > 0) {
+ for (int i = 0; i < total; i++) {
+ final BugreportInfo info = mProcesses.valueAt(i);
+ if (!info.finished) {
+ updateProgress(info);
+ break;
+ }
+ }
+ }
+ }
+
+ /**
* Finishes the service when it's not monitoring any more processes.
*/
private void stopSelfWhenDone() {
@@ -797,6 +843,9 @@
}
info.finished = true;
+ // Stop running on foreground, otherwise share notification cannot be dismissed.
+ stopForegroundWhenDone(id);
+
final Configuration conf = mContext.getResources().getConfiguration();
if ((conf.uiMode & Configuration.UI_MODE_TYPE_MASK) != Configuration.UI_MODE_TYPE_WATCH) {
triggerLocalNotification(mContext, info);
@@ -820,10 +869,10 @@
boolean isPlainText = info.bugreportFile.getName().toLowerCase().endsWith(".txt");
if (!isPlainText) {
// Already zipped, send it right away.
- sendBugreportNotification(context, info, mTakingScreenshot);
+ sendBugreportNotification(info, mTakingScreenshot);
} else {
// Asynchronously zip the file first, then send it.
- sendZippedBugreportNotification(context, info, mTakingScreenshot);
+ sendZippedBugreportNotification(info, mTakingScreenshot);
}
}
@@ -905,7 +954,7 @@
Log.v(TAG, "shareBugReport(): id " + id + " info = " + info);
}
- addDetailsToZipFile(mContext, info);
+ addDetailsToZipFile(info);
final Intent sendIntent = buildSendIntent(mContext, info);
if (sendIntent == null) {
@@ -934,36 +983,35 @@
/**
* Sends a notification indicating the bugreport has finished so use can share it.
*/
- private static void sendBugreportNotification(Context context, BugreportInfo info,
- boolean takingScreenshot) {
+ private void sendBugreportNotification(BugreportInfo info, boolean takingScreenshot) {
// Since adding the details can take a while, do it before notifying user.
- addDetailsToZipFile(context, info);
+ addDetailsToZipFile(info);
final Intent shareIntent = new Intent(INTENT_BUGREPORT_SHARE);
- shareIntent.setClass(context, BugreportProgressService.class);
+ shareIntent.setClass(mContext, BugreportProgressService.class);
shareIntent.setAction(INTENT_BUGREPORT_SHARE);
shareIntent.putExtra(EXTRA_ID, info.id);
shareIntent.putExtra(EXTRA_INFO, info);
- final String title = context.getString(R.string.bugreport_finished_title, info.id);
+ final String title = mContext.getString(R.string.bugreport_finished_title, info.id);
final String content = takingScreenshot ?
- context.getString(R.string.bugreport_finished_pending_screenshot_text)
- : context.getString(R.string.bugreport_finished_text);
- final Notification.Builder builder = newBaseNotification(context)
+ mContext.getString(R.string.bugreport_finished_pending_screenshot_text)
+ : mContext.getString(R.string.bugreport_finished_text);
+ final Notification.Builder builder = newBaseNotification(mContext)
.setContentTitle(title)
.setTicker(title)
.setContentText(content)
- .setContentIntent(PendingIntent.getService(context, info.id, shareIntent,
+ .setContentIntent(PendingIntent.getService(mContext, info.id, shareIntent,
PendingIntent.FLAG_UPDATE_CURRENT))
- .setDeleteIntent(newCancelIntent(context, info));
+ .setDeleteIntent(newCancelIntent(mContext, info));
if (!TextUtils.isEmpty(info.name)) {
builder.setSubText(info.name);
}
Log.v(TAG, "Sending 'Share' notification for ID " + info.id + ": " + title);
- NotificationManager.from(context).notify(TAG, info.id, builder.build());
+ NotificationManager.from(mContext).notify(info.id, builder.build());
}
/**
@@ -971,14 +1019,14 @@
* finishes - at this point there is nothing to be done other than waiting, hence it has no
* pending action.
*/
- private static void sendBugreportBeingUpdatedNotification(Context context, int id) {
+ private void sendBugreportBeingUpdatedNotification(Context context, int id) {
final String title = context.getString(R.string.bugreport_updating_title);
final Notification.Builder builder = newBaseNotification(context)
.setContentTitle(title)
.setTicker(title)
.setContentText(context.getString(R.string.bugreport_updating_wait));
Log.v(TAG, "Sending 'Updating zip' notification for ID " + id + ": " + title);
- NotificationManager.from(context).notify(TAG, id, builder.build());
+ sendForegroundabledNotification(id, builder.build());
}
private static Notification.Builder newBaseNotification(Context context) {
@@ -999,13 +1047,13 @@
/**
* Sends a zipped bugreport notification.
*/
- private static void sendZippedBugreportNotification(final Context context,
- final BugreportInfo info, final boolean takingScreenshot) {
+ private void sendZippedBugreportNotification( final BugreportInfo info,
+ final boolean takingScreenshot) {
new AsyncTask<Void, Void, Void>() {
@Override
protected Void doInBackground(Void... params) {
zipBugreport(info);
- sendBugreportNotification(context, info, takingScreenshot);
+ sendBugreportNotification(info, takingScreenshot);
return null;
}
}.execute();
@@ -1043,7 +1091,7 @@
* If user provided a title, it will be saved into a {@code title.txt} entry; similarly, the
* description will be saved on {@code description.txt}.
*/
- private static void addDetailsToZipFile(Context context, BugreportInfo info) {
+ private void addDetailsToZipFile(BugreportInfo info) {
if (info.bugreportFile == null) {
// One possible reason is a bug in the Parcelization code.
Log.wtf(TAG, "addDetailsToZipFile(): no bugreportFile on " + info);
@@ -1061,7 +1109,8 @@
// It's not possible to add a new entry into an existing file, so we need to create a new
// zip, copy all entries, then rename it.
- sendBugreportBeingUpdatedNotification(context, info.id); // ...and that takes time
+ sendBugreportBeingUpdatedNotification(mContext, info.id); // ...and that takes time
+
final File dir = info.bugreportFile.getParentFile();
final File tmpZip = new File(dir, "tmp-" + info.bugreportFile.getName());
Log.d(TAG, "Writing temporary zip file (" + tmpZip + ") with title and/or description");
@@ -1090,6 +1139,7 @@
// Make sure it only tries to add details once, even it fails the first time.
info.addedDetailsToZip = true;
info.addingDetailsToZip = false;
+ stopForegroundWhenDone(info.id);
}
if (!tmpZip.renameTo(info.bugreportFile)) {
diff --git a/packages/Shell/tests/src/com/android/shell/BugreportReceiverTest.java b/packages/Shell/tests/src/com/android/shell/BugreportReceiverTest.java
index f76fb26..07c1546 100644
--- a/packages/Shell/tests/src/com/android/shell/BugreportReceiverTest.java
+++ b/packages/Shell/tests/src/com/android/shell/BugreportReceiverTest.java
@@ -241,14 +241,6 @@
}
public void testProgress_takeExtraScreenshot() throws Exception {
- takeExtraScreenshotTest(false);
- }
-
- public void testProgress_takeExtraScreenshotServiceDiesAfterScreenshotTaken() throws Exception {
- takeExtraScreenshotTest(true);
- }
-
- private void takeExtraScreenshotTest(boolean serviceDies) throws Exception {
resetProperties();
sendBugreportStarted(1000);
@@ -259,11 +251,6 @@
sendBugreportFinished(ID, mPlainTextPath, mScreenshotPath);
- if (serviceDies) {
- waitShareNotification(ID);
- killService();
- }
-
Bundle extras = acceptBugreportAndGetSharedIntent(ID);
assertActionSendMultiple(extras, BUGREPORT_CONTENT, SCREENSHOT_CONTENT, ID, PID, ZIP_FILE,
NAME, NO_TITLE, NO_DESCRIPTION, 1, RENAMED_SCREENSHOTS);
@@ -272,14 +259,6 @@
}
public void testScreenshotFinishesAfterBugreport() throws Exception {
- screenshotFinishesAfterBugreportTest(false);
- }
-
- public void testScreenshotFinishesAfterBugreportAndServiceDiesBeforeSharing() throws Exception {
- screenshotFinishesAfterBugreportTest(true);
- }
-
- private void screenshotFinishesAfterBugreportTest(boolean serviceDies) throws Exception {
resetProperties();
sendBugreportStarted(1000);
@@ -291,10 +270,6 @@
// There's no indication in the UI about the screenshot finish, so just sleep like a baby...
Thread.sleep(SAFE_SCREENSHOT_DELAY * DateUtils.SECOND_IN_MILLIS);
- if (serviceDies) {
- killService();
- }
-
Bundle extras = acceptBugreportAndGetSharedIntent(ID);
assertActionSendMultiple(extras, BUGREPORT_CONTENT, NO_SCREENSHOT, ID, PID, ZIP_FILE,
NAME, NO_TITLE, NO_DESCRIPTION, 1, RENAMED_SCREENSHOTS);
@@ -562,7 +537,7 @@
public void testShareBugreportAfterServiceDies() throws Exception {
sendBugreportFinished(NO_ID, mPlainTextPath, NO_SCREENSHOT);
- killService();
+ waitForService(false);
Bundle extras = acceptBugreportAndGetSharedIntent(NO_ID);
assertActionSendMultiple(extras, BUGREPORT_CONTENT, NO_SCREENSHOT);
}
@@ -841,15 +816,6 @@
assertFalse("Service '" + service + "' is still running", isServiceRunning(service));
}
- private void killService() {
- waitForService(true);
- Log.v(TAG, "Stopping service");
- boolean stopped = mContext.stopService(new Intent(mContext, BugreportProgressService.class));
- Log.d(TAG, "stopService returned " + stopped);
- waitForService(false);
- assertServiceNotRunning(); // Sanity check.
- }
-
private boolean isServiceRunning(String name) {
ActivityManager manager = (ActivityManager) mContext
.getSystemService(Context.ACTIVITY_SERVICE);
@@ -878,12 +844,6 @@
Thread.currentThread().interrupt();
}
}
- if (!expectRunning) {
- // Typically happens when service is waiting for a screenshot to finish.
- Log.w(TAG, "Service didn't stop; try to kill it again");
- killService();
- return;
- }
fail("Service status didn't change to " + expectRunning);
}