Open Bug 584240 Opened 11 years ago Updated 10 years ago

Make xpconnect/wrappers not morph slim wrappers

Categories

(Core :: JavaScript Engine, defect)

Other Branch
defect
Not set
normal

Tracking

()

People

(Reporter: jorendorff, Assigned: jorendorff)

References

(Blocks 1 open bug)

Details

(Whiteboard: [compartments])

Trying to wrap a slim wrapper with any of the old-style security wrappers causes the slim wrapper to morph into a full XPCWrappedNative.

The new wrappers don't care--except the ones that have x-ray behavior. We must either:

1) make the new stuff work like the old stuff, and morph when we try to wrap a slim wrapper with any of the new-style security wrappers with x-ray behavior; or 

2) make slim wrappers never morph.
Blocks: domperf
Looking at XrayWrapper.cpp, if we want to avoid morphing:

- holder_get/holder_set
These use the XPCWrappedNative to get the XPCNativeScriptableInfo so that they can get flags and the scriptable callback. They'd need to check for slim wrappers (IS_SLIM_WRAPPER(obj)) and use |GetSlimWrapperProto(obj)->GetScriptableInfo()| to get the scriptable info from a slim wrapper and |static_cast<XPCWrappedNative *>(obj->getPrivate())->GetScriptableInfo()| from a XPCWrappedNative wrapper. The only annoying thing here is that until now we've always either had a slim wrapper or a XPCWrappedNative wrapper (security wrapped or not). The hooks thus assume that if they get a null XPCWrappedNative* the JSObject* they get is a slim wrapper. If we allow x-ray wrappers around slim wrapper they'd get a null XPCWrappedNative* and an x-ray wrapper and they would need to fish the slim wrapper out of the x-ray wrapper. We could another JSObject* argument to these hooks I guess (for the "unwrapped" wrapper), the API becomes slightly more complex but we wouldn't have to add code to all the hooks to unwrap security wrappers.

- ResolveNativeProperty
For getting the scriptable info see above.
It looks like the only other change would be to make XPCCallContext::SetName detect a slim wrapper and use |GetSlimWrapperProto(obj)->GetSet()->FindMember(name, &mMember, &mInterface)|.
Not sure how to change the |ccx.GetWrapper() != wn| check.

Just for reference, making hooks work with slim wrappers happens mostly in http://mxr.mozilla.org/mozilla-central/source/js/src/xpconnect/src/xpcwrappednativejsops.cpp#991
Looking at AccessCheck.cpp, if we want to avoid morphing:

- IsFrameId
We could move the do_QueryWrappedNative from http://mxr.mozilla.org/mozilla-central/source/dom/base/nsDOMClassInfo.h#371 into XPConnect and use it here.

- AccessCheck::needsSystemOnlyWrapper
We only create XPCWrappedNative wrappers if NeedsSOW is true, so I think this is ok. Might need to null-check wn. It sure would be nice to store the flag for whether something needs a SOW in the XPCWrappedNativeProto, so we could have a SOW around a slim wrapper. Might be out of scope for this bug.
Looking at WrapperFactory.cpp, if we want to avoid morphing:

- WrapperFactory::Rewrap
Looks like all that's needed is s/IS_WN_WRAPPER_OBJECT(obj)/IS_WRAPPER_CLASS(obj->getClass())/.
blocking2.0: --- → beta5+
Whiteboard: [compartments]
We're going to do #1 to get this in Ff4.
blocking2.0: beta5+ → beta6+
No longer blocks: compartments
Blocks: compartments
Blocks: 594911
#1 is bug 594911. This bug is now about doing approach #2 (don't morph slim wrappers) which we might still want to do for perf, perhaps later.
No longer blocks: compartments, 594911
Summary: Make xpconnect/wrappers work with slim wrappers → Make xpconnect/wrappers not morph slim wrappers
Removing blocking flag.
blocking2.0: beta7+ → ---
You need to log in before you can comment on or make changes to this bug.