1531956 - [hazards] Cannot prove CanonicalizeXPCOMParticipant does not GC

Status: NEW

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

Description

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

Sample hazard below. Relevant call is https://searchfox.org/mozilla-central/rev/00f3836a87b844b5e4bc82f698c559b9966e4be2/xpcom/base/nsCycleCollector.cpp#848-849 but I have never truly understood the situation with QueryInterface.

Function '_ZNK7mozilla23CycleCollectedJSRuntime24NoteGCThingXPCOMChildrenEPKN2js5ClassEP8JSObjectR34nsCycleCollectionTraversalCallback$void mozilla::CycleCollectedJSRuntime::NoteGCThingXPCOMChildren(js::Class*, JSObject*, nsCycleCollectionTraversalCallback*) const' has unrooted 'aObj' of type 'JSObject*' live across GC call 'mozilla::CycleCollectedJSRuntime.NoteCustomGCThingXPCOMChildren:0' at xpcom/base/CycleCollectedJSRuntime.cpp:627
CycleCollectedJSRuntime.cpp:624: Call(1,2, __temp_1 := __builtin_expect(null(aClasp*),0))
CycleCollectedJSRuntime.cpp:624: Assume(2,3, (__temp_1* != 0), true)
CycleCollectedJSRuntime.cpp:624: Call(3,4, MOZ_ReportAssertionFailure("aClasp","/builds/worker/checkouts/gecko/xpcom/base/CycleCollectedJSRuntime.cpp",624))
CycleCollectedJSRuntime.cpp:624: Call(4,5, AnnotateMozCrashReason("MOZ_RELEASE_ASSERT(aClasp)"))
CycleCollectedJSRuntime.cpp:624: Assign(5,6, 0 := 624)
CycleCollectedJSRuntime.cpp:624: Call(6,7, abort())
CycleCollectedJSRuntime.cpp:625: Call(7,8, __temp_3 := GetObjectClass(aObj*))
CycleCollectedJSRuntime.cpp:625: Call(8,9, __temp_2 := __builtin_expect((__temp_3* !=p{js::Class} aClasp*),0))
CycleCollectedJSRuntime.cpp:625: Assume(9,10, (__temp_2* != 0), true)
CycleCollectedJSRuntime.cpp:625: Call(10,11, MOZ_ReportAssertionFailure("aClasp == js::GetObjectClass(aObj)","/builds/worker/checkouts/gecko/xpcom/base/CycleCollectedJSRuntime.cpp",625))
CycleCollectedJSRuntime.cpp:625: Call(11,12, AnnotateMozCrashReason("MOZ_RELEASE_ASSERT(aClasp == js::GetObjectClass(aObj))"))
CycleCollectedJSRuntime.cpp:625: Assign(12,13, 0 := 625)
CycleCollectedJSRuntime.cpp:625: Call(13,14, abort())
CycleCollectedJSRuntime.cpp:627: Call(14,15, __temp_4 := this*.NoteCustomGCThingXPCOMChildren*(aClasp*,aObj*,aCb*)) [[GC call]]
CycleCollectedJSRuntime.cpp:627: Assume(15,16, __temp_4*, false)
CycleCollectedJSRuntime.cpp:634: Assume(16,17, (0 != (aClasp*.flags* & 1)), true)
CycleCollectedJSRuntime.cpp:634: Assume(17,18, (0 != (aClasp*.flags* & 8)), true)
CycleCollectedJSRuntime.cpp:636: Call(18,19, CycleCollectionNoteEdgeName(aCb*,"js::GetObjectPrivate(obj)",0))
CycleCollectedJSRuntime.cpp:637: Call(19,20, __temp_5 := GetObjectPrivate(aObj*))
GC Function: mozilla::CycleCollectedJSRuntime.NoteCustomGCThingXPCOMChildren:0
XPCJSRuntime.NoteCustomGCThingXPCOMChildren:0
uint8 XPCJSRuntime::NoteCustomGCThingXPCOMChildren(js::Class*, JSObject*, nsCycleCollectionTraversalCallback*) const
nsCycleCollectionTraversalCallback.NoteXPCOMChild:0
CCGraphBuilder.NoteXPCOMChild:0
void CCGraphBuilder::NoteXPCOMChild(nsISupports*)
nsCycleCollector.cpp:nsISupports* CanonicalizeXPCOMParticipant(nsISupports*)
nsISupports.QueryInterface:0
(unknown-definition)
internal

Comment 1

[Andrew McCreight [:mccr8]]

In general QI can be implemented in JS, but the one here can't be. I hope.

Comment 2

[Jon Coppeard (:jonco)]

(In reply to Andrew McCreight [:mccr8] from comment #1)
I have also never understood this.

What determines which QIs can be implemented in JS? Is there some way we can assert this property? Or maybe it would be possible to make a special version of QI that can never execute JS.

Comment 3

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

Oops, looks like I never submitted this comment.

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

In general QI can be implemented in JS, but the one here can't be. I hope.

How can I tell? Even if it's complicated; if I need to, I'll work on a new type of annotation. If you QI to a [builtinclass], is it safe?

---

I attempted to track this down. It appears the simple part of the answer is that JS-implemented QueryInterface will go through nsXPCWrappedJSClass::DelegatedQueryInterface, which will end up invoking JS:

  #472683 = uint32 nsXPCWrappedJSClass::DelegatedQueryInterface(nsXPCWrappedJS*, nsID*, void**)
  #472802 = JSObject* nsXPCWrappedJSClass::CallQueryInterfaceOnJSObject(JSContext*, JSObject*, JS::Handle<JSObject*>, nsID*)
  #14195 = uint8 JS_CallFunctionValue(JSContext*, JS::Handle<JSObject*>, JS::Handle<JS::Value>, JS::HandleValueArray*, JS::MutableHandle<JS::Value>)
  #29418 = uint8 js::Call(JSContext*, JS::Handle<JS::Value>, JS::Handle<JS::Value>, js::AnyInvokeArgs*, JS::MutableHandle<JS::Value>)
  #67086 = Interpreter.cpp:uint8 InternalCall(JSContext*, js::AnyInvokeArgs*)
  #67081 = uint8 js::InternalCallOrConstruct(JSContext*, JS::CallArgs*, uint32)
  #67066 = uint8 js::RunScript(JSContext*, js::RunState*)

DelegatedQueryInterface can be called from a variety of places:

  #483456 = uint32 WrappedJSHolder::QueryInterface(nsID*, void**)
  #329767 = uint32 CallQueryInterface(nsXPCWrappedJS*, nsXPCOMCycleCollectionParticipant**) [with T = nsXPCWrappedJS; DestinationType = nsXPCOMCycleCollectionParticipant]
  #472683 = uint32 nsXPCWrappedJSClass::DelegatedQueryInterface(nsXPCWrappedJS*, nsID*, void**)
  #404243 = uint32 mozilla::dom::UnwrapArgImpl(JSContext*, JS::Handle<JSObject*>, nsID*, void**)
  #472824 = uint8 XPCConvert::JSObject2NativeInterface(JSContext*, void**, JS::Handle<JSObject*>, nsID*, nsISupports*, uint32*)
  #937242 = already_AddRefed<nsISupports> mozilla::dom::CallbackObjectHolderBase::ToXPCOMCallback(mozilla::dom::CallbackObject*, const nsIID&) const

So it looks like the analysis can figure this out just fine -- if it has access to the correct type of the component. Looking at the example I started with, it looks like that is not the case -- all we know statically is that it is an nsISupports*. The only other hint we have locally in that function is that we're QI'ing to NS_GET_IID(nsCycleCollectionISupports).

mccr8: two questions. (1) is there a straightforward way to construct a table of all concrete types that implement a given interface? (2) Would that even help here, or do JS components implement nsCycleCollectionISupports anyway?

Comment 4

[Andrew McCreight [:mccr8]]

(In reply to Jon Coppeard (:jonco) from comment #2)

What determines which QIs can be implemented in JS? Is there some way we can assert this property? Or maybe it would be possible to make a special version of QI that can never execute JS.

In general, if an XPIDL class is marked with builtinclass, then it can't be implemented by JS. If the class can't be implemented by JS, then the QI can't be implemented in JS.

Comment 5

[Andrew McCreight [:mccr8]]

Oh, that is not actually correct. While a builtinclass can't be implemented by JS, there's no guarantee that the thing that we're attempting to QI is not implemented in JS, and thus we could end up calling into JS.

Comment 6

[Andrew McCreight [:mccr8]]

So, as sfink sort of said, the way this works is that any nsIFoo object implemented by JS is exposed to C++ as an nsXPCWrappedJS. The QI of that C++ class is nsXPCWrappedJS::QueryInterface(), and it has a few special cases, including nsCycleCollectionISupports, that it checks before it calls into DelegatedQueryInterface(), which is the thing that would actually run JS.

Comment 7

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

Attachment: Bug 1531956 - [hazards] Handle QI to manually handled interfaces r=mccr8

Comment 8

[BugBot [:suhaib / :marco/ :calixte]]

There's a r+ patch which didn't land and no activity in this bug for 2 weeks.
:sfink, could you have a look please?
For more information, please visit auto_nag documentation.

Comment 9

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

(In reply to Release mgmt bot [:sylvestre / :calixte] from comment #8)

There's a r+ patch which didn't land and no activity in this bug for 2 weeks.
:sfink, could you have a look please?

I guess I made a bit of a tangle in the dependency graph, but I'm not sure how to do it better. Basically, there is a dependency cycle between this bug and bug 1531951 -- that bug implements an analysis improvement that can't land until the problems it reveals are fixed, but the fix in this bug uses the implementation in that bug. So they have to land together. I suppose I could lump them into one bug, but there are several different fixes needed that I want to track separately.

I guess I'll just add the dependency cycle. It accurately reflects reality. ...no I won't, bugzilla forbids that. (And I can't blame it.)

Ok, I'm leaving this as-is. Somebody let me know if a dependency link the other way around would be preferred, or if I should lump this bug into the improvement bug.