CleanupPreviousUpdateAction: cancel pending tasks on destroy
Maintain a variable that tracks the task ID of the pending
task in the message loop. On destroy, cancel the pending
task to avoid segfault.
Test: during merge, call update_engine_client --cancel and
it does not crash
Fixes: 169436297
Change-Id: I9f3bccc5d4d5e2aaad1d08ef815c50100220c870
diff --git a/cleanup_previous_update_action.cc b/cleanup_previous_update_action.cc
index 3a7a750..89ed6f8 100644
--- a/cleanup_previous_update_action.cc
+++ b/cleanup_previous_update_action.cc
@@ -67,6 +67,10 @@
last_percentage_(0),
merge_stats_(nullptr) {}
+CleanupPreviousUpdateAction::~CleanupPreviousUpdateAction() {
+ StopActionInternal();
+}
+
void CleanupPreviousUpdateAction::PerformAction() {
StartActionInternal();
}
@@ -97,9 +101,44 @@
return "CleanupPreviousUpdateAction";
}
+// This function is called at the beginning of all delayed functions. By
+// resetting |scheduled_task_|, the delayed function acknowledges that the task
+// has already been executed, therefore there's no need to cancel it in the
+// future. This avoids StopActionInternal() from resetting task IDs in an
+// unexpected way because task IDs could be reused.
+void CleanupPreviousUpdateAction::AcknowledgeTaskExecuted() {
+ if (scheduled_task_ != MessageLoop::kTaskIdNull) {
+ LOG(INFO) << "Executing task " << scheduled_task_;
+ }
+ scheduled_task_ = MessageLoop::kTaskIdNull;
+}
+
+// Check that scheduled_task_ is a valid task ID. Otherwise, terminate the
+// action.
+void CleanupPreviousUpdateAction::CheckTaskScheduled(std::string_view name) {
+ if (scheduled_task_ == MessageLoop::kTaskIdNull) {
+ LOG(ERROR) << "Unable to schedule " << name;
+ processor_->ActionComplete(this, ErrorCode::kError);
+ } else {
+ LOG(INFO) << "CleanupPreviousUpdateAction scheduled task ID "
+ << scheduled_task_ << " for " << name;
+ }
+}
+
void CleanupPreviousUpdateAction::StopActionInternal() {
LOG(INFO) << "Stopping/suspending/completing CleanupPreviousUpdateAction";
running_ = false;
+
+ if (scheduled_task_ != MessageLoop::kTaskIdNull) {
+ if (MessageLoop::current()->CancelTask(scheduled_task_)) {
+ LOG(INFO) << "CleanupPreviousUpdateAction cancelled pending task ID "
+ << scheduled_task_;
+ } else {
+ LOG(ERROR) << "CleanupPreviousUpdateAction unable to cancel task ID "
+ << scheduled_task_;
+ }
+ }
+ scheduled_task_ = MessageLoop::kTaskIdNull;
}
void CleanupPreviousUpdateAction::StartActionInternal() {
@@ -124,14 +163,16 @@
void CleanupPreviousUpdateAction::ScheduleWaitBootCompleted() {
TEST_AND_RETURN(running_);
- MessageLoop::current()->PostDelayedTask(
+ scheduled_task_ = MessageLoop::current()->PostDelayedTask(
FROM_HERE,
base::Bind(&CleanupPreviousUpdateAction::WaitBootCompletedOrSchedule,
base::Unretained(this)),
kCheckBootCompletedInterval);
+ CheckTaskScheduled("WaitBootCompleted");
}
void CleanupPreviousUpdateAction::WaitBootCompletedOrSchedule() {
+ AcknowledgeTaskExecuted();
TEST_AND_RETURN(running_);
if (!kIsRecovery &&
!android::base::GetBoolProperty(kBootCompletedProp, false)) {
@@ -146,15 +187,17 @@
void CleanupPreviousUpdateAction::ScheduleWaitMarkBootSuccessful() {
TEST_AND_RETURN(running_);
- MessageLoop::current()->PostDelayedTask(
+ scheduled_task_ = MessageLoop::current()->PostDelayedTask(
FROM_HERE,
base::Bind(
&CleanupPreviousUpdateAction::CheckSlotMarkedSuccessfulOrSchedule,
base::Unretained(this)),
kCheckSlotMarkedSuccessfulInterval);
+ CheckTaskScheduled("WaitMarkBootSuccessful");
}
void CleanupPreviousUpdateAction::CheckSlotMarkedSuccessfulOrSchedule() {
+ AcknowledgeTaskExecuted();
TEST_AND_RETURN(running_);
if (!kIsRecovery &&
!boot_control_->IsSlotMarkedSuccessful(boot_control_->GetCurrentSlot())) {
@@ -216,14 +259,16 @@
void CleanupPreviousUpdateAction::ScheduleWaitForMerge() {
TEST_AND_RETURN(running_);
- MessageLoop::current()->PostDelayedTask(
+ scheduled_task_ = MessageLoop::current()->PostDelayedTask(
FROM_HERE,
base::Bind(&CleanupPreviousUpdateAction::WaitForMergeOrSchedule,
base::Unretained(this)),
kWaitForMergeInterval);
+ CheckTaskScheduled("WaitForMerge");
}
void CleanupPreviousUpdateAction::WaitForMergeOrSchedule() {
+ AcknowledgeTaskExecuted();
TEST_AND_RETURN(running_);
auto state = snapshot_->ProcessUpdateState(
std::bind(&CleanupPreviousUpdateAction::OnMergePercentageUpdate, this),
diff --git a/cleanup_previous_update_action.h b/cleanup_previous_update_action.h
index 1d8d3d7..fe65e60 100644
--- a/cleanup_previous_update_action.h
+++ b/cleanup_previous_update_action.h
@@ -20,6 +20,7 @@
#include <chrono> // NOLINT(build/c++11) -- for merge times
#include <memory>
#include <string>
+#include <string_view>
#include <brillo/message_loops/message_loop.h>
#include <libsnapshot/snapshot.h>
@@ -51,6 +52,7 @@
BootControlInterface* boot_control,
android::snapshot::ISnapshotManager* snapshot,
CleanupPreviousUpdateActionDelegateInterface* delegate);
+ ~CleanupPreviousUpdateAction();
void PerformAction() override;
void SuspendAction() override;
@@ -74,6 +76,11 @@
bool cancel_failed_{false};
unsigned int last_percentage_{0};
android::snapshot::ISnapshotMergeStats* merge_stats_;
+ brillo::MessageLoop::TaskId scheduled_task_{brillo::MessageLoop::kTaskIdNull};
+
+ // Helpers for task management.
+ void AcknowledgeTaskExecuted();
+ void CheckTaskScheduled(std::string_view name);
void StopActionInternal();
void StartActionInternal();