Switching to gcc 8 unfortunately uncovered some previously-unknown hazards. I suspect they probably have a lot of overlap with the hazards uncovered in bug 1531951.

Many of these hazards are due to

    uint8 mozilla::BasePrincipal::Subsumes(nsIPrincipal*, uint32)
    mozilla::BasePrincipal.SubsumesInternal
    uint8 mozilla::ContentPrincipal::SubsumesInternal(nsIPrincipal*, uint32)
    nsIPrincipal.GetURI
    unresolved nsIPrincipal.GetURI

as in, Subsumes might GC because it calls nsIPrincipal.GetURI. I don't think any implementation of GetURI can GC, but it's remarkably hard to prove it; some subclasses do some crazy stuff. It would help if the analysis knew that nsIPrincipal is [builtinclass], at least; without that, I have to annotate it specifically, which immediately finds another thing it calls, and fixing that finds another...

Also, there is a bug where the analysis won't show the reasons for all GC functions. I have a fix for it somewhere in my patch stack for bug 1531951. For now, I'm running with those patches applied just to make it easier to figure out what to annotate. I'm hoping I won't have to fix all the hazards revealed by bug 1531951 in order to fix this bug, but it's possible that's how it will end up.

Ok, here's a particularly disturbing path, because it looks so plausible:

#53087 = uint8 mozilla::ContentPrincipal::SubsumesInternal(nsIPrincipal*, uint32)
#53076 = uint32 mozilla::ContentPrincipal::GetSiteOrigin(nsTSubstring<char>)
#927437 = uint32 mozilla::ContentPrincipal::GenerateOriginNoSuffixFromURI(nsIURI, nsTSubstring<char>)
#791092 = uint8 NS_URIIsLocalFile(nsIURI)
#422716 = nsINetUtil.ProtocolHasFlags:0
#42430 = mozilla::net::nsIOService.ProtocolHasFlags:0
#42431 = uint32 mozilla::net::nsIOService::ProtocolHasFlags(nsIURI*, uint32, uint8*)
#677533 = uint32 nsIProtocolHandler::DoGetProtocolFlags(nsIURI*, uint32*)
#420922 = nsIProtocolHandlerWithDynamicFlags.GetFlagsForURI:0
#51528 = mozilla::net::ExtensionProtocolHandler.GetFlagsForURI:0
#51529 = uint32 mozilla::net::ExtensionProtocolHandler::GetFlagsForURI(nsIURI*, uint32*)
#720140 = uint8 mozilla::extensions::WebExtensionPolicy::IsPathWebAccessible(nsTSubstring<char16_t>) const
#720141 = uint8 mozilla::extensions::MatchGlobSet::Matches(nsTSubstring<char16_t>) const
#1364832 = uint8 mozilla::extensions::MatchGlob::Matches(nsTSubstring<char16_t>) const
#584754 = uint8 JS::ExecuteRegExpNoStatics(JSContext, JS::Handle<JSObject*>, uint16*, uint64, uint64*, uint8, JS::MutableHandle<JS::Value>)
#570685 = JSFlatString* js::NewStringCopyN(JSContext*, uint16*, uint64) [with js::AllowGC allowGC = (js::AllowGC)1u; CharT = char16_t; size_t = long unsigned int]
...
#477443 = void js::gc::GCRuntime::collect(uint8, js::SliceBudget, int32)

#53076 = uint32 mozilla::ContentPrincipal::GetSiteOrigin(nsTSubstring<char>)

So the good news is that this GetSiteOrigin call is debug-only code.

The bad news is that on that codepath there are several things that could in theory run JS in today's world. Specifically:

1. Someone could introduce a JS impl of nsIEffectiveTLDService, so the access to that service in GetSiteOrigin runs JS.
2. nsIProtocolHandler can be implemented in JS, so that GetProtocolFlags call that nsIProtocolHandler::DoGetProtocolFlags can make can run JS.
3. The stack pasted in comment 1, which seems like a legit concern.

The other bad news is that even in non-debug builds ContentPrincipal::SubsumesInternal will call nsScriptSecurityManager::SecurityCompareURIs which calls NS_SecurityCompareURIs, which can end up looking for the ORIGIN_IS_FULL_SPEC protocol flag, though I think only in Thunderbird, not Firefox...

Ok, it appears that I was wrong -- this path is expected to GC. The problem is more in a caller: https://searchfox.org/mozilla-central/source/dom/bindings/BindingUtils.h#208-282

The hazard here is that unwrappedObj is kept live across a GC at https://searchfox.org/mozilla-central/rev/e5327b05c822cdac24e233afa37d72c0552dbbaf/dom/bindings/BindingUtils.h#265-266. Which is clearly true; it is assigned a value, and if the call succeeds then its value is used. That's all the analysis sees. But some additional complexity is that the value is also passed through the recursive call. The incoming value is used, and in most successful paths it will be updated to a non-invalidated value before returning, so in practice this is not an actual hazard.

Except for the "most". One tricky path is that you call the UnwrapObjectInternal, it calls itself recursively, and that recursive call makes it to https://searchfox.org/mozilla-central/rev/e5327b05c822cdac24e233afa37d72c0552dbbaf/dom/bindings/BindingUtils.h#224. Let's say the preceding UnwrapDOMObject call GCs and invalidates the object pointer (inner call obj, outer call unwrappedObj). Now the outer obj is given the corrupt unwrappedObj. The comment https://searchfox.org/mozilla-central/rev/e5327b05c822cdac24e233afa37d72c0552dbbaf/dom/bindings/BindingUtils.h#274-275 addresses something very similar, but claims to avoid the issue via tempValue. If I'm reading that correctly, though, that works because obj is in fact a pointer to a rooted location in this case. So the assignment to the value param is made safe, since the unstable unwrappedObj has now been saved to a rooted location. But that doesn't help in the scenario I described above, where unwrappedObj was invalidated in the inner call.

Even if that path turns out to be harmless, it's really hard to argue with the hazard analysis here -- not only does a GC pointer appear to be held live across a GC, the corrupted pointer is then used and passed back to the caller. I tried looking at callers to see if maybe there's a way to separate the parameters out so the corrupt value is never used outside of this function. (Because this whole thing feels pretty brittle, and it would be nice for the hazard analysis to be double-checking that we don't perturb things in a way that this becomes a real hazard.) But I got pretty tangled up, and it really looks like the value gets assigned through a MutableHandle<JSObject*> and used. So I'm going to throw up my hands and needinfo bz for clarification.

Perhaps the assumption here is that UnwrapDOMObject would never GC? One of the ways the analysis claims that can GC is:

#725776 = xptdata.cpp:uint32 UnwrapDOMObject(JS::Handle<JS::Value>, void**, JSContext*) [with mozilla::dom::prototypes::id::ID PrototypeID = (mozilla::dom::prototypes::id::ID)139u; T = mozilla::dom::Document; JS::HandleValue = JS::Handle<JS::Value>]
#725777 = uint32 mozilla::dom::UnwrapObject(JS::Handle<JS::Value>, RefPtr<mozilla::dom::Document>*, JSContext*) [with mozilla::dom::prototypes::id::ID PrototypeID = (mozilla::dom::prototypes::id::ID)139u; T = mozilla::dom::Document; U = RefPtr<mozilla::dom::Document>; CxType = JSContext*]
#726938 = uint32 mozilla::dom::UnwrapObject(JSObject*, RefPtr<mozilla::dom::Document>*, JSContext*) [with mozilla::dom::prototypes::id::ID PrototypeID = (mozilla::dom::prototypes::id::ID)139u; T = mozilla::dom::Document; U = mozilla::dom::Document; CxType = JSContext*]
#727542 = uint32 mozilla::dom::binding_detail::UnwrapObjectInternal(JSObject**, RefPtr<mozilla::dom::Document>*, uint16, uint32, JSContext*) [with T = mozilla::dom::Document; bool mayBeWrapper = true; U = RefPtr<mozilla::dom::Document>; V = JSObject*; CxType = JSContext*; mozilla::dom::prototypes::ID = mozilla::dom::prototypes::id::ID; uint32_t = unsigned int]
#511606 = JSObject* js::CheckedUnwrapDynamic(JSObject*, JSContext*, uint8)
#611285 = JSObject* js::UnwrapOneCheckedDynamic(JS::Handle<JSObject*>, JSContext*, uint8)
#1366 = js::Wrapper.dynamicCheckedUnwrapAllowed:0
#108722 = xpc::CrossOriginObjectWrapper.dynamicCheckedUnwrapAllowed:0
#108723 = uint8 xpc::CrossOriginObjectWrapper::dynamicCheckedUnwrapAllowed(JS::Handle<JSObject*>, JSContext*) const
#874560 = uint8 mozilla::dom::MaybeCrossOriginObjectMixins::IsPlatformObjectSameOrigin(JSContext*, JSObject*)
#648926 = uint8 mozilla::BasePrincipal::FastSubsumesConsideringDomainIgnoringFPD(nsIPrincipal*)
#648922 = uint8 mozilla::BasePrincipal::FastSubsumesIgnoringFPD(nsIPrincipal*, uint32)
#418314 = mozilla::BasePrincipal.SubsumesInternal:0

and then you get to the stuff I was talking about in previous comments.

If I allow it to go through ~RefPtr, it can find a lot more. (All paths require at least one of ~RefPtr, RefPtr<T>::assign_with_AddRef, CheckedUnwrapDynamic, CheckedUnwrapStatic, or gray unmarking. And I think the gray unmarking is probably bogus.)

(Oops, sorry. Must remember to reload comment threads; I wrote the above comment before I read bz's response.)

(In reply to Boris Zbarsky [:bzbarsky, bz on IRC] from comment #2)

even in non-debug builds ContentPrincipal::SubsumesInternal will call nsScriptSecurityManager::SecurityCompareURIs which calls NS_SecurityCompareURIs, which can end up looking for the ORIGIN_IS_FULL_SPEC protocol flag, though I think only in Thunderbird, not Firefox...

Yeah, sorry, I think now that it may be largely ok if Subsumes can GC. I think it may only be coming up because of the UnwrapObjectInternal hazard, though I really ought to check to be sure.

Let's say the preceding UnwrapDOMObject call GCs

This can't happen, I believe.

One of the ways the analysis claims that can GC is:

Where is this uint32 UnwrapDOMObject(JS::Handle<JS::Value>, void**, JSContext*) thing coming from? UnwrapDOMObject<T> has the signature T* UnwrapDOMObject(JSObject* obj), is defined at https://searchfox.org/mozilla-central/rev/e5327b05c822cdac24e233afa37d72c0552dbbaf/dom/bindings/BindingUtils.h#100-107 and very clearly cannot GC.

As far as UnwrapObjectInternal goes, there are two cases to consider:

• Case 1 is that mayBeWrapper is false. In this case, UnwrapObjectInternal cannot GC, because it will return at either https://searchfox.org/mozilla-central/rev/e5327b05c822cdac24e233afa37d72c0552dbbaf/dom/bindings/BindingUtils.h#224 or https://searchfox.org/mozilla-central/rev/e5327b05c822cdac24e233afa37d72c0552dbbaf/dom/bindings/BindingUtils.h#231
• Case 2 is that mayBeWrapper is true. In this case, UnwrapObjectInternal can put unwrappedObj on the stack, assign to it (possibly doing some GC before the assignment via CheckedUnwrapDynamic, which didn't use to be able to GC before it was a dynamic check, but now can), then do the recursive call with mayBeWrapper false, which cannot GC (see case 1).

So this looks safe to me: the recursive call done during the lifetime of unwrppedObj in fact cannot GC. The basic issue is that this is actually two different functions with different GC behavior, depending on the value of the mayBeWrapper template parameter, but I assume static analysis treats them as a single function?

Is there a way to teach the static analysis about the compile-time-two-functions thing? If not, we could try to move the shared bit to a macro or something like that, which is the desired compiler output anyway. Or maybe just annotate this somehow as safe? Seems dangerous...

Where is this uint32 UnwrapDOMObject(JS::Handle<JS::Value>, void**, JSContext*) thing coming from?

Ah, from the autogenerated xptdata.cpp, but it's not the thing we're calling here.

Yeah, bz and I talked. UnwrapDOMObject (the actual one that's getting called here) cannot GC, and UnwrapObjectInternal<mayBeWrapper=false> cannot GC but the analysis thinks it can. That's the root problem here.

For now, I think I'll annotate UnwrapObjectInternal<mayBeWrapper=false> as not GCing. It feels like I shouldn't have to. Here's the relevant portion of the CFG (not that I expect anyone to be able to parse this):

Assume(3,4, !null(domClass*), true) //  `if (domClass)` passes, run the if body
Assume(3,9, !null(domClass*), false) // `if (domClass)` fails, skip the body
Assume(4,5, (protoID* == domClass*.mInterfaceChain[protoDepth*]{uint16}*), true)
Assume(4,9, (protoID* == domClass*.mInterfaceChain[protoDepth*]{uint16}*), false)
Call(5,6, __temp_3 := obj*.operator 195())
Call(6,7, __temp_2 := UnwrapDOMObject(__temp_3*))
Call(7,8, value*.operator=(__temp_2))
Assign(8,36, return := 0)
Call(9,10, __temp_6 := obj*.operator 195())
Call(10,11, __temp_5 := IsWrapper(__temp_6*))
Assume(11,12, !__temp_5*, true)
Assume(11,13, !__temp_5*, false)
Assign(12,14, __temp_4 := 1)
Assign(13,14, __temp_4 := 0)
Assume(14,15, __temp_4*, true)
Assume(14,16, __temp_4*, false)

So the if (!mayBeWrapper || !js::IsWrapper(obj)) starts at point 9, which goes straight to the js::IsWrapper(obj) call (it has to extract obj via some operator on the 9->10 edge). Which is really weird! That code should never be run, but the analysis thinks it will be. Something smells very very bad. I think I'll move this bug back to the GC component and use it for this problem.

To be fair, even if the CFG were correct, it is likely that it wouldn't have eliminated dead code and so wouldn't help this case. It's mostly disturbing to me that the analysis is ever exposed to an incorrect CFG. Which is why it's worth checking whether sixgill is mangling things during its translation process.

No need for this to be hidden now; it just turns out to be a source of false positives.

The priority flag is not set for this bug.
:jonco, could you have a look please?

For more information, please visit auto_nag documentation.