From cba38085a5d4a331ba67cb9e634c63dc43ab93c1 Mon Sep 17 00:00:00 2001 From: Max Karolinskiy <41635752+mkarolin@users.noreply.github.com> Date: Wed, 29 Jan 2025 09:30:29 -0500 Subject: [PATCH] Merge pull request #27371 from brave/maxk-cr133-upstream-memory-pressure-fix MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit [cr133 upstream fix] Fix Prevent Hang in Memory Pressure Calculation … --- ...tem_memory_pressure_evaluator_win.cc.patch | 109 +++++++++ ...stem_memory_pressure_evaluator_win.h.patch | 29 +++ ...y_pressure_evaluator_win_unittest.cc.patch | 214 ++++++++++++++++++ ...grams-metadata-memory-histograms.xml.patch | 85 +++++++ 4 files changed, 437 insertions(+) create mode 100644 patches/components-memory_pressure-system_memory_pressure_evaluator_win.cc.patch create mode 100644 patches/components-memory_pressure-system_memory_pressure_evaluator_win.h.patch create mode 100644 patches/components-memory_pressure-system_memory_pressure_evaluator_win_unittest.cc.patch create mode 100644 patches/tools-metrics-histograms-metadata-memory-histograms.xml.patch diff --git a/patches/components-memory_pressure-system_memory_pressure_evaluator_win.cc.patch b/patches/components-memory_pressure-system_memory_pressure_evaluator_win.cc.patch new file mode 100644 index 000000000000..d4613ab9e46a --- /dev/null +++ b/patches/components-memory_pressure-system_memory_pressure_evaluator_win.cc.patch @@ -0,0 +1,109 @@ +diff --git a/components/memory_pressure/system_memory_pressure_evaluator_win.cc b/components/memory_pressure/system_memory_pressure_evaluator_win.cc +index 0984139efc13d74bbfae45b3160ca55a6c02db34..3ac862eaee20c1c726c2f147a5cdc61026c739ed 100644 +--- a/components/memory_pressure/system_memory_pressure_evaluator_win.cc ++++ b/components/memory_pressure/system_memory_pressure_evaluator_win.cc +@@ -13,6 +13,7 @@ + + #include "base/functional/bind.h" + #include "base/metrics/histogram_functions.h" ++#include "base/numerics/safe_conversions.h" + #include "base/system/sys_info.h" + #include "base/task/sequenced_task_runner.h" + #include "base/task/single_thread_task_runner.h" +@@ -222,16 +223,16 @@ void SystemMemoryPressureEvaluator::CheckMemoryPressure() { + + base::MemoryPressureListener::MemoryPressureLevel + SystemMemoryPressureEvaluator::CalculateCurrentPressureLevel() { +- // First to try to collect commit limit histograms. Otherwise, there's a +- // chance of an early exit from this function (e.g. if +- // `GetSystemMemoryStatus` fails), which would cause us to miss collecting the +- // histogram data. +- RecordCommitHistograms(); +- + MEMORYSTATUSEX mem_status = {}; +- if (!GetSystemMemoryStatus(&mem_status)) { ++ bool got_system_memory_status = GetSystemMemoryStatus(&mem_status); ++ // Report retrieval outcome before early returning on failure. ++ base::UmaHistogramBoolean("Memory.MemoryStatusRetrievalSuccess", ++ got_system_memory_status); ++ ++ if (!got_system_memory_status) { + return base::MemoryPressureListener::MEMORY_PRESSURE_LEVEL_NONE; + } ++ RecordCommitHistograms(mem_status); + + // How much system memory is actively available for use right now, in MBs. + int phys_free = static_cast(mem_status.ullAvailPhys / kMBBytes); +@@ -268,58 +269,33 @@ bool SystemMemoryPressureEvaluator::GetSystemMemoryStatus( + return true; + } + +-void SystemMemoryPressureEvaluator::RecordCommitHistograms() { +- PERFORMANCE_INFORMATION performance_info = {}; +- bool get_performance_info_result = +- GetPerformanceInfoWrapper(&performance_info); +- base::UmaHistogramBoolean("Memory.PerformanceInfoRetrievalSuccess", +- get_performance_info_result); +- +- if (!get_performance_info_result) { +- return; +- } ++void SystemMemoryPressureEvaluator::RecordCommitHistograms( ++ const MEMORYSTATUSEX& mem_status) { + // Calculate commit limit in MB. +- uint64_t commit_limit_mb = +- base::strict_cast(performance_info.CommitLimit * +- performance_info.PageSize) / +- kMBBytes; +- // Calculate amount of memory in MB currently committed by the system. +- uint64_t commit_total_mb = +- base::strict_cast(performance_info.CommitTotal * +- performance_info.PageSize) / +- kMBBytes; +- +- // Calculate remaining commit limit in MB. +- uint64_t remaining_limit_mb = commit_limit_mb - commit_total_mb; ++ uint64_t commit_limit_mb = mem_status.ullTotalPageFile / kMBBytes; + +- int commit_limit_int = std::clamp(static_cast(commit_limit_mb), 0, +- std::numeric_limits::max()); ++ // Calculate amount of available commit space in MB. ++ uint64_t commit_available_mb = mem_status.ullAvailPageFile / kMBBytes; + +- int remaining_limit_int = std::clamp(static_cast(remaining_limit_mb), 0, +- std::numeric_limits::max()); +- +- base::UmaHistogramCounts10M("Memory.CommitLimitMB", commit_limit_int); +- base::UmaHistogramCounts10M("Memory.CommitRemainingMB", remaining_limit_int); ++ base::UmaHistogramCounts10M("Memory.CommitLimitMB", ++ base::saturated_cast(commit_limit_mb)); ++ base::UmaHistogramCounts10M("Memory.CommitAvailableMB", ++ base::saturated_cast(commit_available_mb)); + + // Calculate percentage used + int percentage_used; +- if (commit_limit_int == 0) { ++ if (commit_limit_mb == 0) { + // Handle division by zero. + percentage_used = 0; + } else { +- uint64_t temp_percentage = (commit_total_mb * 100) / commit_limit_mb; +- percentage_used = base::saturated_cast(temp_percentage); ++ uint64_t percentage_remaining = ++ (commit_available_mb * 100) / commit_limit_mb; ++ percentage_used = static_cast( ++ percentage_remaining > 100 ? 0u : 100 - percentage_remaining); + } + + base::UmaHistogramPercentage("Memory.CommitPercentageUsed", percentage_used); + } + +-bool SystemMemoryPressureEvaluator::GetPerformanceInfoWrapper( +- PERFORMANCE_INFORMATION* perf_info) { +- DCHECK(perf_info); +- +- return GetPerformanceInfo(perf_info, sizeof(*perf_info)); +-} +- + } // namespace win + } // namespace memory_pressure diff --git a/patches/components-memory_pressure-system_memory_pressure_evaluator_win.h.patch b/patches/components-memory_pressure-system_memory_pressure_evaluator_win.h.patch new file mode 100644 index 000000000000..b96f480b3db3 --- /dev/null +++ b/patches/components-memory_pressure-system_memory_pressure_evaluator_win.h.patch @@ -0,0 +1,29 @@ +diff --git a/components/memory_pressure/system_memory_pressure_evaluator_win.h b/components/memory_pressure/system_memory_pressure_evaluator_win.h +index 984d2db2a732b37088fe824fb1b918243205c3c3..8ac9ced5cfb5f66b470f6119cbaa5b4473fa8f82 100644 +--- a/components/memory_pressure/system_memory_pressure_evaluator_win.h ++++ b/components/memory_pressure/system_memory_pressure_evaluator_win.h +@@ -17,7 +17,6 @@ + + // To not pull in windows.h. + typedef struct _MEMORYSTATUSEX MEMORYSTATUSEX; +-typedef struct _PERFORMANCE_INFORMATION PERFORMANCE_INFORMATION; + + namespace memory_pressure { + namespace win { +@@ -106,14 +105,8 @@ class SystemMemoryPressureEvaluator + // thread. + virtual bool GetSystemMemoryStatus(MEMORYSTATUSEX* mem_status); + +- // Gets system performance information. This is virtual as a unittesting hook. +- // Returns true if the system call succeeds, false otherwise. Can be called on +- // any thread. +- virtual bool GetPerformanceInfoWrapper(PERFORMANCE_INFORMATION* perf_info); +- +- // Gets the commit information, calculate commit usage, and record a +- // histogram. +- void RecordCommitHistograms(); ++ // Records histograms about committed memory based on `mem_status`. ++ static void RecordCommitHistograms(const MEMORYSTATUSEX& mem_status); + + private: + // Threshold amounts of available memory that trigger pressure levels. See diff --git a/patches/components-memory_pressure-system_memory_pressure_evaluator_win_unittest.cc.patch b/patches/components-memory_pressure-system_memory_pressure_evaluator_win_unittest.cc.patch new file mode 100644 index 000000000000..07686757b602 --- /dev/null +++ b/patches/components-memory_pressure-system_memory_pressure_evaluator_win_unittest.cc.patch @@ -0,0 +1,214 @@ +diff --git a/components/memory_pressure/system_memory_pressure_evaluator_win_unittest.cc b/components/memory_pressure/system_memory_pressure_evaluator_win_unittest.cc +index 85781232c9edb57c10182b6178c84422793d4656..fe4ad74ea779df9c895daa8417f8ac9d4a732a45 100644 +--- a/components/memory_pressure/system_memory_pressure_evaluator_win_unittest.cc ++++ b/components/memory_pressure/system_memory_pressure_evaluator_win_unittest.cc +@@ -16,8 +16,6 @@ + + #if BUILDFLAG(IS_WIN) + #include +- +-#include + #endif + + namespace memory_pressure { +@@ -30,10 +28,8 @@ struct PressureSettings { + base::MemoryPressureListener::MemoryPressureLevel level; + }; + +-const char kPerformanceInfoRetrievalSuccessHistogramName[] = +- "Memory.PerformanceInfoRetrievalSuccess"; + const char kCommitLimitMBHistogramName[] = "Memory.CommitLimitMB"; +-const char kCommitRemainingMBHistogramName[] = "Memory.CommitRemainingMB"; ++const char kCommitAvailableMBHistogramName[] = "Memory.CommitAvailableMB"; + const char kCommitPercentageUsedHistogramName[] = "Memory.CommitPercentageUsed"; + + } // namespace +@@ -108,21 +104,14 @@ class TestSystemMemoryPressureEvaluator : public SystemMemoryPressureEvaluator { + + // These fields are unused. + mem_status_.dwMemoryLoad = 0; +- mem_status_.ullTotalPageFile = 0; +- mem_status_.ullAvailPageFile = 0; + mem_status_.ullTotalVirtual = 0; + mem_status_.ullAvailVirtual = 0; + } + +- // Sets up the memory status to reflect the provided performance information. +- void SetCommitLimit(size_t commit_limit, size_t commit_total) { +- perf_info_.CommitLimit = commit_limit; +- perf_info_.CommitTotal = commit_total; +- perf_info_.PageSize = 4096; +- } +- +- void SetPerformanceRetrievalSuccessCall(bool perf_info_success) { +- perfomance_info_retrieval_ = perf_info_success; ++ // Sets up the memory status to reflect commit limit and available. ++ void SetCommitData(uint64_t commit_limit_mb, uint64_t commit_available_mb) { ++ mem_status_.ullTotalPageFile = commit_limit_mb * kMBBytes; ++ mem_status_.ullAvailPageFile = commit_available_mb * kMBBytes; + } + + void SetNone() { SetMemoryFree(moderate_threshold_mb() + 1); } +@@ -131,6 +120,8 @@ class TestSystemMemoryPressureEvaluator : public SystemMemoryPressureEvaluator { + + void SetCritical() { SetMemoryFree(critical_threshold_mb() - 1); } + ++ MEMORYSTATUSEX GetSystemMemoryStatusForTesting() { return mem_status_; } ++ + private: + bool GetSystemMemoryStatus(MEMORYSTATUSEX* mem_status) override { + // Simply copy the memory status set by the test fixture. +@@ -138,21 +129,7 @@ class TestSystemMemoryPressureEvaluator : public SystemMemoryPressureEvaluator { + return true; + } + +- bool GetPerformanceInfoWrapper(PERFORMANCE_INFORMATION* perf_info) override { +- perf_info_.cb = sizeof(PERFORMANCE_INFORMATION); +- if (perf_info_.PageSize < 1) { +- // Hard-coded here just before copying so they're always valid values in +- // case PageSize is 0 to prevent a crash on division. +- perf_info_.PageSize = 4096; +- } +- // Simply copy the memory status set by the test fixture. +- *perf_info = perf_info_; +- return perfomance_info_retrieval_; +- } +- + MEMORYSTATUSEX mem_status_; +- PERFORMANCE_INFORMATION perf_info_{}; +- bool perfomance_info_retrieval_ = false; + }; + + class WinSystemMemoryPressureEvaluatorTest : public testing::Test { +@@ -355,94 +332,71 @@ TEST_F(WinSystemMemoryPressureEvaluatorTest, CheckMemoryPressure) { + testing::Mock::VerifyAndClearExpectations(&evaluator); + } + +-// RecordCommitHistograms emits the correct histograms when GetPerformanceInfo +-// succeeds. +-TEST_F(WinSystemMemoryPressureEvaluatorTest, GetPerformanceInfoSucceeds) { ++// RecordCommitHistograms emits the correct histograms when ++// GetSystemMemoryStatus succeeds. ++TEST_F(WinSystemMemoryPressureEvaluatorTest, RecordCommitHistogramsBasic) { + base::HistogramTester histogram_tester; + TestSystemMemoryPressureEvaluator evaluator(false, nullptr); +- evaluator.SetPerformanceRetrievalSuccessCall(true); +- evaluator.SetCommitLimit(1024, 512); + +- evaluator.RecordCommitHistograms(); ++ evaluator.SetCommitData(/*commit_limit_mb=*/4096, ++ /*commit_available_mb=*/2048); ++ ++ evaluator.RecordCommitHistograms(evaluator.GetSystemMemoryStatusForTesting()); + +- histogram_tester.ExpectUniqueSample( +- kPerformanceInfoRetrievalSuccessHistogramName, true, 1); +- histogram_tester.ExpectUniqueSample(kCommitLimitMBHistogramName, 4, 1); +- histogram_tester.ExpectUniqueSample(kCommitRemainingMBHistogramName, 2, 1); ++ histogram_tester.ExpectUniqueSample(kCommitLimitMBHistogramName, 4096, 1); ++ histogram_tester.ExpectUniqueSample(kCommitAvailableMBHistogramName, 2048, 1); + histogram_tester.ExpectUniqueSample(kCommitPercentageUsedHistogramName, 50, + 1); + } + +-// RecordCommitHistograms emits only the +-// "Memory.PerformanceInfoRetrievalSuccess" histogram when GetPerformanceInfo +-// fails. Should not emit any other histograms. +-TEST_F(WinSystemMemoryPressureEvaluatorTest, GetPerformanceInfoFails) { ++// Verifies behavior when commit limit is zero (division by zero). ++TEST_F(WinSystemMemoryPressureEvaluatorTest, ++ RecordCommitHistogramsDivisionByZero) { + base::HistogramTester histogram_tester; + TestSystemMemoryPressureEvaluator evaluator(false, nullptr); +- evaluator.SetPerformanceRetrievalSuccessCall(false); +- evaluator.SetCommitLimit(1000, 500); + +- evaluator.RecordCommitHistograms(); ++ evaluator.SetCommitData(/*commit_limit_mb=*/0, /*commit_available_mb=*/0); + +- histogram_tester.ExpectUniqueSample( +- kPerformanceInfoRetrievalSuccessHistogramName, false, 1); +- histogram_tester.ExpectTotalCount(kCommitLimitMBHistogramName, 0); +- histogram_tester.ExpectTotalCount(kCommitRemainingMBHistogramName, 0); +- histogram_tester.ExpectTotalCount(kCommitPercentageUsedHistogramName, 0); ++ evaluator.RecordCommitHistograms(evaluator.GetSystemMemoryStatusForTesting()); ++ ++ histogram_tester.ExpectUniqueSample(kCommitLimitMBHistogramName, 0, 1); ++ histogram_tester.ExpectUniqueSample(kCommitAvailableMBHistogramName, 0, 1); ++ histogram_tester.ExpectUniqueSample(kCommitPercentageUsedHistogramName, 0, 1); + } + + // RecordCommitHistograms should be able to handle commit values greater than + // 32-bit integers to calculate and correctly output all histograms. +-TEST_F(WinSystemMemoryPressureEvaluatorTest, GetPerformanceInfoOverflows) { ++TEST_F(WinSystemMemoryPressureEvaluatorTest, RecordCommitHistogramsOverflow) { + base::HistogramTester histogram_tester; + TestSystemMemoryPressureEvaluator evaluator(false, nullptr); +- const int max_int_value = std::numeric_limits::max(); +- evaluator.SetPerformanceRetrievalSuccessCall(true); +- evaluator.SetCommitLimit(max_int_value, max_int_value); + +- evaluator.RecordCommitHistograms(); ++ constexpr uint64_t kLargerThanMaxInt = ++ static_cast(std::numeric_limits::max()) + 1U; ++ evaluator.SetCommitData(/*commit_limit_mb=*/kLargerThanMaxInt, ++ /*commit_available_mb=*/kLargerThanMaxInt); + +- histogram_tester.ExpectUniqueSample( +- kPerformanceInfoRetrievalSuccessHistogramName, true, 1); +- histogram_tester.ExpectUniqueSample(kCommitLimitMBHistogramName, 8388607, 1); +- histogram_tester.ExpectUniqueSample(kCommitRemainingMBHistogramName, 0, 1); +- histogram_tester.ExpectUniqueSample(kCommitPercentageUsedHistogramName, 100, +- 1); +-} +- +-// Verifies behavior when commit limit is zero (division by zero). +-TEST_F(WinSystemMemoryPressureEvaluatorTest, DivisionByZero) { +- base::HistogramTester histogram_tester; +- TestSystemMemoryPressureEvaluator evaluator(false, nullptr); +- evaluator.SetPerformanceRetrievalSuccessCall(true); +- evaluator.SetCommitLimit(0, 0); // Commit limit is zero. +- +- evaluator.RecordCommitHistograms(); ++ evaluator.RecordCommitHistograms(evaluator.GetSystemMemoryStatusForTesting()); + +- histogram_tester.ExpectUniqueSample( +- kPerformanceInfoRetrievalSuccessHistogramName, true, 1); +- histogram_tester.ExpectUniqueSample(kCommitLimitMBHistogramName, 0, 1); +- histogram_tester.ExpectUniqueSample(kCommitRemainingMBHistogramName, 0, 1); +- histogram_tester.ExpectUniqueSample(kCommitPercentageUsedHistogramName, 0, 1); ++ histogram_tester.ExpectUniqueSample(kCommitLimitMBHistogramName, 10000000, 1); ++ histogram_tester.ExpectUniqueSample(kCommitAvailableMBHistogramName, 10000000, ++ 1); + } + +-// Verifies behavior with potential underflow when calculating remaining commit +-// limit. ++// Verifies that RecordCommitHistograms correctly handles the calculation of ++// Memory.CommitPercentageUsed, specifically addressing the potential for ++// underflow in that calculation. + TEST_F(WinSystemMemoryPressureEvaluatorTest, PotentialUnderflow) { + base::HistogramTester histogram_tester; + TestSystemMemoryPressureEvaluator evaluator(false, nullptr); +- evaluator.SetPerformanceRetrievalSuccessCall(true); +- // Set a commit total that is greater than commit limit +- evaluator.SetCommitLimit(1024, 2048); + +- evaluator.RecordCommitHistograms(); ++ evaluator.SetCommitData(/*commit_limit_mb=*/50, ++ /*commit_available_mb=*/100); + +- histogram_tester.ExpectUniqueSample( +- kPerformanceInfoRetrievalSuccessHistogramName, true, 1); +- histogram_tester.ExpectUniqueSample(kCommitLimitMBHistogramName, 4, 1); +- histogram_tester.ExpectUniqueSample(kCommitRemainingMBHistogramName, 0, 1); +- histogram_tester.ExpectUniqueSample(kCommitPercentageUsedHistogramName, 200, +- 1); ++ evaluator.RecordCommitHistograms(evaluator.GetSystemMemoryStatusForTesting()); ++ ++ histogram_tester.ExpectUniqueSample(kCommitLimitMBHistogramName, 50, 1); ++ histogram_tester.ExpectUniqueSample(kCommitAvailableMBHistogramName, 100, 1); ++ histogram_tester.ExpectUniqueSample(kCommitPercentageUsedHistogramName, 0, 1); + } + + } // namespace win diff --git a/patches/tools-metrics-histograms-metadata-memory-histograms.xml.patch b/patches/tools-metrics-histograms-metadata-memory-histograms.xml.patch new file mode 100644 index 000000000000..29835b6591f6 --- /dev/null +++ b/patches/tools-metrics-histograms-metadata-memory-histograms.xml.patch @@ -0,0 +1,85 @@ +diff --git a/tools/metrics/histograms/metadata/memory/histograms.xml b/tools/metrics/histograms/metadata/memory/histograms.xml +index def2bd6b0ba3b1257b9b78ccbf934d0276490047..67d0365b331d54d2b81a698f4e17b6c1fe16a246 100644 +--- a/tools/metrics/histograms/metadata/memory/histograms.xml ++++ b/tools/metrics/histograms/metadata/memory/histograms.xml +@@ -436,6 +436,21 @@ chromium-metrics-reviews@google.com. + TBD. + + ++ ++ aattar@google.com ++ catan-team@chromium.org ++ ++ Reported for Windows only. This represents the amount of virtual memory ++ currently available for the system to commit, measured in megabytes. This ++ value is obtained directly from the ullAvailPageFile field of the ++ MEMORYSTATUSEX struct returned by the GlobalMemoryStatusEx API. It indicates ++ the remaining capacity for applications to commit memory before reaching the ++ system's commit limit. This value is recorded at the same time as the ++ current memory pressure signal. ++ ++ ++ + + aattar@google.com + catan-team@chromium.org +@@ -462,19 +477,6 @@ chromium-metrics-reviews@google.com. + + + +- +- aattar@google.com +- catan-team@chromium.org +- +- Reported for Windows only. Calculated as CommitLimit - TotalCommit, this +- metric indicates the amount of virtual memory still available for +- commitment. It shows how much "headroom"; the system has before it +- runs out of committable memory. This value is recorded at the same time as +- the current memory pressure signal. +- +- +- + + thiabaud@google.com +@@ -1340,6 +1342,19 @@ chromium-metrics-reviews@google.com. + The memory size freed by each low memory kill event. + + ++ ++ aattar@google.com ++ catan-team@chromium.org ++ ++ Reported for Windows only. This status indicates whether the system ++ successfully retrieved memory status via a call to GlobalMemoryStatusEx. The ++ successful retrieval of memory status is a prerequisite for emitting metrics ++ like Memory.CommitLimitMB and Memory.CommitAvailableMB. This helps diagnose ++ issues with memory metric collection on Windows systems. ++ ++ ++ + + lizeb@chromium.org +@@ -1750,18 +1765,6 @@ chromium-metrics-reviews@google.com. + + + +- +- aattar@google.com +- catan-team@chromium.org +- +- Reported for Windows only. This status indicates whether the system +- successfully collected memory performance information, such as +- Memory.CommitLimitMB. This helps diagnose issues with memory metric +- collection on Windows systems. +- +- +- + + thiabaud@google.com