295152 - [FIXr]Sort out whether WrapNative() should ever create XPCNativeWrappers

Status: RESOLVED FIXED

(Core :: DOM: Core & HTML, defect, P1)

Description

[Boris Zbarsky [:bzbarsky]]

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.

Comment 1

[Boris Zbarsky [:bzbarsky]]

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

Comment 2

[Boris Zbarsky [:bzbarsky]]

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

Comment 3

[Johnny Stenback (:jst)]

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.

Comment 4

[Boris Zbarsky [:bzbarsky]]

> 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.

Comment 5

[Boris Zbarsky [:bzbarsky]]

Attachment: Never XPCNativeWrapper up WrapNative results

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

Comment 6

[Johnny Stenback (:jst)]

Comment on attachment 184690 [details] [diff] [review]
Never XPCNativeWrapper up WrapNative results

Yeah, looks good. r+sr=jst

Comment 7

[Brendan Eich [:brendan]]

Comment on attachment 184690 [details] [diff] [review]
Never XPCNativeWrapper up WrapNative results

a=me for 1.8b3!

/be

Comment 8

[Boris Zbarsky [:bzbarsky]]

Fixed for 1.8b3.