Closed Bug 1335317 Opened 3 years ago Closed 3 years ago

stylo: HasFixedPosContainingBlockStyleInternal call during CalcStyleDifference can touch the cached structs

Categories

(Core :: CSS Parsing and Computation, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla54
Tracking Status
firefox54 --- fixed

People

(Reporter: bholley, Unassigned)

References

Details

Attachments

(1 file, 1 obsolete file)

See bug 1294915 comment 32.

Error: Field write nsStyleContext.mCachedResetData
Location: void nsStyleContext::SetStyle(int32, void*) @ layout/style/nsStyleContext.cpp:600
Stack Trace:
nsStyleEffects* nsStyleContext::DoGetStyleEffects() [with bool aComputeData = true] @ ff-dbg/dist/include/nsStyleStructList.h:151
nsStyleEffects* nsStyleContext::StyleEffects() @ ff-dbg/dist/include/nsStyleStructList.h:151
uint8 nsStyleDisplay::HasFixedPosContainingBlockStyleInternal(nsStyleContext*) const [with StyleContextLike = nsStyleContext] @ layout/style/nsStyleStructInlines.h:157
uint8 nsStyleDisplay::IsFixedPosContainingBlockForAppropriateFrame(nsStyleContext*) const [with StyleContextLike = nsStyleContext] @ layout/style/nsStyleStructInlines.h:167
uint32 nsStyleContext::CalcStyleDifferenceInternal(FakeStyleContext*, uint32, uint32*, uint32*) [with StyleContextLike = FakeStyleContext; nsStyleContext::NeutralChangeHandling aNeutralChangeHandling = (nsStyleContext::NeutralChangeHandling)0; uint32_t = unsigned int] @ layout/style/nsStyleContext.cpp:1211
uint32 nsStyleContext::CalcStyleDifference(ServoComputedValues*, uint32, uint32*, uint32*) @ layout/style/nsStyleContext.cpp:1277
Gecko_CalcStyleDifference @ layout/style/ServoBindings.cpp:280
Depends on: 1335319
I think we should just check whether we're in a parallel traversal (bug 1335319), and if so assume that we're doing CalcStyleDifference and it's ok to call Peek() instead.
Comment on attachment 8833045 [details] [diff] [review]
Avoid calling StyleEffects() during the parallel traversal. v1

Review of attachment 8833045 [details] [diff] [review]:
-----------------------------------------------------------------

::: layout/style/nsStyleStructInlines.h
@@ +164,5 @@
> +
> +  if (ServoStyleSet::IsInServoTraversal()) {
> +    // Servo ends up here during CalcStyleDifference, which happens after frame
> +    // construction, which (according to the comment in CalcStyleDifference)
> +    // should always ensure the creation of the effects struct.

Are we really guaranteed that the old style context, in a CalcStyleDifference, will have a cached Effects struct?  The comment in CalcStyleDifference is talking about this from a wasted work point of view, so it's not a big deal if it's wrong, and I think it might be wrong in some cases, e.g. if the element was display:none.  Or am I missing where under frame construction for a display:none element that we'll end up requesting the Effects struct?
I set a breakpoint here:

http://searchfox.org/mozilla-central/rev/d20e4431d0cf40311afa797868bc5c58c54790a2/layout/base/nsCSSFrameConstructor.cpp#5729

for a display:none element, and found that the Effects struct isn't cached.
Attachment #8833045 - Attachment is obsolete: true
Attachment #8833045 - Flags: review?(cam)
Comment on attachment 8833478 [details] [diff] [review]
Avoid calling StyleEffects() during the parallel traversal. v2

Review of attachment 8833478 [details] [diff] [review]:
-----------------------------------------------------------------

::: layout/style/nsStyleContext.cpp
@@ +1259,5 @@
> +    /* This isn't needed directly by CalcStyleDifference, but is needed by    \
> +       nsStyleDisplay::HasFixedPosContainingBlockStyleInternal, which         \
> +       doesn't want to call the side-effect-y variant during servo            \
> +       traversal. */                                                          \
> +    return Style##name_();                                                    \

We don't need this any more.

::: layout/style/nsStyleStructInlines.h
@@ +162,5 @@
> +    return true;
> +  }
> +
> +  if (mozilla::ServoStyleSet::IsInServoTraversal()) {
> +    const ServoComputedValues* values =

Please add a comment in here explaining why we need to inspect the ServoComputedValues directly.
Attachment #8833478 - Flags: review?(cam) → review+
Pushed by bholley@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/9f080fde040f
Avoid calling StyleEffects() during the parallel traversal. r=heycam
https://hg.mozilla.org/mozilla-central/rev/9f080fde040f
Status: NEW → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla54
You need to log in before you can comment on or make changes to this bug.