Closed Bug 1288817 Opened 8 years ago Closed 8 years ago

Refcount XPCNativeScriptableShared

Categories

(Core :: XPConnect, defect)

Product:
Core
Component:
XPConnect
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla51
Tracking Flags:
Tracking Status
firefox50 --- affected
firefox51 --- fixed

People

(Reporter: mccr8, Assigned: mccr8)

References

Details

Attachments

(4 files, 4 obsolete files)

Bug 1288817, part 1 - Make XPCNativeScriptableShared refcounted.
58 bytes, text/x-review-board-request
billm
: review+
Details
Bug 1288817, part 2 - Remove marking methods from XPCNativeScriptableShared and XPCNativeScriptableFlags.
58 bytes, text/x-review-board-request
billm
: review+
Details
Bug 1288817, part 3 - Stop calling XPCNativeScriptableInfo::Mark() from various places because it doesn't do anything.
58 bytes, text/x-review-board-request
billm
: review+
Details
part 1b - Don't delete XPCWrappedNative::mScriptableInfo during shutdown.
1.63 KB, patch
billm
: review+
Details | Diff | Splinter Review 
XPCNativeScriptableShared is held alive via various XPCWrappedNative things, and its lifetime is currently managed by the GC, through an odd Mark() system. Besides the complexity of this system, it is bad because we only free these objects in full GCs, which we'd like to reduce or eliminate.

It seems like we maybe mark all XPCWNs and protos in all GCs, but I think it would be better to just work on eliminating this bizarre setup.

The only fields of XPCNativeScriptableShared are a js::Class and a XPCNativeScriptableFlags (which is just an integer), so I don't think they can point to any JS GC things.

Therefore, I think we can just refcount these objects.
Depends on: 1288823
I still need to clean up my patches some more, but I have something that seems to work.

try run: https://treeherder.mozilla.org/#/jobs?repo=try&revision=6fdb87155d07
Bobby, is it okay if billm reviews this and the other bugs (bug 1288870, bug 1288909) to use refcounting instead of the weird Mark() GC stuff? These patches should only affect the lifetime management of the classes, not anything about what they do, and Bill understands that as well as anybody (or did at some point).

Also, do you think XPCNativeScriptableSharedMap::GetNewOrUsed() is performance critical still? My patches here will add a heap allocation on the fast path, which hopefully won't matter, but I haven't measured anything.

Thanks.
Flags: needinfo?(bobbyholley)
This adds a heap allocation in XPCNativeScriptableSharedMap::GetNewOrUsed() on the fast path. Hopefully that code is not hot enough for it to matter. I could work around this if needed, but it would be ugly.

mNativeScriptableSharedMap has a weak pointer to XPCNativeScriptableShared. I moved this removal from XPCJSRuntime::FinalizeCallback() to the dtor.

There are two types of scriptable: one is a dummy one created in GetNewOrUsed() to do a lookup, and the other is a fully filled out one. The dummy one is not added to the hashtable, and may have had its name stolen if we created a new scriptable. The latter makes it so that you crash if you try to look it up in the hashtable anyways, so this patch only looks up fully filled out scriptables.

This patch also removes MOZ_COUNT_CTOR/DTOR because they are not needed for refcounted classes.
Nothing depends on the value of the mark bit now.
(In reply to Andrew McCreight [:mccr8] from comment #3)
> Bobby, is it okay if billm reviews this and the other bugs (bug 1288870, bug
> 1288909) to use refcounting instead of the weird Mark() GC stuff? These
> patches should only affect the lifetime management of the classes, not
> anything about what they do, and Bill understands that as well as anybody
> (or did at some point).

Yep, you and Bill understand the XPConnect lifetime management as well as I do, probably more. Excited to hear it's going to get simpler! :-)
Flags: needinfo?(bobbyholley)
Blocks: 1289192
Attachment #8774396 - Flags: review?(wmccloskey)
Attachment #8774397 - Flags: review?(wmccloskey)
Attachment #8774398 - Flags: review?(wmccloskey)
This adds a heap allocation in
XPCNativeScriptableSharedMap::GetNewOrUsed() on the fast
path. Hopefully that code is not hot enough for it to matter. I could
work around this if needed, but it would be ugly.

mNativeScriptableSharedMap has a weak pointer to
XPCNativeScriptableShared. I moved this removal from
XPCJSRuntime::FinalizeCallback() to the dtor.

There are two types of scriptable: one is a dummy one created in
GetNewOrUsed() to do a lookup, and the other is a fully filled out
one. The dummy one is not added to the hashtable, and may have had its
name stolen if we created a new scriptable. The latter makes it so
that you crash if you try to look it up in the hashtable anyways, so
this patch only looks up fully filled out scriptables.

This patch also removes MOZ_COUNT_CTOR/DTOR because they are not
needed for refcounted classes.

Review commit: https://reviewboard.mozilla.org/r/68420/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/68420/
Attachment #8776718 - Flags: review?(wmccloskey)
Attachment #8776719 - Flags: review?(wmccloskey)
Attachment #8776720 - Flags: review?(wmccloskey)
Attachment #8774396 - Attachment is obsolete: true
Attachment #8774396 - Flags: review?(wmccloskey)
Attachment #8774397 - Attachment is obsolete: true
Attachment #8774397 - Flags: review?(wmccloskey)
Attachment #8774398 - Attachment is obsolete: true
Attachment #8774398 - Flags: review?(wmccloskey)
Comment on attachment 8776718 [details]
Bug 1288817, part 1 - Make XPCNativeScriptableShared refcounted.

https://reviewboard.mozilla.org/r/68420/#review68558

::: js/xpconnect/src/XPCMaps.h:500
(Diff revision 1)
>  class XPCNativeScriptableSharedMap
>  {
>  public:
>      struct Entry : public PLDHashEntryHdr
>      {
>          XPCNativeScriptableShared* key;

Please comment that this is a weak reference. The hash table entry gets cleared in the XPCNSS destructor.

::: js/xpconnect/src/XPCWrappedNativeJSOps.cpp:1062
(Diff revision 1)
>          mJSClass.oOps = &XPC_WN_ObjectOpsWithEnumerate;
>  }
>  
> +XPCNativeScriptableShared::~XPCNativeScriptableShared()
> +{
> +    if (mJSClass.cOps) {

Please comment that this can be null for an XPCNSS that was created with populate=false.

::: js/xpconnect/src/xpcprivate.h:1552
(Diff revision 1)
>  
>      const JSClass*
>      GetJSClass()          {return mShared->GetJSClass();}
>  
>      void
> -    SetScriptableShared(XPCNativeScriptableShared* shared) {mShared = shared;}
> +    SetScriptableShared(already_AddRefed<XPCNativeScriptableShared>&& shared) {mShared = shared;}

Can you fix the horrible formatting here?

::: js/xpconnect/src/xpcprivate.h:1565
(Diff revision 1)
>      void AutoTrace(JSTracer* trc) {}
>  
>  protected:
>      explicit XPCNativeScriptableInfo(nsIXPCScriptable* scriptable)
> -        : mCallback(scriptable), mShared(nullptr)
> +        : mCallback(scriptable)
>                                 {MOZ_COUNT_CTOR(XPCNativeScriptableInfo);}

And especially here. My god.
Attachment #8776718 - Flags: review?(wmccloskey) → review+
Comment on attachment 8776719 [details]
Bug 1288817, part 2 - Remove marking methods from XPCNativeScriptableShared and XPCNativeScriptableFlags.

https://reviewboard.mozilla.org/r/68422/#review68564

::: js/xpconnect/src/xpcprivate.h:1441
(Diff revision 1)
> -    uint32_t GetFlags() const {return mFlags & ~XPC_WN_SJSFLAGS_MARK_FLAG;}
> +    uint32_t GetFlags() const {return mFlags;}
>      void     SetFlags(uint32_t flags) {mFlags = flags;}

Same with this formatting. Go nuts here if you want.
Attachment #8776719 - Flags: review?(wmccloskey) → review+
Blocks: 1288870
Comment on attachment 8776720 [details]
Bug 1288817, part 3 - Stop calling XPCNativeScriptableInfo::Mark() from various places because it doesn't do anything.

https://reviewboard.mozilla.org/r/68424/#review68566
Attachment #8776720 - Flags: review?(wmccloskey) → review+
I'll add comments in both places, that's a good point.

I'll fix the formatting, too, in those places you pointed out. Generally, I was trying to not fix formatting on lines I didn't actually touch, as I was worried I'd just end up getting sucked into spending a bunch of time on fixing formatting.
Pushed by amccreight@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/824dd062b8e3
part 1 - Make XPCNativeScriptableShared refcounted. r=billm
https://hg.mozilla.org/integration/autoland/rev/79df306f1d11
part 2 - Remove marking methods from XPCNativeScriptableShared and XPCNativeScriptableFlags. r=billm
https://hg.mozilla.org/integration/autoland/rev/1bc60cfdc088
part 3 - Stop calling XPCNativeScriptableInfo::Mark() from various places because it doesn't do anything. r=billm
The breakage was in opt builds not debug builds. Very odd. I'll have to try to reproduce locally, as there's no obvious difference in the code in opt builds.
Flags: needinfo?(continuation)
Philor pointed out that opt Android XPCShell is also very orange. It looks like there are a bunch of UAF crashes inside markGrayReferences: https://treeherder.mozilla.org/logviewer.html#?job_id=1830794&repo=autoland
I can reproduce this locally with
  ./mach xpcshell-test devtools/client/webconsole/new-console-output/test/store/

The stack basically looks the same as Android. I'll need to use an ASan build to figure out what is going on.
The problem is that XPCWrappedNative::SystemIsBeingShutDown() just unconditionally destroys the mScriptableInfo, which can the last release of XPCNativeScriptableShared, which in turn causes a js::ClassOps that is still being used by a JS object to be destroyed. To fix this, I match the existing behavior and simply leak all XPCNativeScriptableShared that are alive once nsXPConnect::mShuttingDown is set to true. Some of my other refcounting patches will need a similar treatment.

I'm doing a full try run, but this fixes the issue for me locally:
  https://treeherder.mozilla.org/#/jobs?repo=try&revision=199c6724c74f

(I reproduced in an ASan build by removing the #define for NS_FREE_PERMANENT_DATA from nscore.h.)
I can't submit an updated patch using MozReview for some reason, so here are my changes. (I added some minor cleanups, too.) I'll land this folded into part 1.

Intentionally leak any XPCNativeScriptableShared that are alive late in shutdown to avoid use-after-frees from JS objects that are still using those js::ClassOps. This matches the existing behavior, which does not sweep these ScriptableShared during shutdown. (This came up in opt xpcshell tests.)

MozReview-Commit-ID: I3m2BtlZc3b
Attachment #8781225 - Flags: review?(wmccloskey)
> Some of my other refcounting patches will need a similar treatment.
The other two classes I converted to refcounting aren't like this, it turns out.
Bill pointed out that we could just skip deleting mScriptableInfo. This is the only place on the heap that holds a strong reference to these shared objects, so I think it will end up having the same effect as my other patch. I was just being overly paranoid about other places possibly dropping their references. It fixed the problem for me locally, and I'll try a Linux debug+opt try run to make sure before landing.
  https://treeherder.mozilla.org/#/jobs?repo=try&revision=2f1c51db8a55

Stop destroying mScriptableInfo in XPCWrappedNative's SystemIsBeingShutDown(), because that will end up destroying XPCNativeScriptableShared, while their js::ClassOps are still in use by JS objects. This matches the existing behavior, which does not sweep these ScriptableShared during shutdown. (This came up in opt
xpcshell tests.)

MozReview-Commit-ID: 3kwMr8xt9RA
Attachment #8781274 - Flags: review?(wmccloskey)
Attachment #8781225 - Attachment is obsolete: true
Attachment #8781225 - Flags: review?(wmccloskey)
Attachment #8781274 - Flags: review?(wmccloskey) → review+
Pushed by amccreight@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/c5be273c8372
part 1 - Make XPCNativeScriptableShared refcounted. r=billm
https://hg.mozilla.org/integration/mozilla-inbound/rev/6bb9524f0036
part 2 - Remove marking methods from XPCNativeScriptableShared and XPCNativeScriptableFlags. r=billm
https://hg.mozilla.org/integration/mozilla-inbound/rev/6b3f96dbbf28
part 3 - Stop calling XPCNativeScriptableInfo::Mark() from various places because it doesn't do anything. r=billm
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: