From bc1f3bff515dc27881be2a2bc22c432f0433d145 Mon Sep 17 00:00:00 2001 From: Rhed Jao Date: Fri, 4 Dec 2020 21:32:12 +0800 Subject: [PATCH 1/3] Fixes BugreportReceiverTest failed Instead of sending INTENT_BUGREPORT_REQUESTED, invoke BugreportRequestedReceiver instance directly. Bug: 174832302 Test: atest BugreportReceiverTest Change-Id: I99a052d74b71d503a655a769064a74246da150e5 --- .../tests/src/com/android/shell/BugreportReceiverTest.java | 7 ++++--- 1 file changed, 4 insertions(+), 3 deletions(-) diff --git a/packages/Shell/tests/src/com/android/shell/BugreportReceiverTest.java b/packages/Shell/tests/src/com/android/shell/BugreportReceiverTest.java index 3b02e3b465577..43147cdfc86dd 100644 --- a/packages/Shell/tests/src/com/android/shell/BugreportReceiverTest.java +++ b/packages/Shell/tests/src/com/android/shell/BugreportReceiverTest.java @@ -576,9 +576,10 @@ public class BugreportReceiverTest { */ private void sendBugreportStarted() throws Exception { Intent intent = new Intent(INTENT_BUGREPORT_REQUESTED); - intent.setPackage("com.android.shell"); - intent.setFlags(Intent.FLAG_RECEIVER_FOREGROUND); - mContext.sendBroadcast(intent); + // Ideally, we should invoke BugreportRequestedReceiver by sending + // INTENT_BUGREPORT_REQUESTED. But the intent has been protected broadcast by the system + // starting from S. + new BugreportRequestedReceiver().onReceive(mContext, intent); ArgumentCaptor listenerCap = ArgumentCaptor.forClass( IDumpstateListener.class); From 8ba3c84cffebe4cb3f545ec9810cf944cab90b3f Mon Sep 17 00:00:00 2001 From: Rhed Jao Date: Mon, 7 Dec 2020 17:58:26 +0800 Subject: [PATCH 2/3] Fixes an error handling in BugreportProgressService Calls onError function when bugreport is finished and file is empty. Bug: 174314124 Test: atest BugreportReceiverTest Change-Id: I4542568fd2d2ad1c75c7c3b223accca4995938a3 --- .../android/shell/BugreportProgressService.java | 9 +++++---- .../com/android/shell/BugreportReceiverTest.java | 15 ++++++++++++++- 2 files changed, 19 insertions(+), 5 deletions(-) diff --git a/packages/Shell/src/com/android/shell/BugreportProgressService.java b/packages/Shell/src/com/android/shell/BugreportProgressService.java index d50f08df947aa..d1a0b49014ce0 100644 --- a/packages/Shell/src/com/android/shell/BugreportProgressService.java +++ b/packages/Shell/src/com/android/shell/BugreportProgressService.java @@ -380,6 +380,11 @@ public class BugreportProgressService extends Service { public void onFinished() { mInfo.renameBugreportFile(); mInfo.renameScreenshots(); + if (mInfo.bugreportFile.length() == 0) { + Log.e(TAG, "Bugreport file empty. File path = " + mInfo.bugreportFile); + onError(BUGREPORT_ERROR_RUNTIME); + return; + } synchronized (mLock) { sendBugreportFinishedBroadcastLocked(); mMainThreadHandler.post(() -> mInfoDialog.onBugreportFinished(mInfo)); @@ -408,10 +413,6 @@ public class BugreportProgressService extends Service { @GuardedBy("mLock") private void sendBugreportFinishedBroadcastLocked() { final String bugreportFilePath = mInfo.bugreportFile.getAbsolutePath(); - if (mInfo.bugreportFile.length() == 0) { - Log.e(TAG, "Bugreport file empty. File path = " + bugreportFilePath); - return; - } if (mInfo.type == BugreportParams.BUGREPORT_MODE_REMOTE) { sendRemoteBugreportFinishedBroadcast(mContext, bugreportFilePath, mInfo.bugreportFile); diff --git a/packages/Shell/tests/src/com/android/shell/BugreportReceiverTest.java b/packages/Shell/tests/src/com/android/shell/BugreportReceiverTest.java index 43147cdfc86dd..89cdeaea199c7 100644 --- a/packages/Shell/tests/src/com/android/shell/BugreportReceiverTest.java +++ b/packages/Shell/tests/src/com/android/shell/BugreportReceiverTest.java @@ -512,6 +512,17 @@ public class BugreportReceiverTest { assertEquals("Didn't change state", STATE_HIDE, newState); } + @Test + public void testBugreportFinished_withEmptyBugreportFile() throws Exception { + sendBugreportStarted(); + + IoUtils.closeQuietly(mBugreportFd); + mBugreportFd = null; + sendBugreportFinished(); + + assertServiceNotRunning(); + } + @Test public void testShareBugreportAfterServiceDies() throws Exception { sendBugreportStarted(); @@ -647,7 +658,9 @@ public class BugreportReceiverTest { * Callbacks to service to finish the bugreport. */ private void sendBugreportFinished() throws Exception { - writeZipFile(mBugreportFd, BUGREPORT_FILE, BUGREPORT_CONTENT); + if (mBugreportFd != null) { + writeZipFile(mBugreportFd, BUGREPORT_FILE, BUGREPORT_CONTENT); + } if (mScreenshotFd != null) { writeScreenshotFile(mScreenshotFd, SCREENSHOT_CONTENT); } From 1893959cf1d0b56de0f6ba98fd308e38c7ff1eb6 Mon Sep 17 00:00:00 2001 From: Rhed Jao Date: Mon, 7 Dec 2020 19:44:13 +0800 Subject: [PATCH 3/3] Returns immediately if the bugreport file already exists There's a case that BugreportProgressService is invoked twice quickly, and both services create the same bugreport file name. The later one may delete current running bugreport file in its clean function, when it detects another bugreport is running. Bug: 174314124 Test: atest BugreportReceiverTest Change-Id: I5e1802c5912f4414f1ad3b8bdaf7c7420332b9d6 --- .../android/shell/BugreportProgressService.java | 15 +++++++++++---- .../com/android/shell/BugreportReceiverTest.java | 13 +++++++++++++ 2 files changed, 24 insertions(+), 4 deletions(-) diff --git a/packages/Shell/src/com/android/shell/BugreportProgressService.java b/packages/Shell/src/com/android/shell/BugreportProgressService.java index d1a0b49014ce0..a2608a0a30570 100644 --- a/packages/Shell/src/com/android/shell/BugreportProgressService.java +++ b/packages/Shell/src/com/android/shell/BugreportProgressService.java @@ -618,12 +618,21 @@ public class BugreportProgressService extends Service { BugreportInfo info = new BugreportInfo(mContext, baseName, name, shareTitle, shareDescription, bugreportType, mBugreportsDir); + synchronized (mLock) { + if (info.bugreportFile.exists()) { + Log.e(TAG, "Failed to start bugreport generation, the requested bugreport file " + + info.bugreportFile + " already exists"); + return; + } + info.createBugreportFile(); + } ParcelFileDescriptor bugreportFd = info.getBugreportFd(); if (bugreportFd == null) { Log.e(TAG, "Failed to start bugreport generation as " + " bugreport parcel file descriptor is null."); return; } + info.createScreenshotFile(mBugreportsDir); ParcelFileDescriptor screenshotFd = null; if (isDefaultScreenshotRequired(bugreportType, /* hasScreenshotButton= */ !mIsTv)) { screenshotFd = info.getDefaultScreenshotFd(); @@ -1920,12 +1929,10 @@ public class BugreportProgressService extends Service { this.shareDescription = shareDescription == null ? "" : shareDescription; this.type = type; this.baseName = baseName; - createBugreportFile(bugreportsDir); - createScreenshotFile(bugreportsDir); + this.bugreportFile = new File(bugreportsDir, getFileName(this, ".zip")); } - void createBugreportFile(File bugreportsDir) { - bugreportFile = new File(bugreportsDir, getFileName(this, ".zip")); + void createBugreportFile() { createReadWriteFile(bugreportFile); } diff --git a/packages/Shell/tests/src/com/android/shell/BugreportReceiverTest.java b/packages/Shell/tests/src/com/android/shell/BugreportReceiverTest.java index 89cdeaea199c7..9476912067417 100644 --- a/packages/Shell/tests/src/com/android/shell/BugreportReceiverTest.java +++ b/packages/Shell/tests/src/com/android/shell/BugreportReceiverTest.java @@ -40,6 +40,7 @@ import static org.mockito.ArgumentMatchers.any; import static org.mockito.ArgumentMatchers.anyBoolean; import static org.mockito.ArgumentMatchers.anyInt; import static org.mockito.Mockito.timeout; +import static org.mockito.Mockito.times; import static org.mockito.Mockito.verify; import android.app.ActivityManager; @@ -534,6 +535,18 @@ public class BugreportReceiverTest { assertActionSendMultiple(extras); } + @Test + public void testBugreportRequestTwice_oneStartBugreportInvoked() throws Exception { + sendBugreportStarted(); + new BugreportRequestedReceiver().onReceive(mContext, + new Intent(INTENT_BUGREPORT_REQUESTED)); + getInstrumentation().waitForIdleSync(); + + verify(mMockIDumpstate, times(1)).startBugreport(anyInt(), any(), any(), any(), + anyInt(), any(), anyBoolean()); + sendBugreportFinished(); + } + private void cancelExistingNotifications() { // Must kill service first, because notifications from a foreground service cannot be // canceled.