From f427936d32dbe1e9c27c0bcf54eff6818bddb906 Mon Sep 17 00:00:00 2001 From: Daniel Cheng Date: Tue, 14 Jun 2022 19:11:17 +0000 Subject: [PATCH 14/59] [M102] Ensure raw_ptr 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. - Implement base::IsPointer and base::RemovePointer, which are similar to std::is_pointer and std::remove_pointer, except they also consider raw_ptr 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 Commit-Queue: Daniel Cheng 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 std::enable_if_t< !(MakeFunctorTraits::is_method && - std::is_pointer_v> && - IsRefCountedType>>::value)> + IsPointerV> && + IsRefCountedType>>::value)> BanUnconstructedRefCountedReceiver(const Receiver& receiver, Unused&&...) {} template @@ -870,8 +870,8 @@ void BanUnconstructedRefCountedReceiver() {} template std::enable_if_t< MakeFunctorTraits::is_method && - std::is_pointer_v> && - IsRefCountedType>>::value> + IsPointerV> && + IsRefCountedType>>::value> BanUnconstructedRefCountedReceiver(const Receiver& receiver, Unused&&...) { DCHECK(receiver); @@ -1006,19 +1006,20 @@ struct MakeBindStateTypeImpl { static_assert(!std::is_array_v>, "First bound argument to a method cannot be an array."); static_assert( - !std::is_pointer_v || - IsRefCountedType>::value, + !IsPointerV || + IsRefCountedType>::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...>::value, "A parameter is a refcounted type and needs scoped_refptr."); public: using Type = BindState< std::decay_t, - std::conditional_t, - scoped_refptr>, + std::conditional_t, + scoped_refptr>, DecayedReceiver>, MakeStorageType...>; }; 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 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 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 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 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 || IsRefCountedType::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 \|\| IsRefCountedType::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 || IsRefCountedType::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 \|\| IsRefCountedType::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]+>> \|\| IsRefCountedType::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 rawptr(&no_ref); + RepeatingCallback 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]+>> \|\| IsRefCountedType::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 rawptr(&no_ref); + RepeatingCallback 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& lhs, return lhs.GetForComparison() >= rhs.GetForComparison(); } +// Template helpers for working with T* or raw_ptr. +template +struct IsPointer : std::false_type {}; + +template +struct IsPointer : std::true_type {}; + +template +struct IsPointer> : std::true_type {}; + +template +inline constexpr bool IsPointerV = IsPointer::value; + +template +struct RemovePointer { + using type = T; +}; + +template +struct RemovePointer { + using type = T; +}; + +template +struct RemovePointer> { + using type = T; +}; + +template +using RemovePointerT = typename RemovePointer::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 +#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 struct NeedsScopedRefptrButGetsRawPtr - : conjunction, - IsRefCountedType>> { + : conjunction, + IsRefCountedType>> { static_assert(!std::is_reference::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 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 CreateHandler() override { + // base::Unretained is safe because the created handler does not outlive the + // mock. return std::make_unique( - 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 CreateHandler() override { + // base::Unretained is safe because the created handler does not outlive the + // mock. return std::make_unique( - 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