avoid potential global state change when marking xpconnect wrappers

RESOLVED FIXED

Status

()

RESOLVED FIXED
8 years ago
8 years ago

People

(Reporter: igor, Assigned: igor)

Tracking

(Blocks: 1 bug)

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: fixed-in-tracemonkey)

Attachments

(1 attachment)

(Assignee)

Description

8 years ago
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.
(Assignee)

Comment 1

8 years ago
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
(Assignee)

Comment 2

8 years ago
Created attachment 536146 [details] [diff] [review]
v1

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 3

8 years ago
Comment on attachment 536146 [details] [diff] [review]
v1

Man this is some ugly code in xpconnect. Thanks for the quick turn-around igor.
Attachment #536146 - Flags: review?(gal) → review+

Comment 4

8 years ago
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).
(Assignee)

Comment 6

8 years ago
http://hg.mozilla.org/tracemonkey/rev/131a5a94e29a
Whiteboard: fixed-in-tracemonkey
http://hg.mozilla.org/mozilla-central/rev/131a5a94e29a
Status: NEW → RESOLVED
Last Resolved: 8 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.