We're using it in worker code so leaving it named xpc_ seems weird, especially since it's a trivial wrapper around JSAPI.

I think it would make sense to add a JS::ExposeObjectToActiveJS that is just an inlined function that passes in JSTRACE_OBJECT.

Yeah, I could add that to this.

Although it's not clear to me how to deal with rooting stuff? Is it ok to just take a raw JSObject*?

I was thinking maybe a null-checked version would be nice, but JS::ExposePossiblyNullObjectToActiveJS is such a terrible name that the null check is better. It is cool to see how rarely we need the the null checks! If you are feeling bold, you could fatally assert when we hit the NULL case for the places you aren't sure about and see how badly that goes on a try run. (In reply to Kyle Huey [:khuey] (khuey@mozilla.com) from comment #3) > Although it's not clear to me how to deal with rooting stuff? Is it ok to > just take a raw JSObject*? Yeah, nothing in there should GC, which is why the existing function can just take a void*. Or if things can, then that's an error in the analysis...

I can review this if you'd like, Bobby.

Comment on attachment 797543 [details] [diff] [review] Patch Review of attachment 797543 [details] [diff] [review]: ----------------------------------------------------------------- Thanks Andrew. I reviewed a little bit of it, so I'll post my comments here. ::: content/canvas/src/ImageData.h @@ +54,4 @@ > } > JSObject* GetDataObject() const > { > + JS::ExposeGCThingToActiveJS(mData, JSTRACE_OBJECT); I assume you audited that this can never be null? ::: content/xul/document/src/XULDocument.cpp @@ +3665,4 @@ > JSContext *cx = aContext->GetNativeContext(); > AutoCxPusher pusher(cx); > JS::Rooted<JSObject*> global(cx, mScriptGlobalObject->GetGlobalJSObject()); > + // XXXkhuey can this ever be null? It cna't, because aScriptObject is non-null, and same-compartment with the global.

With Andrew's suggestion.

Comment on attachment 797661 [details] [diff] [review] Patch Review of attachment 797661 [details] [diff] [review]: ----------------------------------------------------------------- ::: content/base/src/EventSource.cpp @@ +89,5 @@ > tmp->mListenerManager->MarkForCC(); > } > if (!isBlack && tmp->PreservingWrapper()) { > + // This marks the wrapper black. > + tmp->GetWrapper(); I really don't like this. Can you please add a MarkWrapperBlack() or something?

> I really don't like this. Can you please add a MarkWrapperBlack() or > something? Yeah, I was thinking that myself. Of course, it would just call GetWrapper...

Comment on attachment 797661 [details] [diff] [review] Patch Review of attachment 797661 [details] [diff] [review]: ----------------------------------------------------------------- Nice cleanup. Thanks for adding ExposeObjectToActiveJS. Like Ms2ger said, please add a new method to nsWrapperCache called MarkWrapperBlack() or something that doesn't return anything but just calls GetWrapper(), and use that, as that seems better than needing this comment everywhere. Bill, I assume it is okay to add ExposeObjectToActiveJS to GCAPI.h? It doesn't really rise to the level of a "review", but I figure I should get at least a f+ for adding to the GCAPI... ::: dom/base/nsJSEnvironment.cpp @@ +1500,5 @@ > + if (unrootedGlobal) { > + JS::ExposeObjectToActiveJS(unrootedGlobal); > + } > + > + JS::Rooted<JSObject*> global(cx, unrootedGlobal); I think you could also just do the unmarkgray after the rooting, to avoid the unrootedGlobal intermediate variable. No big deal. ::: js/xpconnect/src/xpcprivate.h @@ +2321,5 @@ > */ > JSObject* > GetFlatJSObject() const > + { > + JS::ExposeObjectToActiveJS(mFlatJSObject); Can you really skip this? TraceSelf does a null check on mFlatJSObject. @@ +3600,5 @@ > * kept alive past the next CC. > */ > + jsval GetJSVal() const { > + if (!JSVAL_IS_PRIMITIVE(mJSVal)) > + JS::ExposeObjectToActiveJS(&mJSVal.toObject()); Just replace this whole branch thing with JS::ExposeValueToActiveJS(mJSVal). ::: security/manager/ssl/src/nsCrypto.cpp @@ +51,4 @@ > #include "nsIDOMCryptoDialogs.h" > #include "nsIFormSigningDialog.h" > #include "nsIContentSecurityPolicy.h" > +#include "nsIURI.h" Is this accidental?

(In reply to Andrew McCreight [:mccr8] from comment #10) > Comment on attachment 797661 [details] [diff] [review] > Patch > > Review of attachment 797661 [details] [diff] [review]: > ----------------------------------------------------------------- > > Nice cleanup. Thanks for adding ExposeObjectToActiveJS. > > Like Ms2ger said, please add a new method to nsWrapperCache called > MarkWrapperBlack() or something that doesn't return anything but just calls > GetWrapper(), and use that, as that seems better than needing this comment > everywhere. This seems silly to me, but ok. > Bill, I assume it is okay to add ExposeObjectToActiveJS to GCAPI.h? It > doesn't really rise to the level of a "review", but I figure I should get at > least a f+ for adding to the GCAPI... > > ::: dom/base/nsJSEnvironment.cpp > @@ +1500,5 @@ > > + if (unrootedGlobal) { > > + JS::ExposeObjectToActiveJS(unrootedGlobal); > > + } > > + > > + JS::Rooted<JSObject*> global(cx, unrootedGlobal); > > I think you could also just do the unmarkgray after the rooting, to avoid > the unrootedGlobal intermediate variable. No big deal. > > ::: js/xpconnect/src/xpcprivate.h > @@ +2321,5 @@ > > */ > > JSObject* > > GetFlatJSObject() const > > + { > > + JS::ExposeObjectToActiveJS(mFlatJSObject); > > Can you really skip this? TraceSelf does a null check on mFlatJSObject. > @@ +3600,5 @@ > > * kept alive past the next CC. > > */ > > + jsval GetJSVal() const { > > + if (!JSVAL_IS_PRIMITIVE(mJSVal)) > > + JS::ExposeObjectToActiveJS(&mJSVal.toObject()); > > Just replace this whole branch thing with JS::ExposeValueToActiveJS(mJSVal). Yeah, good call. > ::: security/manager/ssl/src/nsCrypto.cpp > @@ +51,4 @@ > > #include "nsIDOMCryptoDialogs.h" > > #include "nsIFormSigningDialog.h" > > #include "nsIContentSecurityPolicy.h" > > +#include "nsIURI.h" > > Is this accidental? No, this file is bootlegging nsIURI.h through the chain nsWrapperCacheLines.h -> xpcpublic.h -> nsIURI.h.

Comment on attachment 797661 [details] [diff] [review] Patch Review of attachment 797661 [details] [diff] [review]: ----------------------------------------------------------------- Sure, seems fine.

I'm not happy adding null checks to so many places. Could we make ExposeObjectToActiveJS null safe? or have null safe version of it?

This isn't adding any null checks, it is removing them in a number of places, as xpc_UnmarkGrayObject had a null check built in.

The patch adds explicit null check to many places. xpc_UnmarkGrayObject had that implicitly.

Oh, sorry, I assumed you were worried about perf. :)

I'm worried about missing null checks causing crashes.

(In reply to Andrew McCreight [:mccr8] from comment #10) > Comment on attachment 797661 [details] [diff] [review] > Patch > > Review of attachment 797661 [details] [diff] [review]: > ----------------------------------------------------------------- > > Nice cleanup. Thanks for adding ExposeObjectToActiveJS. > > Like Ms2ger said, please add a new method to nsWrapperCache called > MarkWrapperBlack() or something that doesn't return anything but just calls > GetWrapper(), and use that, as that seems better than needing this comment > everywhere. So I actually didn't do this because adding another method to avoid a simple comment seems silly. If you feel strongly about this file a followup and I'll do it. (In reply to Olli Pettay [:smaug] from comment #17) > I'm worried about missing null checks causing crashes. I decided to go ahead and land this anyways. Please file a followup if you want to do something about that.