Closed
Bug 295152
Opened 20 years ago
Closed 20 years ago
[FIXr]Sort out whether WrapNative() should ever create XPCNativeWrappers
Categories
(Core :: DOM: Core & HTML, defect, P1)
Tracking
()
RESOLVED
FIXED
mozilla1.8beta3
People
(Reporter: bzbarsky, Assigned: bzbarsky)
References
Details
Attachments
(1 file)
10.40 KB,
patch
|
jst
:
review+
jst
:
superreview+
brendan
:
approval1.8b3+
|
Details | Diff | Splinter Review |
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.
Assignee | ||
Comment 1•20 years ago
|
||
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
Assignee | ||
Comment 2•20 years ago
|
||
Given the data in comment 1, we need a WrapNative() variant that always returns
the XPCWrappedNative. What's a good api here?
Comment 3•20 years ago
|
||
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.
Assignee | ||
Comment 4•20 years ago
|
||
> 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.
Assignee | ||
Comment 5•20 years ago
|
||
I also made calling methods on an XPCWrappedJS make sure to get the raw
JSObject... that seemed like the right way to go.
Assignee | ||
Updated•20 years ago
|
Attachment #184690 -
Flags: superreview?(jst)
Attachment #184690 -
Flags: review?(jst)
Assignee | ||
Updated•20 years ago
|
Assignee: general → bzbarsky
Priority: -- → P1
Summary: Sort out whether WrapNative() should ever create XPCNativeWrappers → [FIX]Sort out whether WrapNative() should ever create XPCNativeWrappers
Target Milestone: --- → mozilla1.8beta3
Comment 6•20 years ago
|
||
Comment on attachment 184690 [details] [diff] [review]
Never XPCNativeWrapper up WrapNative results
Yeah, looks good. r+sr=jst
Attachment #184690 -
Flags: superreview?(jst)
Attachment #184690 -
Flags: superreview+
Attachment #184690 -
Flags: review?(jst)
Attachment #184690 -
Flags: review+
Updated•20 years ago
|
Flags: blocking1.8b3+
Comment 7•20 years ago
|
||
Comment on attachment 184690 [details] [diff] [review]
Never XPCNativeWrapper up WrapNative results
a=me for 1.8b3!
/be
Attachment #184690 -
Flags: approval1.8b3+
Assignee | ||
Updated•20 years ago
|
Summary: [FIX]Sort out whether WrapNative() should ever create XPCNativeWrappers → [FIXr]Sort out whether WrapNative() should ever create XPCNativeWrappers
Assignee | ||
Comment 8•20 years ago
|
||
Fixed for 1.8b3.
Status: NEW → RESOLVED
Closed: 20 years ago
Resolution: --- → FIXED
Updated•6 years ago
|
Component: DOM → DOM: Core & HTML
You need to log in
before you can comment on or make changes to this bug.
Description
•