Support to screenshot to interactive bugreport.
- Take screenshot only after critical dumpsys has finished -- so calling
app can be shown earlier without disrupting critical dumpsys after
starting bugreport.
- Show a visual indication to indicate screenshot is taken via IDumpstateListener.onScreenshotTaken().
- Copy the screenshot file to calling app after consent is granted for letting
calling app can get and show the screenshot earlier, otherwise calling
app may need to wait several minutes to get the screenshot to show.
- Rename do_fb --> do_screenshot because do_fb basically means do_screenshot.
BUG:149525300
Test: Flash and press bug report shortcut and check the screenshot
Change-Id: Ibaca13bfabc1350448d05df12be5ee9291860c0e
Merged-In: Ibaca13bfabc1350448d05df12be5ee9291860c0e
diff --git a/cmds/dumpstate/DumpstateService.cpp b/cmds/dumpstate/DumpstateService.cpp
index 87ea520..466575f 100644
--- a/cmds/dumpstate/DumpstateService.cpp
+++ b/cmds/dumpstate/DumpstateService.cpp
@@ -120,7 +120,7 @@
options->Initialize(static_cast<Dumpstate::BugreportMode>(bugreport_mode), bugreport_fd,
screenshot_fd);
- if (bugreport_fd.get() == -1 || (options->do_fb && screenshot_fd.get() == -1)) {
+ if (bugreport_fd.get() == -1 || (options->do_screenshot && screenshot_fd.get() == -1)) {
MYLOGE("Invalid filedescriptor");
signalErrorAndExit(listener, IDumpstateListener::BUGREPORT_ERROR_INVALID_INPUT);
}
diff --git a/cmds/dumpstate/binder/android/os/IDumpstateListener.aidl b/cmds/dumpstate/binder/android/os/IDumpstateListener.aidl
index e486460..e17f18e 100644
--- a/cmds/dumpstate/binder/android/os/IDumpstateListener.aidl
+++ b/cmds/dumpstate/binder/android/os/IDumpstateListener.aidl
@@ -61,4 +61,9 @@
* Called when taking bugreport finishes successfully.
*/
void onFinished();
+
+ /**
+ * Called when screenshot is taken.
+ */
+ void onScreenshotTaken(boolean success);
}
diff --git a/cmds/dumpstate/dumpstate.cpp b/cmds/dumpstate/dumpstate.cpp
index 814a4ed..3440116 100644
--- a/cmds/dumpstate/dumpstate.cpp
+++ b/cmds/dumpstate/dumpstate.cpp
@@ -647,6 +647,24 @@
std::lock_guard<std::mutex> lock(lock_);
result_ = APPROVED;
MYLOGD("User approved consent to share bugreport\n");
+
+ // Maybe copy screenshot so calling app can display the screenshot to the user as soon as
+ // consent is granted.
+ if (ds.options_->is_screenshot_copied) {
+ return android::binder::Status::ok();
+ }
+
+ if (!ds.options_->do_screenshot || ds.options_->screenshot_fd.get() == -1 ||
+ !ds.do_early_screenshot_) {
+ return android::binder::Status::ok();
+ }
+
+ bool copy_succeeded = android::os::CopyFileToFd(ds.screenshot_path_,
+ ds.options_->screenshot_fd.get());
+ ds.options_->is_screenshot_copied = copy_succeeded;
+ if (copy_succeeded) {
+ android::os::UnlinkAndLogOnError(ds.screenshot_path_);
+ }
return android::binder::Status::ok();
}
@@ -1407,7 +1425,7 @@
/* Dump Bluetooth HCI logs */
ds.AddDir("/data/misc/bluetooth/logs", true);
- if (ds.options_->do_fb && !ds.do_early_screenshot_) {
+ if (ds.options_->do_screenshot && !ds.do_early_screenshot_) {
MYLOGI("taking late screenshot\n");
ds.TakeScreenshot();
}
@@ -2149,7 +2167,7 @@
ds.base_name_ += "-wifi";
}
- if (ds.options_->do_fb) {
+ if (ds.options_->do_screenshot) {
ds.screenshot_path_ = ds.GetPath(ds.CalledByApi() ? "-tmp.png" : ".png");
}
ds.tmp_path_ = ds.GetPath(".tmp");
@@ -2229,43 +2247,45 @@
}
static void SetOptionsFromMode(Dumpstate::BugreportMode mode, Dumpstate::DumpOptions* options) {
+ // Modify com.android.shell.BugreportProgressService#isDefaultScreenshotRequired as well for
+ // default system screenshots.
options->bugreport_mode = ModeToString(mode);
switch (mode) {
case Dumpstate::BugreportMode::BUGREPORT_FULL:
- options->do_fb = true;
+ options->do_screenshot = true;
options->dumpstate_hal_mode = DumpstateMode::FULL;
break;
case Dumpstate::BugreportMode::BUGREPORT_INTERACTIVE:
// Currently, the dumpstate binder is only used by Shell to update progress.
options->do_start_service = true;
options->do_progress_updates = true;
- options->do_fb = false;
+ options->do_screenshot = true;
options->dumpstate_hal_mode = DumpstateMode::INTERACTIVE;
break;
case Dumpstate::BugreportMode::BUGREPORT_REMOTE:
options->do_vibrate = false;
options->is_remote_mode = true;
- options->do_fb = false;
+ options->do_screenshot = false;
options->dumpstate_hal_mode = DumpstateMode::REMOTE;
break;
case Dumpstate::BugreportMode::BUGREPORT_WEAR:
options->do_start_service = true;
options->do_progress_updates = true;
options->do_zip_file = true;
- options->do_fb = true;
+ options->do_screenshot = true;
options->dumpstate_hal_mode = DumpstateMode::WEAR;
break;
// TODO(b/148168577) rename TELEPHONY everywhere to CONNECTIVITY.
case Dumpstate::BugreportMode::BUGREPORT_TELEPHONY:
options->telephony_only = true;
options->do_progress_updates = true;
- options->do_fb = false;
+ options->do_screenshot = false;
options->dumpstate_hal_mode = DumpstateMode::CONNECTIVITY;
break;
case Dumpstate::BugreportMode::BUGREPORT_WIFI:
options->wifi_only = true;
options->do_zip_file = true;
- options->do_fb = false;
+ options->do_screenshot = false;
options->dumpstate_hal_mode = DumpstateMode::WIFI;
break;
case Dumpstate::BugreportMode::BUGREPORT_DEFAULT:
@@ -2275,12 +2295,13 @@
static void LogDumpOptions(const Dumpstate::DumpOptions& options) {
MYLOGI(
- "do_zip_file: %d do_vibrate: %d use_socket: %d use_control_socket: %d do_fb: %d "
+ "do_zip_file: %d do_vibrate: %d use_socket: %d use_control_socket: %d do_screenshot: %d "
"is_remote_mode: %d show_header_only: %d do_start_service: %d telephony_only: %d "
"wifi_only: %d do_progress_updates: %d fd: %d bugreport_mode: %s dumpstate_hal_mode: %s "
"args: %s\n",
options.do_zip_file, options.do_vibrate, options.use_socket, options.use_control_socket,
- options.do_fb, options.is_remote_mode, options.show_header_only, options.do_start_service,
+ options.do_screenshot, options.is_remote_mode, options.show_header_only,
+ options.do_start_service,
options.telephony_only, options.wifi_only, options.do_progress_updates,
options.bugreport_fd.get(), options.bugreport_mode.c_str(),
toString(options.dumpstate_hal_mode).c_str(), options.args.c_str());
@@ -2313,7 +2334,7 @@
case 'S': use_control_socket = true; break;
case 'v': show_header_only = true; break;
case 'q': do_vibrate = false; break;
- case 'p': do_fb = true; break;
+ case 'p': do_screenshot = true; break;
case 'P': do_progress_updates = true; break;
case 'R': is_remote_mode = true; break;
case 'V': break; // compatibility no-op
@@ -2543,11 +2564,6 @@
Vibrate(150);
}
- if (options_->do_fb && do_early_screenshot_) {
- MYLOGI("taking early screenshot\n");
- TakeScreenshot();
- }
-
if (options_->do_zip_file && zip_file != nullptr) {
if (chown(path_.c_str(), AID_SHELL, AID_SHELL)) {
MYLOGE("Unable to change ownership of zip file %s: %s\n", path_.c_str(),
@@ -2593,19 +2609,20 @@
PrintHeader();
if (options_->telephony_only) {
+ MaybeTakeEarlyScreenshot();
MaybeCheckUserConsent(calling_uid, calling_package);
DumpstateTelephonyOnly(calling_package);
DumpstateBoard();
} else if (options_->wifi_only) {
+ MaybeTakeEarlyScreenshot();
MaybeCheckUserConsent(calling_uid, calling_package);
DumpstateWifiOnly();
} else {
- // Invoking the critical dumpsys calls before DumpTraces() to try and
- // keep the system stats as close to its initial state as possible.
+ // Invoke critical dumpsys first to preserve system state, before doing anything else.
RunDumpsysCritical();
- // Run consent check only after critical dumpsys has finished -- so the consent
- // isn't going to pollute the system state / logs.
+ // Take screenshot and get consent only after critical dumpsys has finished.
+ MaybeTakeEarlyScreenshot();
MaybeCheckUserConsent(calling_uid, calling_package);
// Dump state for the default case. This also drops root.
@@ -2640,7 +2657,9 @@
MYLOGI("User denied consent. Returning\n");
return status;
}
- if (options_->do_fb && options_->screenshot_fd.get() != -1) {
+ if (options_->do_screenshot &&
+ options_->screenshot_fd.get() != -1 &&
+ !options_->is_screenshot_copied) {
bool copy_succeeded = android::os::CopyFileToFd(screenshot_path_,
options_->screenshot_fd.get());
if (copy_succeeded) {
@@ -2694,6 +2713,14 @@
: RunStatus::OK;
}
+void Dumpstate::MaybeTakeEarlyScreenshot() {
+ if (!options_->do_screenshot || !do_early_screenshot_) {
+ return;
+ }
+
+ TakeScreenshot();
+}
+
void Dumpstate::MaybeCheckUserConsent(int32_t calling_uid, const std::string& calling_package) {
if (calling_uid == AID_SHELL || !CalledByApi()) {
// No need to get consent for shell triggered dumpstates, or not through
@@ -3630,6 +3657,11 @@
} else {
MYLOGE("Failed to take screenshot on %s\n", real_path.c_str());
}
+ if (listener_ != nullptr) {
+ // Show a visual indication to indicate screenshot is taken via
+ // IDumpstateListener.onScreenshotTaken()
+ listener_->onScreenshotTaken(status == 0);
+ }
}
bool is_dir(const char* pathname) {
diff --git a/cmds/dumpstate/dumpstate.h b/cmds/dumpstate/dumpstate.h
index 111c098..7e27787 100644
--- a/cmds/dumpstate/dumpstate.h
+++ b/cmds/dumpstate/dumpstate.h
@@ -359,7 +359,8 @@
// Writes bugreport content to a socket; only flatfile format is supported.
bool use_socket = false;
bool use_control_socket = false;
- bool do_fb = false;
+ bool do_screenshot = false;
+ bool is_screenshot_copied = false;
bool is_remote_mode = false;
bool show_header_only = false;
bool do_start_service = false;
@@ -494,6 +495,8 @@
RunStatus DumpstateDefaultAfterCritical();
+ void MaybeTakeEarlyScreenshot();
+
void MaybeCheckUserConsent(int32_t calling_uid, const std::string& calling_package);
// Removes the in progress files output files (tmp file, zip/txt file, screenshot),
diff --git a/cmds/dumpstate/tests/dumpstate_smoke_test.cpp b/cmds/dumpstate/tests/dumpstate_smoke_test.cpp
index dac90d9..f26e4db 100644
--- a/cmds/dumpstate/tests/dumpstate_smoke_test.cpp
+++ b/cmds/dumpstate/tests/dumpstate_smoke_test.cpp
@@ -167,6 +167,12 @@
return binder::Status::ok();
}
+ binder::Status onScreenshotTaken(bool success) override {
+ std::lock_guard<std::mutex> lock(lock_);
+ dprintf(out_fd_, "\rResult of taking screenshot: %s", success ? "success" : "failure");
+ return binder::Status::ok();
+ }
+
bool getIsFinished() {
std::lock_guard<std::mutex> lock(lock_);
return is_finished_;
diff --git a/cmds/dumpstate/tests/dumpstate_test.cpp b/cmds/dumpstate/tests/dumpstate_test.cpp
index 76b9960..0a0c40e 100644
--- a/cmds/dumpstate/tests/dumpstate_test.cpp
+++ b/cmds/dumpstate/tests/dumpstate_test.cpp
@@ -65,6 +65,7 @@
MOCK_METHOD1(onProgress, binder::Status(int32_t progress));
MOCK_METHOD1(onError, binder::Status(int32_t error_code));
MOCK_METHOD0(onFinished, binder::Status());
+ MOCK_METHOD1(onScreenshotTaken, binder::Status(bool success));
protected:
MOCK_METHOD0(onAsBinder, IBinder*());
@@ -173,7 +174,7 @@
EXPECT_FALSE(options_.use_control_socket);
EXPECT_FALSE(options_.show_header_only);
EXPECT_TRUE(options_.do_vibrate);
- EXPECT_FALSE(options_.do_fb);
+ EXPECT_FALSE(options_.do_screenshot);
EXPECT_FALSE(options_.do_progress_updates);
EXPECT_FALSE(options_.is_remote_mode);
EXPECT_EQ(options_.dumpstate_hal_mode, DumpstateMode::DEFAULT);
@@ -199,7 +200,7 @@
// Other options retain default values
EXPECT_TRUE(options_.do_vibrate);
EXPECT_FALSE(options_.show_header_only);
- EXPECT_FALSE(options_.do_fb);
+ EXPECT_FALSE(options_.do_screenshot);
EXPECT_FALSE(options_.do_progress_updates);
EXPECT_FALSE(options_.is_remote_mode);
EXPECT_FALSE(options_.use_socket);
@@ -225,7 +226,7 @@
EXPECT_FALSE(options_.do_zip_file);
EXPECT_FALSE(options_.use_control_socket);
EXPECT_FALSE(options_.show_header_only);
- EXPECT_FALSE(options_.do_fb);
+ EXPECT_FALSE(options_.do_screenshot);
EXPECT_FALSE(options_.do_progress_updates);
EXPECT_FALSE(options_.is_remote_mode);
EXPECT_EQ(options_.dumpstate_hal_mode, DumpstateMode::DEFAULT);
@@ -234,7 +235,7 @@
TEST_F(DumpOptionsTest, InitializeFullBugReport) {
options_.Initialize(Dumpstate::BugreportMode::BUGREPORT_FULL, fd, fd);
EXPECT_TRUE(options_.do_add_date);
- EXPECT_TRUE(options_.do_fb);
+ EXPECT_TRUE(options_.do_screenshot);
EXPECT_TRUE(options_.do_zip_file);
EXPECT_EQ(options_.dumpstate_hal_mode, DumpstateMode::FULL);
@@ -254,7 +255,7 @@
EXPECT_TRUE(options_.do_zip_file);
EXPECT_TRUE(options_.do_progress_updates);
EXPECT_TRUE(options_.do_start_service);
- EXPECT_FALSE(options_.do_fb);
+ EXPECT_TRUE(options_.do_screenshot);
EXPECT_EQ(options_.dumpstate_hal_mode, DumpstateMode::INTERACTIVE);
// Other options retain default values
@@ -271,7 +272,7 @@
EXPECT_TRUE(options_.do_zip_file);
EXPECT_TRUE(options_.is_remote_mode);
EXPECT_FALSE(options_.do_vibrate);
- EXPECT_FALSE(options_.do_fb);
+ EXPECT_FALSE(options_.do_screenshot);
EXPECT_EQ(options_.dumpstate_hal_mode, DumpstateMode::REMOTE);
// Other options retain default values
@@ -284,7 +285,7 @@
TEST_F(DumpOptionsTest, InitializeWearBugReport) {
options_.Initialize(Dumpstate::BugreportMode::BUGREPORT_WEAR, fd, fd);
EXPECT_TRUE(options_.do_add_date);
- EXPECT_TRUE(options_.do_fb);
+ EXPECT_TRUE(options_.do_screenshot);
EXPECT_TRUE(options_.do_zip_file);
EXPECT_TRUE(options_.do_progress_updates);
EXPECT_TRUE(options_.do_start_service);
@@ -301,7 +302,7 @@
TEST_F(DumpOptionsTest, InitializeTelephonyBugReport) {
options_.Initialize(Dumpstate::BugreportMode::BUGREPORT_TELEPHONY, fd, fd);
EXPECT_TRUE(options_.do_add_date);
- EXPECT_FALSE(options_.do_fb);
+ EXPECT_FALSE(options_.do_screenshot);
EXPECT_TRUE(options_.do_zip_file);
EXPECT_TRUE(options_.telephony_only);
EXPECT_TRUE(options_.do_progress_updates);
@@ -318,7 +319,7 @@
TEST_F(DumpOptionsTest, InitializeWifiBugReport) {
options_.Initialize(Dumpstate::BugreportMode::BUGREPORT_WIFI, fd, fd);
EXPECT_TRUE(options_.do_add_date);
- EXPECT_FALSE(options_.do_fb);
+ EXPECT_FALSE(options_.do_screenshot);
EXPECT_TRUE(options_.do_zip_file);
EXPECT_TRUE(options_.wifi_only);
EXPECT_EQ(options_.dumpstate_hal_mode, DumpstateMode::WIFI);
@@ -346,7 +347,7 @@
EXPECT_EQ(status, Dumpstate::RunStatus::OK);
EXPECT_TRUE(options_.do_add_date);
- EXPECT_TRUE(options_.do_fb);
+ EXPECT_TRUE(options_.do_screenshot);
EXPECT_TRUE(options_.do_zip_file);
EXPECT_EQ(options_.dumpstate_hal_mode, DumpstateMode::DEFAULT);
@@ -384,7 +385,7 @@
// Other options retain default values
EXPECT_FALSE(options_.show_header_only);
EXPECT_TRUE(options_.do_vibrate);
- EXPECT_FALSE(options_.do_fb);
+ EXPECT_FALSE(options_.do_screenshot);
EXPECT_FALSE(options_.do_progress_updates);
EXPECT_FALSE(options_.is_remote_mode);
EXPECT_EQ(options_.dumpstate_hal_mode, DumpstateMode::DEFAULT);
@@ -407,7 +408,7 @@
EXPECT_EQ(status, Dumpstate::RunStatus::OK);
EXPECT_TRUE(options_.show_header_only);
EXPECT_FALSE(options_.do_vibrate);
- EXPECT_TRUE(options_.do_fb);
+ EXPECT_TRUE(options_.do_screenshot);
EXPECT_TRUE(options_.do_progress_updates);
EXPECT_TRUE(options_.is_remote_mode);