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: http://searchfox.org/mozilla-central/rev/c9f5cc6b4593d461e87a6221dc0286b3859fd515/layout/style/nsStyleContext.h#715 Why are we setting any bits in the peek case? Shouldn't we just early-return?
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.
(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 ||?
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
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+
Pushed by email@example.com: https://hg.mozilla.org/integration/autoland/rev/871c3cb994be Assert main thread when setting style bits. r=emilio
Status: NEW → RESOLVED
Last Resolved: 2 years 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.