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

RESOLVED FIXED in Firefox 55

Status

()

Core
CSS Parsing and Computation
P1
normal
RESOLVED FIXED
3 months ago
3 months ago

People

(Reporter: bholley, Assigned: bholley)

Tracking

unspecified
mozilla55
Points:
---

Firefox Tracking Flags

(firefox55 fixed)

Details

Attachments

(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: 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?
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)
https://treeherder.mozilla.org/#/jobs?repo=try&revision=adce745b6f26ccf527e11ce0b2c267cd0f69f2b3
Attachment #8849297 - Flags: review?(emilio+bugs) → review+

Comment 7

3 months ago
Pushed by bholley@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/871c3cb994be
Assert main thread when setting style bits. r=emilio

Comment 8

3 months ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/871c3cb994be
Status: NEW → RESOLVED
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.