Use weak pointer to store duplicate requests
Bug: 1423304
Change-Id: I7ab170f085c3d05c582f7065b88c1ad2510cc633
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/4365724
Commit-Queue: Thomas Nguyen <tungnh@chromium.org>
Reviewed-by: Andy Paicu <andypaicu@chromium.org>
Cr-Commit-Position: refs/heads/main@{#1124133}
diff --git a/components/permissions/permission_request.cc b/components/permissions/permission_request.cc
index 7f33022..10f2d44 100644
--- a/components/permissions/permission_request.cc
+++ b/components/permissions/permission_request.cc
@@ -36,6 +36,10 @@
requesting_origin() == other_request->requesting_origin();
}
+base::WeakPtr<PermissionRequest> PermissionRequest::GetWeakPtr() {
+ return weak_factory_.GetWeakPtr();
+}
+
#if BUILDFLAG(IS_ANDROID)
std::u16string PermissionRequest::GetDialogMessageText() const {
int message_id = 0;
diff --git a/components/permissions/permission_request.h b/components/permissions/permission_request.h
index d0c57a5..0328b53 100644
--- a/components/permissions/permission_request.h
+++ b/components/permissions/permission_request.h
@@ -8,6 +8,7 @@
#include <string>
#include "base/functional/callback.h"
+#include "base/memory/weak_ptr.h"
#include "build/build_config.h"
#include "components/content_settings/core/common/content_settings.h"
#include "components/content_settings/core/common/content_settings_types.h"
@@ -83,6 +84,9 @@
virtual std::u16string GetDialogMessageText() const;
#endif
+ // Returns a weak pointer to this instance.
+ base::WeakPtr<PermissionRequest> GetWeakPtr();
+
#if !BUILDFLAG(IS_ANDROID)
// Returns whether displaying a confirmation chip for the request is
// supported.
@@ -163,6 +167,8 @@
// Called when the request is no longer in use so it can be deleted by the
// caller.
base::OnceClosure delete_callback_;
+
+ base::WeakPtrFactory<PermissionRequest> weak_factory_{this};
};
} // namespace permissions
diff --git a/components/permissions/permission_request_manager.cc b/components/permissions/permission_request_manager.cc
index c151daa..bbb71cd 100644
--- a/components/permissions/permission_request_manager.cc
+++ b/components/permissions/permission_request_manager.cc
@@ -233,18 +233,25 @@
}
// Don't re-add an existing request or one with a duplicate text request.
- PermissionRequest* existing_request = GetExistingRequest(request);
- if (existing_request) {
+ if (auto* existing_request = GetExistingRequest(request)) {
+ if (request == existing_request) {
+ return;
+ }
+
// |request| is a duplicate. Add it to |duplicate_requests_| unless it's the
// same object as |existing_request| or an existing duplicate.
- if (request == existing_request)
+ auto iter = FindDuplicateRequestList(existing_request);
+ if (iter == duplicate_requests_.end()) {
+ duplicate_requests_.push_back({request->GetWeakPtr()});
return;
- auto range = duplicate_requests_.equal_range(existing_request);
- for (auto it = range.first; it != range.second; ++it) {
- if (request == it->second)
- return;
}
- duplicate_requests_.insert(std::make_pair(existing_request, request));
+
+ for (const auto& weak_request : (*iter)) {
+ if (weak_request && request == weak_request.get()) {
+ return;
+ }
+ }
+ iter->push_back(request->GetWeakPtr());
return;
}
@@ -1050,12 +1057,60 @@
PermissionRequest* PermissionRequestManager::GetExistingRequest(
PermissionRequest* request) const {
for (PermissionRequest* existing_request : requests_) {
- if (request->IsDuplicateOf(existing_request))
+ if (request->IsDuplicateOf(existing_request)) {
return existing_request;
+ }
}
return pending_permission_requests_.FindDuplicate(request);
}
+PermissionRequestManager::WeakPermissionRequestList::iterator
+PermissionRequestManager::FindDuplicateRequestList(PermissionRequest* request) {
+ for (auto request_list = duplicate_requests_.begin();
+ request_list != duplicate_requests_.end(); ++request_list) {
+ for (auto iter = request_list->begin(); iter != request_list->end();) {
+ // Remove any requests that have been destroyed.
+ const auto& weak_request = (*iter);
+ if (!weak_request) {
+ iter = request_list->erase(iter);
+ continue;
+ }
+
+ // The first valid request in the list will indicate whether all other
+ // members are duplicate or not.
+ if (weak_request->IsDuplicateOf(request)) {
+ return request_list;
+ }
+
+ break;
+ }
+ }
+
+ return duplicate_requests_.end();
+}
+
+PermissionRequestManager::WeakPermissionRequestList::iterator
+PermissionRequestManager::VisitDuplicateRequests(
+ DuplicateRequestVisitor visitor,
+ PermissionRequest* request) {
+ auto request_list = FindDuplicateRequestList(request);
+ if (request_list == duplicate_requests_.end()) {
+ return request_list;
+ }
+
+ for (auto iter = request_list->begin(); iter != request_list->end();) {
+ if (auto& weak_request = (*iter)) {
+ visitor.Run(weak_request);
+ ++iter;
+ } else {
+ // Remove any requests that have been destroyed.
+ iter = request_list->erase(iter);
+ }
+ }
+
+ return request_list;
+}
+
void PermissionRequestManager::PermissionGrantedIncludingDuplicates(
PermissionRequest* request,
bool is_one_time) {
@@ -1063,9 +1118,14 @@
pending_permission_requests_.Count(request))
<< "Only requests in [pending_permission_]requests_ can have duplicates";
request->PermissionGranted(is_one_time);
- auto range = duplicate_requests_.equal_range(request);
- for (auto it = range.first; it != range.second; ++it)
- it->second->PermissionGranted(is_one_time);
+ VisitDuplicateRequests(
+ base::BindRepeating(
+ [](bool is_one_time,
+ const base::WeakPtr<PermissionRequest>& weak_request) {
+ weak_request->PermissionGranted(is_one_time);
+ },
+ is_one_time),
+ request);
}
void PermissionRequestManager::PermissionDeniedIncludingDuplicates(
@@ -1074,9 +1134,12 @@
pending_permission_requests_.Count(request))
<< "Only requests in [pending_permission_]requests_ can have duplicates";
request->PermissionDenied();
- auto range = duplicate_requests_.equal_range(request);
- for (auto it = range.first; it != range.second; ++it)
- it->second->PermissionDenied();
+ VisitDuplicateRequests(
+ base::BindRepeating(
+ [](const base::WeakPtr<PermissionRequest>& weak_request) {
+ weak_request->PermissionDenied();
+ }),
+ request);
}
void PermissionRequestManager::CancelledIncludingDuplicates(
@@ -1086,9 +1149,14 @@
pending_permission_requests_.Count(request))
<< "Only requests in [pending_permission_]requests_ can have duplicates";
request->Cancelled(is_final_decision);
- auto range = duplicate_requests_.equal_range(request);
- for (auto it = range.first; it != range.second; ++it)
- it->second->Cancelled(is_final_decision);
+ VisitDuplicateRequests(
+ base::BindRepeating(
+ [](bool is_final,
+ const base::WeakPtr<PermissionRequest>& weak_request) {
+ weak_request->Cancelled(is_final);
+ },
+ is_final_decision),
+ request);
}
void PermissionRequestManager::RequestFinishedIncludingDuplicates(
@@ -1096,13 +1164,21 @@
DCHECK_EQ(1ul, base::ranges::count(requests_, request) +
pending_permission_requests_.Count(request))
<< "Only requests in [pending_permission_]requests_ can have duplicates";
+ auto duplicate_list = VisitDuplicateRequests(
+ base::BindRepeating(
+ [](const base::WeakPtr<PermissionRequest>& weak_request) {
+ weak_request->RequestFinished();
+ }),
+ request);
+
+ // Note: beyond this point, |request| has probably been deleted, any
+ // dereference of |request| must be done prior this point.
request->RequestFinished();
- // Beyond this point, |request| has probably been deleted.
- auto range = duplicate_requests_.equal_range(request);
- for (auto it = range.first; it != range.second; ++it)
- it->second->RequestFinished();
+
// Additionally, we can now remove the duplicates.
- duplicate_requests_.erase(request);
+ if (duplicate_list != duplicate_requests_.end()) {
+ duplicate_requests_.erase(duplicate_list);
+ }
}
void PermissionRequestManager::AddObserver(Observer* observer) {
diff --git a/components/permissions/permission_request_manager.h b/components/permissions/permission_request_manager.h
index 226b91cc..f60017c 100644
--- a/components/permissions/permission_request_manager.h
+++ b/components/permissions/permission_request_manager.h
@@ -254,6 +254,9 @@
private:
friend class test::PermissionRequestManagerTestApi;
friend class content::WebContentsUserData<PermissionRequestManager>;
+ FRIEND_TEST_ALL_PREFIXES(PermissionRequestManagerTest, WeakDuplicateRequests);
+ FRIEND_TEST_ALL_PREFIXES(PermissionRequestManagerTest,
+ WeakDuplicateRequestsAccept);
explicit PermissionRequestManager(content::WebContents* web_contents);
@@ -328,6 +331,25 @@
// may or may not be the same object as |request|.
PermissionRequest* GetExistingRequest(PermissionRequest* request) const;
+ // Returns an iterator into |duplicate_requests_|, points the matching list,
+ // or duplicate_requests_.end() if no match. The matching list contains all
+ // the weak requests which are duplicate of the given |request| (see
+ // |IsDuplicateOf|)
+ using WeakPermissionRequestList =
+ std::list<std::list<base::WeakPtr<PermissionRequest>>>;
+ WeakPermissionRequestList::iterator FindDuplicateRequestList(
+ PermissionRequest* request);
+
+ // Trigger |visitor| for each live weak request which matches the given
+ // request (see |IsDuplicateOf|) in the |duplicate_requests_|. Returns an
+ // iterator into |duplicate_requests_|, points the matching list, or
+ // duplicate_requests_.end() if no match.
+ using DuplicateRequestVisitor =
+ base::RepeatingCallback<void(const base::WeakPtr<PermissionRequest>&)>;
+ WeakPermissionRequestList::iterator VisitDuplicateRequests(
+ DuplicateRequestVisitor visitor,
+ PermissionRequest* request);
+
// Calls PermissionGranted on a request and all its duplicates.
void PermissionGrantedIncludingDuplicates(PermissionRequest* request,
bool is_one_time);
@@ -393,10 +415,8 @@
PermissionRequestQueue pending_permission_requests_;
- // Maps from the first request of a kind to subsequent requests that were
- // duped against it.
- std::unordered_multimap<PermissionRequest*, PermissionRequest*>
- duplicate_requests_;
+ // Stores the weak pointers of duplicated requests in a 2D list.
+ WeakPermissionRequestList duplicate_requests_;
// Maps each PermissionRequest currently in |requests_| or
// |pending_permission_requests_| to which RenderFrameHost it originated from.
diff --git a/components/permissions/permission_request_manager_unittest.cc b/components/permissions/permission_request_manager_unittest.cc
index 32649a1..b23be4f9 100644
--- a/components/permissions/permission_request_manager_unittest.cc
+++ b/components/permissions/permission_request_manager_unittest.cc
@@ -11,6 +11,7 @@
#include "base/memory/raw_ptr.h"
#include "base/run_loop.h"
#include "base/task/sequenced_task_runner.h"
+#include "base/test/bind.h"
#include "base/test/metrics/histogram_tester.h"
#include "base/test/scoped_feature_list.h"
#include "build/build_config.h"
@@ -515,6 +516,90 @@
EXPECT_FALSE(prompt_factory_->is_visible());
}
+TEST_P(PermissionRequestManagerTest, WeakDuplicateRequests) {
+ manager_->AddRequest(web_contents()->GetPrimaryMainFrame(), &request1_);
+ WaitForBubbleToBeShown();
+ auto dupe_request_1 = request1_.CreateDuplicateRequest();
+ auto dupe_request_2 = request1_.CreateDuplicateRequest();
+ auto dupe_request_3 = request1_.CreateDuplicateRequest();
+ auto dupe_request_4 = request1_.CreateDuplicateRequest();
+ manager_->AddRequest(web_contents()->GetPrimaryMainFrame(),
+ dupe_request_1.get());
+ manager_->AddRequest(web_contents()->GetPrimaryMainFrame(),
+ dupe_request_2.get());
+ manager_->AddRequest(web_contents()->GetPrimaryMainFrame(),
+ dupe_request_3.get());
+ manager_->AddRequest(web_contents()->GetPrimaryMainFrame(),
+ dupe_request_4.get());
+ dupe_request_1.reset();
+ dupe_request_3.reset();
+ EXPECT_EQ(4ul, manager_->duplicate_requests_.front().size());
+ EXPECT_EQ(1ul, manager_->duplicate_requests_.size());
+ auto request_list = manager_->FindDuplicateRequestList(&request1_);
+ EXPECT_NE(request_list, manager_->duplicate_requests_.end());
+ EXPECT_EQ(3ul, manager_->duplicate_requests_.front().size());
+ dupe_request_4.reset();
+ manager_->VisitDuplicateRequests(
+ base::BindRepeating(
+ [](const base::WeakPtr<PermissionRequest>& weak_request) {}),
+ &request1_);
+ EXPECT_EQ(1ul, manager_->duplicate_requests_.front().size());
+ EXPECT_EQ(1ul, manager_->duplicate_requests_.size());
+ Accept();
+ EXPECT_EQ(0ul, manager_->duplicate_requests_.size());
+}
+
+class QuicklyDeletedRequest : public PermissionRequest {
+ public:
+ QuicklyDeletedRequest(const GURL& requesting_origin,
+ RequestType request_type,
+ PermissionRequestGestureType gesture_type)
+ : PermissionRequest(requesting_origin,
+ request_type,
+ gesture_type == PermissionRequestGestureType::GESTURE,
+ base::BindLambdaForTesting(
+ [](ContentSetting result,
+ bool is_one_time,
+ bool is_final_decision) { NOTREACHED(); }),
+ base::NullCallback()) {}
+
+ static std::unique_ptr<QuicklyDeletedRequest> CreateRequest(
+ MockPermissionRequest* request) {
+ return std::make_unique<QuicklyDeletedRequest>(request->requesting_origin(),
+ request->request_type(),
+ request->GetGestureType());
+ }
+};
+
+TEST_P(PermissionRequestManagerTest, WeakDuplicateRequestsAccept) {
+ manager_->AddRequest(web_contents()->GetPrimaryMainFrame(), &request1_);
+ WaitForBubbleToBeShown();
+ manager_->AddRequest(web_contents()->GetPrimaryMainFrame(), &request2_);
+ auto dupe_request_1 = QuicklyDeletedRequest::CreateRequest(&request1_);
+ auto dupe_request_2 = request1_.CreateDuplicateRequest();
+ manager_->AddRequest(web_contents()->GetPrimaryMainFrame(),
+ dupe_request_1.get());
+ manager_->AddRequest(web_contents()->GetPrimaryMainFrame(),
+ dupe_request_2.get());
+ auto dupe_request_3 = QuicklyDeletedRequest::CreateRequest(&request2_);
+ auto dupe_request_4 = request2_.CreateDuplicateRequest();
+ manager_->AddRequest(web_contents()->GetPrimaryMainFrame(),
+ dupe_request_3.get());
+ manager_->AddRequest(web_contents()->GetPrimaryMainFrame(),
+ dupe_request_4.get());
+ dupe_request_1.reset();
+ dupe_request_3.reset();
+ EXPECT_EQ(2ul, manager_->duplicate_requests_.size());
+ EXPECT_EQ(2ul, manager_->duplicate_requests_.front().size());
+ EXPECT_EQ(2ul, manager_->duplicate_requests_.back().size());
+ WaitForBubbleToBeShown();
+ Accept();
+ EXPECT_EQ(1ul, manager_->duplicate_requests_.size());
+ WaitForBubbleToBeShown();
+ Accept();
+ EXPECT_EQ(0ul, manager_->duplicate_requests_.size());
+}
+
TEST_P(PermissionRequestManagerTest, DuplicateRequest) {
manager_->AddRequest(web_contents()->GetPrimaryMainFrame(), &request1_);
WaitForBubbleToBeShown();
