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.

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

chore: cherry-pick f427936d32db from chromium #34685

Merged
merged 4 commits into from Jul 5, 2022
Merged
Show file tree
Hide file tree
Changes from 1 commit
Commits
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
Expand Up @@ -129,3 +129,4 @@ cherry-pick-919b1ffe1fe7.patch
cherry-pick-f1dd785e021e.patch
cherry-pick-b03797bdb1df.patch
posix_replace_doubleforkandexec_with_forkandspawn.patch
cherry-pick-f427936d32db.patch
345 changes: 345 additions & 0 deletions patches/chromium/cherry-pick-f427936d32db.patch
@@ -0,0 +1,345 @@
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] [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}
---

diff --git a/base/bind_internal.h b/base/bind_internal.h
index 60607efa..1b06e54 100644
--- a/base/bind_internal.h
+++ b/base/bind_internal.h
@@ -859,8 +859,8 @@
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 @@
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 @@
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 a5f681f..6844b67 100644
--- a/base/bind_unittest.cc
+++ b/base/bind_unittest.cc
@@ -1169,6 +1169,28 @@
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 @@
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 20b0e0b..2980729 100644
--- a/base/bind_unittest.nc
+++ b/base/bind_unittest.nc
@@ -93,7 +93,7 @@
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 @@
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 @@
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 4d978e9..639713c 100644
--- a/base/memory/raw_ptr.h
+++ b/base/memory/raw_ptr.h
@@ -1051,6 +1051,37 @@
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 9e50458..7afae06 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 @@
// 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 d812d82..ee22b02b 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 @@
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 @@
: 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 @@
: 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 2b85d76..cf57d375 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 @@
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 @@
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(