From c4f6467702e308844ef0769ba17dcb7b7b32a9e6 Mon Sep 17 00:00:00 2001 From: Felipe Leme Date: Tue, 12 Jan 2016 18:12:09 -0800 Subject: [PATCH] Save bugreport info on share intent. When a bugreport is finished, BugreportProgressService sends a INTENT_BUGREPORT_SHARE intent containing the bugreport pid; then when the user clicks the share notification, BugreportProgressService uses the pid to retrieve the bugreport info. The problem with this approach is that if the service dies before the user clicks the notification, the bugreport won't be shared. This change fix this scenario by saving the bugreport info in the share intent. BUG: 26513652 Also added more logging statements. Change-Id: Iba86d06369f843ad88194fb1dad0c8b69764df78 --- .../shell/BugreportProgressService.java | 110 ++++++++++++++-- .../com/android/shell/BugreportReceiver.java | 8 +- .../android/shell/BugreportReceiverTest.java | 122 ++++++++++++++++-- 3 files changed, 219 insertions(+), 21 deletions(-) diff --git a/packages/Shell/src/com/android/shell/BugreportProgressService.java b/packages/Shell/src/com/android/shell/BugreportProgressService.java index 5c807e1bd652c..df8fad4dd5222 100644 --- a/packages/Shell/src/com/android/shell/BugreportProgressService.java +++ b/packages/Shell/src/com/android/shell/BugreportProgressService.java @@ -65,6 +65,7 @@ import android.os.HandlerThread; import android.os.IBinder; import android.os.Looper; import android.os.Message; +import android.os.Parcel; import android.os.Parcelable; import android.os.SystemProperties; import android.os.Vibrator; @@ -108,7 +109,7 @@ import android.widget.Toast; * */ public class BugreportProgressService extends Service { - static final String TAG = "Shell"; + private static final String TAG = "BugreportProgressService"; private static final boolean DEBUG = false; private static final String AUTHORITY = "com.android.shell"; @@ -137,6 +138,7 @@ public class BugreportProgressService extends Service { static final String EXTRA_TITLE = "android.intent.extra.TITLE"; static final String EXTRA_DESCRIPTION = "android.intent.extra.DESCRIPTION"; static final String EXTRA_ORIGINAL_INTENT = "android.intent.extra.ORIGINAL_INTENT"; + static final String EXTRA_INFO = "android.intent.extra.INFO"; private static final int MSG_SERVICE_COMMAND = 1; private static final int MSG_POLL = 2; @@ -325,7 +327,7 @@ public class BugreportProgressService extends Service { takeScreenshot(pid, true); break; case INTENT_BUGREPORT_SHARE: - shareBugreport(pid); + shareBugreport(pid, (BugreportInfo) intent.getParcelableExtra(EXTRA_INFO)); break; case INTENT_BUGREPORT_CANCEL: cancel(pid); @@ -483,6 +485,7 @@ public class BugreportProgressService extends Service { if (mProcesses.indexOfKey(pid) < 0) { Log.w(TAG, "PID not watched: " + pid); } else { + Log.d(TAG, "Removing PID " + pid); mProcesses.remove(pid); } stopSelfWhenDone(); @@ -675,6 +678,11 @@ public class BugreportProgressService extends Service { final int msgId; if (taken) { info.addScreenshot(screenshotFile); + if (info.finished) { + Log.d(TAG, "Screenshot finished after bugreport; updating share notification"); + info.renameScreenshots(mScreenshotsDir); + sendBugreportNotification(mContext, info); + } msgId = R.string.bugreport_screenshot_taken; } else { // TODO: try again using Framework APIs instead of relying on screencap. @@ -814,12 +822,13 @@ public class BugreportProgressService extends Service { * Shares the bugreport upon user's request by issuing a {@link Intent#ACTION_SEND_MULTIPLE} * intent, but issuing a warning dialog the first time. */ - private void shareBugreport(int pid) { - final BugreportInfo info = getInfo(pid); + private void shareBugreport(int pid, BugreportInfo sharedInfo) { + BugreportInfo info = getInfo(pid); if (info == null) { - // Should not happen, so log if it does... - Log.e(TAG, "INTERNAL ERROR: no info for PID " + pid + ": " + mProcesses); - return; + // Service was terminated but notification persisted + info = sharedInfo; + Log.d(TAG, "shareBugreport(): no info for PID " + pid + " on managed processes (" + + mProcesses + "), using info from intent instead (" + info + ")"); } addDetailsToZipFile(info); @@ -850,6 +859,7 @@ public class BugreportProgressService extends Service { shareIntent.setClass(context, BugreportProgressService.class); shareIntent.setAction(INTENT_BUGREPORT_SHARE); shareIntent.putExtra(EXTRA_PID, info.pid); + shareIntent.putExtra(EXTRA_INFO, info); final String title = context.getString(R.string.bugreport_finished_title); final Notification.Builder builder = new Notification.Builder(context) @@ -919,6 +929,11 @@ public class BugreportProgressService extends Service { * description will be saved on {@code description.txt}. */ private void addDetailsToZipFile(BugreportInfo info) { + if (info.bugreportFile == null) { + // One possible reason is a bug in the Parcelization code. + Log.e(TAG, "INTERNAL ERROR: no bugreportFile on " + info); + return; + } // 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. final File dir = info.bugreportFile.getParentFile(); @@ -1281,7 +1296,7 @@ public class BugreportProgressService extends Service { /** * Information about a bugreport process while its in progress. */ - private static final class BugreportInfo { + private static final class BugreportInfo implements Parcelable { private final Context context; /** @@ -1324,6 +1339,11 @@ public class BugreportProgressService extends Service { */ long lastUpdate = System.currentTimeMillis(); + /** + * Time of the last progress update when Parcel was created. + */ + String formattedLastUpdate; + /** * Path of the main bugreport file. */ @@ -1403,6 +1423,11 @@ public class BugreportProgressService extends Service { } String getFormattedLastUpdate() { + if (context == null) { + // Restored from Parcel + return formattedLastUpdate == null ? + Long.toString(lastUpdate) : formattedLastUpdate; + } return DateUtils.formatDateTime(context, lastUpdate, DateUtils.FORMAT_SHOW_DATE | DateUtils.FORMAT_SHOW_TIME); } @@ -1416,5 +1441,74 @@ public class BugreportProgressService extends Service { + "\n\tprogress: " + progress + "/" + max + "(" + percent + ")" + "\n\tlast_update: " + getFormattedLastUpdate(); } + + // Parcelable contract + protected BugreportInfo(Parcel in) { + context = null; + pid = in.readInt(); + name = in.readString(); + title = in.readString(); + description = in.readString(); + max = in.readInt(); + progress = in.readInt(); + lastUpdate = in.readLong(); + formattedLastUpdate = in.readString(); + bugreportFile = readFile(in); + + int screenshotSize = in.readInt(); + for (int i = 1; i <= screenshotSize; i++) { + screenshotFiles.add(readFile(in)); + } + + finished = in.readInt() == 1; + screenshotCounter = in.readInt(); + } + + @Override + public void writeToParcel(Parcel dest, int flags) { + dest.writeInt(pid); + dest.writeString(name); + dest.writeString(title); + dest.writeString(description); + dest.writeInt(max); + dest.writeInt(progress); + dest.writeLong(lastUpdate); + dest.writeString(getFormattedLastUpdate()); + writeFile(dest, bugreportFile); + + dest.writeInt(screenshotFiles.size()); + for (File screenshotFile : screenshotFiles) { + writeFile(dest, screenshotFile); + } + + dest.writeInt(finished ? 1 : 0); + dest.writeInt(screenshotCounter); + } + + @Override + public int describeContents() { + return 0; + } + + private void writeFile(Parcel dest, File file) { + dest.writeString(file == null ? null : file.getPath()); + } + + private File readFile(Parcel in) { + final String path = in.readString(); + return path == null ? null : new File(path); + } + + public static final Parcelable.Creator CREATOR = + new Parcelable.Creator() { + public BugreportInfo createFromParcel(Parcel source) { + return new BugreportInfo(source); + } + + public BugreportInfo[] newArray(int size) { + return new BugreportInfo[size]; + } + }; + } } diff --git a/packages/Shell/src/com/android/shell/BugreportReceiver.java b/packages/Shell/src/com/android/shell/BugreportReceiver.java index c8898b97d8785..9afa9005c43b1 100644 --- a/packages/Shell/src/com/android/shell/BugreportReceiver.java +++ b/packages/Shell/src/com/android/shell/BugreportReceiver.java @@ -19,7 +19,6 @@ package com.android.shell; import static com.android.shell.BugreportProgressService.EXTRA_BUGREPORT; import static com.android.shell.BugreportProgressService.EXTRA_ORIGINAL_INTENT; import static com.android.shell.BugreportProgressService.INTENT_BUGREPORT_FINISHED; -import static com.android.shell.BugreportProgressService.TAG; import static com.android.shell.BugreportProgressService.getFileExtra; import java.io.File; @@ -37,6 +36,7 @@ import android.util.Log; * {@link Intent#ACTION_SEND_MULTIPLE}. */ public class BugreportReceiver extends BroadcastReceiver { + private static final String TAG = "BugreportReceiver"; /** * Always keep the newest 8 bugreport files; 4 reports and 4 screenshots are @@ -74,7 +74,11 @@ public class BugreportReceiver extends BroadcastReceiver { new AsyncTask() { @Override protected Void doInBackground(Void... params) { - FileUtils.deleteOlderFiles(bugreportFile.getParentFile(), minCount, minAge); + try { + FileUtils.deleteOlderFiles(bugreportFile.getParentFile(), minCount, minAge); + } catch (RuntimeException e) { + Log.e(TAG, "RuntimeException deleting old files", e); + } result.finish(); return null; } diff --git a/packages/Shell/tests/src/com/android/shell/BugreportReceiverTest.java b/packages/Shell/tests/src/com/android/shell/BugreportReceiverTest.java index d1a07ea21fc4e..52e1b56d16d7f 100644 --- a/packages/Shell/tests/src/com/android/shell/BugreportReceiverTest.java +++ b/packages/Shell/tests/src/com/android/shell/BugreportReceiverTest.java @@ -65,6 +65,7 @@ import android.text.format.DateUtils; import android.util.Log; import com.android.shell.ActionSendMultipleConsumerActivity.CustomActionSendMultipleListener; +import com.android.shell.BugreportProgressService; /** * Integration tests for {@link BugreportReceiver}. @@ -89,6 +90,9 @@ public class BugreportReceiverTest extends InstrumentationTestCase { // Timeout for UI operations, in milliseconds. private static final int TIMEOUT = (int) BugreportProgressService.POLLING_FREQUENCY * 4; + // Timeout for when waiting for a screenshot to finish. + private static final int SAFE_SCREENSHOT_DELAY = SCREENSHOT_DELAY_SECONDS + 10; + private static final String BUGREPORTS_DIR = "bugreports"; private static final String BUGREPORT_FILE = "test_bugreport.txt"; private static final String ZIP_FILE = "test_bugreport.zip"; @@ -109,6 +113,7 @@ public class BugreportReceiverTest extends InstrumentationTestCase { private static final String NO_NAME = null; private static final String NO_SCREENSHOT = null; private static final String NO_TITLE = null; + private static final Integer NO_PID = null; private String mDescription; @@ -122,6 +127,7 @@ public class BugreportReceiverTest extends InstrumentationTestCase { @Override protected void setUp() throws Exception { + Log.i(TAG, "#### setup() on " + getName()); Instrumentation instrumentation = getInstrumentation(); mContext = instrumentation.getTargetContext(); mUiBot = new UiBot(UiDevice.getInstance(instrumentation), TIMEOUT); @@ -164,13 +170,21 @@ public class BugreportReceiverTest extends InstrumentationTestCase { Bundle extras = sendBugreportFinishedAndGetSharedIntent(PID, mPlainTextPath, mScreenshotPath); - assertActionSendMultiple(extras, BUGREPORT_FILE, BUGREPORT_CONTENT, PID, ZIP_FILE, + assertActionSendMultiple(extras, BUGREPORT_CONTENT, SCREENSHOT_CONTENT, PID, ZIP_FILE, NAME, NO_TITLE, NO_DESCRIPTION, 1, true); assertServiceNotRunning(); } 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); @@ -179,14 +193,49 @@ public class BugreportReceiverTest extends InstrumentationTestCase { assertScreenshotButtonEnabled(false); waitForScreenshotButtonEnabled(true); - Bundle extras = - sendBugreportFinishedAndGetSharedIntent(PID, mPlainTextPath, mScreenshotPath); - assertActionSendMultiple(extras, BUGREPORT_FILE, BUGREPORT_CONTENT, PID, ZIP_FILE, + sendBugreportFinished(PID, mPlainTextPath, mScreenshotPath); + + if (serviceDies) { + waitShareNotification(); + killService(); + } + + Bundle extras = acceptBugreportAndGetSharedIntent(); + assertActionSendMultiple(extras, BUGREPORT_CONTENT, SCREENSHOT_CONTENT, PID, ZIP_FILE, NAME, NO_TITLE, NO_DESCRIPTION, 2, true); assertServiceNotRunning(); } + public void testScreenshotFinishesAfterBugreport() throws Exception { + screenshotFinishesAfterBugreportTest(false); + } + + public void testScreenshotFinishesAfterBugreportAndServiceDiesBeforeSharing() throws Exception { + screenshotFinishesAfterBugreportTest(true); + } + + private void screenshotFinishesAfterBugreportTest(boolean serviceDies) throws Exception { + resetProperties(); + + sendBugreportStarted(1000); + sendBugreportFinished(PID, mPlainTextPath, NO_SCREENSHOT); + waitShareNotification(); + + // 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(); + assertActionSendMultiple(extras, BUGREPORT_CONTENT, NO_SCREENSHOT, PID, ZIP_FILE, + NAME, NO_TITLE, NO_DESCRIPTION, 1, true); + + assertServiceNotRunning(); + } + public void testProgress_changeDetailsInvalidInput() throws Exception { resetProperties(); @@ -227,7 +276,7 @@ public class BugreportReceiverTest extends InstrumentationTestCase { Bundle extras = sendBugreportFinishedAndGetSharedIntent(PID, mPlainTextPath, mScreenshotPath); - assertActionSendMultiple(extras, BUGREPORT_FILE, BUGREPORT_CONTENT, PID, TITLE, + assertActionSendMultiple(extras, BUGREPORT_CONTENT, SCREENSHOT_CONTENT, PID, TITLE, NEW_NAME, TITLE, mDescription, 1, true); assertServiceNotRunning(); @@ -266,7 +315,7 @@ public class BugreportReceiverTest extends InstrumentationTestCase { Bundle extras = sendBugreportFinishedAndGetSharedIntent(PID, plainText? mPlainTextPath : mZipPath, mScreenshotPath); - assertActionSendMultiple(extras, BUGREPORT_FILE, BUGREPORT_CONTENT, PID, TITLE, + assertActionSendMultiple(extras, BUGREPORT_CONTENT, SCREENSHOT_CONTENT, PID, TITLE, NEW_NAME, TITLE, mDescription, 1, true); assertServiceNotRunning(); @@ -317,8 +366,8 @@ public class BugreportReceiverTest extends InstrumentationTestCase { // Finally, share bugreport. Bundle extras = acceptBugreportAndGetSharedIntent(); - assertActionSendMultiple(extras, BUGREPORT_FILE, BUGREPORT_CONTENT, PID, TITLE, - NAME, TITLE, mDescription, 1, waitScreenshot); + assertActionSendMultiple(extras, BUGREPORT_CONTENT, SCREENSHOT_CONTENT, PID, TITLE, + NAME, TITLE, mDescription, 1, true); assertServiceNotRunning(); } @@ -328,7 +377,7 @@ public class BugreportReceiverTest extends InstrumentationTestCase { BugreportPrefs.setWarningState(mContext, BugreportPrefs.STATE_SHOW); // Send notification and click on share. - sendBugreportFinished(null, mPlainTextPath, null); + sendBugreportFinished(NO_PID, mPlainTextPath, null); acceptBugreport(); // Handle the warning @@ -350,6 +399,13 @@ public class BugreportReceiverTest extends InstrumentationTestCase { assertEquals("Didn't change state", BugreportPrefs.STATE_HIDE, newState); } + public void testShareBugreportAfterServiceDies() throws Exception { + sendBugreportFinished(NO_PID, mPlainTextPath, NO_SCREENSHOT); + killService(); + Bundle extras = acceptBugreportAndGetSharedIntent(); + assertActionSendMultiple(extras, BUGREPORT_CONTENT, NO_SCREENSHOT); + } + public void testBugreportFinished_plainBugreportAndScreenshot() throws Exception { Bundle extras = sendBugreportFinishedAndGetSharedIntent(mPlainTextPath, mScreenshotPath); assertActionSendMultiple(extras, BUGREPORT_CONTENT, SCREENSHOT_CONTENT); @@ -443,6 +499,13 @@ public class BugreportReceiverTest extends InstrumentationTestCase { return mListener.getExtras(); } + /** + * Waits for the notification to share the finished bugreport. + */ + private void waitShareNotification() { + mUiBot.getNotification(mContext.getString(R.string.bugreport_finished_title)); + } + /** * Accepts the notification to share the finished bugreport. */ @@ -512,7 +575,8 @@ public class BugreportReceiverTest extends InstrumentationTestCase { expectedNumberScreenshots ++; // Add screenshot received by dumpstate } int expectedSize = expectedNumberScreenshots + 1; // All screenshots plus the bugreport file - assertEquals("wrong number of attachments", expectedSize, attachments.size()); + assertEquals("wrong number of attachments (" + attachments + ")", + expectedSize, attachments.size()); // Need to interact through all attachments, since order is not guaranteed. Uri zipUri = null; @@ -607,6 +671,15 @@ public class BugreportReceiverTest extends InstrumentationTestCase { 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); @@ -618,6 +691,33 @@ public class BugreportReceiverTest extends InstrumentationTestCase { return false; } + private void waitForService(boolean expectRunning) { + String service = BugreportProgressService.class.getName(); + boolean actualRunning; + for (int i = 1; i <= 5; i++) { + actualRunning = isServiceRunning(service); + Log.d(TAG, "Attempt " + i + " to check status of service '" + + service + "': expected=" + expectRunning + ", actual= " + actualRunning); + if (actualRunning == expectRunning) { + return; + } + try { + Thread.sleep(DateUtils.SECOND_IN_MILLIS); + } catch (InterruptedException e) { + Log.w(TAG, "thread interrupted"); + 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); + } + private static void createTextFile(String path, String content) throws IOException { Log.v(TAG, "createFile(" + path + ")"); try (Writer writer = new BufferedWriter(new OutputStreamWriter( @@ -669,7 +769,7 @@ public class BugreportReceiverTest extends InstrumentationTestCase { private UiObject waitForScreenshotButtonEnabled(boolean expectedEnabled) throws Exception { UiObject screenshotButton = getScreenshotButton(); - int maxAttempts = SCREENSHOT_DELAY_SECONDS + 5; + int maxAttempts = SAFE_SCREENSHOT_DELAY; int i = 0; do { boolean enabled = screenshotButton.isEnabled();