From cc8e33617215a5e6e3300bc4ee51317d8a3770ae Mon Sep 17 00:00:00 2001 From: Moonchild Date: Tue, 18 Feb 2025 21:42:59 +0100 Subject: [PATCH] [layout] Always reserve 64 bits for frame property value This ensures frame property values can always be stored even in edge cases on 32-bit machines (where a void ptr is half the width). --- layout/base/FrameProperties.h | 92 ++++++++++++----------------- layout/generic/nsContainerFrame.cpp | 12 ++-- 2 files changed, 45 insertions(+), 59 deletions(-) diff --git a/layout/base/FrameProperties.h b/layout/base/FrameProperties.h index 3884b07bd7..1a9a987776 100644 --- a/layout/base/FrameProperties.h +++ b/layout/base/FrameProperties.h @@ -68,8 +68,7 @@ template struct FramePropertyDescriptor : public FramePropertyDescriptorUntyped { typedef void Destructor(T* aPropertyValue); - typedef void DestructorWithFrame(const nsIFrame* aaFrame, - T* aPropertyValue); + typedef void DestructorWithFrame(const nsIFrame* aFrame, T* aPropertyValue); template static constexpr const FramePropertyDescriptor NewWithDestructor() @@ -109,9 +108,9 @@ private: }; // SmallValueHolder is a placeholder intended to be used as template -// argument of FramePropertyDescriptor for types which can fit into the -// size of a pointer directly. This class should never be defined, so -// that we won't use it for unexpected purpose by mistake. +// argument of FramePropertyDescriptor for types which can fit directly into our +// internal value slot (i.e. types that can fit in 64 bits). This class should +// never be defined, so that we won't use it for unexpected purpose by mistake. template class SmallValueHolder; @@ -134,12 +133,6 @@ struct FramePropertyTypeHelper> * The FrameProperties class is optimized for storing 0 or 1 properties on * a given frame. Storing very large numbers of properties on a single * frame will not be efficient. - * - * Property values are passed as void* but do not actually have to be - * valid pointers. You can use NS_INT32_TO_PTR/NS_PTR_TO_INT32 to - * store int32_t values. Null/zero values can be stored and retrieved. - * Of course, the destructor function (if any) must handle such values - * correctly. */ class FrameProperties { @@ -174,8 +167,8 @@ public: void Set(Descriptor aProperty, PropertyType aValue, const nsIFrame* aFrame) { - void* ptr = ReinterpretHelper::ToPointer(aValue); - SetInternal(aProperty, ptr, aFrame); + uint64_t v = ReinterpretHelper::ToInternalValue(aValue); + SetInternal(aProperty, v, aFrame); } /** @@ -214,8 +207,8 @@ public: PropertyType Get(Descriptor aProperty, bool* aFoundResult = nullptr) const { - void* ptr = GetInternal(aProperty, aFoundResult); - return ReinterpretHelper::FromPointer(ptr); + uint64_t v = GetInternal(aProperty, aFoundResult); + return ReinterpretHelper::FromInternalValue(v); } /** * Remove a property value. This requires a linear search through @@ -232,8 +225,8 @@ public: PropertyType Remove(Descriptor aProperty, bool* aFoundResult = nullptr) { - void* ptr = RemoveInternal(aProperty, aFoundResult); - return ReinterpretHelper::FromPointer(ptr); + uint64_t v = RemoveInternal(aProperty, aFoundResult); + return ReinterpretHelper::FromInternalValue(v); } /** * Remove and destroy a property value. This requires a linear search @@ -297,13 +290,13 @@ private: FrameProperties& operator=(const FrameProperties&) = delete; inline void - SetInternal(UntypedDescriptor aProperty, void* aValue, + SetInternal(UntypedDescriptor aProperty, uint64_t aValue, const nsIFrame* aFrame); - inline void* + inline uint64_t GetInternal(UntypedDescriptor aProperty, bool* aFoundResult) const; - inline void* + inline uint64_t RemoveInternal(UntypedDescriptor aProperty, bool* aFoundResult); inline void @@ -312,56 +305,48 @@ private: template struct ReinterpretHelper { - static_assert(sizeof(PropertyType) <= sizeof(void*), - "size of the value must never be larger than a pointer"); + static_assert(sizeof(PropertyType) <= sizeof(uint64_t), + "size of the value must never be larger than 64 bits"); - static void* ToPointer(PropertyType aValue) + static uint64_t ToInternalValue(PropertyType aValue) { - void* ptr = nullptr; - memcpy(&ptr, &aValue, sizeof(aValue)); - return ptr; + uint64_t v = 0; + memcpy(&v, &aValue, sizeof(aValue)); + return v; } - static PropertyType FromPointer(void* aPtr) + static PropertyType FromInternalValue(uint64_t aInternalValue) { PropertyType value; - memcpy(&value, &aPtr, sizeof(value)); + memcpy(&value, &aInternalValue, sizeof(value)); return value; } }; - template - struct ReinterpretHelper - { - static void* ToPointer(T* aValue) - { - return static_cast(aValue); - } - - static T* FromPointer(void* aPtr) - { - return static_cast(aPtr); - } - }; - /** * Stores a property descriptor/value pair. */ struct PropertyValue { - PropertyValue() : mProperty(nullptr), mValue(nullptr) {} - PropertyValue(UntypedDescriptor aProperty, void* aValue) + PropertyValue() : mProperty(nullptr), mValue(0) {} + PropertyValue(UntypedDescriptor aProperty, uint64_t aValue) : mProperty(aProperty), mValue(aValue) {} + // NOTE: This function converts our internal 64-bit-integer representation + // to a pointer-type representation. This is lossy on 32-bit systems, but it + // should be fine, as long as we *only* do this in cases where we're sure + // that the stored property-value is in fact a pointer. And we should have + // that assurance, since only pointer-typed frame properties are expected to + // have a destructor void DestroyValueFor(const nsIFrame* aFrame) { if (mProperty->mDestructor) { - mProperty->mDestructor(mValue); + mProperty->mDestructor(ReinterpretHelper::FromInternalValue(mValue)); } else if (mProperty->mDestructorWithFrame) { - mProperty->mDestructorWithFrame(aFrame, mValue); + mProperty->mDestructorWithFrame(aFrame, ReinterpretHelper::FromInternalValue(mValue)); } } UntypedDescriptor mProperty; - void* mValue; + uint64_t mValue; }; /** @@ -387,7 +372,7 @@ private: /** * This class encapsulates the properties of a frame. */ -inline void* +inline uint64_t FrameProperties::GetInternal(UntypedDescriptor aProperty, bool* aFoundResult) const { @@ -398,7 +383,7 @@ FrameProperties::GetInternal(UntypedDescriptor aProperty, if (aFoundResult) { *aFoundResult = false; } - return nullptr; + return 0; } if (aFoundResult) { @@ -409,7 +394,8 @@ return mProperties.ElementAt(index).mValue; } inline void -FrameProperties::SetInternal(UntypedDescriptor aProperty, void* aValue, +FrameProperties::SetInternal(UntypedDescriptor aProperty, + uint64_t aValue, const nsIFrame* aFrame) { MOZ_ASSERT(aProperty, "Null property?"); @@ -425,7 +411,7 @@ FrameProperties::SetInternal(UntypedDescriptor aProperty, void* aValue, mProperties.AppendElement(PropertyValue(aProperty, aValue)); } -inline void* +inline uint64_t FrameProperties::RemoveInternal(UntypedDescriptor aProperty, bool* aFoundResult) { MOZ_ASSERT(aProperty, "Null property?"); @@ -435,14 +421,14 @@ FrameProperties::RemoveInternal(UntypedDescriptor aProperty, bool* aFoundResult) if (aFoundResult) { *aFoundResult = false; } - return nullptr; + return 0; } if (aFoundResult) { *aFoundResult = true; } -void* result = mProperties.ElementAt(index).mValue; +uint64_t result = mProperties.ElementAt(index).mValue; mProperties.RemoveElementAt(index); return result; diff --git a/layout/generic/nsContainerFrame.cpp b/layout/generic/nsContainerFrame.cpp index be750a4c7a..d3e6e18877 100644 --- a/layout/generic/nsContainerFrame.cpp +++ b/layout/generic/nsContainerFrame.cpp @@ -225,7 +225,7 @@ nsContainerFrame::DestroyFrom(nsIFrame* aDestructRoot) if (MOZ_UNLIKELY(!mProperties.IsEmpty())) { using T = mozilla::FrameProperties::UntypedDescriptor; bool hasO = false, hasOC = false, hasEOC = false, hasBackdrop = false; - mProperties.ForEach([&] (const T& aProp, void*) { + mProperties.ForEach([&] (const T& aProp, uint64_t) { if (aProp == OverflowProperty()) { hasO = true; } else if (aProp == OverflowContainersProperty()) { @@ -306,20 +306,20 @@ nsContainerFrame::GetChildLists(nsTArray* aLists) const { mFrames.AppendIfNonempty(aLists, kPrincipalList); using T = mozilla::FrameProperties::UntypedDescriptor; - mProperties.ForEach([this, aLists] (const T& aProp, void* aValue) { + mProperties.ForEach([this, aLists] (const T& aProp, uint64_t aValue) { typedef const nsFrameList* L; if (aProp == OverflowProperty()) { - L(aValue)->AppendIfNonempty(aLists, kOverflowList); + reinterpret_cast(aValue)->AppendIfNonempty(aLists, kOverflowList); } else if (aProp == OverflowContainersProperty()) { MOZ_ASSERT(IsFrameOfType(nsIFrame::eCanContainOverflowContainers), "found unexpected OverflowContainersProperty"); - L(aValue)->AppendIfNonempty(aLists, kOverflowContainersList); + reinterpret_cast(aValue)->AppendIfNonempty(aLists, kOverflowContainersList); } else if (aProp == ExcessOverflowContainersProperty()) { MOZ_ASSERT(IsFrameOfType(nsIFrame::eCanContainOverflowContainers), "found unexpected ExcessOverflowContainersProperty"); - L(aValue)->AppendIfNonempty(aLists, kExcessOverflowContainersList); + reinterpret_cast(aValue)->AppendIfNonempty(aLists, kExcessOverflowContainersList); } else if (aProp == BackdropProperty()) { - L(aValue)->AppendIfNonempty(aLists, kBackdropList); + reinterpret_cast(aValue)->AppendIfNonempty(aLists, kBackdropList); } return true; });