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/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,
