Improve management of command-line passed shared memory handle.
This CL changes the shared memory handle switch logic to allow the
browser process to retain its handle in order to properly dispose of it
when the child process assumes ownership and/or terminates.
On Windows, in particular, we are seeing evidence that these handles are
being leaked and/or perhaps being inadvertently being mapped twice.
Ownership of the handles is shared between the child process host and
the child process launcher/helper, which operate asynchronously to one
another. As such, the handles are managed as refcounted data.
Note: The handles are not accessed asynchronously. Either the launcher
(via command line) or the host (via IPC) passes the handle to the child,
but their destruction order is non-deterministic.
This is a reland of commit f9fc4f9fc7071a2760089ebd9aaae3f473ba2076
It fixes a bug the original CL which prevented the histogram shared
memory from being passed to the browser child process via IPC when
passing it via shared memory is disabled.
Original change's description:
> Change-Id: Ie4d17ad7ce0d29218f926f137dda61d94983d22a
> Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/6098050
> Reviewed-by: Arthur Sonzogni <[email protected]>
> Commit-Queue: Roger McFarlane <[email protected]>
> Reviewed-by: Etienne Pierre-Doray <[email protected]>
> Reviewed-by: Alexei Svitkine <[email protected]>
> Cr-Commit-Position: refs/heads/main@{#1408794}
Bug: 351671612, 40109064, 40818143
Change-Id: I638e7b47acd35188782588abf425ed867bb71dc1
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/6187263
Reviewed-by: Alexei Svitkine <[email protected]>
Reviewed-by: Arthur Sonzogni <[email protected]>
Commit-Queue: Roger McFarlane <[email protected]>
Reviewed-by: Etienne Pierre-Doray <[email protected]>
Cr-Commit-Position: refs/heads/main@{#1409683}
diff --git a/base/metrics/field_trial.cc b/base/metrics/field_trial.cc
index 30ade16d..daccb74 100644
--- a/base/metrics/field_trial.cc
+++ b/base/metrics/field_trial.cc
@@ -627,7 +627,7 @@
if (params_associator->GetFieldTrialParamsWithoutFallback(
*trial.trial_name, *trial.group_name, ¶ms)) {
if (params.size() > 0) {
- // Add comma to seprate from previous entry if it exists.
+ // Add comma to separate from previous entry if it exists.
if (!output.empty()) {
output.append(1, ',');
}
@@ -760,15 +760,14 @@
CHECK(global_->readonly_allocator_region_.IsValid());
global_->field_trial_allocator_->UpdateTrackingHistograms();
- shared_memory::AddToLaunchParameters(
- switches::kFieldTrialHandle,
- global_->readonly_allocator_region_.Duplicate(),
+ shared_memory::AddToLaunchParameters(switches::kFieldTrialHandle,
+ global_->readonly_allocator_region_,
#if BUILDFLAG(IS_APPLE)
- kFieldTrialRendezvousKey,
+ kFieldTrialRendezvousKey,
#elif BUILDFLAG(IS_POSIX)
- descriptor_key, descriptor_to_share,
+ descriptor_key, descriptor_to_share,
#endif
- command_line, launch_options);
+ command_line, launch_options);
// Append --enable-features and --disable-features switches corresponding
// to the features enabled on the command-line, so that child and browser
diff --git a/base/metrics/histogram_shared_memory.cc b/base/metrics/histogram_shared_memory.cc
index 02534f2..9c58ba0 100644
--- a/base/metrics/histogram_shared_memory.cc
+++ b/base/metrics/histogram_shared_memory.cc
@@ -115,8 +115,7 @@
}
// static
-bool HistogramSharedMemory::PassOnCommandLineIsEnabled(
- std::string_view process_type) {
+bool HistogramSharedMemory::PassOnCommandLineIsEnabled(int process_type) {
// On ChromeOS and for "utility" processes on other platforms there seems to
// be one or more mechanisms on startup which walk through all inherited
// shared memory regions and take a read-only handle to them. When we later
@@ -127,40 +126,32 @@
// Example: The call to OpenSymbolFiles() in base/debug/stack_trace_posix.cc
// grabs a read-only handle to the shmem region for some process types.
//
- // TODO(crbug.com/40109064): Fix ChromeOS and utility processes.
+ // TODO(crbug.com/40109064): Fix ChromeOS GPU and Android utility processes.
+ // Constants from content::ProcessType;
+ [[maybe_unused]] constexpr int PROCESS_TYPE_GPU = 9;
+ [[maybe_unused]] constexpr int PROCESS_TYPE_UTILITY = 6;
return (FeatureList::IsEnabled(kPassHistogramSharedMemoryOnLaunch)
#if BUILDFLAG(IS_CHROMEOS)
- && process_type != "gpu-process"
+ && process_type != PROCESS_TYPE_GPU
#elif BUILDFLAG(IS_ANDROID)
- && process_type != "utility"
+ && process_type != PROCESS_TYPE_UTILITY
#endif
);
}
// static
void HistogramSharedMemory::AddToLaunchParameters(
- UnsafeSharedMemoryRegion histogram_shmem_region,
+ const UnsafeSharedMemoryRegion& histogram_shmem_region,
#if BUILDFLAG(IS_POSIX) && !BUILDFLAG(IS_APPLE)
GlobalDescriptors::Key descriptor_key,
ScopedFD& descriptor_to_share,
#endif
CommandLine* command_line,
LaunchOptions* launch_options) {
+ CHECK(histogram_shmem_region.IsValid());
CHECK(command_line);
-
- const std::string process_type = command_line->GetSwitchValueASCII("type");
- const bool enabled = PassOnCommandLineIsEnabled(process_type);
-
- DVLOG(1) << (enabled ? "A" : "Not a")
- << "dding histogram shared memory launch parameters for "
- << process_type << " process.";
-
- if (!enabled) {
- return;
- }
-
shared_memory::AddToLaunchParameters(::switches::kMetricsSharedMemoryHandle,
- std::move(histogram_shmem_region),
+ histogram_shmem_region,
#if BUILDFLAG(IS_APPLE)
kRendezvousKey,
#elif BUILDFLAG(IS_POSIX)
diff --git a/base/metrics/histogram_shared_memory.h b/base/metrics/histogram_shared_memory.h
index 88f39fa6..d16d5d2 100644
--- a/base/metrics/histogram_shared_memory.h
+++ b/base/metrics/histogram_shared_memory.h
@@ -85,8 +85,8 @@
#endif
// Returns true if passing the shared memory handle via command-line arguments
- // is enabled.
- static bool PassOnCommandLineIsEnabled(std::string_view process_type);
+ // is enabled. |process_type| values should come from content:::ProcessType.
+ static bool PassOnCommandLineIsEnabled(int process_type);
// Updates the launch parameters to share |unsafe_memory_region| to a
// child process that is about to be launched. This should be called in the
@@ -97,7 +97,7 @@
// caller is expected to transmit the descriptor to the launch flow for the
// zygote.
static void AddToLaunchParameters(
- UnsafeSharedMemoryRegion unsafe_memory_region,
+ const UnsafeSharedMemoryRegion& unsafe_memory_region,
#if BUILDFLAG(IS_POSIX) && !BUILDFLAG(IS_APPLE)
GlobalDescriptors::Key descriptor_key,
ScopedFD& descriptor_to_share,
diff --git a/base/metrics/histogram_shared_memory_unittest.cc b/base/metrics/histogram_shared_memory_unittest.cc
index 9f1f96a..182e733 100644
--- a/base/metrics/histogram_shared_memory_unittest.cc
+++ b/base/metrics/histogram_shared_memory_unittest.cc
@@ -53,34 +53,46 @@
EXPECT_EQ(kArbitrarySize, shared_memory->allocator->size());
}
-TEST(HistogramSharedMemoryTest, PassSharedMemoryRegion_Disabled) {
- // Ensure the feature is disabled.
+namespace {
+// Constants from content::ProcessType;
+constexpr int PROCESS_TYPE_RENDERER = 3;
+constexpr int PROCESS_TYPE_GPU = 9;
+constexpr int PROCESS_TYPE_UTILITY = 6;
+} // namespace
+
+TEST(HistogramSharedMemoryTest, PassOnCommandLineIsDisabled) {
test::ScopedFeatureList feature_list;
feature_list.InitAndDisableFeature(kPassHistogramSharedMemoryOnLaunch);
- // Create a shared memory region to pass.
- auto memory = UnsafeSharedMemoryRegion::Create(kArbitrarySize);
- ASSERT_TRUE(memory.IsValid());
+ EXPECT_FALSE(
+ HistogramSharedMemory::PassOnCommandLineIsEnabled(PROCESS_TYPE_RENDERER));
+ EXPECT_FALSE(
+ HistogramSharedMemory::PassOnCommandLineIsEnabled(PROCESS_TYPE_GPU));
+ EXPECT_FALSE(
+ HistogramSharedMemory::PassOnCommandLineIsEnabled(PROCESS_TYPE_UTILITY));
+}
- // Initialize the command line and launch options.
- CommandLine command_line = GetMultiProcessTestChildBaseCommandLine();
- command_line.AppendSwitchASCII("type", "test-child");
- LaunchOptions launch_options;
+TEST(HistogramSharedMemoryTest, PassOnCommandLineIsEnabled) {
+ test::ScopedFeatureList feature_list;
+ feature_list.InitAndEnableFeature(kPassHistogramSharedMemoryOnLaunch);
-#if BUILDFLAG(IS_POSIX) && !BUILDFLAG(IS_APPLE)
- ScopedFD descriptor_to_share;
-#endif
+ EXPECT_TRUE(
+ HistogramSharedMemory::PassOnCommandLineIsEnabled(PROCESS_TYPE_RENDERER));
+#if BUILDFLAG(IS_CHROMEOS)
+ EXPECT_FALSE(
+ HistogramSharedMemory::PassOnCommandLineIsEnabled(PROCESS_TYPE_GPU));
+#else // !BUILDFLAG(IS_CHROMEOS)
+ EXPECT_TRUE(
+ HistogramSharedMemory::PassOnCommandLineIsEnabled(PROCESS_TYPE_GPU));
+#endif // !BUILDFLAG(IS_CHROMEOS)
- // Update the launch parameters.
- HistogramSharedMemory::AddToLaunchParameters(memory.Duplicate(),
-#if BUILDFLAG(IS_POSIX) && !BUILDFLAG(IS_APPLE)
- kArbitraryDescriptorKey,
- descriptor_to_share,
-#endif // BUILDFLAG(IS_POSIX)
- &command_line, &launch_options);
-
- // The metrics shared memory handle should NOT be added to the command line.
- EXPECT_FALSE(command_line.HasSwitch(switches::kMetricsSharedMemoryHandle));
+#if BUILDFLAG(IS_ANDROID)
+ EXPECT_FALSE(
+ HistogramSharedMemory::PassOnCommandLineIsEnabled(PROCESS_TYPE_UTILITY));
+#else // !BUILDFLAG(IS_ANDROID)
+ EXPECT_TRUE(
+ HistogramSharedMemory::PassOnCommandLineIsEnabled(PROCESS_TYPE_UTILITY));
+#endif // !BUILDFLAG(IS_ANDROID)
}
MULTIPROCESS_TEST_MAIN(InitFromLaunchParameters) {
@@ -142,7 +154,7 @@
#endif
// Update the launch parameters.
- HistogramSharedMemory::AddToLaunchParameters(memory.Duplicate(),
+ HistogramSharedMemory::AddToLaunchParameters(memory,
#if BUILDFLAG(IS_POSIX) && !BUILDFLAG(IS_APPLE)
kArbitraryDescriptorKey,
descriptor_to_share,
