Closed Bug 1142717 Opened 7 years ago Closed 6 years ago

Various XPCWrappedNativeTearoff cleanups


(Core :: XPConnect, defect)

Not set





(Reporter: mccr8, Assigned: mccr8)




(7 files)

No description provided.
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.
Flags: needinfo?(bobbyholley)
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.
Flags: needinfo?(bobbyholley)
Attachment #8578104 - Flags: review?(bobbyholley) → review+
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.
Attachment #8578109 - Flags: review?(bobbyholley)
This has been 1 since the what looks like the initial landing in 2001, so clearly we don't need this generality.
Attachment #8578110 - Flags: review?(bobbyholley)
Attachment #8578111 - Flags: review?(bobbyholley)
Inline it into XPCWrappedNativeTearOff.
Attachment #8578112 - Flags: review?(bobbyholley)
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.
Attachment #8578111 - Flags: review?(bobbyholley)
Attachment #8578112 - Flags: review?(bobbyholley)
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.
Attachment #8578107 - Flags: review?(bobbyholley) → review+
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>.
Attachment #8578108 - Flags: review?(bobbyholley) → review+
Attachment #8578109 - Flags: review?(bobbyholley) → review+
Attachment #8578110 - Flags: review?(bobbyholley) → review+
Keywords: leave-open
I can file a new bug if I decide to work on the last 2 parts.
Closed: 6 years ago
Keywords: leave-open
Resolution: --- → FIXED
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).
Flags: needinfo?(continuation)
Blocks: 1183754
Thanks for the reminder. I updated my patches for bug 1183754. I'll push them to try and then upload them for review.
Flags: needinfo?(continuation)
You need to log in before you can comment on or make changes to this bug.