Some consumers of WrapNative definitely do NOT want an XPCNativeWrapper. WE should check whether any do. If not, then calling WrapNative shouldn't even bother creating an XPCNativeWrapper.

So at the moment we have the following WrapNative callers (excluding samples and tests): 1) window.arguments attachment (wraps the window and the arguments). 2) mozJSComponentLoader wraps the nsIComponentManager and the localfile. 3) XPCDispObject in XPCDispObject::WrapIDispatch 4) XPCIDispatchExtension in CommonConstructor 5) nsXPConnect::InitClassesWithNewWrappedGlobal 6) Components.interfaces/interfacesByID/classes/classesByID newResolve hooks 7) nsXPCConstructor::CallOrConstruct and nsXPCComponents_Exception::CallOrConstruct 8) nsJSCID::CreateInstance/GetService 9) xpc_NewIDObject 10) XPCThrower::ThrowExceptionObject 11) _getpluginelement 12) nsCrypto::GenerateCRMFRequest 13) nsContentUtils::ReparentContentWrapper 14) nsHTMLScriptEventHandler::Invoke 15) nsEventListenerManager various places 16) Various places in XBL 17) nsXULTemplateBuilder::InitHTMLTemplateRoot 18) nsDOMClassInfo::WrapNative 19) nsGlobalWindow::SetNewDocument (re-wrapping |navigator|). 20) nsJSContext::InitContext 21) nsJSEventListener::HandleEvent 22) nsHTTPIndex::OnStartRequest For #1, I think we want the "real" JSObject for the window, but want to allow XPCNativeWrappers for the arguments as needed. For #2, we probably don't care either way. For #3 and #4, I have no idea. For #5 we want the "real" JSObject For #6-#12, I'm not sure. For #13 we want the "real" JSObject For #14, not sure. For #15 we want the "real" JSObject (and assert it!) For #16 we want the "real" JSObject For #17 I have no idea. For #18, at least some places (eg precreate() parent wrapping) want the "real" JSObject; not sure about the rest. For #19, not sure For #20, we want the "real" JSObject For #21, we want the "real" JSObject For #22, we want the "real" JSObject

Given the data in comment 1, we need a WrapNative() variant that always returns the XPCWrappedNative. What's a good api here?

I'm pretty convinced that we want the "real" JSObject in all those cases. So far I haven't seen any code that would want a native wrapper when calling WrapNative(). Such code could certainly appear though, so I would suggest we leave the API as is for wrapping natives and add a SafeWrapNative() or whatever, either now or when there's a need for it. We should make WrapNative() guarantee now that it gets you the "real" JSObject... bz, I think you had that change in your tree already at one point. Want to attach a patch? That'll if nothing else at least make us do a bit less work when wrapping objects that today gets wrapped in a native wrapper only to be unwrapped right away.

> So far I haven't seen any code that would want a native wrapper when calling > WrapNative(). I think wrapping up natives for window.arguments should possibly create native wrappers as needed.... Otherwise we might end up passing around objects that are not safe to use for the newly-opened window (and since they're coming as as JSObjects on the callee's side, the callee won't get wrappers for them even if needed). But yeah, I agree that it's simplest to make WrapNative itself not do any native wrapping, ever. I'll attach a patch for that.

I also made calling methods on an XPCWrappedJS make sure to get the raw JSObject... that seemed like the right way to go.

Comment on attachment 184690 [details] [diff] [review] Never XPCNativeWrapper up WrapNative results Yeah, looks good. r+sr=jst

Comment on attachment 184690 [details] [diff] [review] Never XPCNativeWrapper up WrapNative results a=me for 1.8b3! /be

Fixed for 1.8b3.