stylo: nsStyleContext::PeekStyleEffects and friends mutate the style context

RESOLVED FIXED in Firefox 55



CSS Parsing and Computation
3 months ago
3 months ago


(Reporter: bholley, Assigned: bholley)



Firefox Tracking Flags

(firefox55 fixed)



(1 attachment)

From bug 1294915 comment 51:

Error: Field write nsStyleContext.mBits
Location: void nsStyleContext::AddStyleBit(uint64*) @ layout/style/nsStyleContext.h:323 ### SafeArguments: arg0
Stack Trace:
nsStyleEffects* nsStyleContext::DoGetStyleEffects() [with bool aComputeData = false] @ ff-dbg/dist/include/nsStyleStructList.h:151
nsStyleEffects* nsStyleContext::PeekStyleEffects() @ ff-dbg/dist/include/nsStyleStructList.h:151
uint32 nsStyleContext::CalcStyleDifferenceInternal(FakeStyleContext*, uint32, uint32*, uint32*) [with StyleContextLike = FakeStyleContext; uint32_t = unsigned int] @ ff-dbg/layout/style/nsStyleStructList.h:151 ### SafeArguments: arg0 arg2 arg3
uint32 nsStyleContext::CalcStyleDifference(ServoComputedValues*, uint32, uint32*, uint32*) @ layout/style/nsStyleContext.cpp:1328 ### SafeArguments: arg2 arg3
Gecko_CalcStyleDifference @ layout/style/ServoBindings.cpp:318

P1, blocks static analysis.
Emilio, I don't understand this line:

Why are we setting any bits in the peek case? Shouldn't we just early-return?
Flags: needinfo?(emilio+bugs)
Isn't that line the one that ensures we early return if aComputeData is false? As far as I know we shouldn't be setting any bits, per that line.

I can try to verify it, but perhaps it's a static analysis bug, which can't see the needToCompute and aComputeData variable dependencies arising from that condition? That'd be weird though.
Flags: needinfo?(emilio+bugs)
(In reply to Emilio Cobos Álvarez [:emilio] from comment #2)
> Isn't that line the one that ensures we early return if aComputeData is
> false? As far as I know we shouldn't be setting any bits, per that line.

That line has an && with needToCompute, which depends on some bitflags on the style context. Did you mean ||?
Flags: needinfo?(emilio+bugs)
We chatted on IRC. I understand the code now, and the issue is that it's relying on a level of reasoning beyond what the static analysis can do. I'll attach an assertion which should fix it.
Assignee: nobody → bobbyholley
Flags: needinfo?(emilio+bugs)
Created attachment 8849297 [details] [diff] [review]
Assert main thread when setting style bits. v1

We also assert against the Servo traversal because we don't currently test the
parallel traversal on CI (though we will soon).

MozReview-Commit-ID: 9GRNizE44Oi
Attachment #8849297 - Flags: review?(emilio+bugs)
Attachment #8849297 - Flags: review?(emilio+bugs) → review+

Comment 7

3 months ago
Pushed by
Assert main thread when setting style bits. r=emilio

Comment 8

3 months ago
Last Resolved: 3 months ago
status-firefox55: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla55
You need to log in before you can comment on or make changes to this bug.