Closed Bug 660430 Opened 9 years ago Closed 9 years ago

avoid potential global state change when marking xpconnect wrappers


(Core :: JavaScript Engine, defect)

Not set





(Reporter: igor, Assigned: igor)


(Blocks 1 open bug)


(Whiteboard: fixed-in-tracemonkey)


(1 file)

For the bug 638660 we need that any JSClass::trace should allow to to be invoked from non-main thread and even potentially allow to be invoked for the same object from different threads at the same time. 

XPC_WN_Shared_Trace and XPC_WN_Helper_Trace from xpcwrappednativejsops.cpp violates this as they call GetWrappedNativeOfJSObject that in turn instantiate XPCCallContxt. The latter fails for non-main thread.

One way to fix this is to change GetWrappedNativeOfJSObject not to depend on XPCCallContxt and even JSContext. If this is not possible in general, then we should provide a light version of the function tailored for GC marking needs.
I was wrong about CallContext instantiating during marking. That is not used. So what is only problematic is a call to a security manager when XPCWrappedNative::GetWrappedNativeOfJSObject calls XPCWrapper::Unwrap.
Summary: avoid ccx, allocations and any global state change when marking xpconnect wrappers → avoid potential global state change when marking xpconnect wrappers
Attached patch v1Splinter Review
The patch passes null as cx parameter to XPCWrapper::GetWrappedNativeOfJSObject and changes the latter to use XPCWrapper::UnsafeUnwrapSecurityWrapper if so.
Attachment #536146 - Flags: review?(gal)
Comment on attachment 536146 [details] [diff] [review]

Man this is some ugly code in xpconnect. Thanks for the quick turn-around igor.
Attachment #536146 - Flags: review?(gal) → review+
blake, do we still need that security check and could we do a compartment check instead?
For now, we probably need to keep the security check. Once we have a compartment-per-window, we could probably switch to a compartment check (if I'm understanding your question correctly).
Whiteboard: fixed-in-tracemonkey
Closed: 9 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.