1343388 - stylo: Fix up more static analysis hazards

Status: RESOLVED FIXED

(Core :: CSS Parsing and Computation, defect)

Description

[Bobby Holley (:bholley)]

Another round.

Comment 1

[Bobby Holley (:bholley)]

Attachment: Part 1 - Don't write to undisplayed contents map cache during servo traversal. v1

MozReview-Commit-ID: 732bV0X80Gk

Comment 2

[Bobby Holley (:bholley)]

Attachment: Part 2 - Use threadsafe style struct accessor in assertion. v1

MozReview-Commit-ID: 5DdcKgyADbE

Comment 3

[Bobby Holley (:bholley)]

Attachment: Part 3 - Use threadsafe style struct accessors in CalcStyleDifference when analyzing visited-link style contexts. v1

MozReview-Commit-ID: 1SJlS4PwBoy

Comment 4

[Bobby Holley (:bholley)]

Attachment: Part 4 - Assert against the servo traversal when serializing gecko declarations. v1

MozReview-Commit-ID: LnYYbx742X1

Comment 5

[Bobby Holley (:bholley)]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=816fbc151fe39cf02da911e15934a430fb8d6882

Comment 6

[Emilio Cobos Álvarez (:emilio)]

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.

Comment 7

[Emilio Cobos Álvarez (:emilio)]

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?

Comment 8

[Bobby Holley (:bholley)]

(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.

Comment 9

[Pulsebot]

Pushed by [e-mail]:
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

[Carsten Book [:Tomcat]]

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