Various XPCWrappedNativeTearoff cleanups

RESOLVED FIXED

Status

()

RESOLVED FIXED
4 years ago
3 years ago

People

(Reporter: mccr8, Assigned: mccr8)

Tracking

Trunk
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(7 attachments)

Comment hidden (empty)
(Assignee)

Comment 1

4 years ago
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)
(Assignee)

Comment 3

4 years ago
Created attachment 8578104 [details] [diff] [review]
part 1 - Remove unused forward declaration of xpc_GetJSPrivate.

Unrelated bonus cleanup.

try run: https://treeherder.mozilla.org/#/jobs?repo=try&revision=29ea902a6a20
Attachment #8578104 - Flags: review?(bobbyholley)
(Assignee)

Comment 4

4 years ago
Created attachment 8578107 [details] [diff] [review]
part 2 - Use nsCOMPtr for obj in XPCWrappedNative::InitTearOff.
Attachment #8578107 - Flags: review?(bobbyholley)
(Assignee)

Comment 5

4 years ago
Created attachment 8578108 [details] [diff] [review]
part 3 - Make XPCWrappedNativeTearOff::mNative a smart pointer.
Attachment #8578108 - Flags: review?(bobbyholley)
Attachment #8578104 - Flags: review?(bobbyholley) → review+
(Assignee)

Comment 6

4 years ago
Created attachment 8578109 [details] [diff] [review]
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.
Attachment #8578109 - Flags: review?(bobbyholley)
(Assignee)

Comment 7

4 years ago
Created attachment 8578110 [details] [diff] [review]
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.
Attachment #8578110 - Flags: review?(bobbyholley)
(Assignee)

Comment 8

4 years ago
Created attachment 8578111 [details] [diff] [review]
part 6 - Use an nsAutoPtr for mNextChunk.
Attachment #8578111 - Flags: review?(bobbyholley)
(Assignee)

Comment 9

4 years ago
Created attachment 8578112 [details] [diff] [review]
part 7 - Eliminate XPCWrappedNativeTearOffChunk.

Inline it into XPCWrappedNativeTearOff.
Attachment #8578112 - Flags: review?(bobbyholley)
(Assignee)

Comment 10

4 years ago
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)
(Assignee)

Updated

4 years ago
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+
(Assignee)

Updated

4 years ago
Keywords: leave-open
(Assignee)

Comment 15

3 years ago
I can file a new bug if I decide to work on the last 2 parts.
Status: NEW → RESOLVED
Last Resolved: 3 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)
(Assignee)

Updated

3 years ago
Blocks: 1183754
(Assignee)

Comment 17

3 years ago
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.