I'm pretty sure I already have a bug open from when I tried this before, but I can't find it right now. This new variant is surprisingly svelte: 15 files changed, 142 insertions(+), 41 deletions(-). The combination of |explicit operator bool|, better knowledge of how the CC works, and past experience doing this new effort go much quicker. So far I've booted up a browser and messed around in twitter and some news sites for a few minutes. Hopefully, we'll see how good the coverage was with the following try run: https://treeherder.mozilla.org/#/jobs?repo=try&revision=9f2bcdffddf0 Most of the changes should be fairly obvious, but I'll annotate with my notes inline in the patch so that we're all on the same page (and try is actually green) before asking for reviews.

https://treeherder.mozilla.org/#/jobs?repo=try&revision=2b2b6be86211 TH and/or TC threw a tantrum last night.

Work in progress patch.

Patch to add a read barrier to Heap<T> to perform ExposeToActiveJS automatically. Most of the patch making sure we don't fire the read barrier where we don't want to do this, e.g. during GC and CC. The AutoEnterCycleCollection guard object asserts that we don't do this during CC. Try run: https://treeherder.mozilla.org/#/jobs?repo=try&revision=d7d540e41f7f

Comment on attachment 8796225 [details] [diff] [review] bug1297558-auto-expose v3 Requesting review for non SM parts.

Comment on attachment 8796225 [details] [diff] [review] bug1297558-auto-expose v3 Review of attachment 8796225 [details] [diff] [review]: ----------------------------------------------------------------- Thanks for fixing this! This is very nice. We could potentially rip out a lot of the manual Expose calls now. r=me for the XPConnect and non-js/ changes. ::: js/xpconnect/src/xpcprivate.h @@ +890,5 @@ > return mGlobalJSObject; > } > > JSObject* > + GetGlobalJSObjectPreserveColor() const {return mGlobalJSObject.unbarrieredGet();} While you are touching these lines, please add a space before the return and after the ;, here and in other places. ::: xpcom/base/nsCycleCollector.cpp @@ +2659,5 @@ > mCollector->RemoveObjectFromGraph(o.mPointer); > o.mRefCnt->stabilizeForDeletion(); > + { > + JS::AutoEnterCycleCollection autocc(mCollector->Context()->Context()); > + o.mParticipant->Trace(o.mPointer, *this, nullptr); I guess this has to be tightly scoped because DeleteCycleCollectable could do all sorts of random stuff. @@ +2689,5 @@ > virtual void Trace(JS::Heap<JS::Value>* aValue, const char* aName, > void* aClosure) const override > { > + const JS::Value& val = aValue->unbarrieredGet(); > + if (val.isMarkable() && ValueIsGrayCCThing(val)) { Could you instead change this to take a JS::Heap<JS::Value>* or & argument and do the unbarriedGet inside there? This is the only caller and I think it might be a little clearer. @@ +3770,5 @@ > mMergedInARow = 0; > return false; > } > > + JS::AutoEnterCycleCollection autocc(Context()->Context()); Hmm, this is peculiar. What is this actually guarding? I would guess UsefulToMergeZones() but both of those calls are empty now. It would be a little nicer if you just moved that into XPCJSContext::UsefulToMergeZones() (or really, in a patch that makes them do something). @@ +3843,5 @@ > bool mergeZones = ShouldMergeZones(aCCType); > mResults.mMergedZones = mergeZones; > > JS::AutoAssertOnGC nogc; > + JS::AutoEnterCycleCollection autocc(mJSContext->Context()); ...or alternatively, you could hoist this autocc above the ShouldMergeZones (maybe put it after the "Set up the data structures for building the graph" comment), and then you should be able to drop it from ShouldMergeZones.

Comment on attachment 8796225 [details] [diff] [review] bug1297558-auto-expose v3 Review of attachment 8796225 [details] [diff] [review]: ----------------------------------------------------------------- This looks great. I really hope it sticks. What would be the symptom of excess unmarking? (Other than assertion failures.) More memory usage? ::: js/xpconnect/src/xpcprivate.h @@ +2225,5 @@ > + JSCompartment* Compartment() const { > + return js::GetObjectCompartment(mJSObj.unbarrieredGet()); > + } > + > +private: Is the redundant private: intentional? (I'm not sure if you wanted a separate one for the data members or not.)

(In reply to Andrew McCreight [:mccr8] from comment #7) Thanks for the speedy review! > I guess this has to be tightly scoped because DeleteCycleCollectable could > do all sorts of random stuff. Yes. > @@ +2689,5 @@ > > virtual void Trace(JS::Heap<JS::Value>* aValue, const char* aName, > > void* aClosure) const override > > { > > + const JS::Value& val = aValue->unbarrieredGet(); > > + if (val.isMarkable() && ValueIsGrayCCThing(val)) { > > Could you instead change this to take a JS::Heap<JS::Value>* or & argument > and do the unbarriedGet inside there? This is the only caller and I think it > might be a little clearer. I tried this but still you need the unbarrieredGet() in Trace for the following two lines. This seemed more complicated so I left it as was. > @@ +3843,5 @@ > > bool mergeZones = ShouldMergeZones(aCCType); > > mResults.mMergedZones = mergeZones; > > > > JS::AutoAssertOnGC nogc; > > + JS::AutoEnterCycleCollection autocc(mJSContext->Context()); > > ...or alternatively, you could hoist this autocc above the ShouldMergeZones > (maybe put it after the "Set up the data structures for building the graph" > comment), and then you should be able to drop it from ShouldMergeZones. Yes, that looks like the best thing to do.

(In reply to Steve Fink [:sfink] [:s:] from comment #8) > What would be the symptom of > excess unmarking? (Other than assertion failures.) More memory usage? Leaks at shutdown is the usual symptom. > Is the redundant private: intentional? Nope, fixed.

Pushed by jcoppeard@mozilla.com: https://hg.mozilla.org/integration/mozilla-inbound/rev/138dd1cfe696 Use a read barrier on Heap to ExposeToActiveJS r=sfink r=mccr8

So I'm a little confused. Doesn't this fundamentally break things like CallbackObject::CallbackPreserveColor()? I was expecting something that preserved those semantics via an unsafeGet() or whatever, but I'm not seeing anything like that in the patch...

Ah, we _do_ have unbarrieredGet() in the patch, used for some *PreserveColor functions. Just not consistently?

Also afaict the assert in CallbackObject::CallbackKnownNotGray() is now busted, because of the .get() call it had in there (which shouldn't have been there; my fault).

Sorry, I guess CallbackObject::CallbackPreserveColor is ok because it uses .address(), which is not barriered. Sorry for the false alarm. I still think the assert in CallbackKnownNotGray is busted, though.

(In reply to Boris Zbarsky [:bz] (still a bit busy) from comment #16) You're right, CallbackKnownNotGray is broken. I've filed bug 1307372.

Pushed by jcoppeard@mozilla.com: https://hg.mozilla.org/integration/mozilla-inbound/rev/1538850bba0f Remove explicit calls to Expose*ToActiveJS r=mccr8

Do we not need to do something similar for TenuredHeap?

(In reply to Boris Zbarsky [:bz] (still a bit busy) from comment #20) > Do we not need to do something similar for TenuredHeap? Bug 1306382