From 9f4b16fa810cb9a43f448d2f81fd81f58af240dc Mon Sep 17 00:00:00 2001 From: "trop[bot]" <37223003+trop[bot]@users.noreply.github.com> Date: Sun, 8 Jun 2025 12:56:35 +0200 Subject: [PATCH] fix: rework lifetime mgmt of `ClearDataTask`/`ClearDataOperation` (#47412) * fix: rework lifetime mgmt of ClearDataTask/ClearDataOperation Co-authored-by: Shelley Vohr * Update shell/browser/api/electron_api_session.cc Co-authored-by: Robo Co-authored-by: Shelley Vohr * Update shell/browser/api/electron_api_session.cc Co-authored-by: Robo Co-authored-by: Shelley Vohr * Update shell/browser/api/electron_api_session.cc Co-authored-by: Robo Co-authored-by: Shelley Vohr * Update shell/browser/api/electron_api_session.cc Co-authored-by: Robo Co-authored-by: Shelley Vohr --------- Co-authored-by: trop[bot] <37223003+trop[bot]@users.noreply.github.com> Co-authored-by: Shelley Vohr --- shell/browser/api/electron_api_session.cc | 64 ++++++++++++++--------- 1 file changed, 38 insertions(+), 26 deletions(-) diff --git a/shell/browser/api/electron_api_session.cc b/shell/browser/api/electron_api_session.cc index dcb89142e73f..563d63aa95e7 100644 --- a/shell/browser/api/electron_api_session.cc +++ b/shell/browser/api/electron_api_session.cc @@ -84,6 +84,7 @@ #include "shell/common/gin_converters/time_converter.h" #include "shell/common/gin_converters/usb_protected_classes_converter.h" #include "shell/common/gin_converters/value_converter.h" +#include "shell/common/gin_helper/cleaned_up_at_exit.h" #include "shell/common/gin_helper/dictionary.h" #include "shell/common/gin_helper/error_thrower.h" #include "shell/common/gin_helper/object_template_builder.h" @@ -198,9 +199,11 @@ std::vector GetDataTypesFromMask( // Represents a task to clear browsing data for the `clearData` API method. // -// This type manages its own lifetime, deleting itself once the task finishes -// completely. -class ClearDataTask { +// This type manages its own lifetime, +// 1) deleting itself once all the operations created by this task are +// completed. 2) through gin_helper::CleanedUpAtExit, ensuring it's destroyed +// before the node environment shuts down. +class ClearDataTask : public gin_helper::CleanedUpAtExit { public: // Starts running a task. This function will return before the task is // finished, but will resolve or reject the |promise| when it finishes. @@ -211,7 +214,7 @@ class ClearDataTask { std::vector origins, BrowsingDataFilterBuilder::Mode filter_mode, BrowsingDataFilterBuilder::OriginMatchingMode origin_matching_mode) { - std::shared_ptr task(new ClearDataTask(std::move(promise))); + auto* task = new ClearDataTask(std::move(promise)); // This method counts as an operation. This is important so we can call // `OnOperationFinished` at the end of this method as a fallback if all the @@ -255,42 +258,36 @@ class ClearDataTask { } // This static method counts as an operation. - task->OnOperationFinished(std::nullopt); + task->OnOperationFinished(nullptr, std::nullopt); } private: // An individual |content::BrowsingDataRemover::Remove...| operation as part - // of a full |ClearDataTask|. This class manages its own lifetime, cleaning - // itself up after the operation completes and notifies the task of the - // result. + // of a full |ClearDataTask|. This class is owned by ClearDataTask and cleaned + // up either when the operation completes or when ClearDataTask is destroyed. class ClearDataOperation : private BrowsingDataRemover::Observer { public: - static void Run(std::shared_ptr task, - BrowsingDataRemover* remover, - BrowsingDataRemover::DataType data_type_mask, - std::unique_ptr filter_builder) { - auto* operation = new ClearDataOperation(task, remover); + ClearDataOperation(ClearDataTask* task, BrowsingDataRemover* remover) + : task_(task) { + observation_.Observe(remover); + } + void Start(BrowsingDataRemover* remover, + BrowsingDataRemover::DataType data_type_mask, + std::unique_ptr filter_builder) { remover->RemoveWithFilterAndReply(base::Time::Min(), base::Time::Max(), data_type_mask, kClearOriginTypeAll, - std::move(filter_builder), operation); + std::move(filter_builder), this); } // BrowsingDataRemover::Observer: void OnBrowsingDataRemoverDone( BrowsingDataRemover::DataType failed_data_types) override { - task_->OnOperationFinished(failed_data_types); - delete this; + task_->OnOperationFinished(this, failed_data_types); } private: - ClearDataOperation(std::shared_ptr task, - BrowsingDataRemover* remover) - : task_(task) { - observation_.Observe(remover); - } - - std::shared_ptr task_; + raw_ptr task_; base::ScopedObservation observation_{this}; }; @@ -299,18 +296,20 @@ class ClearDataTask { : promise_(std::move(promise)) {} static void StartOperation( - std::shared_ptr task, + ClearDataTask* task, BrowsingDataRemover* remover, BrowsingDataRemover::DataType data_type_mask, std::unique_ptr filter_builder) { // Track this operation task->operations_running_ += 1; - ClearDataOperation::Run(task, remover, data_type_mask, - std::move(filter_builder)); + auto& operation = task->operations_.emplace_back( + std::make_unique(task, remover)); + operation->Start(remover, data_type_mask, std::move(filter_builder)); } void OnOperationFinished( + ClearDataOperation* operation, std::optional failed_data_types) { DCHECK_GT(operations_running_, 0); operations_running_ -= 1; @@ -319,6 +318,16 @@ class ClearDataTask { failed_data_types_ |= failed_data_types.value(); } + if (operation) { + operations_.erase( + std::remove_if( + operations_.begin(), operations_.end(), + [operation](const std::unique_ptr& op) { + return op.get() == operation; + }), + operations_.end()); + } + // If this is the last operation, then the task is finished if (operations_running_ == 0) { OnTaskFinished(); @@ -346,11 +355,14 @@ class ClearDataTask { promise_.Reject(error); } + + delete this; } int operations_running_ = 0; BrowsingDataRemover::DataType failed_data_types_ = 0ULL; gin_helper::Promise promise_; + std::vector> operations_; }; base::Value::Dict createProxyConfig(ProxyPrefs::ProxyMode proxy_mode,