Closed
Bug 1142717
Opened 9 years ago
Closed 9 years ago
Various XPCWrappedNativeTearoff cleanups
Categories
(Core :: XPConnect, defect)
Core
XPConnect
Tracking
()
RESOLVED
FIXED
People
(Reporter: mccr8, Assigned: mccr8)
References
Details
Attachments
(7 files)
976 bytes,
patch
|
bholley
:
review+
|
Details | Diff | Splinter Review |
4.74 KB,
patch
|
bholley
:
review+
|
Details | Diff | Splinter Review |
6.14 KB,
patch
|
bholley
:
review+
|
Details | Diff | Splinter Review |
1.89 KB,
patch
|
bholley
:
review+
|
Details | Diff | Splinter Review |
12.17 KB,
patch
|
bholley
:
review+
|
Details | Diff | Splinter Review |
2.16 KB,
patch
|
Details | Diff | Splinter Review | |
9.60 KB,
patch
|
Details | Diff | Splinter Review |
No description provided.
Assignee | ||
Comment 1•9 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)
Comment 2•9 years ago
|
||
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•9 years ago
|
||
Unrelated bonus cleanup. try run: https://treeherder.mozilla.org/#/jobs?repo=try&revision=29ea902a6a20
Attachment #8578104 -
Flags: review?(bobbyholley)
Assignee | ||
Comment 4•9 years ago
|
||
Attachment #8578107 -
Flags: review?(bobbyholley)
Assignee | ||
Comment 5•9 years ago
|
||
Attachment #8578108 -
Flags: review?(bobbyholley)
Updated•9 years ago
|
Attachment #8578104 -
Flags: review?(bobbyholley) → review+
Assignee | ||
Comment 6•9 years ago
|
||
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•9 years ago
|
||
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•9 years ago
|
||
Attachment #8578111 -
Flags: review?(bobbyholley)
Assignee | ||
Comment 9•9 years ago
|
||
Inline it into XPCWrappedNativeTearOff.
Attachment #8578112 -
Flags: review?(bobbyholley)
Assignee | ||
Comment 10•9 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•9 years ago
|
Attachment #8578112 -
Flags: review?(bobbyholley)
Comment 11•9 years ago
|
||
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 12•9 years ago
|
||
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+
Updated•9 years ago
|
Attachment #8578109 -
Flags: review?(bobbyholley) → review+
Updated•9 years ago
|
Attachment #8578110 -
Flags: review?(bobbyholley) → review+
Assignee | ||
Updated•9 years ago
|
Keywords: leave-open
Assignee | ||
Comment 13•9 years ago
|
||
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•9 years ago
|
||
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
Assignee | ||
Comment 15•9 years ago
|
||
I can file a new bug if I decide to work on the last 2 parts.
Comment 16•8 years ago
|
||
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 | ||
Comment 17•8 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.
Description
•