584240 - Make xpconnect/wrappers not morph slim wrappers

Status: RESOLVED INVALID

(Core :: JavaScript Engine, defect)

Description

[Jason Orendorff [:jorendorff]]

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.

Comment 1

[Peter Van der Beken [:peterv]]

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

Comment 2

[Peter Van der Beken [:peterv]]

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.

Comment 3

[Peter Van der Beken [:peterv]]

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())/.

Comment 4

[Jason Orendorff [:jorendorff]]

We're going to do #1 to get this in Ff4.

Comment 5

[Jason Orendorff [:jorendorff]]

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

Comment 6

[Peter Van der Beken [:peterv]]

Removing blocking flag.

Comment 7

[BugBot [:suhaib / :marco/ :calixte]]

The bug assignee didn't login in Bugzilla in the last 7 months.
:sdetar, could you have a look please?
For more information, please visit auto_nag documentation.

Comment 8

[Jan de Mooij [:jandem]]

I think some of these wrapper types no longer exist.