1288870 - Refcount XPCNativeInterface

Status: RESOLVED FIXED

(Core :: XPConnect, defect)

Description

[Andrew McCreight [:mccr8]]

Similar to bug 1288817. This class is used more, and has some nasty bits like placement new and the call context state machine that have to be dealt with, but it seems doable. It does entrain jsid, but it never tells the GC about them, so I think I can ignore that part of it. I have some patches in progress.

Comment 1

[Andrew McCreight [:mccr8]]

ASan try run: https://treeherder.mozilla.org/#/jobs?repo=try&revision=9008017e28f2

Comment 2

[Andrew McCreight [:mccr8]]

Attachment: part 1 - Add NS_INLINE_DECL_REFCOUNTING_WITH_DESTROY.

This is the same basic idea as NS_IMPL_RELEASE_WITH_DESTROY. I need this because I am making XPCNativeInterface refcounted, and it uses some weird placement new stuff requiring a special function to deallocate the object. (It does this to store an array of arbitrary length inline, presumably for some sort of time or space reason.)

Comment 3

[Andrew McCreight [:mccr8]]

Attachment: part 2 - Make XPCNativeInterface refcounted.

There are four classes that call Root() on XPCNativeInterface, and thus keep interfaces alive. Each of these gets converted to use a RefPtr:

1. XPCCallContext. This could be on some kind of hot path, but the FindMemberCall involves various string operations and hashtable lookups, so adding a single AddRef shouldn't matter. One weirdness here is that the context only roots the interface when |mState >= HAVE_NAME|. With a RefPtr<>, this requires nulling out mInterface. Fortunately, in most cases where it moves from rooting to non-rooting, it already does this. The one case it does not is in SystemIsBeingShutDown(), so my patch adds that.

2. XPCNativeSet. This holds an array of interfaces in a weird placement new array at the end of the object. I wasn't sure how a non-POD class would interact with the way the array is handled with casting, so I manually AddRef and Release things put into or removed from the array.

3. AutoMarkingNativeInterfacePtr simply becomes RefPtr<>. This is the bulk of the patch, in terms of number of lines changed.

4. Similarly, the one AutoMarkingNativeInterfacePtrArrayPtr becomes nsTArray<RefPtr<>>. This is the last use of the auto marking array class, so I deleted it.

Here are some other notes on what the patch does:

- XPCNativeInterfaces are created with placement new. This requires a special version of refcounting that calls DestroyInstance, defined in the previous patch. The GetNewOrUsed methods used to explicitly call DestroyInstance(), but with refcounting this is no longer needed.

- The Mark() etc. methods are gutted so they don't do anything and mMarked is removed because it is no longer used. The methods will be cleaned up in later patches in this bug.

- Interfaces are removed from mIID2NativeInterfaceMap in the dtor instead of during sweeping, requiring an extra hash table lookup.

- All of the methods that can create a new interface (NewInstance, GetISupports, GetNewOrUsed) now return an already_AddRefed<>, which gives some static checking that we don't accidentally fail to hold onto a newly created interface.

Comment 4

[Andrew McCreight [:mccr8]]

Attachment: part 3 - Remove the now-vestigial Mark code for XPCNativeInterface.

PCNativeInterface::Mark(), Unmark() and IsMarked() don't do anything any more, so anything that calls them can be deleted. This removes the only use of XPCCallContext::CanGetInterface(), so delete that, too.

Also, inline XPCNativeSet::MarkSelfOnly(), because XPCNativeSet::Mark() now only calls it.

Comment 5

[Andrew McCreight [:mccr8]]

Attachment: part 2 - Make XPCNativeInterface refcounted.

I removed some AutoJSContext that aren't needed any more.

Comment 6

[Andrew McCreight [:mccr8]]

Attachment: part 2 - Make XPCNativeInterface refcounted.

Minor rebasing over your suggested fixes in bug 1289137.

Comment 7

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

Comment on attachment 8779355 [details] [diff] [review]
part 2 - Make XPCNativeInterface refcounted.

Review of attachment 8779355 [details] [diff] [review]:
-----------------------------------------------------------------

::: js/xpconnect/src/XPCWrappedNativeInfo.cpp
@@ +730,5 @@
>      // Stick the nsISupports in front and skip additional nsISupport(s)
>      XPCNativeInterface** outp = (XPCNativeInterface**) &obj->mInterfaces;
>      uint16_t memberCount = 1;   // for the one member in nsISupports
>  
> +    NS_ADDREF(*(outp++) = isup);

Could you use the forget().take() thing here as well?

::: js/xpconnect/src/xpcprivate.h
@@ +1410,5 @@
>      uint16_t                mInterfaceCount : 15;
>      uint16_t                mMarked : 1;
> +    // Always last - object sized for array.
> +    // These are strong references.
> +    XPCNativeInterface*     mInterfaces[1];

I think it makes sense for some of the XPCNativeSet member functions to use refcounting in their signatures. The fact that you can get a raw XPCNativeInterface from FindMember is a little worrisome to me. Could this instead use RefPtr<XPCNativeInterface>* in the signature?

Comment 8

[Andrew McCreight [:mccr8]]

(In reply to Bill McCloskey (:billm) from comment #7)
> Could you use the forget().take() thing here as well?
Unfortunately not because isup is used later in the method.

> Could this instead use RefPtr<XPCNativeInterface>* in the signature?

Hmm, maybe. I'll try something like that. I'll go through and check every method with an XPCNI** argument.

Comment 9

[Andrew McCreight [:mccr8]]

Attachment: part 2b - Return a strong pointer from XPCNativeMember::GetCallInfo().

MozReview-Commit-ID: 3sF4Oql2Pbn

Comment 10

[Andrew McCreight [:mccr8]]

Attachment: part 2c - Use a strong reference for the returned interface in XPCNativeSet::FindMember.

DefinePropertyIfFound is slightly odd: it passes in a NI*, but the argument can be null, in which case we get a new one. This patch just adds a new local that is a strong pointer so we can keep it alive.

These two patches fix all of the places that are passing in a ** thing. There are a few places that arguable we should also hold strong references to interfaces, but I think they are okay because they are interfaces from a set that is on the stack, so if the set stays alive, so should the interface, as sets are immutable. And if the set dies, we have a UAF anyways. Maybe I should think about adding a bunch more stack rooting in refcounting for sets patch, though...

I'll land these two patches rolled into part 2.

Comment 11

[Andrew McCreight [:mccr8]]

opt and debug are green:
  https://treeherder.mozilla.org/#/jobs?repo=try&revision=ffe1e782e117
  https://treeherder.mozilla.org/#/jobs?repo=try&revision=1e95ddb75840

Comment 12

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/8d71aba5c1e7
part 1 - Add NS_INLINE_DECL_REFCOUNTING_WITH_DESTROY. r=froydnj
https://hg.mozilla.org/integration/mozilla-inbound/rev/4f0ab1a0d8dd
part 2 - Make XPCNativeInterface refcounted. r=billm
https://hg.mozilla.org/integration/mozilla-inbound/rev/83bbd356da97
part 3 - Remove the now-vestigial Mark code for XPCNativeInterface. r=billm

Comment 13

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

All backed out for introducing some hazards: https://treeherder.mozilla.org/logviewer.html#?job_id=34165475&repo=mozilla-inbound 

https://hg.mozilla.org/integration/mozilla-inbound/rev/93545d47fddb

Comment 14

[Andrew McCreight [:mccr8]]

What is happening is that my patch adds local variables of type RefPtr<XPCNativeInterface>, and when those leave the scope, we can end up calling ~XPCNativeInterface. This in turn calls IID2NativeInterfaceMap::Remove(), which calls a method on nsIInterfaceInfo, which is a virtual method, so the hazard analysis decides it might GC. Of course, the two actual implementations are totally trivial. I'll talk to sfink and try to figure out what to do here.

Comment 15

[Andrew McCreight [:mccr8]]

Attachment: part 2b - Mark nsIInterfaceInfo{,Manager} builtinclass.

I think technically you could implement one of these in JS now, which might prevent me from reasonably asserting that these can never GC. I doubt anybody would ever do that, so it should be okay. There are zero references to these two interfaces in all of addon DXR, and none in Firefox JS.

try run: https://treeherder.mozilla.org/#/jobs?repo=try&revision=3ed682720e49&filter-tier=1
  (still in progress but I can't imagine it will find anything)

MozReview-Commit-ID: 3AkW0AkqmNx

Comment 16

[Nathan Froyd [:froydnj]]

Comment on attachment 8783023 [details] [diff] [review]
part 2b - Mark nsIInterfaceInfo{,Manager} builtinclass.

Review of attachment 8783023 [details] [diff] [review]:
-----------------------------------------------------------------

We should have been much more aggressive in marking things builtinclass. :(

Comment 17

[Andrew McCreight [:mccr8]]

The builtinclass thing didn't help, so I'll spin that into a separate bug.

With sfink's advice, I marked nsIInterfaceInfo.GetIIDShared as not GCing, but that's just exposed another source of GCing in ~XPCNativeInterface, PLDHashTableOps.clearEntry. I don't know how well the rooting analysis does with structs of function pointers.

I wrote patches to just do the additional rooting in the two places the hazard analysis reports, and that works without being too bad, but this will leave around a bogus hazard for anybody in the future who carries out further changes to the code.

Comment 18

[Andrew McCreight [:mccr8]]

Attachment: part 2 - Handlify the argument to XPCNativeSet::FindMember().

This is to work around false GC hazards in ~XPCNativeInterface in part 4 of this bug. Putting RefPtr<XPCNativeInterface> on the stack means that ~XPCNativeInterface can get called in various places, and the GC hazard analysis does not understand the virtual methods involved, so it assumes they might possibly GC.

This fixes one hazard by taking a jsid handle argument. The callers already pass in handles, so no other changes are needed.

MozReview-Commit-ID: LpNpTlujpkm

Comment 19

[Andrew McCreight [:mccr8]]

Attachment: part 3 - Root the return value of the prewrap callback.

r?mrbkap for the XPConnect changes, r?terrence for the rest.

Like part 2, this patch is to work around a false GC hazard in ~XPCNativeInterface in part 4. This hazard is around the return value of WrapperFactory::PrepareForWrapping(), because ~XPCCallContext might call ~XPCNativeInterface. The fix is to pass the return value into a mutable handle. Unfortunately, this function is used in the JSAPI, so JS minor engine changes are also needed.

MozReview-Commit-ID: GwFxmrXFXmb

Comment 20

[Blake Kaplan (:mrbkap) (inactive)]

Comment on attachment 8783667 [details] [diff] [review]
part 3 - Root the return value of the prewrap callback.

Review of attachment 8783667 [details] [diff] [review]:
-----------------------------------------------------------------

I have a couple of ideas, but r=me with or without them.

::: js/xpconnect/wrappers/WrapperFactory.cpp
@@ +167,5 @@
>          // navigated-away-from Window. Strip any CCWs.
>          obj = js::UncheckedUnwrap(obj);
>          if (JS_IsDeadWrapper(obj)) {
>              JS_ReportError(cx, "Can't wrap dead object");
> +            retObj.set(nullptr);

Maybe set retObj to nullptr on entry so you don't have to explicitly set it to null when we want to return null? Also, it's a bit cheeky, but you could possibly use a ScopeExit to coalesce all of the |waive ? WaiveXray(...) : ...| checks now that we're sticking the return value in an out param as opposed to simply returning it.

Comment 21

[Andrew McCreight [:mccr8]]

(In reply to Blake Kaplan (:mrbkap) from comment #20)
> Maybe set retObj to nullptr on entry so you don't have to explicitly set it
> to null when we want to return null?

Sure, I'll do that.

> Also, it's a bit cheeky, but you could
> possibly use a ScopeExit to coalesce all of the |waive ? WaiveXray(...) :
> ...| checks now that we're sticking the return value in an out param as
> opposed to simply returning it.

That's not a bad idea. The current situation is kind of ugly. I'll file a followup for it.

Comment 22

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/4737a86a4c0e
part 1 - Add NS_INLINE_DECL_REFCOUNTING_WITH_DESTROY. r=froydnj
https://hg.mozilla.org/integration/mozilla-inbound/rev/d82cba5e2b6a
part 2 - Handlify the argument to XPCNativeSet::FindMember(). r=mrbkap
https://hg.mozilla.org/integration/mozilla-inbound/rev/ffc79e7e8297
part 3 - Root the return value of the prewrap callback. r=mrbkap,terrence
https://hg.mozilla.org/integration/mozilla-inbound/rev/5cdc3e5e091e
part 4 - Make XPCNativeInterface refcounted. r=billm
https://hg.mozilla.org/integration/mozilla-inbound/rev/842bf278fe45
part 5 - Remove the now-vestigial Mark code for XPCNativeInterface. r=billm

Comment 23

[Andrew McCreight [:mccr8]]

Comment on attachment 8783023 [details] [diff] [review]
part 2b - Mark nsIInterfaceInfo{,Manager} builtinclass.

I landed this in a separate bug.

Comment 24

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/4737a86a4c0e
https://hg.mozilla.org/mozilla-central/rev/d82cba5e2b6a
https://hg.mozilla.org/mozilla-central/rev/ffc79e7e8297
https://hg.mozilla.org/mozilla-central/rev/5cdc3e5e091e
https://hg.mozilla.org/mozilla-central/rev/842bf278fe45