1726671 - Missing hazards due to template parameter parsing

Status: RESOLVED FIXED

(Core :: JavaScript: GC, defect, P1)

Description

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

I was tracking down why bug 1720422 introduced a hazard when it looks like that hazard should have already been present.

It turns out to be due to template parameter processing. When we have a type dom::Klass<JS::Danger>, and Klass is annotated as MOZ_INHERIT_TYPE_ANNOTATIONS_FROM_TEMPLATE_ARGS, and JS::Danger is a GC pointer type, then Klass<JS::Danger> is also supposed to be regarded as a GC pointer (even if it is implemented in some awful way that obscures the fact that it contains a JS::Danger field or whatever. AlignedStorage2 is a typical culprit.)

The problem is that the parsing of template args is known to be incomplete. (We unfortunately don't have an actual decomposable type structure for this, so need to resort to string parsing.) It didn't seem like it would need to deal with anything too complex at the time I wrote it, so I didn't try very hard:

    // Unfortunately, we just have a string type name, not the full structure
    // of a templatized type, so we will have to resort to loose (buggy)
    // pattern matching.
    //
    // Currently, the simplest ways I know of to break this are:
    //
    //   foo<T>::bar<U>
    //   foo<bar<T,U>>
    //

This failed on mozilla::AlignedStorage2<mozilla::dom::TypedArray<unsigned char, JS::UnwrapArrayBufferMaybeShared, JS::GetArrayBufferMaybeSharedData, JS::GetArrayBufferMaybeSharedLengthAndData, JS::NewArrayBuffer> >, so failed to see that it should be a GC pointer type. Simplified, that's AlignedStorage2<TypedArray<foo>>, which is an even simpler failure mode than in the above comment and I should not have allowed this to fail that easily.

Anyway, bug 1720422 simplifies the type enough for it to be parsed correctly, exposing the pre-existing hazards.

The straightforward approach would be to:

1. Fix the hazards, land the fixes.
2. Fix the parsing (change it to actually parse instead of doing lame pattern-matching.)
3. Land bug 1720422.

...except I suspect #1 will require adding new facilities for Gecko code to do what it wants to do safely. And I would much rather base that work off of the changes in bug 1720422. So instead, I think I will

1. Fix the parsing
2. Annotate away the existing hazards (land these together)
3. Land bug 1720422
4. Fix the hazards

I have one annotation I've tried for #2, but it only suppressed some of the hazards, which caused me to circle back and figure out what's really going on. (Which I did and thereby produced this bug report.)

Comment 1

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

It wasn't just the parsing. Once I fixed the parsing, it hit a (known, documented) limitation in how type info is propagated for templates that are supposed to inherit GC sensitivity from the template parameters. It's a graph traversal ordering problem.

With those fixed, I get 9 hazards in various bindings *Init types, plus one wasm hazard false positive. I think the former are probably false positives as well, but I need to look into them further.

I have a patch for the wasm false positive, using a new annotation mechanism.

I'm going to call this bug a sec-audit. There's a good chance all of the *Init hazards are false alarms as well, but I won't know until I dig further into what's going on there.

Comment 2

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

Attachment: Bug 1726671 - [hazard] Fix parsing and processing of template parameters

Comment 3

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

I was wrong, the hazards were much easier to fix than expected once peterv clued me in to RootedDictionary and RootingCx(). So it looks like I'll be able to fix the hazards, fix the parsing, then land the Typed Array stack (no additional fixes are needed.)

Comment 4

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

[hazard] Fix parsing and processing of template parameters r=jonco
https://hg.mozilla.org/integration/autoland/rev/b647ffc0d5dfbc28a8403543bec89a5d27936837
https://hg.mozilla.org/mozilla-central/rev/b647ffc0d5df