1335308 - stylo: Add some assertions to document our invariants and aid the static analysis

Status: RESOLVED FIXED

(Core :: CSS Parsing and Computation, defect)

Comment 1

[Bobby Holley (:bholley)]

Attachment: Part 1 - Assert main thread in Gecko_LoadStyleSheet. v1

Comment 2

[Bobby Holley (:bholley)]

Attachment: Part 2 - Assert against non-length units in Gecko_CSSValue_SetAbsoluteLength. v1

Brian, what's the correct annotation to add for the static analysis?

Comment 3

[Brian Hackett [Laid off!]]

Comment on attachment 8831942 [details] [diff] [review]
Part 2 - Assert against non-length units in Gecko_CSSValue_SetAbsoluteLength. v1

Review of attachment 8831942 [details] [diff] [review]:
-----------------------------------------------------------------

Handling this in the static analysis would be kind of hard.  It would be difficult to train the static analysis to understand properties of CSS values and how that affects control flow as it explores the stack.  Using an annotation would work, but it would be rather fragile --- something like "ignore calls to nsCSSValue::DoReset when Gecko_CSSValue_SetAbsoluteLength is on the stack".  Allowing annotations to inspect the stack is problematic for soundness of the analysis (i.e. conservatively determining the program's possible behaviors), because we don't necessarily reanalyze functions if we find multiple different stacks that reach them.

The best way to teach the static analysis is to add assertions for either NS_IsMainThread or for the flag that is added in bug 1335319 along paths which should not execute during a parallel traversal.  In this case that would be in the various ->Release() calls in nsCSSValue::DoReset I think, though maybe there is a better pinchpoint that doesn't clutter up the code as much.

Comment 4

[Bobby Holley (:bholley)]

Attachment: Part 3 - Assert against releasing refcounted nsCSSValue members off-main-thread. v1

Comment 5

[Bobby Holley (:bholley)]

Attachment: Part 4 - Assert against destroying images and counters in generated content off-main-thread. v1

We'll need to deal with this at some point, but let's just forbid it for now
to appease the static analysis.

Comment 6

[Cameron McCormack (:heycam)]

Comment on attachment 8833033 [details] [diff] [review]
Part 3 - Assert against releasing refcounted nsCSSValue members off-main-thread. v1

Review of attachment 8833033 [details] [diff] [review]:
-----------------------------------------------------------------

mURL and mImage do have thread-safe refcounts, but that's probably beside the point.

When do we actually call Reset() on an nsCSSValue that is not already eCSSUnit_Null, from style worker threads?  If we don't, it might be easier just to do:

  if (mUnit == eCSSUnit_Null) {
    return;
  }

  MOZ_ASSERT(NS_IsMainThread());

at the top of DoReset instead.

Comment 7

[Bobby Holley (:bholley)]

(In reply to Cameron McCormack (:heycam) from comment #6)
> Comment on attachment 8833033 [details] [diff] [review]
> Part 3 - Assert against releasing refcounted nsCSSValue members
> off-main-thread. v1
> 
> Review of attachment 8833033 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> mURL and mImage do have thread-safe refcounts, but that's probably beside
> the point.
> 
> When do we actually call Reset() on an nsCSSValue that is not already
> eCSSUnit_Null, from style worker threads?  If we don't, it might be easier
> just to do:
> 
>   if (mUnit == eCSSUnit_Null) {
>     return;
>   }
> 
>   MOZ_ASSERT(NS_IsMainThread());
> 
> at the top of DoReset instead.

I took emilio's comment in bug 1294915 comment 34 to imply that we called it for values that already had a length in it.

Comment 8

[Emilio Cobos Álvarez (:emilio)]

(In reply to Bobby Holley (:bholley) (busy with Stylo) from comment #7)
> (In reply to Cameron McCormack (:heycam) from comment #6)
> > Comment on attachment 8833033 [details] [diff] [review]
> > Part 3 - Assert against releasing refcounted nsCSSValue members
> > off-main-thread. v1
> > 
> > Review of attachment 8833033 [details] [diff] [review]:
> > -----------------------------------------------------------------
> > 
> > mURL and mImage do have thread-safe refcounts, but that's probably beside
> > the point.
> > 
> > When do we actually call Reset() on an nsCSSValue that is not already
> > eCSSUnit_Null, from style worker threads?  If we don't, it might be easier
> > just to do:
> > 
> >   if (mUnit == eCSSUnit_Null) {
> >     return;
> >   }
> > 
> >   MOZ_ASSERT(NS_IsMainThread());
> > 
> > at the top of DoReset instead.
> 
> I took emilio's comment in bug 1294915 comment 34 to imply that we called it
> for values that already had a length in it.

I believe we can destruct a length nsCSSValue if we explicitly inherit transform, then reset it with another rule, but I could be wrong given I haven't followed all the transform business lately.

Comment 9

[Bobby Holley (:bholley)]

Attachment: Part 5 - Proxy mSpecifiedTransform releases that occur during the servo traversal to the main thread. v1

This isn't really an assertion, but goes with part 3.

Comment 10

[Bobby Holley (:bholley)]

Attachment: Part 5 - Proxy mSpecifiedTransform releases that occur during the servo traversal to the main thread. v2

Comment 11

[Cameron McCormack (:heycam)]

Comment on attachment 8834758 [details] [diff] [review]
Part 5 - Proxy mSpecifiedTransform releases that occur during the servo traversal to the main thread. v2

Review of attachment 8834758 [details] [diff] [review]:
-----------------------------------------------------------------

I get slightly nervous when I see behaviour changes between debug and non-debug builds, though I guess it's self contained enough here that I don't need to worry.

Comment 12

[Bobby Holley (:bholley)]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=98924c761ac4cda411cc07c992fbddb37081ed6d

Comment 13

[Bobby Holley (:bholley)]

And an m-c try push: https://treeherder.mozilla.org/#/jobs?repo=try&revision=eceeed6f236ca1cf2b34c4dfc7e07ae4552bab4a

Comment 14

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/f15fdc620527
Assert main thread in Gecko_LoadStyleSheet. r=emilio
https://hg.mozilla.org/integration/mozilla-inbound/rev/ee7bdede91b6
Assert against non-length units in Gecko_CSSValue_SetAbsoluteLength. r=emilio
https://hg.mozilla.org/integration/mozilla-inbound/rev/9d10905855f9
Assert against releasing refcounted nsCSSValue members off-main-thread. r=heycam
https://hg.mozilla.org/integration/mozilla-inbound/rev/a00867c65c49
Assert against destroying images and counters in generated content off-main-thread. r=emilio
https://hg.mozilla.org/integration/mozilla-inbound/rev/61cd520a0057
Proxy mSpecifiedTransform releases that occur during the servo traversal to the main thread. r=heycam

Comment 15

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/f15fdc620527
https://hg.mozilla.org/mozilla-central/rev/ee7bdede91b6
https://hg.mozilla.org/mozilla-central/rev/9d10905855f9
https://hg.mozilla.org/mozilla-central/rev/a00867c65c49
https://hg.mozilla.org/mozilla-central/rev/61cd520a0057