Bug 1322317: Fix CalcStyleDifference assumptions and PeekStyleContext semantics. r?heycam
For the reasoning for this change, please see the related bugs and:
http://logs.glob.uno/?c=mozilla%23layout&s=22+Feb+2017&e=22+Feb+2017#c27236
Mainly, before this change, there was nothing forcing style structs computed in
a style context to remain computed for the new style context after a call to
CalcStyleDifference. This can make us skip change hints when a style change
doesn't force to recompute one of these structs.
MozReview-Commit-ID: FoWbLjt97Uu
Signed-off-by: Emilio Cobos Álvarez <emilio@crisal.io>
--- a/layout/base/ServoRestyleManager.cpp
+++ b/layout/base/ServoRestyleManager.cpp
@@ -226,16 +226,18 @@ ServoRestyleManager::RecreateStyleContex
RefPtr<nsStyleContext> newContext = nullptr;
if (recreateContext) {
MOZ_ASSERT(styleFrame || displayContentsNode);
newContext =
aStyleSet->GetContext(computedValues.forget(), aParentContext, nullptr,
CSSPseudoElementType::NotPseudo, aElement);
+ newContext->EnsureStructsForServo(oldStyleContext);
+
// XXX This could not always work as expected: there are kinds of content
// with the first split and the last sharing style, but others not. We
// should handle those properly.
for (nsIFrame* f = styleFrame; f;
f = GetNextContinuationWithSameStyle(f, oldStyleContext)) {
f->SetStyleContext(newContext);
}
--- a/layout/style/nsStyleContext.cpp
+++ b/layout/style/nsStyleContext.cpp
@@ -174,21 +174,61 @@ nsStyleContext::FinishConstruction(bool
}
#define eStyleStruct_LastItem (nsStyleStructID_Length - 1)
NS_ASSERTION(NS_STYLE_INHERIT_MASK & NS_STYLE_INHERIT_BIT(LastItem),
"NS_STYLE_INHERIT_MASK must be bigger, and other bits shifted");
#undef eStyleStruct_LastItem
}
+void
+nsStyleContext::EnsureStructsForServo(const nsStyleContext* aOldContext)
+{
+ MOZ_ASSERT(aOldContext);
+ MOZ_ASSERT(mSource.IsServoComputedValues() &&
+ aOldContext->mSource.IsServoComputedValues());
+
+ // NOTE(emilio): We could do better here. We only call Style##name_() because
+ // we need to run FinishStyle, but otherwise this is only a bitwise or.
+ //
+ // We could reduce the FFI traffic we do only doing it for structs that have
+ // non-trivial FinishStyle.
+#define STYLE_STRUCT(name_, checkdata_cb) \
+ if (aOldContext->mBits & NS_STYLE_INHERIT_BIT(name_)) { \
+ mozilla::Unused << Style##name_(); \
+ }
+
+#include "nsStyleStructList.h"
+
+#undef STYLE_STRUCT
+
+#ifdef DEBUG
+ auto oldMask = aOldContext->mBits & NS_STYLE_INHERIT_MASK;
+ auto newMask = mBits & NS_STYLE_INHERIT_MASK;
+ MOZ_ASSERT((oldMask & newMask) == oldMask,
+ "Should have at least as many structs computed as the "
+ "old context!");
+#endif
+}
+
nsStyleContext::~nsStyleContext()
{
NS_ASSERTION((nullptr == mChild) && (nullptr == mEmptyChild), "destructing context with children");
+ MOZ_ASSERT_IF(mSource.IsServoComputedValues(), !mCachedResetData);
#ifdef DEBUG
+ if (mSource.IsServoComputedValues()) {
+ MOZ_ASSERT(!mCachedResetData,
+ "Servo shouldn't cache reset structs in nsStyleContext");
+ for (const auto* data : mCachedInheritedData.mStyleStructs) {
+ MOZ_ASSERT(!data,
+ "Servo shouldn't cache inherit structs in nsStyleContext");
+ }
+ }
+
if (sExpensiveStyleStructAssertionsEnabled) {
// Assert that the style structs we are about to destroy are not referenced
// anywhere else in the style context tree. These checks are expensive,
// which is why they are not enabled by default.
nsStyleContext* root = this;
while (root->mParent) {
root = root->mParent;
}
@@ -581,16 +621,18 @@ nsStyleContext::CreateEmptyStyleData(con
// top of this function.
SetStyle(aSID, result);
return result;
}
void
nsStyleContext::SetStyle(nsStyleStructID aSID, void* aStruct)
{
+ MOZ_ASSERT(!mSource.IsServoComputedValues(),
+ "Servo shouldn't cache style structs in the style context!");
// This method should only be called from nsRuleNode! It is not a public
// method!
NS_ASSERTION(aSID >= 0 && aSID < nsStyleStructID_Length, "out of bounds");
// NOTE: nsCachedStyleData::GetStyleData works roughly the same way.
// See the comments there (in nsRuleNode.h) for more details about
// what this is doing and why.
--- a/layout/style/nsStyleContext.h
+++ b/layout/style/nsStyleContext.h
@@ -380,16 +380,24 @@ public:
#define STYLE_STRUCT(name_, checkdata_cb_) \
const nsStyle##name_ * PeekStyle##name_() { \
return DoGetStyle##name_<false>(); \
}
#include "nsStyleStructList.h"
#undef STYLE_STRUCT
/**
+ * Ensures that this context's computed struct list is at least the old
+ * context's.
+ *
+ * aOldContext must not be null.
+ */
+ void EnsureStructsForServo(const nsStyleContext* aOldContext);
+
+ /**
* Compute the style changes needed during restyling when this style
* context is being replaced by aNewContext. (This is nonsymmetric since
* we optimize by skipping comparison for styles that have never been
* requested.)
*
* This method returns a change hint (see nsChangeHint.h). All change
* hints apply to the frame and its later continuations or ib-split
* siblings. Most (all of those except the "NotHandledForDescendants"
@@ -596,129 +604,129 @@ private:
#else
#define AUTO_CHECK_DEPENDENCY(sid_)
#endif
// Helper functions for GetStyle* and PeekStyle*
#define STYLE_STRUCT_INHERITED(name_, checkdata_cb_) \
template<bool aComputeData> \
const nsStyle##name_ * DoGetStyle##name_() { \
- const nsStyle##name_ * cachedData = \
- static_cast<nsStyle##name_*>( \
- mCachedInheritedData.mStyleStructs[eStyleStruct_##name_]); \
- if (cachedData) /* Have it cached already, yay */ \
- return cachedData; \
- if (!aComputeData) { \
- /* We always cache inherited structs on the context when we */ \
- /* compute them. */ \
+ if (mSource.IsGeckoRuleNode()) { \
+ const nsStyle##name_ * cachedData = \
+ static_cast<nsStyle##name_*>( \
+ mCachedInheritedData.mStyleStructs[eStyleStruct_##name_]); \
+ if (cachedData) /* Have it cached already, yay */ \
+ return cachedData; \
+ if (!aComputeData) { \
+ /* We always cache inherited structs on the context when we */\
+ /* compute them. */ \
+ return nullptr; \
+ } \
+ /* Have the rulenode deal */ \
+ AUTO_CHECK_DEPENDENCY(eStyleStruct_##name_); \
+ const nsStyle##name_ * newData = \
+ mSource.AsGeckoRuleNode()-> \
+ GetStyle##name_<aComputeData>(this, mBits); \
+ /* always cache inherited data on the style context; the rule */\
+ /* node set the bit in mBits for us if needed. */ \
+ mCachedInheritedData.mStyleStructs[eStyleStruct_##name_] = \
+ const_cast<nsStyle##name_ *>(newData); \
+ return newData; \
+ } \
+ /** \
+ * Always forward to the parent to grab the inherited style struct\
+ * we're a text node. \
+ * \
+ * This causes the parent element's style context to cache any \
+ * inherited structs we request for a text node, which means we \
+ * don't have to compute change hints for the text node, as \
+ * handling the change on the parent element is sufficient. \
+ * \
+ * Note that adding the inherit bit is ok, because the struct \
+ * pointer returned by the parent and the child is owned by \
+ * Servo. This is fine if the pointers are the same (as it \
+ * should, read below), because both style context sources will \
+ * hold it. \
+ * \
+ * In the case of a mishandled frame, we could end up with the \
+ * pointer to and old parent style, but that's fine too, since \
+ * the parent style context will remain alive until we reframe, \
+ * in which case we'll discard both style contexts. Also, we \
+ * hold a strong reference to the parent style context, which \
+ * makes it a non-issue. \
+ * \
+ * Also, note that the assertion below should be true, except \
+ * for those frames we still don't handle correctly, like \
+ * anonymous table wrappers, in which case the pointers will \
+ * differ. \
+ * \
+ * That means we're not going to restyle correctly text frames \
+ * of anonymous table wrappers, for example. It's kind of \
+ * embarrassing, but I think it's not worth it to add more \
+ * logic here unconditionally, given that's going to be fixed. \
+ * \
+ * TODO(emilio): Convert to a strong assertion once we support \
+ * all kinds of random frames. In fact, this can be a great \
+ * assertion to debug them. \
+ */ \
+ if (mPseudoTag == nsCSSAnonBoxes::mozText) { \
+ MOZ_ASSERT(mParent); \
+ const nsStyle##name_* data = \
+ mParent->DoGetStyle##name_<aComputeData>(); \
+ NS_WARNING_ASSERTION(!data || \
+ data == Servo_GetStyle##name_(mSource.AsServoComputedValues()), \
+ "bad data"); \
+ return data; \
+ } \
+ \
+ const bool needToCompute = !(mBits & NS_STYLE_INHERIT_BIT(name_));\
+ if (!aComputeData && needToCompute) { \
return nullptr; \
} \
- /* Have the rulenode deal */ \
- AUTO_CHECK_DEPENDENCY(eStyleStruct_##name_); \
- const nsStyle##name_ * newData; \
- if (mSource.IsGeckoRuleNode()) { \
- newData = mSource.AsGeckoRuleNode()-> \
- GetStyle##name_<aComputeData>(this, mBits); \
- } else { \
- /** \
- * Reach the parent to grab the inherited style struct if \
- * we're a text node. \
- * \
- * This causes the parent element's style context to cache any \
- * inherited structs we request for a text node, which means we \
- * don't have to compute change hints for the text node, as \
- * handling the change on the parent element is sufficient. \
- * \
- * Note that adding the inherit bit is ok, because the struct \
- * pointer returned by the parent and the child is owned by \
- * Servo. This is fine if the pointers are the same (as it \
- * should, read below), because both style context sources will \
- * hold it. \
- * \
- * In the case of a mishandled frame, we could end up with the \
- * pointer to and old parent style, but that's fine too, since \
- * the parent style context will remain alive until we reframe, \
- * in which case we'll discard both style contexts. Also, we \
- * hold a strong reference to the parent style context, which \
- * makes it a non-issue. \
- * \
- * Also, note that the assertion below should be true, except \
- * for those frames we still don't handle correctly, like \
- * anonymous table wrappers, in which case the pointers will \
- * differ. \
- * \
- * That means we're not going to restyle correctly text frames \
- * of anonymous table wrappers, for example. It's kind of \
- * embarrassing, but I think it's not worth it to add more \
- * logic here unconditionally, given that's going to be fixed. \
- * \
- * TODO(emilio): Convert to a strong assertion once we support \
- * all kinds of random frames. In fact, this can be a great \
- * assertion to debug them. \
- */ \
- if (mPseudoTag == nsCSSAnonBoxes::mozText) { \
- MOZ_ASSERT(mParent); \
- newData = mParent->DoGetStyle##name_<true>(); \
- NS_WARNING_ASSERTION( \
- newData == Servo_GetStyle##name_(mSource.AsServoComputedValues()), \
- "bad newData"); \
- } else { \
- newData = \
- Servo_GetStyle##name_(mSource.AsServoComputedValues()); \
- } \
- /* perform any remaining main thread work on the struct */ \
- const_cast<nsStyle##name_*>(newData)->FinishStyle(PresContext());\
+ \
+ const nsStyle##name_* data = \
+ Servo_GetStyle##name_(mSource.AsServoComputedValues()); \
+ /* perform any remaining main thread work on the struct */ \
+ if (needToCompute) { \
+ const_cast<nsStyle##name_*>(data)->FinishStyle(PresContext()); \
/* the Servo-backed StyleContextSource owns the struct */ \
AddStyleBit(NS_STYLE_INHERIT_BIT(name_)); \
} \
- /* always cache inherited data on the style context; the rule */ \
- /* node set the bit in mBits for us if needed. */ \
- mCachedInheritedData.mStyleStructs[eStyleStruct_##name_] = \
- const_cast<nsStyle##name_ *>(newData); \
- return newData; \
+ return data; \
}
+
#define STYLE_STRUCT_RESET(name_, checkdata_cb_) \
template<bool aComputeData> \
const nsStyle##name_ * DoGetStyle##name_() { \
- if (mCachedResetData) { \
- const nsStyle##name_ * cachedData = \
- static_cast<nsStyle##name_*>( \
- mCachedResetData->mStyleStructs[eStyleStruct_##name_]); \
- if (cachedData) /* Have it cached already, yay */ \
- return cachedData; \
- } \
- /* Have the rulenode deal */ \
- AUTO_CHECK_DEPENDENCY(eStyleStruct_##name_); \
- const nsStyle##name_ * newData; \
if (mSource.IsGeckoRuleNode()) { \
- newData = mSource.AsGeckoRuleNode()-> \
+ if (mCachedResetData) { \
+ const nsStyle##name_ * cachedData = \
+ static_cast<nsStyle##name_*>( \
+ mCachedResetData->mStyleStructs[eStyleStruct_##name_]); \
+ if (cachedData) /* Have it cached already, yay */ \
+ return cachedData; \
+ } \
+ /* Have the rulenode deal */ \
+ AUTO_CHECK_DEPENDENCY(eStyleStruct_##name_); \
+ return mSource.AsGeckoRuleNode()-> \
GetStyle##name_<aComputeData>(this); \
- } else { \
- newData = \
- Servo_GetStyle##name_(mSource.AsServoComputedValues()); \
- /* perform any remaining main thread work on the struct */ \
- const_cast<nsStyle##name_*>(newData)->FinishStyle(PresContext());\
- /* The Servo-backed StyleContextSource owns the struct. \
- * \
- * XXXbholley: Unconditionally caching reset structs here \
- * defeats the memory optimization where we lazily allocate \
- * mCachedResetData, so that we can avoid performing an FFI \
- * call each time we want to get the style structs. We should \
- * measure the tradeoffs at some point. If the FFI overhead is \
- * low and the memory win significant, we should consider \
- * _always_ grabbing the struct over FFI, and potentially \
- * giving mCachedInheritedData the same treatment. \
- * \
- * Note that there is a similar comment in StyleData(). \
- */ \
+ } \
+ const bool needToCompute = !(mBits & NS_STYLE_INHERIT_BIT(name_));\
+ if (!aComputeData && needToCompute) { \
+ return nullptr; \
+ } \
+ const nsStyle##name_* data = \
+ Servo_GetStyle##name_(mSource.AsServoComputedValues()); \
+ /* perform any remaining main thread work on the struct */ \
+ if (needToCompute) { \
+ const_cast<nsStyle##name_*>(data)->FinishStyle(PresContext()); \
+ /* the Servo-backed StyleContextSource owns the struct */ \
AddStyleBit(NS_STYLE_INHERIT_BIT(name_)); \
- SetStyle(eStyleStruct_##name_, \
- const_cast<nsStyle##name_*>(newData)); \
} \
- return newData; \
+ return data; \
}
#include "nsStyleStructList.h"
#undef STYLE_STRUCT_RESET
#undef STYLE_STRUCT_INHERITED
// Helper for ClearCachedInheritedStyleDataOnDescendants.
void DoClearCachedInheritedStyleDataOnDescendants(uint32_t aStructs);
