M104: Use weak pointer to OfflinePageArchivePublisher as Unpublish() may run after publisher is destroyed
Also remove some unneeded includes.
(cherry picked from commit 1544702b7568494a9c5226d3dadf3dbf12e6e25a)
Bug: 1337798
Change-Id: I47097cbb7341fc731789991ada9e63c7513d8cc7
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/3733637
Reviewed-by: Dan H <harringtond@chromium.org>
Commit-Queue: Ian Wells <iwells@chromium.org>
Cr-Original-Commit-Position: refs/heads/main@{#1019208}
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/3738620
Commit-Queue: Dan H <harringtond@chromium.org>
Auto-Submit: Ian Wells <iwells@chromium.org>
Cr-Commit-Position: refs/branch-heads/5112@{#702}
Cr-Branched-From: b13d3fe7b3c47a56354ef54b221008afa754412e-refs/heads/main@{#1012729}
diff --git a/chrome/browser/offline_pages/android/offline_page_archive_publisher_impl.cc b/chrome/browser/offline_pages/android/offline_page_archive_publisher_impl.cc
index 1d560c1c..d763e9e 100644
--- a/chrome/browser/offline_pages/android/offline_page_archive_publisher_impl.cc
+++ b/chrome/browser/offline_pages/android/offline_page_archive_publisher_impl.cc
@@ -21,6 +21,7 @@
#include "chrome/browser/offline_pages/android/offline_page_bridge.h"
#include "components/offline_pages/core/archive_manager.h"
#include "components/offline_pages/core/model/offline_page_model_utils.h"
+#include "components/offline_pages/core/offline_page_archive_publisher.h"
#include "components/offline_pages/core/offline_store_utils.h"
namespace offline_pages {
@@ -203,4 +204,9 @@
return Java_OfflinePageArchivePublisherBridge_remove(env, j_ids);
}
+base::WeakPtr<OfflinePageArchivePublisher>
+OfflinePageArchivePublisherImpl::GetWeakPtr() {
+ return weak_ptr_factory_.GetWeakPtr();
+}
+
} // namespace offline_pages
diff --git a/chrome/browser/offline_pages/android/offline_page_archive_publisher_impl.h b/chrome/browser/offline_pages/android/offline_page_archive_publisher_impl.h
index de988ea..b356648 100644
--- a/chrome/browser/offline_pages/android/offline_page_archive_publisher_impl.h
+++ b/chrome/browser/offline_pages/android/offline_page_archive_publisher_impl.h
@@ -7,11 +7,9 @@
#include <cstdint>
-#include "base/callback.h"
#include "base/memory/raw_ptr.h"
#include "components/offline_pages/core/offline_page_archive_publisher.h"
#include "components/offline_pages/core/offline_page_item.h"
-#include "components/offline_pages/core/offline_page_types.h"
namespace base {
class SequencedTaskRunner;
@@ -57,9 +55,12 @@
void UnpublishArchives(
const std::vector<PublishedArchiveId>& publish_ids) const override;
+ base::WeakPtr<OfflinePageArchivePublisher> GetWeakPtr() override;
+
private:
raw_ptr<ArchiveManager> archive_manager_;
raw_ptr<Delegate> delegate_;
+ base::WeakPtrFactory<OfflinePageArchivePublisher> weak_ptr_factory_{this};
};
} // namespace offline_pages
diff --git a/components/offline_pages/core/model/offline_page_model_taskified.cc b/components/offline_pages/core/model/offline_page_model_taskified.cc
index db2bdd8..8223668 100644
--- a/components/offline_pages/core/model/offline_page_model_taskified.cc
+++ b/components/offline_pages/core/model/offline_page_model_taskified.cc
@@ -32,8 +32,8 @@
#include "components/offline_pages/core/model/update_publish_id_task.h"
#include "components/offline_pages/core/model/visuals_availability_task.h"
#include "components/offline_pages/core/offline_clock.h"
+#include "components/offline_pages/core/offline_page_archive_publisher.h"
#include "components/offline_pages/core/offline_page_client_policy.h"
-#include "components/offline_pages/core/offline_page_feature.h"
#include "components/offline_pages/core/offline_page_metadata_store.h"
#include "components/offline_pages/core/offline_page_model.h"
#include "components/offline_pages/core/offline_store_utils.h"
@@ -186,7 +186,7 @@
CreateArchivesDirectoryIfNeeded();
}
-OfflinePageModelTaskified::~OfflinePageModelTaskified() {}
+OfflinePageModelTaskified::~OfflinePageModelTaskified() = default;
void OfflinePageModelTaskified::AddObserver(Observer* observer) {
observers_.AddObserver(observer);
@@ -582,9 +582,9 @@
// Remove the page from the system download manager. We don't need to wait for
// completion before calling the delete page callback.
- task_runner_->PostTask(FROM_HERE,
- base::BindOnce(&OfflinePageModelTaskified::Unpublish,
- archive_publisher_.get(), publish_ids));
+ task_runner_->PostTask(
+ FROM_HERE, base::BindOnce(&OfflinePageModelTaskified::Unpublish,
+ archive_publisher_->GetWeakPtr(), publish_ids));
if (!callback.is_null())
std::move(callback).Run(result);
@@ -609,9 +609,9 @@
}
void OfflinePageModelTaskified::Unpublish(
- OfflinePageArchivePublisher* publisher,
+ base::WeakPtr<OfflinePageArchivePublisher> publisher,
const std::vector<PublishedArchiveId>& publish_ids) {
- if (!publish_ids.empty())
+ if (publisher && !publish_ids.empty())
publisher->UnpublishArchives(publish_ids);
}
@@ -663,7 +663,7 @@
void OfflinePageModelTaskified::OnPersistentPageConsistencyCheckDone(
bool success,
const std::vector<PublishedArchiveId>& ids_of_deleted_pages) {
- Unpublish(archive_publisher_.get(), ids_of_deleted_pages);
+ Unpublish(archive_publisher_->GetWeakPtr(), ids_of_deleted_pages);
}
void OfflinePageModelTaskified::OnClearCachedPagesDone(
diff --git a/components/offline_pages/core/model/offline_page_model_taskified.h b/components/offline_pages/core/model/offline_page_model_taskified.h
index 385021c6..f94e8ca 100644
--- a/components/offline_pages/core/model/offline_page_model_taskified.h
+++ b/components/offline_pages/core/model/offline_page_model_taskified.h
@@ -10,18 +10,14 @@
#include <string>
#include <vector>
-#include "base/callback.h"
-#include "base/memory/ref_counted.h"
#include "base/memory/weak_ptr.h"
#include "base/observer_list.h"
-#include "components/keyed_service/core/keyed_service.h"
#include "components/offline_pages/core/model/clear_storage_task.h"
#include "components/offline_pages/core/offline_page_archive_publisher.h"
#include "components/offline_pages/core/offline_page_archiver.h"
#include "components/offline_pages/core/offline_page_model.h"
#include "components/offline_pages/core/offline_page_model_event_logger.h"
#include "components/offline_pages/core/offline_page_types.h"
-#include "components/offline_pages/core/offline_store_types.h"
#include "components/offline_pages/task/task_queue.h"
class GURL;
@@ -185,7 +181,7 @@
PublishArchiveResult publish_results);
// Method for unpublishing the page from downloads.
- static void Unpublish(OfflinePageArchivePublisher* publisher,
+ static void Unpublish(base::WeakPtr<OfflinePageArchivePublisher> publisher,
const std::vector<PublishedArchiveId>& publish_ids);
// Other utility methods.
diff --git a/components/offline_pages/core/offline_page_archive_publisher.h b/components/offline_pages/core/offline_page_archive_publisher.h
index cea4b63..0088402 100644
--- a/components/offline_pages/core/offline_page_archive_publisher.h
+++ b/components/offline_pages/core/offline_page_archive_publisher.h
@@ -7,7 +7,6 @@
#include <cstdint>
-#include "base/callback.h"
#include "base/files/file_path.h"
#include "components/offline_pages/core/offline_page_item.h"
#include "components/offline_pages/core/offline_page_types.h"
@@ -60,7 +59,7 @@
base::OnceCallback<void(const OfflinePageItem& /* offline_page */,
PublishArchiveResult /* archive_result */)>;
- virtual ~OfflinePageArchivePublisher() {}
+ virtual ~OfflinePageArchivePublisher() = default;
// Publishes the page on a background thread, then returns to the
// OfflinePageModelTaskified's done callback.
@@ -72,6 +71,8 @@
// Removes archives from downloads.
virtual void UnpublishArchives(
const std::vector<PublishedArchiveId>& archive_ids) const = 0;
+
+ virtual base::WeakPtr<OfflinePageArchivePublisher> GetWeakPtr() = 0;
};
} // namespace offline_pages
diff --git a/components/offline_pages/core/offline_page_test_archive_publisher.cc b/components/offline_pages/core/offline_page_test_archive_publisher.cc
index 7b6128b..e5d61622 100644
--- a/components/offline_pages/core/offline_page_test_archive_publisher.cc
+++ b/components/offline_pages/core/offline_page_test_archive_publisher.cc
@@ -8,12 +8,10 @@
#include <utility>
#include "base/bind.h"
-#include "base/files/file_util.h"
#include "base/location.h"
#include "base/task/single_thread_task_runner.h"
#include "components/offline_pages/core/archive_manager.h"
#include "testing/gtest/include/gtest/gtest.h"
-#include "url/gurl.h"
namespace offline_pages {
@@ -69,4 +67,9 @@
last_removed_id_ = archive_ids.back();
}
+base::WeakPtr<OfflinePageArchivePublisher>
+OfflinePageTestArchivePublisher::GetWeakPtr() {
+ return weak_ptr_factory_.GetWeakPtr();
+}
+
} // namespace offline_pages
diff --git a/components/offline_pages/core/offline_page_test_archive_publisher.h b/components/offline_pages/core/offline_page_test_archive_publisher.h
index f796016..1ec17695 100644
--- a/components/offline_pages/core/offline_page_test_archive_publisher.h
+++ b/components/offline_pages/core/offline_page_test_archive_publisher.h
@@ -7,11 +7,10 @@
#include <cstdint>
-#include "base/callback.h"
#include "base/memory/raw_ptr.h"
+#include "base/memory/weak_ptr.h"
#include "components/offline_pages/core/offline_page_archive_publisher.h"
#include "components/offline_pages/core/offline_page_item.h"
-#include "components/offline_pages/core/offline_page_types.h"
namespace base {
class SequencedTaskRunner;
@@ -35,6 +34,8 @@
void UnpublishArchives(
const std::vector<PublishedArchiveId>& archive_ids) const override;
+ base::WeakPtr<OfflinePageArchivePublisher> GetWeakPtr() override;
+
void set_archive_attempt_failure(bool fail) {
archive_attempt_failure_ = fail;
}
@@ -56,6 +57,7 @@
mutable PublishedArchiveId last_removed_id_;
raw_ptr<ArchiveManager> archive_manager_;
+ base::WeakPtrFactory<OfflinePageArchivePublisher> weak_ptr_factory_{this};
};
} // namespace offline_pages