1400442 - stylo heap write analysis: Move known bugs into bug-commented annotations in the analysis code, and fix the rest

Status: RESOLVED FIXED

(Core :: CSS Parsing and Computation, enhancement)

Description

[Steve Fink [:sfink] [:s:]]

Currently, we have a handful of known hazards, with an expectation count that controls whether the job will turn red. Now that they are all understood, I am going to whitelist them all, which means we should have a count of zero. They will be recorded as blocking bug 1356458 instead.

This should make it easier to see added hazards, as you won't have to ignore the known ones when looking at the hazard results file.

This bug will also improve the analysis so that it only whitelists a small number of Gecko_ functions in the first place (these are the ones that are commented with bugs and block 1356458).

Comment 1

[Steve Fink [:sfink] [:s:]]

Attachment: CFG dumping utility, handy for debugging,

Debugging-only tool, not used by actual analysis, no need for review.

Comment 2

[Steve Fink [:sfink] [:s:]]

Attachment: Annotate atof as not doing any racy writes

Comment 3

[Steve Fink [:sfink] [:s:]]

Attachment: Targeted annotation for static local array of member pointers in LangGroupFontPrefs::Initialize

The code is

    void
    LangGroupFontPrefs::Initialize(nsIAtom* aLangGroupAtom)
    {
        nsFont* fontTypes[] = {
            &mDefaultVariableFont,
            &mDefaultFixedFont,
            &mDefaultSerifFont,
            &mDefaultSansSerifFont,
            &mDefaultMonospaceFont,
            &mDefaultCursiveFont,
            &mDefaultFantasyFont
        };

        nsFont* font = fontTypes[3];
        font->size = 42;
    }

'this' is known to be a safe pointer (exclusively owned by the current thread), so a pointer to one of its members is also safe. But the analysis can't track safety across all that, so I have a special-case annotation here that says that fontTypes[3] returns a safe pointer if and only if 'this' is safe.

Note that all of those fields (eg mDefaultVariableFont) are nsFont structs, not pointers, so although you'd expect this to be one dereference away from a safe pointer's memory, it is not; assigning to font->size ends up being a write to some offset within the 'this' pointer, which is known to be safe here.

Comment 4

[Steve Fink [:sfink] [:s:]]

Attachment: Annotate MOZ_CrashPrintf

Comment 5

[Steve Fink [:sfink] [:s:]]

Attachment: Broaden the annotation for string appending functions to handle more types

Comment 6

[Steve Fink [:sfink] [:s:]]

Attachment: Fix handling of parameterNames

This is for nicer output only, and does not affect the computation. A WorkListEntry contains a stack of CallSites, and the top of the stack represents the entry itself and so should share parameterNames. This changes fixes cases where some names were being registered in a different table than ended up being used by printouts.

Comment 7

[Steve Fink [:sfink] [:s:]]

Attachment: Refactor the --function debugging command line option (and alias it to -f) to analyzeHeapWrites.js,

Comment 8

[Steve Fink [:sfink] [:s:]]

Attachment: analyzeHeapWrites: implement a cache for checking whether local variables are safe pointers

At some point, I convinced myself that this cache was useful for performance. I'm not entirely sure that's true. (When stepping through the debugger, I saw it going back to the CFG repeatedly for the same variables. But I don't *really* know whether it matters.)

Comment 9

[Steve Fink [:sfink] [:s:]]

Attachment: analyzeHeapWrites: getter_Copies preserves safety (similar to getter_AddRefs)

Phew. I thought hard about how to handle getter_Copies, and when I finally went to implement it, I found that you'd already done exactly what I was intending for getter_Addrefs. \o/

Comment 10

[Steve Fink [:sfink] [:s:]]

Attachment: Backward-compatible js::Class[Ops].trace annotation

This really doesn't matter, but I was running some sanity tests on old xdbs, and it looks like these things have changed names.

Comment 11

[Steve Fink [:sfink] [:s:]]

Attachment: Annotate border colors array as being thread-owned by container

nsStyleStruct has the field:

  nsBorderColors** mBorderColors;

It starts out nullptr, and when it is needed, it allocates an array of 4 nsBorderColors pointers. But the nsStyleStruct exclusively owns the array; nothing else can get at it. This change teaches the analysis that if 'this' is a safe nsStyleStruct*, then it should treat mBorderColors as if it were an inline length-4 array.

Comment 12

[Steve Fink [:sfink] [:s:]]

Attachment: Annotate nsTArray::InsertElementAt as returning a value that propagates safety of the array

Comment 13

[Steve Fink [:sfink] [:s:]]

Attachment: Annotate Servo_ComputedValues_EqualCustomProperties as safe

I believe it is safe to say that Servo_ComputedValues_EqualCustomProperties won't do any racy writes.

Comment 14

[Steve Fink [:sfink] [:s:]]

Attachment: Annotate border colors array as being thread-owned by container

Oops, missed a followup fix to handle when isSafeLocalVariable is called with a totally different type of CFG node.

Comment 15

[Steve Fink [:sfink] [:s:]]

Attachment: Annotate border colors array as being thread-owned by container

Third time's a charm. There were two different fragments left out of the first upload here. I also needed to annotate the incoming parameters.

Comment 16

[Steve Fink [:sfink] [:s:]]

Attachment: stylo heap write analysis: Miscellaneous minor annotations

Comment 17

[Steve Fink [:sfink] [:s:]]

Attachment: Assert that Gecko_ShouldCreateStyleThreadPool is only called on the main thread

I don't actually know who I should be asking for review here. You? emilio? Please redirect as appropriate.

Comment 18

[Steve Fink [:sfink] [:s:]]

Attachment: Special-case annotation for a->b->emplace(); a->b->Init()

A bunch of machinery just to annotate

    keyframe->mTimingFunction.emplace()
    keyframe->mTimingFunction->Init()

as passing in a safe 'this' to Init().

Comment 19

[Steve Fink [:sfink] [:s:]]

Attachment: Trim down whitelists to only what is required, and mark all known issues with bug numbers

This doesn't exactly need review; the try server will review it for me. But it's the big one that removes all of the whole-function annotations that don't have a good reason to exist, and drops the expectation to zero.

\o/

Comment 20

[Steve Fink [:sfink] [:s:]]

Comment on attachment 8908898 [details] [diff] [review]
Backward-compatible js::Class[Ops].trace annotation

Marking r=me for tidiness.

Comment 21

[Steve Fink [:sfink] [:s:]]

Comment on attachment 8908888 [details] [diff] [review]
CFG dumping utility, handy for debugging,

r=me, debugging tool only

Comment 22

[Steve Fink [:sfink] [:s:]]

Comment on attachment 8908895 [details] [diff] [review]
Refactor the --function debugging command line option (and alias it to -f) to analyzeHeapWrites.js,

r=me, debugging code only.

Comment 23

[Steve Fink [:sfink] [:s:]]

Comment on attachment 8908908 [details] [diff] [review]
Assert that Gecko_ShouldCreateStyleThreadPool is only called on the main thread

r=emilio via IRC

Comment 24

[Steve Fink [:sfink] [:s:]]

Attachment: Propagate safety through RefPtr

Oops, missed this patch when uploading stuff.

Comment 25

[Steve Fink [:sfink] [:s:]]

Comment on attachment 8908889 [details] [diff] [review]
Annotate atof as not doing any racy writes

This one patch got missed, but it's trivial, so r=me.

Comment 26

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/9a3d18f5faee
CFG dumping utility, handy for debugging, r=me
https://hg.mozilla.org/integration/mozilla-inbound/rev/6696c7a9ed71
Propagate safety through RefPtr, r=bhackett
https://hg.mozilla.org/integration/mozilla-inbound/rev/4901b60d0126
Annotate atof as not doing any racy writes, r=me
https://hg.mozilla.org/integration/mozilla-inbound/rev/c5239fd503f2
Targeted annotation for static local array of member pointers in LangGroupFontPrefs::Initialize, r=bhackett
https://hg.mozilla.org/integration/mozilla-inbound/rev/84018f3951f9
Annotate MOZ_CrashPrintf, r=jonco
https://hg.mozilla.org/integration/mozilla-inbound/rev/46c262a38922
Broaden the annotation for string appending functions to handle more types, r=jonco
https://hg.mozilla.org/integration/mozilla-inbound/rev/410efc5458e0
Fix handling of parameterNames, r=jonco
https://hg.mozilla.org/integration/mozilla-inbound/rev/c37bd96f868a
Refactor the --function debugging command line option (and alias it to -f) to analyzeHeapWrites.js, r=me
https://hg.mozilla.org/integration/mozilla-inbound/rev/3835877e5870
analyzeHeapWrites: implement a cache for checking whether local variables are safe pointers, r=jonco
https://hg.mozilla.org/integration/mozilla-inbound/rev/4ff3e372de3d
analyzeHeapWrites: getter_Copies preserves safety (similar to getter_AddRefs), r=bhackett
https://hg.mozilla.org/integration/mozilla-inbound/rev/8fe39aef352a
Backward-compatible js::Class[Ops].trace annotation, r=me
https://hg.mozilla.org/integration/mozilla-inbound/rev/eb2b0c8fa756
Annotate border colors array as being thread-owned by container, r=bhackett
https://hg.mozilla.org/integration/mozilla-inbound/rev/8ec91b85f043
Annotate nsTArray::InsertElementAt as returning a value that propagates safety of the array, r=jonco
https://hg.mozilla.org/integration/mozilla-inbound/rev/08c0fdb95713
Annotate Servo_ComputedValues_EqualCustomProperties as safe, r=manishearth
https://hg.mozilla.org/integration/mozilla-inbound/rev/239eedb24ef2
stylo heap write analysis: Miscellaneous minor annotations, r=jonco
https://hg.mozilla.org/integration/mozilla-inbound/rev/696b8f10fcf3
Assert that Gecko_ShouldCreateStyleThreadPool is only called on the main thread, r=emilio
https://hg.mozilla.org/integration/mozilla-inbound/rev/d7209c7f31af
Special-case annotation for a->b->emplace(); a->b->Init(), r=bhackett
https://hg.mozilla.org/integration/mozilla-inbound/rev/1a53a959fd02
Trim down whitelists to only what is required, and mark all known issues with bug numbers, r=jonco

Comment 27

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/9a3d18f5faee
https://hg.mozilla.org/mozilla-central/rev/6696c7a9ed71
https://hg.mozilla.org/mozilla-central/rev/4901b60d0126
https://hg.mozilla.org/mozilla-central/rev/c5239fd503f2
https://hg.mozilla.org/mozilla-central/rev/84018f3951f9
https://hg.mozilla.org/mozilla-central/rev/46c262a38922
https://hg.mozilla.org/mozilla-central/rev/410efc5458e0
https://hg.mozilla.org/mozilla-central/rev/c37bd96f868a
https://hg.mozilla.org/mozilla-central/rev/3835877e5870
https://hg.mozilla.org/mozilla-central/rev/4ff3e372de3d
https://hg.mozilla.org/mozilla-central/rev/8fe39aef352a
https://hg.mozilla.org/mozilla-central/rev/eb2b0c8fa756
https://hg.mozilla.org/mozilla-central/rev/8ec91b85f043
https://hg.mozilla.org/mozilla-central/rev/08c0fdb95713
https://hg.mozilla.org/mozilla-central/rev/239eedb24ef2
https://hg.mozilla.org/mozilla-central/rev/696b8f10fcf3
https://hg.mozilla.org/mozilla-central/rev/d7209c7f31af
https://hg.mozilla.org/mozilla-central/rev/1a53a959fd02