PCScan: Don't check bitness when reporting if experiment is enabled

IsPartitionAllocPCScanBrowserOnlyEnabled used to return false
unconditionally on 32-bit architectures. This led to necessity to set up
different populations in Omaha for each architecture, which is error
prone. After changing this, the responsibility for checking the bitness
falls now on EnablePCScanForMallocPartitionsInBrowserProcessIfNeeded.

The caveat is that the population with PartitionAllocPCScanBrowserOnly
turned on on 32-bit systems will now have to be ignored. This is ok
because we get enough data on Windows.

Make a similar change to EnablePCScanForMallocPartitionsIfNeeded for
consistency, and overall clean up the code to look more symmetrical,
while at it.

Bug: 11297512
Change-Id: Ia4eccdbea149389e3a192c5b618cb938a701c388
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2597240Reviewed-by: Steven Holte <holte@chromium.org>
Reviewed-by: Will Harris <wfh@chromium.org>
Reviewed-by: Kentaro Hara <haraken@chromium.org>
Reviewed-by: Ken Rockot <rockot@google.com>
Reviewed-by: Anton Bikineev <bikineev@chromium.org>
Commit-Queue: Bartek Nowierski <bartekn@chromium.org>
Auto-Submit: Bartek Nowierski <bartekn@chromium.org>
Cr-Commit-Position: refs/heads/master@{#841732}

base/allocator/allocator_shim.h

@@ -8,6 +8,7 @@
 #include <stddef.h>
 
 #include "base/allocator/buildflags.h"
+#include "base/allocator/partition_allocator/partition_alloc_features.h"
 #include "base/base_export.h"
 #include "build/build_config.h"
 
@@ -163,9 +164,8 @@ BASE_EXPORT void InitializeAllocatorShim();
 BASE_EXPORT void EnablePartitionAllocMemoryReclaimer();
 #endif
 
-#if BUILDFLAG(USE_PARTITION_ALLOC_AS_MALLOC)
+#if BUILDFLAG(USE_PARTITION_ALLOC_AS_MALLOC) && ALLOW_PCSCAN
 BASE_EXPORT void EnablePCScan();
-BASE_EXPORT void EnablePCScanIfNeeded();
 #endif
 
 }  // namespace allocator

base/allocator/allocator_shim_default_dispatch_to_partition_alloc.cc

@@ -278,10 +278,12 @@ void EnablePartitionAllocMemoryReclaimer() {
       AlignedAllocator());
 }
 
+#if ALLOW_PCSCAN
 void EnablePCScan() {
   Allocator()->EnablePCScan();
   AlignedAllocator()->EnablePCScan();
 }
+#endif
 
 #if defined(OS_WIN)
 // Call this as soon as possible during startup.
@@ -293,12 +295,6 @@ void ConfigurePartitionAlloc() {
 }
 #endif  // defined(OS_WIN)
 
-void EnablePCScanIfNeeded() {
-  if (!features::IsPartitionAllocPCScanEnabled())
-    return;
-  EnablePCScan();
-}
-
 }  // namespace allocator
 }  // namespace base
 

base/allocator/partition_allocator/partition_alloc_features.cc

@@ -20,16 +20,18 @@ const Feature kPartitionAllocGigaCage{"PartitionAllocGigaCage32bit",
                                       FEATURE_ENABLED_BY_DEFAULT};
 #endif
 
+#if ALLOW_PCSCAN
 // If enabled, PCScan is turned on by default for all partitions that don't
 // disable it explicitly.
 const Feature kPartitionAllocPCScan{"PartitionAllocPCScan",
                                     FEATURE_DISABLED_BY_DEFAULT};
+#endif  // ALLOW_PCSCAN
 
 #if BUILDFLAG(USE_PARTITION_ALLOC_AS_MALLOC)
 // If enabled, PCScan is turned on only for the browser's malloc partition.
 const Feature kPartitionAllocPCScanBrowserOnly{
     "PartitionAllocPCScanBrowserOnly", FEATURE_DISABLED_BY_DEFAULT};
-#endif
+#endif  // BUILDFLAG(USE_PARTITION_ALLOC_AS_MALLOC)
 
 }  // namespace features
 }  // namespace base

base/allocator/partition_allocator/partition_alloc_features.h

@@ -20,6 +20,12 @@
 #include <VersionHelpers.h>
 #endif
 
+#if defined(PA_HAS_64_BITS_POINTERS) && !ENABLE_REF_COUNT_FOR_BACKUP_REF_PTR
+#define ALLOW_PCSCAN 1
+#else
+#define ALLOW_PCSCAN 0
+#endif
+
 namespace base {
 
 struct Feature;
@@ -27,10 +33,13 @@ struct Feature;
 namespace features {
 
 extern const BASE_EXPORT Feature kPartitionAllocGigaCage;
+
+#if ALLOW_PCSCAN
 extern const BASE_EXPORT Feature kPartitionAllocPCScan;
+#endif  // ALLOW_PCSCAN
 #if BUILDFLAG(USE_PARTITION_ALLOC_AS_MALLOC)
 extern const BASE_EXPORT Feature kPartitionAllocPCScanBrowserOnly;
-#endif
+#endif  // BUILDFLAG(USE_PARTITION_ALLOC_AS_MALLOC)
 
 ALWAYS_INLINE bool IsPartitionAllocGigaCageEnabled() {
 #if defined(PA_HAS_64_BITS_POINTERS) && defined(OS_WIN)
@@ -64,24 +73,6 @@ ALWAYS_INLINE bool IsPartitionAllocGigaCageEnabled() {
 #endif  // BUILDFLAG(USE_PARTITION_ALLOC_AS_MALLOC)
 }
 
-ALWAYS_INLINE bool IsPartitionAllocPCScanEnabled() {
-#if defined(PA_HAS_64_BITS_POINTERS) && !ENABLE_REF_COUNT_FOR_BACKUP_REF_PTR
-  return FeatureList::IsEnabled(kPartitionAllocPCScan);
-#else  // PA_HAS_64_BITS_POINTERS
-  return false;
-#endif
-}
-
-#if BUILDFLAG(USE_PARTITION_ALLOC_AS_MALLOC)
-ALWAYS_INLINE bool IsPartitionAllocPCScanBrowserOnlyEnabled() {
-#if defined(PA_HAS_64_BITS_POINTERS) && !ENABLE_REF_COUNT_FOR_BACKUP_REF_PTR
-  return FeatureList::IsEnabled(kPartitionAllocPCScanBrowserOnly);
-#else  // PA_HAS_64_BITS_POINTERS
-  return false;
-#endif
-}
-#endif
-
 }  // namespace features
 }  // namespace base
 

base/allocator/partition_allocator/partition_root.cc

@@ -4,10 +4,10 @@
 
 #include "base/allocator/partition_allocator/partition_root.h"
 
-#include "base/allocator/partition_allocator/checked_ptr_support.h"
 #include "base/allocator/partition_allocator/oom.h"
 #include "base/allocator/partition_allocator/page_allocator.h"
 #include "base/allocator/partition_allocator/partition_alloc_check.h"
+#include "base/allocator/partition_allocator/partition_alloc_features.h"
 #include "base/allocator/partition_allocator/partition_bucket.h"
 #include "base/allocator/partition_allocator/partition_cookie.h"
 #include "base/allocator/partition_allocator/partition_oom.h"
@@ -25,10 +25,9 @@ template <bool thread_safe>
 typename PartitionRoot<thread_safe>::PCScanMode PartitionOptionsToPCScanMode(
     PartitionOptions::PCScan opt) {
   using Root = PartitionRoot<thread_safe>;
-  // PCScan is currently only supported on 64-bit systems.
-  // Mark partitions non-scannable on 32-bit systems unconditionally, so that
-  // address space for quarantine bitmaps doesn't get reserved.
-#if defined(PA_HAS_64_BITS_POINTERS) && !ENABLE_REF_COUNT_FOR_BACKUP_REF_PTR
+  // Mark partitions non-scannable unconditionally when PCScan isn't allowed, so
+  // that address space for quarantine bitmaps doesn't get reserved.
+#if ALLOW_PCSCAN
   switch (opt) {
     case PartitionOptions::PCScan::kAlwaysDisabled:
       return Root::PCScanMode::kNonScannable;

base/allocator/partition_allocator/pcscan_unittest.cc

@@ -4,13 +4,15 @@
 
 #include "base/allocator/partition_allocator/partition_root.h"
 #if !defined(MEMORY_TOOL_REPLACES_ALLOCATOR)
-#if defined(PA_HAS_64_BITS_POINTERS)
 
 #include "base/allocator/partition_allocator/pcscan.h"
 
 #include "base/allocator/partition_allocator/partition_alloc.h"
+#include "base/allocator/partition_allocator/partition_alloc_features.h"
 #include "testing/gtest/include/gtest/gtest.h"
 
+#if ALLOW_PCSCAN
+
 namespace base {
 namespace internal {
 
@@ -330,5 +332,5 @@ TEST_F(PCScanTest, DanglingInterPartitionReference) {
 }  // namespace internal
 }  // namespace base
 
-#endif  // defined(PA_HAS_64_BITS_POINTERS)
+#endif  // ALLOW_PCSCAN
 #endif  // defined(MEMORY_TOOL_REPLACES_ALLOCATOR)

chrome/browser/metrics/chrome_browser_main_extra_parts_metrics.cc

@@ -559,13 +559,23 @@ void ChromeBrowserMainExtraPartsMetrics::PreBrowserStart() {
   );
 
 #if defined(OS_WIN) && BUILDFLAG(USE_PARTITION_ALLOC_AS_MALLOC)
-  // Records whether or not BackupRefPtr and/or PCScan is enabled.
+  // Records whether or not BackupRefPtr and/or PCScan is enabled. This is meant
+  // for a 3-way experiment with 2 binaries:
+  // - binary A: deployed to 66% users, with half of them having PCScan on and
+  //             half off (BackupRefPtr fully off)
+  // - binary B: deployed to 33% users, with BackupRefPtr on (PCSCan fully off)
+  //
+  // NOTE, deliberately don't use ALLOW_PCSCAN which depends on bitness. In the
+  // 32-bit case, PCScan is always disabled, but we'll deliberately misrepresent
+  // it as enabled here (and later ignored when analyzing results), in order to
+  // keep each population at 33%.
   ChromeMetricsServiceAccessor::RegisterSyntheticFieldTrial(
       "BackupRefPtrAndPCScan",
 #if ENABLE_REF_COUNT_FOR_BACKUP_REF_PTR
       "BackupRefPtrEnabled"
 #else
-      base::features::IsPartitionAllocPCScanBrowserOnlyEnabled()
+      base::FeatureList::IsEnabled(
+          base::features::kPartitionAllocPCScanBrowserOnly)
           ? "PCScanEnabled"
           : "Disabled"
 #endif

content/app/content_main_runner_impl.cc

@@ -240,16 +240,19 @@ void InitializeV8IfNeeded(const base::CommandLine& command_line,
 }
 
 void EnablePCScanForMallocPartitionsIfNeeded() {
-#if BUILDFLAG(USE_PARTITION_ALLOC_AS_MALLOC)
+#if BUILDFLAG(USE_PARTITION_ALLOC_AS_MALLOC) && ALLOW_PCSCAN
   CHECK(base::FeatureList::GetInstance());
-  base::allocator::EnablePCScanIfNeeded();
+  if (base::FeatureList::IsEnabled(base::features::kPartitionAllocPCScan)) {
+    base::allocator::EnablePCScan();
+  }
 #endif
 }
 
 void EnablePCScanForMallocPartitionsInBrowserProcessIfNeeded() {
-#if BUILDFLAG(USE_PARTITION_ALLOC_AS_MALLOC)
+#if BUILDFLAG(USE_PARTITION_ALLOC_AS_MALLOC) && ALLOW_PCSCAN
   CHECK(base::FeatureList::GetInstance());
-  if (base::features::IsPartitionAllocPCScanBrowserOnlyEnabled()) {
+  if (base::FeatureList::IsEnabled(
+          base::features::kPartitionAllocPCScanBrowserOnly)) {
     base::allocator::EnablePCScan();
   }
 #endif

third_party/blink/renderer/platform/wtf/allocator/partitions.cc

@@ -34,7 +34,6 @@
 #include "base/allocator/partition_allocator/memory_reclaimer.h"
 #include "base/allocator/partition_allocator/oom.h"
 #include "base/allocator/partition_allocator/page_allocator.h"
-#include "base/allocator/partition_allocator/partition_alloc_constants.h"
 #include "base/allocator/partition_allocator/partition_alloc_features.h"
 #include "base/debug/alias.h"
 #include "base/no_destructor.h"
@@ -49,7 +48,7 @@ namespace WTF {
 const char* const Partitions::kAllocatedObjectPoolName =
     "partition_alloc/allocated_objects";
 
-#if defined(PA_HAS_64_BITS_POINTERS) && !ENABLE_REF_COUNT_FOR_BACKUP_REF_PTR
+#if ALLOW_PCSCAN
 // Runs PCScan on WTF partitions.
 const base::Feature kPCScanBlinkPartitions{"PCScanBlinkPartitions",
                                            base::FEATURE_DISABLED_BY_DEFAULT};
@@ -106,8 +105,8 @@ bool Partitions::InitializeOnce() {
   buffer_root_ = buffer_allocator->root();
   layout_root_ = layout_allocator->root();
 
-#if defined(PA_HAS_64_BITS_POINTERS) && !ENABLE_REF_COUNT_FOR_BACKUP_REF_PTR
-  if (base::features::IsPartitionAllocPCScanEnabled() ||
+#if ALLOW_PCSCAN
+  if (base::FeatureList::IsEnabled(base::features::kPartitionAllocPCScan) ||
       base::FeatureList::IsEnabled(kPCScanBlinkPartitions)) {
 #if !BUILDFLAG(USE_PARTITION_ALLOC_AS_MALLOC)
     fast_malloc_root_->EnablePCScan();