I made a mistake and orphaned several sixgill fixes on a different topological branch. When I rebase to include the fixes from both branches, I see a few new hazards.

One is in this code:

bool XPCJSRuntime::DescribeCustomObjects(JSObject* obj, const JSClass* clasp,
                                         char (&name)[72]) const {
  XPCWrappedNativeProto* p = XPCWrappedNativeProto::Get(obj);
  nsCOMPtr<nsIXPCScriptable> scr = p->GetScriptable();
  SprintfLiteral(name, "JS Object (%s - %s)", clasp->name, scr->GetJSClass()->name);
  return true;
}

The analysis sees that scr is a ref counted pointer, and its reference is decremented at the end of the scope. If for some bizarre reason scr->GetJSClass() were to decrement all other references to the scriptable, then this could GC when Releasing the nsIXPCScriptable, which is a [scriptable] non-[builtinclass] interface.

I would like an annotation to say "we're not doing anything here that could possibly drop the ref count".

Updating the existing ref counted variable handling mechanism exposed a sixgill bug: this code

void aggr_init_safe() {
  GC();
  auto [ok, nogc] = pair_returning_function();
}

is considered to be a hazard because the CFG produced is:

Call(1,2, GC())
Call(2,3, __temp_2 := pair_returning_function())
Call(3,4, ok := get(__temp_1))
Call(4,5, nogc := get(__temp_1))
Call(5,6, __temp_1.~__dt_comp ())

Notice that the return value of pair_returning_function() is stored in __temp_2, and __temp_1 is never initialized at all. I made the scan more conservative as a side effect of prepping for the change here, and because it doesn't see an initialization for __temp_1 it assumes it is live throughout the body of the function (as if it were a parameter or global variable).

This is sort of a side effect of bug 1450379 where I improperly "fixed" this case: before that bug, it would get confused by the construct and just throw out the whole function. After, it generates an almost-correct CFG.

It's not as concise as it could be, but after the fix here the CFG is:

Call(1,2, GC())
Call(2,3, __temp_2 := pair_returning_function())
Assign(3,4, __temp_1 := __temp_2*)
Call(4,5, ok := get(__temp_1))
Call(5,6, nogc := get(__temp_1))
Call(6,7, __temp_1.~__dt_comp ())

One of those temporaries isn't really needed, but it seems fine to have it for a relatively uncommon construct. (Note: these descriptions of the CFG leave out some information, so the above is a bit cryptic. The two get calls are actually std::pair<bool, AutoCheckCannotGC>::get(bool&&) and std::pair<bool, AutoCheckCannotGC>::get(AutoCheckCannotGC&&) which get autogenerated from the c++11 syntax in the code.)

Set release status flags based on info from the regressing bug 1450379

After running the analysis on the full tree, it seems my earlier conservative change is not going to fly. It produces too many false alarms because it requires live ranges to start with an assignment, and there are a number of other ways that something could be initialized (eg by passing a pointer as a retparam to an initialization function).

Which means that the bugfix here isn't strictly necessary. But given that it repairs an erroneous break in the data flow, it may end up being important for other parts of the analysis (like bug 1816165).

Note that I kind of piled two related things into this bug. The wanted but not necessary fix is comment 1. The issue in comment 0 is still valid and required for avoiding some false positives. (And none of this needs backporting.)

Set release status flags based on info from the regressing bug 1450379