1142717 - Various XPCWrappedNativeTearoff cleanups

Status: RESOLVED FIXED

(Core :: XPConnect, defect)

Comment 1

[Andrew McCreight [:mccr8]]

Bobby, are you okay with me eliminating XPC_WRAPPED_NATIVE_TEAROFFS_PER_CHUNK?  This controls the number of tearoffs in each tearoff chunk, but it has been 1 since the initial landing of this code in 2001.  I was going to eliminate the array and just make each chunk contain a single tearoff.  It adds some complexity, because there are a number of extra loops.

Then, once the array is eliminated, chunks can be removed by making tearoffs a linked list.  I have some WIP patches if you want to see more concretely what this looks like.

Comment 2

[Bobby Holley (:bholley)]

Please do - I tried this 3 years ago in bug 569506, and got stuck on some very weird GC crashes that I couldn't reproduce locally. Probably worth trying again.

Comment 3

[Andrew McCreight [:mccr8]]

Attachment: part 1 - Remove unused forward declaration of xpc_GetJSPrivate.

Unrelated bonus cleanup.

try run: https://treeherder.mozilla.org/#/jobs?repo=try&revision=29ea902a6a20

Comment 4

[Andrew McCreight [:mccr8]]

Attachment: part 2 - Use nsCOMPtr for obj in XPCWrappedNative::InitTearOff.

Comment 5

[Andrew McCreight [:mccr8]]

Attachment: part 3 - Make XPCWrappedNativeTearOff::mNative a smart pointer.

Comment 6

[Andrew McCreight [:mccr8]]

Attachment: part 4 - Add MOZ_COUNT_CTOR/DTOR for XPCWrappedNativeTearOff.

This isn't ideal, because we'll end up double-counting the tearoff contained in the XPCWN, but that seems better than just not counting these.

Comment 7

[Andrew McCreight [:mccr8]]

Attachment: part 5 - Eliminate XPC_WRAPPED_NATIVE_TEAROFFS_PER_CHUNK.

This has been 1 since the what looks like the initial landing in 2001, so clearly we don't need this generality.

Comment 8

[Andrew McCreight [:mccr8]]

Attachment: part 6 - Use an nsAutoPtr for mNextChunk.

Comment 9

[Andrew McCreight [:mccr8]]

Attachment: part 7 - Eliminate XPCWrappedNativeTearOffChunk.

Inline it into XPCWrappedNativeTearOff.

Comment 10

[Andrew McCreight [:mccr8]]

Comment on attachment 8578111 [details] [diff] [review]
part 6 - Use an nsAutoPtr for mNextChunk.

I've been told I should use UniquePointer instead of nsAutoPtr, so I guess I'll look into that.

Comment 11

[Bobby Holley (:bholley)]

Comment on attachment 8578107 [details] [diff] [review]
part 2 - Use nsCOMPtr for obj in XPCWrappedNative::InitTearOff.

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

r=me with that.

::: js/xpconnect/src/XPCWrappedNative.cpp
@@ +1161,5 @@
>      // Determine if the object really does this interface...
>  
>      const nsIID* iid = aInterface->GetIID();
>      nsISupports* identity = GetIdentityObject();
> +    nsCOMPtr<nsISupports> obj;

Could you rename this to something like |qiResult| or something? |obj| is a really bad name for this.

Also, I think this should actually be nsRefPtr<nsISupports>, and include a comment saying that we're explicitly not using the COMPtr machinery and managing QI manually. IMO nsCOMPtr<nsISupports> implies that the pointer inside is the canonical nsISupports pointer, which isn't necessarily true here.

Comment 12

[Bobby Holley (:bholley)]

Comment on attachment 8578108 [details] [diff] [review]
part 3 - Make XPCWrappedNativeTearOff::mNative a smart pointer.

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

::: js/xpconnect/src/XPCWrappedNative.cpp
@@ +904,5 @@
>                  to->JSObjectFinalized();
>              }
>  
>              // We also need to release any native pointers held...
> +            nsCOMPtr<nsISupports> obj = to->TakeNative();

Less important here, but if you want to rename this to something other than |obj| I wouldn't object. :-)

::: js/xpconnect/src/xpcprivate.h
@@ +1976,5 @@
>      XPCWrappedNativeTearOff& operator= (const XPCWrappedNativeTearOff& r) = delete;
>  
>  private:
>      XPCNativeInterface* mInterface;
> +    nsCOMPtr<nsISupports> mNative;

Again, I think this should be nsRefPtr<nsISupports>.

Comment 13

[Andrew McCreight [:mccr8]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/8e04782685f4
https://hg.mozilla.org/integration/mozilla-inbound/rev/4e8385a50da1
https://hg.mozilla.org/integration/mozilla-inbound/rev/faab462bd5c5
https://hg.mozilla.org/integration/mozilla-inbound/rev/de43ef55e8ef
https://hg.mozilla.org/integration/mozilla-inbound/rev/418f69d905f0
https://hg.mozilla.org/integration/mozilla-inbound/rev/fe24d9f4790f

I split the qiResult renaming into a separate patch, replaced the COMPtrs with RefPtrs and renamed obj to native or something.

Comment 14

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/8e04782685f4
https://hg.mozilla.org/mozilla-central/rev/4e8385a50da1
https://hg.mozilla.org/mozilla-central/rev/faab462bd5c5
https://hg.mozilla.org/mozilla-central/rev/de43ef55e8ef
https://hg.mozilla.org/mozilla-central/rev/418f69d905f0
https://hg.mozilla.org/mozilla-central/rev/fe24d9f4790f

Comment 15

[Andrew McCreight [:mccr8]]

I can file a new bug if I decide to work on the last 2 parts.

Comment 16

[Bobby Holley (:bholley)]

Andrew, did the last two patches in this bug cause try failures, or was it just an issue of AutoPtr vs UniquePtr? I'd take a patch with nsAutoPtr if it means clearing up the current weirdness with XPCWrappedNativeChunks (which aren't really chunks at all).

Comment 17

[Andrew McCreight [:mccr8]]

Thanks for the reminder. I updated my patches for bug 1183754. I'll push them to try and then upload them for review.