925914 - Root object used in NoteGCThingXPCOMChildren

Status: RESOLVED INVALID

(Core :: XPCOM, defect)

Description

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

Hazard:

Function 'void mozilla::CycleCollectedJSRuntime::NoteGCThingXPCOMChildren(js::Class*, JSObject*, nsCycleCollectionTraversalCallback*) const' has unrooted 'aObj' of type 'JSObject*' live across GC call 'void CycleCollectionNoteEdgeName(nsCycleCollectionTraversalCallback*, int8*, uint32)' at xpcom/base/CycleCollectedJSRuntime.cpp:589

Comment 1

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

Attachment: Root object used in NoteGCThingXPCOMChildren

This one may be a false positive due to NS_DebugBreak thinking it can GC, but it doesn't seem like it'd do any harm to root.

Comment 2

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

Oh, right. Except I totally cheated and used nsContentUtils::GetSafeJSContext, and I have no idea what that does.

Comment 3

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

Please don't use the typedefs in this code, it's not under js/.

Comment 4

[Andrew McCreight [:mccr8]]

Comment on attachment 816063 [details] [diff] [review]
Root object used in NoteGCThingXPCOMChildren

Oops, I forgot about this review.

NoteGCThingXPCOMChildren is only called during cycle collection, and we really shouldn't GC during this part of the CC.  If we're do, we're almost certainly going to crash anyways, even with non-generational GC.  Really, the analysis should be getting angrier in this whole section, but I think it just can't tell that we're passing around a GC thing as a void*.  Can we do the assert-no-gc-thing here?  Assuming that isn't horribly expensive.

Comment 5

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

(In reply to Andrew McCreight [:mccr8] from comment #4)
> Comment on attachment 816063 [details] [diff] [review]
> Root object used in NoteGCThingXPCOMChildren
> 
> Oops, I forgot about this review.
> 
> NoteGCThingXPCOMChildren is only called during cycle collection, and we
> really shouldn't GC during this part of the CC.  If we're do, we're almost
> certainly going to crash anyways, even with non-generational GC.  Really,
> the analysis should be getting angrier in this whole section, but I think it
> just can't tell that we're passing around a GC thing as a void*.  Can we do
> the assert-no-gc-thing here?  Assuming that isn't horribly expensive.

Happily, the analysis was only thinking that any of this could GC because of NS_DebugBreak, which I have now annotated. So this patch is unneeded.

GC Function: void CycleCollectionNoteEdgeNameImpl(nsCycleCollectionTraversalCallback*, int8*, uint32)
    void GCGraphBuilder::NoteNextEdgeName(int8*)
    nsCString* nsCString::operator=(int8*)
    void nsACString_internal::Assign(int8*)
    NS_DebugBreak
    uint32 CrashReporter::AppendAppNotesToCrashReport(nsACString_internal*)
    uint32 CrashReporter::AnnotateCrashReport(nsACString_internal*, nsACString_internal*)
    uint32_t nsBaseHashtable<KeyClass, DataType, UserDataType>::EnumerateRead(nsBaseHashtable<KeyClass, DataType, UserDataType>::EnumReadFunction, void*) const [with KeyClass = nsCStringHashKey; DataType = nsCString; UserDataType = nsCString; uint32_t = unsigned int; nsBaseHashtable<KeyClass, DataType, UserDataType>::EnumReadFunction = PLDHashOperator (*)(const nsACString_internal&, nsCString, void*); PLDHashOperator = PLDHashOperator; nsBaseHashtable<KeyClass, DataType, UserDataType>::KeyType = const nsACString_internal&]
    PL_DHashTableEnumerate
    IndirectCall: etor