740064 - Refactor XrayWrapper

Status: RESOLVED FIXED

(Core :: XPConnect, defect)

Description

[Peter Van der Beken [:peterv]]

We're adding a third type of XrayWrappers, we should consolidate the two existing ones and templatize.

Comment 1

[Peter Van der Beken [:peterv]]

Attachment: v1

Comment 2

[Bobby Holley (:bholley)]

Comment on attachment 610216 [details] [diff] [review]
v1

+class XPCWrappedNativeXrayTraits
...
+    static JSObject* getInnerObject(JSObject *wrapper);

Can we inline this, or drop the inline in ProxyXrayTraits, so that the two are consistent?

+class ProxyXrayTraits
...
+private:
+    static JSObject* getHolderObject(JSContext *cx, JSObject *wrapper,
+                                     bool createHolder)
+    {
+        if (!js::GetProxyExtra(wrapper, 0).isUndefined())
+            return &js::GetProxyExtra(wrapper, 0).toObject();
+
+        if (!createHolder)
+            return nsnull;
+
+        return createHolderObject(cx, wrapper);
+    }

It sure would be nice to do this eagerly so that we can drop the |cx| and simplify the signature situation with getHolderObject. Add that to the followup list?

-    NS_ASSERTION(js::GetObjectJSClass(holder) == &HolderClass, "expected a native property holder object");
+    NS_ASSERTION(js::GetObjectJSClass(holder) == &HolderClass,
+                 "expected a native property holder object");
...
-    NS_ASSERTION(js::GetObjectJSClass(holder) == &HolderClass, "expected a native property holder object");
+    NS_ASSERTION(js::GetObjectJSClass(holder) == &HolderClass,
+                 "expected a native property holder object");
...
-    NS_ASSERTION(js::GetObjectJSClass(holder) == &HolderClass, "expected a native property holder object");
+    NS_ASSERTION(js::GetObjectJSClass(holder) == &HolderClass,
+                 "expected a native property holder object");

As long as we're changing these, let's just turn them into MOZ_ASSERTS so that we can kill
the string entirely. There are some more of these scattered throughout the file.


+class AutoLeaveHelper

Thanks for making this non-templated. :-)



+template <typename Base, typename Traits>
+XrayWrapper<Base, Traits>::XrayWrapper(unsigned flags)
+  : Base(flags | WrapperFactory::IS_XRAY_WRAPPER_FLAG)
+{
+}
+
+template <typename Base, typename Traits>
+XrayWrapper<Base, Traits>::~XrayWrapper()
+{
+}

Can we just inline these?


 
-            if (!JS_GetPropertyDescriptorById(cx, wnObject, id,
+            if (!JS_GetPropertyDescriptorById(cx, obj, id,
                                               (set ? JSRESOLVE_ASSIGNING : 0) | JSRESOLVE_QUALIFIED,
-                                              desc))
+                                              desc)) {

Just FYI: There was recently a JS style guide change to make the open brace here go on its own line.
 
-// The 'holder' here isn't actually of [[Class]] HolderClass like those used by
-// XrayWrapper. Instead, it's a funny hybrid of the 'expando' and 'holder'
-// properties. However, we store it in the same slot. Exercise caution.

I think it would be good to preserve this comment, but maybe that's just because I wrote it.
 
+typedef XrayWrapper<js::CrossCompartmentWrapper, ProxyXrayTraits > XrayProxy;

The XrayProxy we use in PXOW should inherit js::CrossCompartmentSecurityWrapper. Looks like that was already broken though...

Comment 3

[Peter Van der Beken [:peterv]]

(In reply to Bobby Holley (:bholley) from comment #2)
> Can we inline this, or drop the inline in ProxyXrayTraits, so that the two
> are consistent?

This involved moving more code around, so I didn't do it.

> +template <typename Base, typename Traits>
> +XrayWrapper<Base, Traits>::XrayWrapper(unsigned flags)
> +  : Base(flags | WrapperFactory::IS_XRAY_WRAPPER_FLAG)
> +{
> +}
> +
> +template <typename Base, typename Traits>
> +XrayWrapper<Base, Traits>::~XrayWrapper()
> +{
> +}
> 
> Can we just inline these?


I ended up not doing this, because it means more includes in the header.

> The XrayProxy we use in PXOW should inherit
> js::CrossCompartmentSecurityWrapper. Looks like that was already broken
> though...

Followup it is.

https://hg.mozilla.org/integration/mozilla-inbound/rev/1faf67b119e6

Comment 4

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/1faf67b119e6