From ec8529652dbb13c7b9fb39b0e5038ea61c69cf60 Mon Sep 17 00:00:00 2001 From: Alex Franchuk Date: Wed, 12 Mar 2025 03:17:49 +0000 Subject: [PATCH] Bug 1951925 - Fix base content memory usage regressions r=nika This disarms a footgun in the FreezableMapping::Freeze API, which was causing read-only mappings in the global stylesheet cache to fail. These failures caused content processes to re-load the style sheets, increasing memory usage. Differential Revision: https://phabricator.services.mozilla.com/D240928 --- dom/ipc/MemMapSnapshot.cpp | 2 +- gfx/thebes/SharedFontList.cpp | 2 +- intl/hyphenation/glue/nsHyphenator.cpp | 11 ++--------- ipc/glue/SharedMemoryMapping.cpp | 11 ++++++++--- ipc/glue/SharedMemoryMapping.h | 7 ++++++- ipc/gtest/TestSharedMemory.cpp | 24 ++++++++++++++++++------ js/xpconnect/src/XPCSelfHostedShmem.cpp | 2 +- layout/style/GlobalStyleSheetCache.cpp | 2 +- 8 files changed, 38 insertions(+), 23 deletions(-) diff --git a/dom/ipc/MemMapSnapshot.cpp b/dom/ipc/MemMapSnapshot.cpp index ccadf614c3c9..5e08c1f53bf3 100644 --- a/dom/ipc/MemMapSnapshot.cpp +++ b/dom/ipc/MemMapSnapshot.cpp @@ -32,7 +32,7 @@ Result MemMapSnapshot::Init(size_t aSize) { Result MemMapSnapshot::Finalize() { MOZ_ASSERT(mMem); - auto [_, readOnlyHandle] = std::move(mMem).Freeze(); + auto readOnlyHandle = std::move(mMem).Freeze(); if (NS_WARN_IF(!readOnlyHandle)) { return Err(NS_ERROR_FAILURE); } diff --git a/gfx/thebes/SharedFontList.cpp b/gfx/thebes/SharedFontList.cpp index da2744fcee21..e0951fac89a2 100644 --- a/gfx/thebes/SharedFontList.cpp +++ b/gfx/thebes/SharedFontList.cpp @@ -804,7 +804,7 @@ bool FontList::AppendShmBlock(uint32_t aSizeNeeded) { MOZ_CRASH("failed to create shared memory"); return false; } - auto [newShm, readOnly] = std::move(handle).Map().Freeze(); + auto [readOnly, newShm] = std::move(handle).Map().FreezeWithMutableMapping(); if (!newShm || !newShm.Address()) { MOZ_CRASH("failed to map shared memory"); return false; diff --git a/intl/hyphenation/glue/nsHyphenator.cpp b/intl/hyphenation/glue/nsHyphenator.cpp index 46e4038dd3f3..18c39ed2ae3e 100644 --- a/intl/hyphenation/glue/nsHyphenator.cpp +++ b/intl/hyphenation/glue/nsHyphenator.cpp @@ -109,8 +109,7 @@ static ipc::ReadOnlySharedMemoryHandle CopyToShmem(const CompiledData* aData) { } memcpy(buffer, mapped_hyph_compiled_data_ptr(aData), size); - auto [_, readOnlyHandle] = std::move(map).Freeze(); - return std::move(readOnlyHandle); + return std::move(map).Freeze(); } static ipc::ReadOnlySharedMemoryHandle LoadFromURI(nsIURI* aURI, @@ -161,13 +160,7 @@ static ipc::ReadOnlySharedMemoryHandle LoadFromURI(nsIURI* aURI, return nullptr; } - auto [_, readOnlyHandle] = std::move(map).Freeze(); - - if (!readOnlyHandle) { - return nullptr; - } - - return std::move(readOnlyHandle); + return std::move(map).Freeze(); } // Read from the URI into a temporary buffer, compile it, then copy the diff --git a/ipc/glue/SharedMemoryMapping.cpp b/ipc/glue/SharedMemoryMapping.cpp index f3e07197640c..ece5e5a46af3 100644 --- a/ipc/glue/SharedMemoryMapping.cpp +++ b/ipc/glue/SharedMemoryMapping.cpp @@ -164,10 +164,15 @@ FreezableMapping::Mapping(FreezableHandle&& aHandle, uint64_t aOffset, MapSubregion(mHandle, aOffset, aSize, aFixedAddress, false); } -std::tuple FreezableMapping::Freeze() && { +ReadOnlyHandle FreezableMapping::Freeze() && { + return std::move(*this).Unmap().Freeze(); +} + +std::tuple +FreezableMapping::FreezeWithMutableMapping() && { auto handle = std::move(mHandle); - return std::make_tuple(ConvertMappingTo(std::move(*this)), - std::move(handle).Freeze()); + return std::make_tuple(std::move(handle).Freeze(), + ConvertMappingTo(std::move(*this))); } FreezableHandle FreezableMapping::Unmap() && { diff --git a/ipc/glue/SharedMemoryMapping.h b/ipc/glue/SharedMemoryMapping.h index 79b90f7c1040..0af7188f9ed3 100644 --- a/ipc/glue/SharedMemoryMapping.h +++ b/ipc/glue/SharedMemoryMapping.h @@ -220,13 +220,18 @@ struct Mapping : MappingData { Mapping(FreezableHandle&& aHandle, uint64_t aOffset, size_t aSize, void* aFixedAddress = nullptr); + /** + * Freeze the shared memory region. + */ + ReadOnlyHandle Freeze() &&; + /** * Freeze the shared memory region. * * The returned Mapping will still be valid and writable until it is deleted, * however no new writable mappings can be created. */ - std::tuple Freeze() &&; + std::tuple FreezeWithMutableMapping() &&; /** * Unmap the shared memory, returning the freezable handle. diff --git a/ipc/gtest/TestSharedMemory.cpp b/ipc/gtest/TestSharedMemory.cpp index ddc8f4799fba..73eb449489f6 100644 --- a/ipc/gtest/TestSharedMemory.cpp +++ b/ipc/gtest/TestSharedMemory.cpp @@ -226,12 +226,22 @@ TEST(IPCSharedMemoryMapping, FreezableFreeze) auto handle = shared_memory::CreateFreezable(1); auto mapping = std::move(handle).Map(); - auto [m, roHandle] = std::move(mapping).Freeze(); + auto roHandle = std::move(mapping).Freeze(); ASSERT_SHMEM(mapping, 0); - ASSERT_SHMEM(m, 1); ASSERT_SHMEM(roHandle, 1); } +TEST(IPCSharedMemoryMapping, FreezableFreezeWithMutableMapping) +{ + auto handle = shared_memory::CreateFreezable(1); + + auto mapping = std::move(handle).Map(); + auto [roHandle, m] = std::move(mapping).FreezeWithMutableMapping(); + ASSERT_SHMEM(mapping, 0); + ASSERT_SHMEM(roHandle, 1); + ASSERT_SHMEM(m, 1); +} + TEST(IPCSharedMemoryMapping, FreezableUnmap) { auto handle = shared_memory::CreateFreezable(1); @@ -258,7 +268,7 @@ TEST(IPCSharedMemory, FreezeAndMapRW) *mem = 'A'; // Freeze - auto [rwMapping, roHandle] = std::move(mapping).Freeze(); + auto [roHandle, rwMapping] = std::move(mapping).FreezeWithMutableMapping(); ASSERT_TRUE(rwMapping); ASSERT_TRUE(roHandle); @@ -287,7 +297,7 @@ TEST(IPCSharedMemory, FreezeAndReprotect) *mem = 'A'; // Freeze - auto [rwMapping, roHandle] = std::move(mapping).Freeze(); + auto [roHandle, rwMapping] = std::move(mapping).FreezeWithMutableMapping(); ASSERT_TRUE(rwMapping); ASSERT_TRUE(roHandle); @@ -387,7 +397,8 @@ TEST(IPCSharedMemory, ROCopyAndWrite) auto handle = ipc::shared_memory::CreateFreezable(1); ASSERT_TRUE(handle); - auto [rwMapping, roHandle] = std::move(handle).Map().Freeze(); + auto [roHandle, rwMapping] = + std::move(handle).Map().FreezeWithMutableMapping(); ASSERT_TRUE(rwMapping); ASSERT_TRUE(roHandle); @@ -412,7 +423,8 @@ TEST(IPCSharedMemory, ROCopyAndRewrite) auto handle = ipc::shared_memory::CreateFreezable(1); ASSERT_TRUE(handle); - auto [rwMapping, roHandle] = std::move(handle).Map().Freeze(); + auto [roHandle, rwMapping] = + std::move(handle).Map().FreezeWithMutableMapping(); ASSERT_TRUE(rwMapping); ASSERT_TRUE(roHandle); diff --git a/js/xpconnect/src/XPCSelfHostedShmem.cpp b/js/xpconnect/src/XPCSelfHostedShmem.cpp index f0ab36899ee7..4aee9dc17276 100644 --- a/js/xpconnect/src/XPCSelfHostedShmem.cpp +++ b/js/xpconnect/src/XPCSelfHostedShmem.cpp @@ -56,7 +56,7 @@ void xpc::SelfHostedShmem::InitFromParent(ContentType aXdr) { void* address = mapping.Address(); memcpy(address, aXdr.Elements(), aXdr.LengthBytes()); - std::tie(std::ignore, mHandle) = std::move(mapping).Freeze(); + mHandle = std::move(mapping).Freeze(); mMem = mHandle.Map(); } diff --git a/layout/style/GlobalStyleSheetCache.cpp b/layout/style/GlobalStyleSheetCache.cpp index 416b5972eb1f..24e0e5352a3e 100644 --- a/layout/style/GlobalStyleSheetCache.cpp +++ b/layout/style/GlobalStyleSheetCache.cpp @@ -393,7 +393,7 @@ void GlobalStyleSheetCache::InitSharedSheetsInParent() { // Finished writing into the shared memory. Freeze it, so that a process // can't confuse other processes by changing the UA style sheet contents. - auto [_, readOnlyHandle] = std::move(mapping).Freeze(); + auto readOnlyHandle = std::move(mapping).Freeze(); if (NS_WARN_IF(!readOnlyHandle)) { return; }