1595974 - [hazards] nsCOMArray<nsINode> does not root

Status: RESOLVED INVALID

(Core :: JavaScript: GC, defect)

Description

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

There are some places that have a nsCOMArray<nsINode> on the stack when we could GC, and I don't see how that roots its contents (it might, I just don't see it.)

nsCOMArray<nsINode> is considered invalidatable because it holds nsINodes that inherit from EventTarget that inherits from nsWrapperCache that has an mWrapper field of type JSObject*. nsWrapperCache does have code for tracing, so perhaps nsCOMArray somehow arranges for that to be called when necessary? There's a comment explaining how nsWrapperCache handles compaction via an objectMovedOp class hook. And perhaps anything with a wrapper is guaranteed to be kept live? And perhaps wrappers are not nursery-allocated?

Anyway, to figure out the right place to annotate, I need to understand why this isn't a problem.

Example hazard:

mozilla::dom::Document::AdoptNode(nsINode*, mozilla::ErrorResult*)' has unrooted 'nodesWithProperties' of type 'nsCOMArray<nsINode>' live across GC call '_ZN17nsDOMAttributeMap20BlastSubtreeToPiecesEP7nsINode$void nsDOMAttributeMap::BlastSubtreeToPieces(nsINode*)' at dom/base/Document.cpp:9399
GC Function: _ZN17nsDOMAttributeMap20BlastSubtreeToPiecesEP7nsINode$void nsDOMAttributeMap::BlastSubtreeToPieces(nsINode*)
    uint32 mozilla::dom::Element::UnsetAttr(int32, nsAtom*, uint8)
    uint32 AttrArray::RemoveAttrAt(uint32, nsAttrValue*)
    uint32 AttrArray::MakeMappedUnique(nsMappedAttributes*)
    already_AddRefed<nsMappedAttributes> nsHTMLStyleSheet::UniqueMappedAttributes(nsMappedAttributes*)
    PLDHashEntryHdr* PLDHashTable::Add(void*, mozilla::fallible_t*)
    PLDHashTableOps.initEntry
    arbitrary function pointer PLDHashTableOps.initEntry

So this terminates in a probably bogus PLDHashTableOps function pointer, but there are other ways this could GC:

#722553 = uint32 mozilla::dom::Element::UnsetAttr(int32, nsAtom*, uint8)
#1569074 = void mozilla::dom::Element::PostIdMaybeChange(int32, nsAtom*, nsAttrValue*)
#1568567 = void mozilla::dom::Element::AddToIdTable(nsAtom*)
#1568568 = void mozilla::dom::ShadowRoot::AddToIdTable(mozilla::dom::Element*, nsAtom*)
#1563932 = void mozilla::IdentifierMapEntry::AddIdElement(mozilla::dom::Element*)
#1563927 = void mozilla::IdentifierMapEntry::FireChangeCallbacks(mozilla::dom::Element*, mozilla::dom::Element*, uint8)
#1563929 = mozilla::IdentifierMapEntry::ChangeCallback.mCallback

or

722553 = uint32 mozilla::dom::Element::UnsetAttr(int32, nsAtom*, uint8)
#1263536 = void mozAutoDocUpdate::~mozAutoDocUpdate() [[complete_dtor]]
#1263537 = void mozAutoDocUpdate::~mozAutoDocUpdate() [[base_dtor]]
#1253642 = void mozilla::dom::Document::EndUpdate()
#1564812 = void mozilla::dom::Document::MaybeEditingStateChanged()
#1564726 = uint32 mozilla::dom::Document::EditingStateChanged()
#1564772 = uint8 mozilla::dom::Document::ExecCommand(nsTSubstring<char16_t>*, uint8, nsTSubstring<char16_t>*, nsIPrincipal*, mozilla::ErrorResult*)
#1564783 = mozilla::dom::Document::InternalCommandData.mGetEditorCommandFunc

or even, if it's possible for script to be unblocked,

#722553 = uint32 mozilla::dom::Element::UnsetAttr(int32, nsAtom*, uint8)
#1250777 = void mozilla::AsyncEventDispatcher::RunDOMEventWhenSafe()
#761508 = void nsContentUtils::AddScriptRunner(nsIRunnable*)
#1338968 = void nsContentUtils::AddScriptRunner(already_AddRefed<nsIRunnable>)
#420545 = nsIRunnable.Run:0
#33443 = mozilla::Runnable.Run:0
#410136 = MainFunctionRunnable.Run:0
#410137 = nsEmbedFunctions.cpp:uint32 {anonymous}::MainFunctionRunnable::Run()
#2070964 = MainFunctionRunnable.mFunction

Comment 1

[Boris Zbarsky [:bzbarsky]]

nsCOMArray<nsINode> is conceptually equivalent to nsTArray<RefPtr<nsINode>>. It holds strong pointers to the things inside it. That keeps the object itself (the nsINode) alive. That may or may not keep the object's mWrapper alive, depending on whether PreservingWrapper() is true.

If PreservingWrapper() is false, then the mWrapper could get GCed; that would invoke the finalize hook of the appropriate nsINode subclass, which would then clear mWrapper.

mWrapper could also be moved. That should trigger the UpdateWrapper call to update the value of mWrapper. This comes from the objectMovedOp of the relevant JSClass, which is invoked both when moving (from RelocateCell, I think) and when tenuring a nursery-allocated thing (from TenuringTracer::moveToTenuredSlow). And yes, mWrapper can be nursery-allocated.

So I think this is all safe, as far as it goes. In terms of annotating things, how are we annotating nsTArray<RefPtr<nsINode>>, if at all?

Comment 2

[Andrew McCreight [:mccr8]]

It is pretty strange that it is complaining about nsCOMArray specifically. Forget about nsTArray<RefPtr<nsINode>>, why doesn't it complain about nsINode* on the stack? That's way more dangerous. (I would like it if it did complain about that!)

Comment 3

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

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

nsCOMArray<nsINode> is conceptually equivalent to nsTArray<RefPtr<nsINode>>. It holds strong pointers to the things inside it. That keeps the object itself (the nsINode) alive. That may or may not keep the object's mWrapper alive, depending on whether PreservingWrapper() is true.

If PreservingWrapper() is false, then the mWrapper could get GCed; that would invoke the finalize hook of the appropriate nsINode subclass, which would then clear mWrapper.

mWrapper could also be moved. That should trigger the UpdateWrapper call to update the value of mWrapper. This comes from the objectMovedOp of the relevant JSClass, which is invoked both when moving (from RelocateCell, I think) and when tenuring a nursery-allocated thing (from TenuringTracer::moveToTenuredSlow). And yes, mWrapper can be nursery-allocated.

So I think this is all safe, as far as it goes. In terms of annotating things, how are we annotating nsTArray<RefPtr<nsINode>>, if at all?

Excellent, thank you for the explanation!

As for nsTArray, it is one of several containers that is not yet annotated as inheriting the attributes (eg GC instability) from the type of its contents. I am working on landing a patch that does this for several containers, including both nsCOMArray and nsTArray. I will remove nsCOMArray from the patch. The patch is currently in bug 1582895 but I may split it out (unsurprisingly, it was trickier that I had hoped.)

Comment 4

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

(In reply to Andrew McCreight [:mccr8] from comment #2)

It is pretty strange that it is complaining about nsCOMArray specifically. Forget about nsTArray<RefPtr<nsINode>>, why doesn't it complain about nsINode* on the stack? That's way more dangerous. (I would like it if it did complain about that!)

Where is nsINode* on the stack?

But it's not going to complain about that. nsINode* boils down to JSObject**. The hazard analysis doesn't complain about those, since it has no good way to determine whether the pointer it's pointing to is rooted/traced or not.

The reason why it's specifically complaining about nsCOMArray is that I have a patch adding MOZ_INHERIT_TYPE_ANNOTATIONS_FROM_TEMPLATE_ARGS to various container types. (And I had forgotten that nsCOMArray was in that list, pushed this aging patch stack, and then started wondering why I was only now seeing these hazards...)

Comment 5

[Andrew McCreight [:mccr8]]

(In reply to Steve Fink [:sfink] [:s:] from comment #4)

Where is nsINode* on the stack?

Well, here's a bunch of body element pointers on the stack:
https://searchfox.org/mozilla-central/search?q=bodyelement*&path=

Back to one of your original questions, wrapper cached things are traced because they get added to a big table of objects in the nsWrapperCache::HoldJSObjects() call. That adds it to CycleCollectedJSRuntime::mJSHolderMap, which gets traced as part of the gray root tracing.

Comment 6

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

Oh, I think you were wondering why nsCOMArray<nsINode> would complain if nsINode* does not? That's because Collection<T> is considered to directly contain a T. In practice, Collection will usually allocate a separate memory chunk or chunks to hold the Ts (though if it has some inline storage, maybe not.) The actual storage location doesn't matter, really, it's just that the analysis knows that if you do have a T hidden inside of a collection, then it shouldn't assume you're going to root it. So it'll get mad if you have a Collection<T> on the stack (and T is a GC pointer). If you want to do that, you'll need to use a Rooted<Collection<T>> if it is supported.

Short version: the hazard analysis complains about JSObject* on the stack but not JSObject**. And a Collection<T> is considered to be equivalent to

struct Collection {
  T t1;
  T t2;
  ...
};

and not

struct Collection {
  T* all_the_Ts;
};

Comment 7

[Andrew McCreight [:mccr8]]

Ah, ok. In this case, it is a T*.

Comment 8

[Andrew McCreight [:mccr8]]

I don't think this is actually a sec bug, for multiple reasons.

Comment 9

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

Yeah, it's not a bug either. Sorry, I should've closed this.