Closed
Bug 1343388
Opened 7 years ago
Closed 7 years ago
stylo: Fix up more static analysis hazards
Categories
(Core :: CSS Parsing and Computation, defect)
Core
CSS Parsing and Computation
Tracking
()
RESOLVED
FIXED
mozilla54
Tracking | Status | |
---|---|---|
firefox54 | --- | fixed |
People
(Reporter: bholley, Assigned: bholley)
References
Details
Attachments
(4 files)
1.07 KB,
patch
|
emilio
:
review+
|
Details | Diff | Splinter Review |
1.05 KB,
patch
|
emilio
:
review+
|
Details | Diff | Splinter Review |
1.60 KB,
patch
|
emilio
:
review+
|
Details | Diff | Splinter Review |
1.67 KB,
patch
|
emilio
:
review+
|
Details | Diff | Splinter Review |
Another round.
Assignee | ||
Comment 1•7 years ago
|
||
MozReview-Commit-ID: 732bV0X80Gk
Attachment #8842221 -
Flags: review?(emilio+bugs)
Assignee | ||
Comment 2•7 years ago
|
||
MozReview-Commit-ID: 5DdcKgyADbE
Attachment #8842222 -
Flags: review?(emilio+bugs)
Assignee | ||
Comment 3•7 years ago
|
||
MozReview-Commit-ID: 1SJlS4PwBoy
Attachment #8842224 -
Flags: review?(emilio+bugs)
Assignee | ||
Comment 4•7 years ago
|
||
MozReview-Commit-ID: LnYYbx742X1
Attachment #8842225 -
Flags: review?(emilio+bugs)
Assignee | ||
Comment 5•7 years ago
|
||
https://treeherder.mozilla.org/#/jobs?repo=try&revision=816fbc151fe39cf02da911e15934a430fb8d6882
Comment 6•7 years ago
|
||
Comment on attachment 8842221 [details] [diff] [review] Part 1 - Don't write to undisplayed contents map cache during servo traversal. v1 Review of attachment 8842221 [details] [diff] [review]: ----------------------------------------------------------------- Gah. I'm dubious about this cache being useful at all FWIW, I can't think of a place where we'd repeteadly look up the same node twice here. Perhaps we should look at removing them, but that's fine as a followup I guess.
Attachment #8842221 -
Flags: review?(emilio+bugs) → review+
Updated•7 years ago
|
Attachment #8842222 -
Flags: review?(emilio+bugs) → review+
Comment 7•7 years ago
|
||
Comment on attachment 8842224 [details] [diff] [review] Part 3 - Use threadsafe style struct accessors in CalcStyleDifference when analyzing visited-link style contexts. v1 Review of attachment 8842224 [details] [diff] [review]: ----------------------------------------------------------------- ::: layout/style/nsStyleContext.cpp @@ +1212,5 @@ > // not having a style-if-visited), but not the other way around. > #define STYLE_FIELD(name_) thisVisStruct->name_ != otherVisStruct->name_ || > #define STYLE_STRUCT(name_, fields_) \ > if (!change && PeekStyle##name_()) { \ > + const nsStyle##name_* thisVisStruct = thisVis->ThreadsafeStyle##name_(); \ Regarding this... This works, but should we do this anyway? As far as I know we don't reach here, since we don't ever call SetStyleIfVisited for Servo style contexts. Perhaps we could just assert we're not a servo style context / that we're on main-thread / that we aren't in traversal for now?
Attachment #8842224 -
Flags: review?(emilio+bugs) → review+
Updated•7 years ago
|
Attachment #8842225 -
Flags: review?(emilio+bugs) → review+
Assignee | ||
Comment 8•7 years ago
|
||
(In reply to Emilio Cobos Álvarez [:emilio] from comment #7) > Comment on attachment 8842224 [details] [diff] [review] > Part 3 - Use threadsafe style struct accessors in CalcStyleDifference when > analyzing visited-link style contexts. v1 > > Review of attachment 8842224 [details] [diff] [review]: > ----------------------------------------------------------------- > > ::: layout/style/nsStyleContext.cpp > @@ +1212,5 @@ > > // not having a style-if-visited), but not the other way around. > > #define STYLE_FIELD(name_) thisVisStruct->name_ != otherVisStruct->name_ || > > #define STYLE_STRUCT(name_, fields_) \ > > if (!change && PeekStyle##name_()) { \ > > + const nsStyle##name_* thisVisStruct = thisVis->ThreadsafeStyle##name_(); \ > > Regarding this... This works, but should we do this anyway? > > As far as I know we don't reach here, since we don't ever call > SetStyleIfVisited for Servo style contexts. Perhaps we could just assert > we're not a servo style context / that we're on main-thread / that we aren't > in traversal for now? That seems like a reasonable, lower-risk approach. I'll go ahead and do that.
Pushed by bholley@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/5b8c22b505a4 Don't write to undisplayed contents map cache during servo traversal. r=emilio https://hg.mozilla.org/integration/autoland/rev/2d3c0b4c3e4a Use threadsafe style struct accessor in assertion. r=emilio https://hg.mozilla.org/integration/autoland/rev/042624a21b86 Assert against the servo traversal when analyzing visited-link style contexts. r=emilio https://hg.mozilla.org/integration/autoland/rev/a79c630604eb Assert against the servo traversal when serializing gecko declarations. r=emilio
Comment 10•7 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/5b8c22b505a4 https://hg.mozilla.org/mozilla-central/rev/2d3c0b4c3e4a https://hg.mozilla.org/mozilla-central/rev/042624a21b86 https://hg.mozilla.org/mozilla-central/rev/a79c630604eb
Status: NEW → RESOLVED
Closed: 7 years ago
status-firefox54:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla54
You need to log in
before you can comment on or make changes to this bug.
Description
•