Closed Bug 1343388 Opened 3 years ago Closed 3 years ago

stylo: Fix up more static analysis hazards

Categories

(Core :: CSS Parsing and Computation, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla54
Tracking Status
firefox54 --- fixed

People

(Reporter: bholley, Assigned: bholley)

References

Details

Attachments

(4 files)

Another round.
MozReview-Commit-ID: 732bV0X80Gk
Attachment #8842221 - Flags: review?(emilio+bugs)
MozReview-Commit-ID: 5DdcKgyADbE
Attachment #8842222 - Flags: review?(emilio+bugs)
MozReview-Commit-ID: LnYYbx742X1
Attachment #8842225 - Flags: review?(emilio+bugs)
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+
Attachment #8842222 - Flags: review?(emilio+bugs) → review+
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+
Attachment #8842225 - Flags: review?(emilio+bugs) → review+
(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
You need to log in before you can comment on or make changes to this bug.