Closed Bug 1335308 Opened 5 years ago Closed 5 years ago

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

Categories

(Core :: CSS Parsing and Computation, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla54
Tracking Status
firefox54 --- fixed

People

(Reporter: bholley, Unassigned)

References

Details

Attachments

(5 files, 1 obsolete file)

No description provided.
Brian, what's the correct annotation to add for the static analysis?
Attachment #8831942 - Flags: review?(emilio+bugs)
Attachment #8831942 - Flags: feedback?(bhackett1024)
Attachment #8831941 - Flags: review?(emilio+bugs) → review+
Attachment #8831942 - Flags: review?(emilio+bugs) → review+
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.
Attachment #8831942 - Flags: feedback?(bhackett1024)
We'll need to deal with this at some point, but let's just forbid it for now
to appease the static analysis.
Attachment #8833106 - Flags: review?(emilio+bugs)
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.
Attachment #8833033 - Flags: review?(cam) → review+
Attachment #8833106 - Flags: review?(emilio+bugs) → review+
(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.
(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.
See Also: → 1336643
This isn't really an assertion, but goes with part 3.
Attachment #8834730 - Flags: review?(cam)
Attachment #8834730 - Flags: review?(cam) → review+
Attachment #8834730 - Attachment is obsolete: true
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.
Attachment #8834758 - Flags: review?(cam) → review+
Pushed by bholley@mozilla.com:
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
You need to log in before you can comment on or make changes to this bug.