From a407fb81372c6571c28d603504b23ece2d6bc79e Mon Sep 17 00:00:00 2001 From: Abhijeet Kaur Date: Fri, 27 Mar 2020 12:51:12 +0000 Subject: Update Last ID early in the bugreport generation The lifecycle of a bugreport triggered by Shell: 1. Triggers a bugreport using bugreport API. At this point shell does not have any state of the bugreport. mBugreportInfos is the sparse array in Shell that tracks bugreportInfos keyed with the ID. The ID of the bugreport is currently incremented in RunInternal() which is called in a different thread triggered by the DumpstateService (for an API call) 2. dumpstate calls one of the callbacks #onProgress, #onError and #onFinished(), it is then that the corresponding bugreportinfo is tracked in mBugreportInfos keyed with the ID. Problem: Consider a case when 2 bugreports are triggered such that Shell has not received #onProgress for the first one, and received #onError for the second one. This results in a race condition as Shell assumes that the first bugreport has called onError. This race condition occurs due to the fact that Shell depends on the callback to track the bugreports. Solution: Update Id at a definite time (not in some other thread) in Initialize so that the service calling the API can track it right away. Bug: 152292912 Test: Tigger consecutive calls to bugreport from ActivityManager is WAI Test: adb bugreport (open the finished bugreport and check that the correct ID is shown) Test: atest dumpstate_test Change-Id: I63966800725d2aaa1d1d9d6eb997b86d564acf44 --- cmds/dumpstate/dumpstate.cpp | 17 ++++++++++------- 1 file changed, 10 insertions(+), 7 deletions(-) (limited to 'cmds/dumpstate/dumpstate.cpp') diff --git a/cmds/dumpstate/dumpstate.cpp b/cmds/dumpstate/dumpstate.cpp index 772b9fe069..b475c90c04 100644 --- a/cmds/dumpstate/dumpstate.cpp +++ b/cmds/dumpstate/dumpstate.cpp @@ -2395,6 +2395,13 @@ void Dumpstate::SetOptions(std::unique_ptr options) { options_ = std::move(options); } +void Dumpstate::Initialize() { + /* gets the sequential id */ + uint32_t last_id = android::base::GetIntProperty(PROPERTY_LAST_ID, 0); + id_ = ++last_id; + android::base::SetProperty(PROPERTY_LAST_ID, std::to_string(last_id)); +} + Dumpstate::RunStatus Dumpstate::Run(int32_t calling_uid, const std::string& calling_package) { Dumpstate::RunStatus status = RunInternal(calling_uid, calling_package); if (listener_ != nullptr) { @@ -2505,11 +2512,6 @@ Dumpstate::RunStatus Dumpstate::RunInternal(int32_t calling_uid, : ""; progress_.reset(new Progress(stats_path)); - /* gets the sequential id */ - uint32_t last_id = android::base::GetIntProperty(PROPERTY_LAST_ID, 0); - id_ = ++last_id; - android::base::SetProperty(PROPERTY_LAST_ID, std::to_string(last_id)); - if (acquire_wake_lock(PARTIAL_WAKE_LOCK, WAKE_LOCK_NAME) < 0) { MYLOGE("Failed to acquire wake lock: %s\n", strerror(errno)); } else { @@ -2837,8 +2839,9 @@ Dumpstate::RunStatus Dumpstate::ParseCommandlineAndRun(int argc, char* argv[]) { assert(options_->bugreport_fd.get() == -1); // calling_uid and calling_package are for user consent to share the bugreport with - // an app; they are irrelvant here because bugreport is only written to a local - // directory, and not shared. + // an app; they are irrelevant here because bugreport is triggered via command line. + // Update Last ID before calling Run(). + Initialize(); status = Run(-1 /* calling_uid */, "" /* calling_package */); } return status; -- cgit v1.2.3-59-g8ed1b