Closed Bug 1288909 Opened 3 years ago Closed 3 years ago

Refcount XPCNativeSet

Categories

(Core :: XPConnect, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla52
Tracking Status
firefox50 --- affected
firefox52 --- fixed

People

(Reporter: mccr8, Assigned: mccr8)

References

Details

Attachments

(4 files)

This will be very similar to bug 1288870.

One difficulty is that mClassInfo2NativeSetMap contains weak references to native sets, but the key is a nsIClassInfo*, so I'm not sure how to remove the native set from the map when it dies. One solution might be to put the key into the native set, to allow the lookup. Another would be to give the map a strong reference, and then have sweeping code that removes entries where the refcount of the native set is 1. This would always be safe to do, so it would not be a problem from the perspective of zonal GCs.
Blocks: 1289192
Assignee: nobody → continuation
I implemented a version of this that adds a strong reference to mClassInfo2NativeSetMap, and sweeps in the finalize callback. However, I noticed that in some local experimentation that it wasn't ever sweeping it. Reading the code, I think it is okay to leave this table weak.

The only code that adds to this table is XPCNativeSet::GetNewOrUsed(nsIClassInfo* classInfo), which in turn is only called by XPCNativeSet::GetNewOrUsed(nsIClassInfo* classInfo). This code then creates a new XPCWrappedNativeProto, which (with my patch) holds a strong reference to the set that has been added to the table. This set is never changed or released until the dtor for the proto, which calls XPCNativeSet::ClearCacheEntryForClassInfo(), removing the entry from the hashtable. Thus, the livetime of the set is always going to be longer than the lifetime of the entry.
Depends on: 1289457
Depends on: 1290239
I'm going to wait for the various blocking bugs to settle out before posting patches, but you can see them in the try patches if you are curious. It is almost the same as bug 1288870, except for a few tricky issues with global maps.
The blocking bugs have settled out enough that I'm going to ask for review.

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

No new GC hazards. I still need to do an opt run and an ASan run to make sure that is okay.
Blocks: 1292694
Just some trivial rebasing.
Comment on attachment 8779406 [details]
Bug 1288909, part 1 - Implement refcounting of XPCNativeSet.

https://reviewboard.mozilla.org/r/70398/#review74648

1. Could we make XPCNativeSetKey hold a RefPtr (and maybe mark it MOZ_STACK_CLASS?). It seems like it shouldn't matter, and it feels safer.

2. If I understand the argument about mClassInfo2NativeSetMap, then it seems like we could convert the map's XPCNativeSet reference to a strong reference without causing any leaks. That feels safer than what you have now (in that an error causes a leak instead of a crash). Would that work?

::: js/xpconnect/src/XPCWrappedNativeInfo.cpp:465
(Diff revision 3)
>  }
>  
>  /***************************************************************************/
>  // XPCNativeSet
>  
> +XPCNativeSet::~XPCNativeSet() {

Brace should be on the next line.

::: js/xpconnect/src/XPCWrappedNativeInfo.cpp:466
(Diff revision 3)
>  
>  /***************************************************************************/
>  // XPCNativeSet
>  
> +XPCNativeSet::~XPCNativeSet() {
> +

Please remove the blank line.
Attachment #8779406 - Flags: review?(wmccloskey) → review+
Comment on attachment 8779407 [details]
Bug 1288909, part 2 - Remove a bunch of now-useless XPCNativeSet marking-related things.

https://reviewboard.mozilla.org/r/70400/#review74706
Attachment #8779407 - Flags: review?(wmccloskey) → review+
Comment on attachment 8779408 [details]
Bug 1288909, part 3 - XPCWrappedNative:: and XPCWrappedNativeProto::Mark() don't do anything any more.

https://reviewboard.mozilla.org/r/70402/#review74708

::: js/xpconnect/src/XPCJSRuntime.cpp
(Diff revision 3)
> -            // NativeSets, and the WrappedNativeJSClasses...
> -
> -            // Do the marking...
> -            XPCWrappedNativeScope::MarkAllWrappedNativesAndProtos();
> -
> -            for (auto i = self->mDetachedWrappedNativeProtoMap->Iter(); !i.Done(); i.Next()) {

I looked to see what this map is for and I can't find any place where we add things to it. Could you look into deleting it?
Attachment #8779408 - Flags: review?(wmccloskey) → review+
> 1. Could we make XPCNativeSetKey hold a RefPtr (and maybe mark it
> MOZ_STACK_CLASS?). It seems like it shouldn't matter, and it feels safer.

Good point. Both the XPCNativeSet and XPCNativeInterface fields can be refpointers. I've also added the MOZ_STACK_CLASS annotation.
 
> 2. If I understand the argument about mClassInfo2NativeSetMap, then it seems
> like we could convert the map's XPCNativeSet reference to a strong reference
> without causing any leaks. That feels safer than what you have now (in that
> an error causes a leak instead of a crash). Would that work?

That sounds like a good idea. The patch is a little involved because PLDHashtable is annoying so I'll put it up for review once try comes back green.

I also fixed the style nits.

> Could you look into deleting it?

I filed bug 1300830 for removing it.
Comment on attachment 8789033 [details]
Bug 1288909, part 4 - Use a strong reference to the set in ClassInfo2NativeSetMap.

https://reviewboard.mozilla.org/r/77308/#review78260
Attachment #8789033 - Flags: review?(wmccloskey) → review+
Pushed by amccreight@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/9769cc42f549
part 1 - Implement refcounting of XPCNativeSet. r=billm
https://hg.mozilla.org/integration/autoland/rev/8dd979e76e1c
part 2 - Remove a bunch of now-useless XPCNativeSet marking-related things. r=billm
https://hg.mozilla.org/integration/autoland/rev/862e46017481
part 3 - XPCWrappedNative:: and XPCWrappedNativeProto::Mark() don't do anything any more. r=billm
https://hg.mozilla.org/integration/autoland/rev/5dc379423bd6
part 4 - Use a strong reference to the set in ClassInfo2NativeSetMap. r=billm
You need to log in before you can comment on or make changes to this bug.