Closed
Bug 1348602
Opened 8 years ago
Closed 8 years ago
stylo: nsStyleContext::PeekStyleEffects and friends mutate the style context
Categories
(Core :: CSS Parsing and Computation, defect, P1)
Core
CSS Parsing and Computation
Tracking
()
RESOLVED
FIXED
mozilla55
Tracking | Status | |
---|---|---|
firefox55 | --- | fixed |
People
(Reporter: bholley, Assigned: bholley)
References
Details
Attachments
(1 file)
(deleted),
patch
|
emilio
:
review+
|
Details | Diff | Splinter Review |
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.
Assignee | ||
Comment 1•8 years ago
|
||
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)
Comment 2•8 years ago
|
||
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)
Assignee | ||
Comment 3•8 years ago
|
||
(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)
Assignee | ||
Comment 4•8 years ago
|
||
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)
Assignee | ||
Comment 5•8 years ago
|
||
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)
Assignee | ||
Comment 6•8 years ago
|
||
Updated•8 years ago
|
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
Comment 8•8 years ago
|
||
bugherder |
Status: NEW → RESOLVED
Closed: 8 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.
Description
•