Closed Bug 1348602 Opened 7 years ago Closed 7 years ago

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

Categories

(Core :: CSS Parsing and Computation, defect, P1)

defect

Tracking

()

RESOLVED FIXED
mozilla55
Tracking Status
firefox55 --- fixed

People

(Reporter: bholley, Assigned: bholley)

References

Details

Attachments

(1 file)

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)
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 bholley@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/871c3cb994be
Assert main thread when setting style bits. r=emilio
https://hg.mozilla.org/mozilla-central/rev/871c3cb994be
Status: NEW → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla55
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: