358 lines
16 KiB
Diff
358 lines
16 KiB
Diff
|
From f427936d32dbe1e9c27c0bcf54eff6818bddb906 Mon Sep 17 00:00:00 2001
|
||
|
From: Daniel Cheng <dcheng@chromium.org>
|
||
|
Date: Tue, 14 Jun 2022 19:11:17 +0000
|
||
|
Subject: [PATCH 14/59] [M102] Ensure raw_ptr<T> and T* are treated identically
|
||
|
in //base callback.
|
||
|
|
||
|
There are safety checks associated with raw pointers (e.g. ensuring
|
||
|
receiver pointers are not raw pointers). Make sure these checks are
|
||
|
applied whether the input type is T* or raw_ptr<T>.
|
||
|
|
||
|
- Implement base::IsPointer<T> and base::RemovePointer<T>, which are
|
||
|
similar to std::is_pointer<T> and std::remove_pointer<T>, except they
|
||
|
also consider raw_ptr<T> a raw pointer type.
|
||
|
- Fix failures from the strengthened asserts: WebAppInstallFinalizer
|
||
|
does not need a callback at all, while the privacy sandbox dialog
|
||
|
tests can safely use base::Unretained().
|
||
|
- Add test cases to cover this in the //base callback nocompile test
|
||
|
suite.
|
||
|
- Fix the existing nocompile tests, which did not escape `||` and
|
||
|
inadvertently matched any error text.
|
||
|
|
||
|
(cherry picked from commit 00c072a2c7f24921af3bbf8441abb34ecb0551a6)
|
||
|
|
||
|
Bug: 1335458
|
||
|
Change-Id: I470e3d5bc35ed52bf125136db738a868ef90b7e7
|
||
|
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/3700700
|
||
|
Reviewed-by: Lei Zhang <thestig@chromium.org>
|
||
|
Commit-Queue: Daniel Cheng <dcheng@chromium.org>
|
||
|
Cr-Original-Commit-Position: refs/heads/main@{#1013266}
|
||
|
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/3703779
|
||
|
Cr-Commit-Position: refs/branch-heads/5005@{#1173}
|
||
|
Cr-Branched-From: 5b4d9450fee01f821b6400e947b3839727643a71-refs/heads/main@{#992738}
|
||
|
---
|
||
|
base/bind_internal.h | 17 +++++-----
|
||
|
base/bind_unittest.cc | 28 +++++++++++++++++
|
||
|
base/bind_unittest.nc | 31 +++++++++++++++++--
|
||
|
base/memory/raw_ptr.h | 31 +++++++++++++++++++
|
||
|
.../raw_scoped_refptr_mismatch_checker.h | 5 +--
|
||
|
...privacy_sandbox_dialog_handler_unittest.cc | 26 +++++++++-------
|
||
|
.../web_app_install_finalizer.cc | 15 ++++-----
|
||
|
7 files changed, 121 insertions(+), 32 deletions(-)
|
||
|
|
||
|
diff --git a/base/bind_internal.h b/base/bind_internal.h
|
||
|
index 60607efadb30..1b06e54b65a4 100644
|
||
|
--- a/base/bind_internal.h
|
||
|
+++ b/base/bind_internal.h
|
||
|
@@ -859,8 +859,8 @@ bool QueryCancellationTraits(const BindStateBase* base,
|
||
|
template <typename Functor, typename Receiver, typename... Unused>
|
||
|
std::enable_if_t<
|
||
|
!(MakeFunctorTraits<Functor>::is_method &&
|
||
|
- std::is_pointer_v<std::decay_t<Receiver>> &&
|
||
|
- IsRefCountedType<std::remove_pointer_t<std::decay_t<Receiver>>>::value)>
|
||
|
+ IsPointerV<std::decay_t<Receiver>> &&
|
||
|
+ IsRefCountedType<RemovePointerT<std::decay_t<Receiver>>>::value)>
|
||
|
BanUnconstructedRefCountedReceiver(const Receiver& receiver, Unused&&...) {}
|
||
|
|
||
|
template <typename Functor>
|
||
|
@@ -870,8 +870,8 @@ void BanUnconstructedRefCountedReceiver() {}
|
||
|
template <typename Functor, typename Receiver, typename... Unused>
|
||
|
std::enable_if_t<
|
||
|
MakeFunctorTraits<Functor>::is_method &&
|
||
|
- std::is_pointer_v<std::decay_t<Receiver>> &&
|
||
|
- IsRefCountedType<std::remove_pointer_t<std::decay_t<Receiver>>>::value>
|
||
|
+ IsPointerV<std::decay_t<Receiver>> &&
|
||
|
+ IsRefCountedType<RemovePointerT<std::decay_t<Receiver>>>::value>
|
||
|
BanUnconstructedRefCountedReceiver(const Receiver& receiver, Unused&&...) {
|
||
|
DCHECK(receiver);
|
||
|
|
||
|
@@ -1006,19 +1006,20 @@ struct MakeBindStateTypeImpl<true, Functor, Receiver, BoundArgs...> {
|
||
|
static_assert(!std::is_array_v<std::remove_reference_t<Receiver>>,
|
||
|
"First bound argument to a method cannot be an array.");
|
||
|
static_assert(
|
||
|
- !std::is_pointer_v<DecayedReceiver> ||
|
||
|
- IsRefCountedType<std::remove_pointer_t<DecayedReceiver>>::value,
|
||
|
+ !IsPointerV<DecayedReceiver> ||
|
||
|
+ IsRefCountedType<RemovePointerT<DecayedReceiver>>::value,
|
||
|
"Receivers may not be raw pointers. If using a raw pointer here is safe"
|
||
|
" and has no lifetime concerns, use base::Unretained() and document why"
|
||
|
" it's safe.");
|
||
|
+
|
||
|
static_assert(!HasRefCountedTypeAsRawPtr<std::decay_t<BoundArgs>...>::value,
|
||
|
"A parameter is a refcounted type and needs scoped_refptr.");
|
||
|
|
||
|
public:
|
||
|
using Type = BindState<
|
||
|
std::decay_t<Functor>,
|
||
|
- std::conditional_t<std::is_pointer_v<DecayedReceiver>,
|
||
|
- scoped_refptr<std::remove_pointer_t<DecayedReceiver>>,
|
||
|
+ std::conditional_t<IsPointerV<DecayedReceiver>,
|
||
|
+ scoped_refptr<RemovePointerT<DecayedReceiver>>,
|
||
|
DecayedReceiver>,
|
||
|
MakeStorageType<BoundArgs>...>;
|
||
|
};
|
||
|
diff --git a/base/bind_unittest.cc b/base/bind_unittest.cc
|
||
|
index a5f681fe53b9..6844b6796d9f 100644
|
||
|
--- a/base/bind_unittest.cc
|
||
|
+++ b/base/bind_unittest.cc
|
||
|
@@ -1169,6 +1169,28 @@ TYPED_TEST(BindVariantsTest, UniquePtrReceiver) {
|
||
|
TypeParam::Bind(&NoRef::VoidMethod0, std::move(no_ref)).Run();
|
||
|
}
|
||
|
|
||
|
+TYPED_TEST(BindVariantsTest, ImplicitRefPtrReceiver) {
|
||
|
+ StrictMock<HasRef> has_ref;
|
||
|
+ EXPECT_CALL(has_ref, AddRef()).Times(1);
|
||
|
+ EXPECT_CALL(has_ref, Release()).Times(1);
|
||
|
+ EXPECT_CALL(has_ref, HasAtLeastOneRef()).WillRepeatedly(Return(true));
|
||
|
+
|
||
|
+ HasRef* ptr = &has_ref;
|
||
|
+ auto ptr_cb = TypeParam::Bind(&HasRef::HasAtLeastOneRef, ptr);
|
||
|
+ EXPECT_EQ(1, std::move(ptr_cb).Run());
|
||
|
+}
|
||
|
+
|
||
|
+TYPED_TEST(BindVariantsTest, RawPtrReceiver) {
|
||
|
+ StrictMock<HasRef> has_ref;
|
||
|
+ EXPECT_CALL(has_ref, AddRef()).Times(1);
|
||
|
+ EXPECT_CALL(has_ref, Release()).Times(1);
|
||
|
+ EXPECT_CALL(has_ref, HasAtLeastOneRef()).WillRepeatedly(Return(true));
|
||
|
+
|
||
|
+ raw_ptr<HasRef> rawptr(&has_ref);
|
||
|
+ auto rawptr_cb = TypeParam::Bind(&HasRef::HasAtLeastOneRef, rawptr);
|
||
|
+ EXPECT_EQ(1, std::move(rawptr_cb).Run());
|
||
|
+}
|
||
|
+
|
||
|
// Tests for Passed() wrapper support:
|
||
|
// - Passed() can be constructed from a pointer to scoper.
|
||
|
// - Passed() can be constructed from a scoper rvalue.
|
||
|
@@ -1751,6 +1773,12 @@ TEST(BindDeathTest, BanFirstOwnerOfRefCountedType) {
|
||
|
EXPECT_CALL(has_ref, HasAtLeastOneRef()).WillOnce(Return(false));
|
||
|
base::BindOnce(&HasRef::VoidMethod0, &has_ref);
|
||
|
});
|
||
|
+
|
||
|
+ EXPECT_DCHECK_DEATH({
|
||
|
+ raw_ptr<HasRef> rawptr(&has_ref);
|
||
|
+ EXPECT_CALL(has_ref, HasAtLeastOneRef()).WillOnce(Return(false));
|
||
|
+ base::BindOnce(&HasRef::VoidMethod0, rawptr);
|
||
|
+ });
|
||
|
}
|
||
|
|
||
|
} // namespace
|
||
|
diff --git a/base/bind_unittest.nc b/base/bind_unittest.nc
|
||
|
index 20b0e0ba2cee..29807298ca3c 100644
|
||
|
--- a/base/bind_unittest.nc
|
||
|
+++ b/base/bind_unittest.nc
|
||
|
@@ -93,7 +93,7 @@ void WontCompile() {
|
||
|
method_to_const_cb.Run();
|
||
|
}
|
||
|
|
||
|
-#elif defined(NCTEST_METHOD_BIND_NEEDS_REFCOUNTED_OBJECT) // [r"fatal error: static_assert failed due to requirement '!std::is_pointer_v<base::NoRef *> || IsRefCountedType<base::NoRef, void>::value' \"Receivers may not be raw pointers. If using a raw pointer here is safe and has no lifetime concerns, use base::Unretained() and document why it's safe.\""]
|
||
|
+#elif defined(NCTEST_METHOD_BIND_NEEDS_REFCOUNTED_OBJECT) // [r"fatal error: static_assert failed due to requirement '!IsPointerV<base::NoRef \*> \|\| IsRefCountedType<base::NoRef, void>::value' \"Receivers may not be raw pointers. If using a raw pointer here is safe and has no lifetime concerns, use base::Unretained\(\) and document why it's safe.\""]
|
||
|
|
||
|
|
||
|
// Method bound to non-refcounted object.
|
||
|
@@ -106,7 +106,7 @@ void WontCompile() {
|
||
|
no_ref_cb.Run();
|
||
|
}
|
||
|
|
||
|
-#elif defined(NCTEST_CONST_METHOD_NEEDS_REFCOUNTED_OBJECT) // [r"fatal error: static_assert failed due to requirement '!std::is_pointer_v<base::NoRef *> || IsRefCountedType<base::NoRef, void>::value' \"Receivers may not be raw pointers. If using a raw pointer here is safe and has no lifetime concerns, use base::Unretained() and document why it's safe.\""]
|
||
|
+#elif defined(NCTEST_CONST_METHOD_BIND_NEEDS_REFCOUNTED_OBJECT) // [r"fatal error: static_assert failed due to requirement '!IsPointerV<base::NoRef \*> \|\| IsRefCountedType<base::NoRef, void>::value' \"Receivers may not be raw pointers. If using a raw pointer here is safe and has no lifetime concerns, use base::Unretained\(\) and document why it's safe.\""]
|
||
|
|
||
|
// Const Method bound to non-refcounted object.
|
||
|
//
|
||
|
@@ -118,6 +118,33 @@ void WontCompile() {
|
||
|
no_ref_const_cb.Run();
|
||
|
}
|
||
|
|
||
|
+#elif defined(NCTEST_METHOD_BIND_RAW_PTR_RECEIVER_NEEDS_REFCOUNTED_OBJECT) // [r"fatal error: static_assert failed due to requirement '!IsPointerV<base::raw_ptr<base::NoRef, [^>]+>> \|\| IsRefCountedType<base::NoRef, void>::value' \"Receivers may not be raw pointers. If using a raw pointer here is safe and has no lifetime concerns, use base::Unretained\(\) and document why it's safe.\""]
|
||
|
+
|
||
|
+
|
||
|
+// Method bound to non-refcounted object.
|
||
|
+//
|
||
|
+// We require refcounts unless you have Unretained().
|
||
|
+void WontCompile() {
|
||
|
+ NoRef no_ref;
|
||
|
+ raw_ptr<NoRef> rawptr(&no_ref);
|
||
|
+ RepeatingCallback<void()> no_ref_cb =
|
||
|
+ BindRepeating(&NoRef::VoidMethod0, rawptr);
|
||
|
+ no_ref_cb.Run();
|
||
|
+}
|
||
|
+
|
||
|
+#elif defined(NCTEST_CONST_METHOD_BIND_RAW_PTR_RECEIVER_NEEDS_REFCOUNTED_OBJECT) // [r"fatal error: static_assert failed due to requirement '!IsPointerV<base::raw_ptr<base::NoRef, [^>]+>> \|\| IsRefCountedType<base::NoRef, void>::value' \"Receivers may not be raw pointers. If using a raw pointer here is safe and has no lifetime concerns, use base::Unretained\(\) and document why it's safe.\""]
|
||
|
+
|
||
|
+// Const Method bound to non-refcounted object.
|
||
|
+//
|
||
|
+// We require refcounts unless you have Unretained().
|
||
|
+void WontCompile() {
|
||
|
+ NoRef no_ref;
|
||
|
+ raw_ptr<NoRef> rawptr(&no_ref);
|
||
|
+ RepeatingCallback<void()> no_ref_const_cb =
|
||
|
+ BindRepeating(&NoRef::VoidConstMethod0, rawptr);
|
||
|
+ no_ref_const_cb.Run();
|
||
|
+}
|
||
|
+
|
||
|
#elif defined(NCTEST_CONST_POINTER) // [r"static_assert failed.+?BindArgument<0>::ForwardedAs<.+?>::ToParamWithType<.+?>::kCanBeForwardedToBoundFunctor.+?Type mismatch between bound argument and bound functor's parameter\."]
|
||
|
// Const argument used with non-const pointer parameter of same type.
|
||
|
//
|
||
|
diff --git a/base/memory/raw_ptr.h b/base/memory/raw_ptr.h
|
||
|
index 4d978e979863..639713cd6199 100644
|
||
|
--- a/base/memory/raw_ptr.h
|
||
|
+++ b/base/memory/raw_ptr.h
|
||
|
@@ -1051,6 +1051,37 @@ ALWAYS_INLINE bool operator>=(const raw_ptr<U, I>& lhs,
|
||
|
return lhs.GetForComparison() >= rhs.GetForComparison();
|
||
|
}
|
||
|
|
||
|
+// Template helpers for working with T* or raw_ptr<T>.
|
||
|
+template <typename T>
|
||
|
+struct IsPointer : std::false_type {};
|
||
|
+
|
||
|
+template <typename T>
|
||
|
+struct IsPointer<T*> : std::true_type {};
|
||
|
+
|
||
|
+template <typename T, typename I>
|
||
|
+struct IsPointer<raw_ptr<T, I>> : std::true_type {};
|
||
|
+
|
||
|
+template <typename T>
|
||
|
+inline constexpr bool IsPointerV = IsPointer<T>::value;
|
||
|
+
|
||
|
+template <typename T>
|
||
|
+struct RemovePointer {
|
||
|
+ using type = T;
|
||
|
+};
|
||
|
+
|
||
|
+template <typename T>
|
||
|
+struct RemovePointer<T*> {
|
||
|
+ using type = T;
|
||
|
+};
|
||
|
+
|
||
|
+template <typename T, typename I>
|
||
|
+struct RemovePointer<raw_ptr<T, I>> {
|
||
|
+ using type = T;
|
||
|
+};
|
||
|
+
|
||
|
+template <typename T>
|
||
|
+using RemovePointerT = typename RemovePointer<T>::type;
|
||
|
+
|
||
|
} // namespace base
|
||
|
|
||
|
using base::raw_ptr;
|
||
|
diff --git a/base/memory/raw_scoped_refptr_mismatch_checker.h b/base/memory/raw_scoped_refptr_mismatch_checker.h
|
||
|
index 9e50458ec98b..7afae066fa3e 100644
|
||
|
--- a/base/memory/raw_scoped_refptr_mismatch_checker.h
|
||
|
+++ b/base/memory/raw_scoped_refptr_mismatch_checker.h
|
||
|
@@ -7,6 +7,7 @@
|
||
|
|
||
|
#include <type_traits>
|
||
|
|
||
|
+#include "base/memory/raw_ptr.h"
|
||
|
#include "base/template_util.h"
|
||
|
|
||
|
// It is dangerous to post a task with a T* argument where T is a subtype of
|
||
|
@@ -35,8 +36,8 @@ struct IsRefCountedType<T,
|
||
|
// pointer type and are convertible to a RefCounted(Base|ThreadSafeBase) type.
|
||
|
template <typename T>
|
||
|
struct NeedsScopedRefptrButGetsRawPtr
|
||
|
- : conjunction<std::is_pointer<T>,
|
||
|
- IsRefCountedType<std::remove_pointer_t<T>>> {
|
||
|
+ : conjunction<base::IsPointer<T>,
|
||
|
+ IsRefCountedType<base::RemovePointerT<T>>> {
|
||
|
static_assert(!std::is_reference<T>::value,
|
||
|
"NeedsScopedRefptrButGetsRawPtr requires non-reference type.");
|
||
|
};
|
||
|
diff --git a/chrome/browser/ui/webui/privacy_sandbox/privacy_sandbox_dialog_handler_unittest.cc b/chrome/browser/ui/webui/privacy_sandbox/privacy_sandbox_dialog_handler_unittest.cc
|
||
|
index d812d82a08c3..ee22b02bdbed 100644
|
||
|
--- a/chrome/browser/ui/webui/privacy_sandbox/privacy_sandbox_dialog_handler_unittest.cc
|
||
|
+++ b/chrome/browser/ui/webui/privacy_sandbox/privacy_sandbox_dialog_handler_unittest.cc
|
||
|
@@ -93,9 +93,7 @@ class PrivacySandboxDialogHandlerTest : public testing::Test {
|
||
|
content::TestWebUI* web_ui() { return web_ui_.get(); }
|
||
|
PrivacySandboxDialogHandler* handler() { return handler_.get(); }
|
||
|
TestingProfile* profile() { return &profile_; }
|
||
|
- raw_ptr<MockPrivacySandboxDialogView> dialog_mock() {
|
||
|
- return dialog_mock_.get();
|
||
|
- }
|
||
|
+ MockPrivacySandboxDialogView* dialog_mock() { return dialog_mock_.get(); }
|
||
|
MockPrivacySandboxService* mock_privacy_sandbox_service() {
|
||
|
return mock_privacy_sandbox_service_;
|
||
|
}
|
||
|
@@ -120,15 +118,18 @@ class PrivacySandboxConsentDialogHandlerTest
|
||
|
: public PrivacySandboxDialogHandlerTest {
|
||
|
protected:
|
||
|
std::unique_ptr<PrivacySandboxDialogHandler> CreateHandler() override {
|
||
|
+ // base::Unretained is safe because the created handler does not outlive the
|
||
|
+ // mock.
|
||
|
return std::make_unique<PrivacySandboxDialogHandler>(
|
||
|
- base::BindOnce(&MockPrivacySandboxDialogView::Close, dialog_mock()),
|
||
|
+ base::BindOnce(&MockPrivacySandboxDialogView::Close,
|
||
|
+ base::Unretained(dialog_mock())),
|
||
|
base::BindOnce(&MockPrivacySandboxDialogView::ResizeNativeView,
|
||
|
- dialog_mock()),
|
||
|
+ base::Unretained(dialog_mock())),
|
||
|
base::BindOnce(&MockPrivacySandboxDialogView::ShowNativeView,
|
||
|
- dialog_mock()),
|
||
|
+ base::Unretained(dialog_mock())),
|
||
|
base::BindOnce(
|
||
|
&MockPrivacySandboxDialogView::OpenPrivacySandboxSettings,
|
||
|
- dialog_mock()),
|
||
|
+ base::Unretained(dialog_mock())),
|
||
|
PrivacySandboxService::DialogType::kConsent);
|
||
|
}
|
||
|
};
|
||
|
@@ -247,15 +248,18 @@ class PrivacySandboxNoticeDialogHandlerTest
|
||
|
: public PrivacySandboxDialogHandlerTest {
|
||
|
protected:
|
||
|
std::unique_ptr<PrivacySandboxDialogHandler> CreateHandler() override {
|
||
|
+ // base::Unretained is safe because the created handler does not outlive the
|
||
|
+ // mock.
|
||
|
return std::make_unique<PrivacySandboxDialogHandler>(
|
||
|
- base::BindOnce(&MockPrivacySandboxDialogView::Close, dialog_mock()),
|
||
|
+ base::BindOnce(&MockPrivacySandboxDialogView::Close,
|
||
|
+ base::Unretained(dialog_mock())),
|
||
|
base::BindOnce(&MockPrivacySandboxDialogView::ResizeNativeView,
|
||
|
- dialog_mock()),
|
||
|
+ base::Unretained(dialog_mock())),
|
||
|
base::BindOnce(&MockPrivacySandboxDialogView::ShowNativeView,
|
||
|
- dialog_mock()),
|
||
|
+ base::Unretained(dialog_mock())),
|
||
|
base::BindOnce(
|
||
|
&MockPrivacySandboxDialogView::OpenPrivacySandboxSettings,
|
||
|
- dialog_mock()),
|
||
|
+ base::Unretained(dialog_mock())),
|
||
|
PrivacySandboxService::DialogType::kNotice);
|
||
|
}
|
||
|
};
|
||
|
diff --git a/chrome/browser/web_applications/web_app_install_finalizer.cc b/chrome/browser/web_applications/web_app_install_finalizer.cc
|
||
|
index 2b85d7645980..cf57d375d0d3 100644
|
||
|
--- a/chrome/browser/web_applications/web_app_install_finalizer.cc
|
||
|
+++ b/chrome/browser/web_applications/web_app_install_finalizer.cc
|
||
|
@@ -505,10 +505,6 @@ void WebAppInstallFinalizer::SetWebAppManifestFieldsAndWriteData(
|
||
|
web_app_info.shortcuts_menu_icon_bitmaps;
|
||
|
IconsMap other_icon_bitmaps = web_app_info.other_icon_bitmaps;
|
||
|
|
||
|
- auto write_icons_callback = base::BindOnce(
|
||
|
- &WebAppIconManager::WriteData, icon_manager_, app_id,
|
||
|
- std::move(icon_bitmaps), std::move(shortcuts_menu_icon_bitmaps),
|
||
|
- std::move(other_icon_bitmaps));
|
||
|
auto write_translations_callback = base::BindOnce(
|
||
|
&WebAppInstallFinalizer::WriteTranslations,
|
||
|
weak_ptr_factory_.GetWeakPtr(), app_id, std::move(web_app_info));
|
||
|
@@ -516,11 +512,12 @@ void WebAppInstallFinalizer::SetWebAppManifestFieldsAndWriteData(
|
||
|
base::BindOnce(&WebAppInstallFinalizer::CommitToSyncBridge,
|
||
|
weak_ptr_factory_.GetWeakPtr(), std::move(web_app));
|
||
|
|
||
|
- std::move(write_icons_callback)
|
||
|
- .Run(base::BindOnce(
|
||
|
- std::move(write_translations_callback),
|
||
|
- base::BindOnce(std::move(commit_to_sync_bridge_callback),
|
||
|
- std::move(commit_callback))));
|
||
|
+ icon_manager_->WriteData(
|
||
|
+ app_id, std::move(icon_bitmaps), std::move(shortcuts_menu_icon_bitmaps),
|
||
|
+ std::move(other_icon_bitmaps),
|
||
|
+ base::BindOnce(std::move(write_translations_callback),
|
||
|
+ base::BindOnce(std::move(commit_to_sync_bridge_callback),
|
||
|
+ std::move(commit_callback))));
|
||
|
}
|
||
|
|
||
|
void WebAppInstallFinalizer::WriteTranslations(
|
||
|
--
|
||
|
2.37.0
|
||
|
|