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();