Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

chore: cherry-pick f427936d32db from chromium #34685

Merged
merged 4 commits into from Jul 5, 2022
Merged

chore: cherry-pick f427936d32db from chromium #34685

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
fb8cb62
chore: [18-x-y] cherry-pick f427936d32db from chromium
nornagon Jun 21, 2022
a32ae34
Merge branch '18-x-y' into cherry-pick/18-x-y/chromium/f427936d32db
electron-bot Jun 23, 2022
d58f415
fix patch.
ppontes Jun 28, 2022
4b634e7
chore: update patches
patchup[bot] Jun 28, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
1 change: 1 addition & 0 deletions patches/chromium/.patches
View file
Open in desktop
Expand Up @@ -129,4 +129,5 @@ cherry-pick-919b1ffe1fe7.patch
cherry-pick-f1dd785e021e.patch
cherry-pick-b03797bdb1df.patch
posix_replace_doubleforkandexec_with_forkandspawn.patch
cherry-pick-f427936d32db.patch
cherry-pick-22c61cfae5d1.patch
338 changes: 338 additions & 0 deletions patches/chromium/cherry-pick-f427936d32db.patch
View file
Open in desktop
@@ -0,0 +1,338 @@
From 0000000000000000000000000000000000000000 Mon Sep 17 00:00:00 2001
From: Daniel Cheng <dcheng@chromium.org>
Date: Tue, 14 Jun 2022 19:11:17 +0000
Subject: 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}

diff --git a/base/bind_internal.h b/base/bind_internal.h
index ea2ab97fae8adb9d171f9bf4dd367a12498112ec..a14ba7865c3581ee467fec39bd94bd58b6d07f3f 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 a5f681fe53b97a5a98f5918ac5cbc9e6cbbf790b..6844b6796d9f19e98baf13adae772d01eedf6846 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 20b0e0ba2ceea9576cae13bf08cdb0e979f70018..29807298ca3c13fd54c45c14d496bebdb3123816 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 9172af8ffe03bd038825b885ca46b7ffe8a3f90f..045c397fafb676eacd9b1030e114bd3875f918cd 100644
--- a/base/memory/raw_ptr.h
+++ b/base/memory/raw_ptr.h
@@ -882,6 +882,37 @@ RAW_PTR_FUNC_ATTRIBUTES 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 9e50458ec98bb7e9041355fa06d14c6ba703b85c..7afae066fa3e318b346fac6b60ee55873b65f909 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 47d3cc7579b8375e7e356385c0537eec59dd591a..a2808c7d198d7b2e469e2c639e028cbf9cf9b583 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
@@ -67,9 +67,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_;
}
@@ -93,13 +91,16 @@ 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::OpenPrivacySandboxSettings,
- dialog_mock()),
+ base::Unretained(dialog_mock())),
PrivacySandboxService::DialogType::kConsent);
}
};
@@ -193,13 +194,16 @@ 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::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 7f36ec57e697b871b677c09c255cbce9bcb09b3a..b4e6af317aa25d7f8eb11bfda1d81c999a93e1fb 100644
--- a/chrome/browser/web_applications/web_app_install_finalizer.cc
+++ b/chrome/browser/web_applications/web_app_install_finalizer.cc
@@ -568,10 +568,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));
@@ -579,11 +575,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(