From 93048fc66fb7f66389c05ab6f87ab717529efefc Mon Sep 17 00:00:00 2001 From: "L. David Baron" Date: Tue, 4 Apr 2017 20:59:21 -0700 Subject: [PATCH] Bug 1353187 - Guard access to the frame property table with a frame state bit. r=dholbert This protects all accesses to the frame property table with a bit stored on the frame. This means we avoid hashtable operations when asking about frame properties on frames that have no properties. The changes to RestyleManager, and the new HasSkippingBitCheck API, are needed because RestyleManager depended on being able to ask for properties on a deleted frame (knowing that the property in question could not have been set on any new frames since the deleted frame was destroyed), in order to use the destruction of the properties that happens at frame destruction as a mechanism for learning that the frame was destroyed. The changes there preserve the use of that mechanism, although it becomes a bit uglier. The ugliness is well-deserved. MozReview-Commit-ID: BScmDUlWq65 --HG-- extra : transplant_source : %C8%C0%CD%DC%12g%5B%8ER%3A%FF%A7a%F8%91%D4%2C%9BF%2B --- layout/base/FramePropertyTable.cpp | 32 +++++++++++++++++++++--- layout/base/FramePropertyTable.h | 40 ++++++++++++++++++++++++------ layout/base/RestyleManager.cpp | 16 +++++++++--- layout/generic/nsFrame.cpp | 5 ++-- layout/generic/nsFrameStateBits.h | 3 +++ 5 files changed, 80 insertions(+), 16 deletions(-) diff --git a/layout/base/FramePropertyTable.cpp b/layout/base/FramePropertyTable.cpp index bc824432fdce..b84a88c6b016 100644 --- a/layout/base/FramePropertyTable.cpp +++ b/layout/base/FramePropertyTable.cpp @@ -22,6 +22,7 @@ FramePropertyTable::SetInternal( if (mLastFrame != aFrame || !mLastEntry) { mLastFrame = aFrame; mLastEntry = mEntries.PutEntry(aFrame); + aFrame->AddStateBits(NS_FRAME_HAS_PROPERTIES); } Entry* entry = mLastEntry; @@ -63,7 +64,8 @@ FramePropertyTable::SetInternal( void* FramePropertyTable::GetInternal( - const nsIFrame* aFrame, UntypedDescriptor aProperty, bool* aFoundResult) + const nsIFrame* aFrame, UntypedDescriptor aProperty, bool aSkipBitCheck, + bool* aFoundResult) { NS_ASSERTION(aFrame, "Null frame?"); NS_ASSERTION(aProperty, "Null property?"); @@ -72,6 +74,10 @@ FramePropertyTable::GetInternal( *aFoundResult = false; } + if (!aSkipBitCheck && !(aFrame->GetStateBits() & NS_FRAME_HAS_PROPERTIES)) { + return nullptr; + } + // We can end up here during parallel style traversal, in which case the main // thread is blocked. Reading from the cache is fine on any thread, but we // only want to write to it in the main-thread case. @@ -82,6 +88,8 @@ FramePropertyTable::GetInternal( mLastEntry = entry; } + MOZ_ASSERT(entry || aSkipBitCheck, + "NS_FRAME_HAS_PROPERTIES bit should match whether entry exists"); if (!entry) return nullptr; @@ -111,7 +119,8 @@ FramePropertyTable::GetInternal( void* FramePropertyTable::RemoveInternal( - nsIFrame* aFrame, UntypedDescriptor aProperty, bool* aFoundResult) + nsIFrame* aFrame, UntypedDescriptor aProperty, bool aSkipBitCheck, + bool* aFoundResult) { MOZ_ASSERT(NS_IsMainThread()); NS_ASSERTION(aFrame, "Null frame?"); @@ -121,11 +130,17 @@ FramePropertyTable::RemoveInternal( *aFoundResult = false; } + if (!aSkipBitCheck && !(aFrame->GetStateBits() & NS_FRAME_HAS_PROPERTIES)) { + return nullptr; + } + if (mLastFrame != aFrame) { mLastFrame = aFrame; mLastEntry = mEntries.GetEntry(aFrame); } Entry* entry = mLastEntry; + MOZ_ASSERT(entry || aSkipBitCheck, + "NS_FRAME_HAS_PROPERTIES bit should match whether entry exists"); if (!entry) return nullptr; @@ -136,6 +151,7 @@ FramePropertyTable::RemoveInternal( // Here it's ok to use RemoveEntry() -- which may resize mEntries -- // because we null mLastEntry at the same time. mEntries.RemoveEntry(entry); + aFrame->RemoveStateBits(NS_FRAME_HAS_PROPERTIES); mLastEntry = nullptr; if (aFoundResult) { *aFoundResult = true; @@ -176,14 +192,14 @@ FramePropertyTable::RemoveInternal( void FramePropertyTable::DeleteInternal( - nsIFrame* aFrame, UntypedDescriptor aProperty) + nsIFrame* aFrame, UntypedDescriptor aProperty, bool aSkipBitCheck) { MOZ_ASSERT(NS_IsMainThread()); NS_ASSERTION(aFrame, "Null frame?"); NS_ASSERTION(aProperty, "Null property?"); bool found; - void* v = RemoveInternal(aFrame, aProperty, &found); + void* v = RemoveInternal(aFrame, aProperty, aSkipBitCheck, &found); if (found) { PropertyValue pv(aProperty, v); pv.DestroyValueFor(aFrame); @@ -210,7 +226,13 @@ FramePropertyTable::DeleteAllFor(nsIFrame* aFrame) { NS_ASSERTION(aFrame, "Null frame?"); + if (!(aFrame->GetStateBits() & NS_FRAME_HAS_PROPERTIES)) { + return; + } + Entry* entry = mEntries.GetEntry(aFrame); + MOZ_ASSERT(entry, + "NS_FRAME_HAS_PROPERTIES bit should match whether entry exists"); if (!entry) return; @@ -226,6 +248,8 @@ FramePropertyTable::DeleteAllFor(nsIFrame* aFrame) // mLastEntry points into mEntries, so we use RawRemoveEntry() which will not // resize mEntries. mEntries.RawRemoveEntry(entry); + + // Don't bother unsetting NS_FRAME_HAS_PROPERTIES, since aFrame is going away } void diff --git a/layout/base/FramePropertyTable.h b/layout/base/FramePropertyTable.h index 2c832fc0e66b..5a9a3672f22a 100644 --- a/layout/base/FramePropertyTable.h +++ b/layout/base/FramePropertyTable.h @@ -188,12 +188,25 @@ public: * * - Calling Has() before Set() in cases where we don't want to overwrite * an existing value for the frame property. + * + * The HasSkippingBitCheck variant doesn't test NS_FRAME_HAS_PROPERTIES + * on aFrame, so it is safe to call after aFrame has been destroyed as + * long as, since that destruction happened, it isn't possible for a + * new frame to have been created and the same property added. */ template bool Has(const nsIFrame* aFrame, Descriptor aProperty) { bool foundResult = false; - mozilla::Unused << GetInternal(aFrame, aProperty, &foundResult); + mozilla::Unused << GetInternal(aFrame, aProperty, false, &foundResult); + return foundResult; + } + + template + bool HasSkippingBitCheck(const nsIFrame* aFrame, Descriptor aProperty) + { + bool foundResult = false; + mozilla::Unused << GetInternal(aFrame, aProperty, true, &foundResult); return foundResult; } @@ -212,7 +225,7 @@ public: PropertyType Get(const nsIFrame* aFrame, Descriptor aProperty, bool* aFoundResult = nullptr) { - void* ptr = GetInternal(aFrame, aProperty, aFoundResult); + void* ptr = GetInternal(aFrame, aProperty, false, aFoundResult); return ReinterpretHelper::FromPointer(ptr); } /** @@ -231,7 +244,7 @@ public: PropertyType Remove(nsIFrame* aFrame, Descriptor aProperty, bool* aFoundResult = nullptr) { - void* ptr = RemoveInternal(aFrame, aProperty, aFoundResult); + void* ptr = RemoveInternal(aFrame, aProperty, false, aFoundResult); return ReinterpretHelper::FromPointer(ptr); } /** @@ -239,11 +252,23 @@ public: * hashtable lookup (using the frame as the key) and a linear search * through the properties of that frame. If the frame has no such * property, nothing happens. + * + * The DeleteSkippingBitCheck variant doesn't test + * NS_FRAME_HAS_PROPERTIES on aFrame, so it is safe to call after + * aFrame has been destroyed as long as, since that destruction + * happened, it isn't possible for a new frame to have been created + * and the same property added. */ template void Delete(nsIFrame* aFrame, Descriptor aProperty) { - DeleteInternal(aFrame, aProperty); + DeleteInternal(aFrame, aProperty, false); + } + + template + void DeleteSkippingBitCheck(nsIFrame* aFrame, Descriptor aProperty) + { + DeleteInternal(aFrame, aProperty, true); } /** * Remove and destroy all property values for a frame. This requires one @@ -262,12 +287,13 @@ protected: void* aValue); void* GetInternal(const nsIFrame* aFrame, UntypedDescriptor aProperty, - bool* aFoundResult); + bool aSkipBitCheck, bool* aFoundResult); void* RemoveInternal(nsIFrame* aFrame, UntypedDescriptor aProperty, - bool* aFoundResult); + bool aSkipBitCheck, bool* aFoundResult); - void DeleteInternal(nsIFrame* aFrame, UntypedDescriptor aProperty); + void DeleteInternal(nsIFrame* aFrame, UntypedDescriptor aProperty, + bool aSkipBitCheck); template struct ReinterpretHelper diff --git a/layout/base/RestyleManager.cpp b/layout/base/RestyleManager.cpp index 85c4088bd809..3b59fbc61794 100644 --- a/layout/base/RestyleManager.cpp +++ b/layout/base/RestyleManager.cpp @@ -1396,7 +1396,8 @@ RestyleManager::ProcessRestyledFrames(nsStyleChangeList& aChangeList) break; } - const nsStyleChangeData& data = aChangeList[i]; + nsStyleChangeData& mutable_data = aChangeList[i]; + const nsStyleChangeData& data = mutable_data; nsIFrame* frame = data.mFrame; nsIContent* content = data.mContent; nsChangeHint hint = data.mHint; @@ -1407,7 +1408,11 @@ RestyleManager::ProcessRestyledFrames(nsStyleChangeList& aChangeList) "Reflow hint bits set without actually asking for a reflow"); // skip any frame that has been destroyed due to a ripple effect - if (frame && !propTable->Get(frame, ChangeListProperty())) { + if (frame && !propTable->HasSkippingBitCheck(frame, ChangeListProperty())) { + // Null out the pointer since the frame was already destroyed. + // This is important so we don't try to delete its + // ChangeListProperty() below. + mutable_data.mFrame = nullptr; continue; } @@ -1475,6 +1480,11 @@ RestyleManager::ProcessRestyledFrames(nsStyleChangeList& aChangeList) } if (hint & nsChangeHint_ReconstructFrame) { + // We're about to destroy data.mFrame, so null out the pointer. + // This is important so we don't try to delete its + // ChangeListProperty() below. + mutable_data.mFrame = nullptr; + // If we ever start passing true here, be careful of restyles // that involve a reframe and animations. In particular, if the // restyle we're processing here is an animation restyle, but @@ -1664,7 +1674,7 @@ RestyleManager::ProcessRestyledFrames(nsStyleChangeList& aChangeList) // is not in fact in a consistent state. for (const nsStyleChangeData& data : aChangeList) { if (data.mFrame) { - propTable->Delete(data.mFrame, ChangeListProperty()); + propTable->DeleteSkippingBitCheck(data.mFrame, ChangeListProperty()); } #ifdef DEBUG diff --git a/layout/generic/nsFrame.cpp b/layout/generic/nsFrame.cpp index e28b09d0066d..03bb9c420280 100644 --- a/layout/generic/nsFrame.cpp +++ b/layout/generic/nsFrame.cpp @@ -9982,8 +9982,9 @@ nsFrame::BoxReflow(nsBoxLayoutState& aState, parentReflowInput(aPresContext, parentFrame, aRenderingContext, LogicalSize(parentWM, parentSize), ReflowInput::DUMMY_PARENT_REFLOW_STATE); - parentFrame->RemoveStateBits(~nsFrameState(0)); - parentFrame->AddStateBits(savedState); + const nsFrameState bitsToLeaveUntouched = NS_FRAME_HAS_PROPERTIES; + parentFrame->RemoveStateBits(~bitsToLeaveUntouched); + parentFrame->AddStateBits(savedState & ~bitsToLeaveUntouched); // This may not do very much useful, but it's probably worth trying. if (parentSize.width != NS_INTRINSICSIZE) diff --git a/layout/generic/nsFrameStateBits.h b/layout/generic/nsFrameStateBits.h index 8306239e7551..c0a955598d3f 100644 --- a/layout/generic/nsFrameStateBits.h +++ b/layout/generic/nsFrameStateBits.h @@ -270,6 +270,9 @@ FRAME_STATE_BIT(Generic, 54, NS_FRAME_HAS_LAYER_ACTIVITY_PROPERTY) // a stylo tree traversal. FRAME_STATE_BIT(Generic, 55, NS_FRAME_OWNS_ANON_BOXES) +// Frame has properties in the nsIFrame::Properties() hash. +FRAME_STATE_BIT(Generic, 56, NS_FRAME_HAS_PROPERTIES) + // Set for all descendants of MathML sub/supscript elements (other than the // base frame) to indicate that the SSTY font feature should be used. FRAME_STATE_BIT(Generic, 58, NS_FRAME_MATHML_SCRIPT_DESCENDANT)