660233 - Move nodePrincipal, documentURIObject, and baseURIObject onto XrayWrapper and remove from nsDOMClassInfo

Status: RESOLVED FIXED

(Core :: XPConnect, defect, P1)

Comment 1

[Andreas Gal :gal]

Attachment: draft patch, untested

Comment 2

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

Comment on attachment 551618 [details] [diff] [review]
draft patch, untested

>--- a/js/src/xpconnect/wrappers/XrayWrapper.cpp
>+++ b/js/src/xpconnect/wrappers/XrayWrapper.cpp
>+baseURIObject_getter(JSContext *cx, JSObject *wrapper, jsid id, jsval *vp)
>+{
>+    nsCOMPtr<nsIURI> uri;
>+    // ...
>+    uri = identity->GetBaseURI();

How about declaring uri here?

> XrayWrapper<Base>::resolveOwnProperty(JSContext *cx, JSObject *wrapper, jsid id, bool set,
>                                       PropertyDescriptor *desc_in)
> {
>+        else if (id == rt->GetStringID(XPCJSRuntime::IDX_BASEURIOBJECT)) {
>+            desc->getter = Is<nsIContent>
>+                           ? baseURIObject_getter<nsIContent>
>+                           : baseURIObject_getter>nsIDocument>

This line is clearly untested.

Comment 3

[Andreas Gal :gal]

Attachment: patch

Comment 4

[Andreas Gal :gal]

> How about declaring uri here?

uri is assigned to in both parts of the if and then used below it.

> 
> This line is clearly untested.

I upload patches early and often :) But looks like I have a volunteer to review this, judging by early review feedback =)

Comment 5

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

Comment on attachment 551767 [details] [diff] [review]
patch

Are we looking at the same code? I see

+    nsCOMPtr<nsIURI> uri;
+    nsCOMPtr<T> native = do_QueryWrappedNative(wn);
+    if (!native) {
+        JS_ReportError(cx, "Unexpected object");
+        return false;
+    }
+    uri = native->GetBaseURI();

And I approve of all patches that remove code from nsDOMClassInfo.

Comment 6

[Andreas Gal :gal]

Attachment: patch

Aeeehm .... yes, there was an if/else _before_ I templated that function. Its still early here, my brain hasn't booted yet. Sorry. Fixed.

Comment 7

[Andreas Gal :gal]

Comment on attachment 551769 [details] [diff] [review]
patch

Blake, do I still have to check for UniversalXPConnect? Content shouldn't be able to get hold of XrayWrappers around DOM elements, IMO, but I might be wrong.

Comment 8

[Andreas Gal :gal]

Attachment: patch

mrbkap, two questions here:
Is being able to read baseURIObject and nodePrincipal a security concern? should we check for privilege in the getters?

Comment 9

[Andreas Gal :gal]

And the 2nd question: can I just always reflect these properties onto the wrapper if the wrapper isn't transparent? and what happens when the wrapper is no longer transparent?

Comment 10

[Andreas Gal :gal]

Comment on attachment 551925 [details] [diff] [review]
patch

Note the two questions below

Comment 11

[Blake Kaplan (:mrbkap) (inactive)]

(In reply to Andreas Gal :gal from comment #8)
> mrbkap, two questions here:
> Is being able to read baseURIObject and nodePrincipal a security concern?
> should we check for privilege in the getters?

I don't understand the first question. It's a security concern in that it exposes non-DOM stuff to web pages. In practice, I don't think content code would be able to exploit getting its hands on the baseURIObject or the nodePrincipal. I think we should check for privileges in the getter for consistency's sake with what we currently do.

I did realize one difference between this patch and the current behavior: for nodes in chrome documents, this property won't exist anymore. I don't know how much of a problem that is, though. I never really knew the motivation behind these properties, so I don't know if that's a problem.

Comment 12

[Blake Kaplan (:mrbkap) (inactive)]

Comment on attachment 551925 [details] [diff] [review]
patch

Review of attachment 551925 [details] [diff] [review]:
-----------------------------------------------------------------

I'm clearing this review request based on my question above about system objects, but with the one comment below and that question addressed, this would otherwise be able to land.

::: js/src/xpconnect/wrappers/XrayWrapper.cpp
@@ +540,4 @@
>      if (!WrapperFactory::IsPartiallyTransparent(wrapper) &&
> +        ((id == rt->GetStringID(XPCJSRuntime::IDX_WRAPPED_JSOBJECT)) ||
> +         (((id == rt->GetStringID(XPCJSRuntime::IDX_BASEURIOBJECT) &&
> +            (Is<nsIContent>(wrapper) || Is<nsIDocument>(wrapper))) ||

Give that Is does a QI, it's probably worth stashing the result of these calls somewhere for the additional check down below.

Comment 13

[Boris Zbarsky [:bzbarsky]]

The motivation for these properties was to allow chrome code to get base URIs and principals, primarily for content nodes (instead of trying to get strings that maybe and maybe not map to those).

We probably need to document the change for chrome nodes.

Did the old code work for safe object wrappers?  I'm guessing no, since those accesses would have happened with low privileges, but if it did we need to document that behavior change too.

As far as pages getting their hands on this stuff...  I _think_ it shouldn't work because neither of those has classinfo so wrapper creation would fail the security check XPConnect does.  But if you're able to get access to those objects from untrusted script, then whether it's safe starts depending on what they can do with the objects.  If they can mutate the URI, that's bad, for example.

Comment 14

[Blake Kaplan (:mrbkap) (inactive)]

(In reply to Boris Zbarsky (:bz) from comment #13)
> Did the old code work for safe object wrappers?  I'm guessing no, since
> those accesses would have happened with low privileges

This is correct.

Comment 15

[Blake Kaplan (:mrbkap) (inactive)]

Comment on attachment 551925 [details] [diff] [review]
patch

r=me with my comment above addressed.

Comment 16

[Andreas Gal :gal]

Attachment: patch

Comment 17

[Andreas Gal :gal]

Comment on attachment 557628 [details] [diff] [review]
patch

>diff --git a/dom/base/nsDOMClassInfo.cpp b/dom/base/nsDOMClassInfo.cpp
>--- a/dom/base/nsDOMClassInfo.cpp
>+++ b/dom/base/nsDOMClassInfo.cpp
>@@ -521,19 +521,17 @@ static const char kDOMStringBundleURL[] 
>   nsIXPCScriptable::WANT_FINALIZE |                                           \
>   nsIXPCScriptable::WANT_EQUALITY |                                           \
>   nsIXPCScriptable::WANT_ENUMERATE |                                          \
>   nsIXPCScriptable::DONT_ENUM_QUERY_INTERFACE |                               \
>   nsIXPCScriptable::WANT_OUTER_OBJECT)
> 
> #define NODE_SCRIPTABLE_FLAGS                                                 \
>  ((DOM_DEFAULT_SCRIPTABLE_FLAGS |                                             \
>-   nsIXPCScriptable::WANT_GETPROPERTY |                                       \
>-   nsIXPCScriptable::WANT_ADDPROPERTY |                                       \
>-   nsIXPCScriptable::WANT_SETPROPERTY) &                                      \
>+   nsIXPCScriptable::WANT_ADDPROPERTY) &                                      \
>   ~nsIXPCScriptable::USE_JSSTUB_FOR_ADDPROPERTY)
> 
> // We need to let JavaScript QI elements to interfaces that are not in
> // the classinfo since XBL can be used to dynamically implement new
> // unknown interfaces on elements, accessibility relies on this being
> // possible.
> 
> #define ELEMENT_SCRIPTABLE_FLAGS                                              \
>@@ -1604,18 +1602,16 @@ jsid nsDOMClassInfo::sEnumerate_id      
> jsid nsDOMClassInfo::sNavigator_id       = JSID_VOID;
> jsid nsDOMClassInfo::sDocument_id        = JSID_VOID;
> jsid nsDOMClassInfo::sFrames_id          = JSID_VOID;
> jsid nsDOMClassInfo::sSelf_id            = JSID_VOID;
> jsid nsDOMClassInfo::sOpener_id          = JSID_VOID;
> jsid nsDOMClassInfo::sAll_id             = JSID_VOID;
> jsid nsDOMClassInfo::sTags_id            = JSID_VOID;
> jsid nsDOMClassInfo::sAddEventListener_id= JSID_VOID;
>-jsid nsDOMClassInfo::sBaseURIObject_id   = JSID_VOID;
>-jsid nsDOMClassInfo::sNodePrincipal_id   = JSID_VOID;
> jsid nsDOMClassInfo::sDocumentURIObject_id=JSID_VOID;
> jsid nsDOMClassInfo::sJava_id            = JSID_VOID;
> jsid nsDOMClassInfo::sPackages_id        = JSID_VOID;
> jsid nsDOMClassInfo::sWrappedJSObject_id = JSID_VOID;
> jsid nsDOMClassInfo::sURL_id             = JSID_VOID;
> jsid nsDOMClassInfo::sKeyPath_id         = JSID_VOID;
> jsid nsDOMClassInfo::sAutoIncrement_id   = JSID_VOID;
> jsid nsDOMClassInfo::sUnique_id          = JSID_VOID;
>@@ -1867,18 +1863,16 @@ nsDOMClassInfo::DefineStaticJSVals(JSCon
>   SET_JSID_TO_STRING(sNavigator_id,       cx, "navigator");
>   SET_JSID_TO_STRING(sDocument_id,        cx, "document");
>   SET_JSID_TO_STRING(sFrames_id,          cx, "frames");
>   SET_JSID_TO_STRING(sSelf_id,            cx, "self");
>   SET_JSID_TO_STRING(sOpener_id,          cx, "opener");
>   SET_JSID_TO_STRING(sAll_id,             cx, "all");
>   SET_JSID_TO_STRING(sTags_id,            cx, "tags");
>   SET_JSID_TO_STRING(sAddEventListener_id,cx, "addEventListener");
>-  SET_JSID_TO_STRING(sBaseURIObject_id,   cx, "baseURIObject");
>-  SET_JSID_TO_STRING(sNodePrincipal_id,   cx, "nodePrincipal");
>   SET_JSID_TO_STRING(sDocumentURIObject_id,cx,"documentURIObject");
>   SET_JSID_TO_STRING(sJava_id,            cx, "java");
>   SET_JSID_TO_STRING(sPackages_id,        cx, "Packages");
>   SET_JSID_TO_STRING(sWrappedJSObject_id, cx, "wrappedJSObject");
>   SET_JSID_TO_STRING(sURL_id,             cx, "URL");
>   SET_JSID_TO_STRING(sKeyPath_id,         cx, "keyPath");
>   SET_JSID_TO_STRING(sAutoIncrement_id,   cx, "autoIncrement");
>   SET_JSID_TO_STRING(sUnique_id,          cx, "unique");
>@@ -4857,18 +4851,16 @@ nsDOMClassInfo::ShutDown()
>   sNavigator_id       = JSID_VOID;
>   sDocument_id        = JSID_VOID;
>   sFrames_id          = JSID_VOID;
>   sSelf_id            = JSID_VOID;
>   sOpener_id          = JSID_VOID;
>   sAll_id             = JSID_VOID;
>   sTags_id            = JSID_VOID;
>   sAddEventListener_id= JSID_VOID;
>-  sBaseURIObject_id   = JSID_VOID;
>-  sNodePrincipal_id   = JSID_VOID;
>   sDocumentURIObject_id=JSID_VOID;
>   sJava_id            = JSID_VOID;
>   sPackages_id        = JSID_VOID;
>   sWrappedJSObject_id = JSID_VOID;
>   sKeyPath_id         = JSID_VOID;
>   sAutoIncrement_id   = JSID_VOID;
>   sUnique_id          = JSID_VOID;
>   sOnload_id          = JSID_VOID;
>@@ -7202,93 +7194,31 @@ nsNodeSH::AddProperty(nsIXPConnectWrappe
>   return NS_OK;
> }
> 
> NS_IMETHODIMP
> nsNodeSH::NewResolve(nsIXPConnectWrappedNative *wrapper, JSContext *cx,
>                      JSObject *obj, jsid id, PRUint32 flags,
>                      JSObject **objp, PRBool *_retval)
> {
>-  if ((id == sBaseURIObject_id || id == sNodePrincipal_id) &&
>-      IsPrivilegedScript()) {
>-    return DefineVoidProp(cx, obj, id, objp);
>-  }
>-
>   if (id == sOnload_id || id == sOnerror_id) {
>     // Make sure that this node can't go away while waiting for a
>     // network load that could fire an event handler.
>     // XXXbz won't this fail if the listener is added using
>     // addEventListener?  On the other hand, even if I comment this
>     // code out I can't seem to reproduce the bug it was trying to
>     // fix....
>     nsNodeSH::PreserveWrapper(GetNative(wrapper, obj));
>   }
> 
>   return nsDOMGenericSH::NewResolve(wrapper, cx, obj, id, flags, objp,
>                                     _retval);
> }
> 
> NS_IMETHODIMP
>-nsNodeSH::GetProperty(nsIXPConnectWrappedNative *wrapper, JSContext *cx,
>-                      JSObject *obj, jsid id, jsval *vp, PRBool *_retval)
>-{
>-  if (id == sBaseURIObject_id && IsPrivilegedScript()) {
>-    // I wish GetBaseURI lived on nsINode
>-    nsCOMPtr<nsIURI> uri;
>-    nsCOMPtr<nsIContent> content = do_QueryWrappedNative(wrapper, obj);
>-    if (content) {
>-      uri = content->GetBaseURI();
>-      NS_ENSURE_TRUE(uri, NS_ERROR_OUT_OF_MEMORY);
>-    } else {
>-      nsCOMPtr<nsIDocument> doc = do_QueryWrappedNative(wrapper, obj);
>-      NS_ENSURE_TRUE(doc, NS_ERROR_UNEXPECTED);
>-
>-      uri = doc->GetBaseURI();
>-      NS_ENSURE_TRUE(uri, NS_ERROR_NOT_AVAILABLE);
>-    }
>-
>-    nsCOMPtr<nsIXPConnectJSObjectHolder> holder;
>-    nsresult rv = WrapNative(cx, JS_GetGlobalForScopeChain(cx), uri,
>-                             &NS_GET_IID(nsIURI), PR_TRUE, vp,
>-                             getter_AddRefs(holder));
>-    return NS_FAILED(rv) ? rv : NS_SUCCESS_I_DID_SOMETHING;
>-  }
>-
>-  if (id == sNodePrincipal_id && IsPrivilegedScript()) {
>-    nsCOMPtr<nsINode> node = do_QueryWrappedNative(wrapper, obj);
>-    NS_ENSURE_TRUE(node, NS_ERROR_UNEXPECTED);
>-
>-    nsCOMPtr<nsIXPConnectJSObjectHolder> holder;
>-    nsresult rv = WrapNative(cx, JS_GetGlobalForScopeChain(cx),
>-                             node->NodePrincipal(), &NS_GET_IID(nsIPrincipal),
>-                             PR_TRUE, vp, getter_AddRefs(holder));
>-    return NS_FAILED(rv) ? rv : NS_SUCCESS_I_DID_SOMETHING;
>-  }    
>-
>-  // Note: none of our ancestors want GetProperty
>-  return NS_OK;
>-}
>-
>-NS_IMETHODIMP
>-nsNodeSH::SetProperty(nsIXPConnectWrappedNative *wrapper, JSContext *cx,
>-                      JSObject *obj, jsid id, jsval *vp, PRBool *_retval)
>-{
>-  if ((id == sBaseURIObject_id || id == sNodePrincipal_id) &&
>-      IsPrivilegedScript()) {
>-    // We don't want privileged script that can read this property to set it,
>-    // but _do_ want to allow everyone else to set a value they can then read.
>-    //
>-    // XXXbz Is there a better error we could use here?
>-    return NS_ERROR_DOM_NOT_SUPPORTED_ERR;
>-  }
>-
>-  return NS_OK;
>-}
>-
>-NS_IMETHODIMP
> nsNodeSH::GetFlags(PRUint32 *aFlags)
> {
>   *aFlags = DOMCLASSINFO_STANDARD_FLAGS | nsIClassInfo::CONTENT_NODE;
> 
>   return NS_OK;
> }
> 
> void
>diff --git a/dom/base/nsDOMClassInfo.h b/dom/base/nsDOMClassInfo.h
>--- a/dom/base/nsDOMClassInfo.h
>+++ b/dom/base/nsDOMClassInfo.h
>@@ -277,18 +277,16 @@ public:
>   static jsid sNavigator_id;
>   static jsid sDocument_id;
>   static jsid sFrames_id;
>   static jsid sSelf_id;
>   static jsid sOpener_id;
>   static jsid sAll_id;
>   static jsid sTags_id;
>   static jsid sAddEventListener_id;
>-  static jsid sBaseURIObject_id;
>-  static jsid sNodePrincipal_id;
>   static jsid sDocumentURIObject_id;
>   static jsid sJava_id;
>   static jsid sPackages_id;
>   static jsid sWrappedJSObject_id;
>   static jsid sURL_id;
>   static jsid sKeyPath_id;
>   static jsid sAutoIncrement_id;
>   static jsid sUnique_id;
>@@ -522,20 +520,16 @@ protected:
> public:
>   NS_IMETHOD PreCreate(nsISupports *nativeObj, JSContext *cx,
>                        JSObject *globalObj, JSObject **parentObj);
>   NS_IMETHOD AddProperty(nsIXPConnectWrappedNative *wrapper, JSContext *cx,
>                          JSObject *obj, jsid id, jsval *vp, PRBool *_retval);
>   NS_IMETHOD NewResolve(nsIXPConnectWrappedNative *wrapper, JSContext *cx,
>                         JSObject *obj, jsid id, PRUint32 flags,
>                         JSObject **objp, PRBool *_retval);
>-  NS_IMETHOD GetProperty(nsIXPConnectWrappedNative *wrapper, JSContext *cx,
>-                         JSObject *obj, jsid id, jsval *vp, PRBool *_retval);
>-  NS_IMETHOD SetProperty(nsIXPConnectWrappedNative *wrapper, JSContext *cx,
>-                         JSObject *obj, jsid id, jsval *vp, PRBool *_retval);
>   NS_IMETHOD GetFlags(PRUint32 *aFlags);
> 
>   virtual void PreserveWrapper(nsISupports *aNative);
> 
>   static nsIClassInfo *doCreate(nsDOMClassInfoData* aData)
>   {
>     return new nsNodeSH(aData);
>   }
>diff --git a/js/src/xpconnect/src/xpcjsruntime.cpp b/js/src/xpconnect/src/xpcjsruntime.cpp
>--- a/js/src/xpconnect/src/xpcjsruntime.cpp
>+++ b/js/src/xpconnect/src/xpcjsruntime.cpp
>@@ -75,17 +75,19 @@ const char* XPCJSRuntime::mStrings[] = {
>     "Object",               // IDX_OBJECT
>     "Function",             // IDX_FUNCTION
>     "prototype",            // IDX_PROTOTYPE
>     "createInstance",       // IDX_CREATE_INSTANCE
>     "item",                 // IDX_ITEM
>     "__proto__",            // IDX_PROTO
>     "__iterator__",         // IDX_ITERATOR
>     "__exposedProps__",     // IDX_EXPOSEDPROPS
>-    "__scriptOnly__"        // IDX_SCRIPTONLY
>+    "__scriptOnly__",       // IDX_SCRIPTONLY
>+    "baseURIObject",        // IDX_BASEURIOBJECT
>+    "nodePrincipal"         // IDX_NODEPRINCIPAL
> };
> 
> /***************************************************************************/
> 
> // data holder class for the enumerator callback below
> struct JSDyingJSObjectData
> {
>     JSContext* cx;
>diff --git a/js/src/xpconnect/src/xpcprivate.h b/js/src/xpconnect/src/xpcprivate.h
>--- a/js/src/xpconnect/src/xpcprivate.h
>+++ b/js/src/xpconnect/src/xpcprivate.h
>@@ -720,16 +720,18 @@ public:
>         IDX_FUNCTION                ,
>         IDX_PROTOTYPE               ,
>         IDX_CREATE_INSTANCE         ,
>         IDX_ITEM                    ,
>         IDX_PROTO                   ,
>         IDX_ITERATOR                ,
>         IDX_EXPOSEDPROPS            ,
>         IDX_SCRIPTONLY              ,
>+        IDX_BASEURIOBJECT           ,
>+        IDX_NODEPRINCIPAL           ,
>         IDX_TOTAL_COUNT // just a count of the above
>     };
> 
>     jsid GetStringID(uintN index) const
>     {
>         NS_ASSERTION(index < IDX_TOTAL_COUNT, "index out of range");
>         return mStrIDs[index];
>     }
>diff --git a/js/src/xpconnect/wrappers/XrayWrapper.cpp b/js/src/xpconnect/wrappers/XrayWrapper.cpp
>--- a/js/src/xpconnect/wrappers/XrayWrapper.cpp
>+++ b/js/src/xpconnect/wrappers/XrayWrapper.cpp
>@@ -41,16 +41,19 @@
> #include "AccessCheck.h"
> #include "FilteringWrapper.h"
> #include "CrossOriginWrapper.h"
> #include "WrapperFactory.h"
> 
> #include "jscntxt.h"
> #include "jsiter.h"
> 
>+#include "nsIContent.h"
>+#include "nsIDocument.h"
>+
> #include "XPCWrapper.h"
> #include "xpcprivate.h"
> 
> namespace xpc {
> 
> using namespace js;
> 
> static const uint32 JSSLOT_WN = 0;
>@@ -327,21 +330,112 @@ ResolveNativeProperty(JSContext *cx, JSO
>     // Define the property.
>     return JS_DefinePropertyById(cx, holder, id, desc->value,
>                                  desc->getter, desc->setter, desc->attrs);
> }
> 
> static JSBool
> wrappedJSObject_getter(JSContext *cx, JSObject *wrapper, jsid id, jsval *vp)
> {
>+    if (!wrapper->isWrapper() || !WrapperFactory::IsXrayWrapper(wrapper)) {
>+        JS_ReportError(cx, "Unexpected object");
>+        return false;
>+    }
>+
>     *vp = OBJECT_TO_JSVAL(wrapper);
> 
>     return WrapperFactory::WaiveXrayAndWrap(cx, vp);
> }
> 
>+template <typename T>
>+static bool
>+Is(JSObject *wrapper)
>+{
>+    JSObject *holder = GetHolder(wrapper);
>+    XPCWrappedNative *wn = GetWrappedNativeFromHolder(holder);
>+    nsCOMPtr<T> native = do_QueryWrappedNative(wn);
>+    return !!native;
>+}
>+
>+template <typename T>
>+static JSBool
>+baseURIObject_getter(JSContext *cx, JSObject *wrapper, jsid id, jsval *vp)
>+{
>+    if (!wrapper->isWrapper() || !WrapperFactory::IsXrayWrapper(wrapper)) {
>+        JS_ReportError(cx, "Unexpected object");
>+        return false;
>+    }
>+
>+    JSObject *holder = GetHolder(wrapper);
>+    XPCWrappedNative *wn = GetWrappedNativeFromHolder(holder);
>+    nsCOMPtr<T> native = do_QueryWrappedNative(wn);
>+    if (!native) {
>+        JS_ReportError(cx, "Unexpected object");
>+        return false;
>+    }
>+    nsCOMPtr<nsIURI> uri = native->GetBaseURI();
>+    if (!uri) {
>+        JS_ReportOutOfMemory(cx);
>+        return false;
>+    }
>+
>+    JSObject *scope = JS_GetGlobalForScopeChain(cx);
>+    nsCOMPtr<nsIXPConnectJSObjectHolder> objectHolder;
>+    nsresult rv =
>+        nsXPConnect::FastGetXPConnect()->WrapNativeToJSVal(cx, scope, uri, nsnull,
>+                                                           &NS_GET_IID(nsIURI), PR_TRUE,
>+                                                           vp, getter_AddRefs(objectHolder));
>+    if (NS_FAILED(rv)) {
>+        XPCThrower::Throw(rv, cx);
>+        return false;
>+    }
>+    return true;
>+}
>+
>+static JSBool
>+nsIContent_baseURIObject_getter(JSContext *cx, JSObject *wrapper, jsid id, jsval *vp)
>+{
>+    return baseURIObject_getter<nsIContent>(cx, wrapper, id, vp);
>+}
>+
>+static JSBool
>+nsIDocument_baseURIObject_getter(JSContext *cx, JSObject *wrapper, jsid id, jsval *vp)
>+{
>+    return baseURIObject_getter<nsIDocument>(cx, wrapper, id, vp);
>+}
>+
>+static JSBool
>+nodePrincipal_getter(JSContext *cx, JSObject *wrapper, jsid id, jsval *vp)
>+{
>+    if (!wrapper->isWrapper() || !WrapperFactory::IsXrayWrapper(wrapper)) {
>+        JS_ReportError(cx, "Unexpected object");
>+        return false;
>+    }
>+
>+    JSObject *holder = GetHolder(wrapper);
>+    XPCWrappedNative *wn = GetWrappedNativeFromHolder(holder);
>+    nsCOMPtr<nsINode> node = do_QueryWrappedNative(wn);
>+    if (!node) {
>+        JS_ReportError(cx, "Unexpected object");
>+        return false;
>+    }
>+
>+    nsCOMPtr<nsIXPConnectJSObjectHolder> objectHolder;
>+    JSObject *scope = JS_GetGlobalForScopeChain(cx);
>+    nsresult rv =
>+        nsXPConnect::FastGetXPConnect()->WrapNativeToJSVal(cx, scope, node->NodePrincipal(), nsnull,
>+                                                           &NS_GET_IID(nsIPrincipal), PR_TRUE,
>+                                                           vp, getter_AddRefs(objectHolder));
>+    if (NS_FAILED(rv)) {
>+        XPCThrower::Throw(rv, cx);
>+        return false;
>+    }
>+    return true;
>+}
>+
> static JSBool
> XrayToString(JSContext *cx, uintN argc, jsval *vp)
> {
>     JSObject *wrapper = JS_THIS_OBJECT(cx, vp);
>     if (!wrapper->isWrapper() || !WrapperFactory::IsXrayWrapper(wrapper)) {
>         JS_ReportError(cx, "XrayToString called on an incompatible object");
>         return false;
>     }
>@@ -396,67 +490,84 @@ class AutoLeaveHelper
> 
>   private:
>     XrayWrapper<Base> &xray;
>     JSContext *cx;
>     JSObject *wrapper;
> };
> 
> static bool
>-Transparent(JSContext *cx, JSObject *wrapper)
>+IsPrivilegedScript()
>+{
>+    // Redirect access straight to the wrapper if UniversalXPConnect is enabled.
>+    nsIScriptSecurityManager *ssm = XPCWrapper::GetSecurityManager();
>+    if (ssm) {
>+        PRBool privileged;
>+        if (NS_SUCCEEDED(ssm->IsCapabilityEnabled("UniversalXPConnect", &privileged)) && privileged)
>+            return true;
>+    }
>+    return false;
>+}
>+
>+namespace XrayUtils {
>+
>+bool
>+IsTransparent(JSContext *cx, JSObject *wrapper)
> {
>     if (WrapperFactory::HasWaiveXrayFlag(wrapper))
>         return true;
> 
>     if (!WrapperFactory::IsPartiallyTransparent(wrapper))
>         return false;
> 
>     // Redirect access straight to the wrapper if UniversalXPConnect is enabled.
>-    nsIScriptSecurityManager *ssm = XPCWrapper::GetSecurityManager();
>-    if (ssm) {
>-        PRBool privileged;
>-        if (NS_SUCCEEDED(ssm->IsCapabilityEnabled("UniversalXPConnect", &privileged)) && privileged)
>-            return true;
>-    }
>+    if (IsPrivilegedScript())
>+        return true;
> 
>     return AccessCheck::documentDomainMakesSameOrigin(cx, wrapper->unwrap());
> }
> 
>-namespace XrayUtils {
>-
>-bool
>-IsTransparent(JSContext *cx, JSObject *wrapper)
>-{
>-    return Transparent(cx, wrapper);
>-}
>-
> }
> 
> template <typename Base>
> bool
> XrayWrapper<Base>::resolveOwnProperty(JSContext *cx, JSObject *wrapper, jsid id, bool set,
>                                       PropertyDescriptor *desc_in)
> {
>     JSPropertyDescriptor *desc = Jsvalify(desc_in);
> 
>     // Partially transparent wrappers (which used to be known as XOWs) don't
>     // have a .wrappedJSObject property.
>+    XPCJSRuntime* rt = nsXPConnect::GetRuntimeInstance();
>+    bool isNsIContent = false;
>     if (!WrapperFactory::IsPartiallyTransparent(wrapper) &&
>-        id == nsXPConnect::GetRuntimeInstance()->GetStringID(XPCJSRuntime::IDX_WRAPPED_JSOBJECT)) {
>+        ((id == rt->GetStringID(XPCJSRuntime::IDX_WRAPPED_JSOBJECT)) ||
>+         (((id == rt->GetStringID(XPCJSRuntime::IDX_BASEURIOBJECT) &&
>+            ((isNsIContent = Is<nsIContent>(wrapper)) || Is<nsIDocument>(wrapper))) ||
>+           (id == rt->GetStringID(XPCJSRuntime::IDX_NODEPRINCIPAL) &&
>+            Is<nsINode>(wrapper))) && IsPrivilegedScript()))) {
>         bool status;
>         JSWrapper::Action action = set ? JSWrapper::SET : JSWrapper::GET;
>         desc->obj = NULL; // default value
>         if (!this->enter(cx, wrapper, id, action, &status))
>             return status;
> 
>         AutoLeaveHelper<Base> helper(*this, cx, wrapper);
> 
>         desc->obj = wrapper;
>         desc->attrs = JSPROP_ENUMERATE|JSPROP_SHARED;
>-        desc->getter = wrappedJSObject_getter;
>+        if (id == rt->GetStringID(XPCJSRuntime::IDX_WRAPPED_JSOBJECT))
>+            desc->getter = wrappedJSObject_getter;
>+        else if (id == rt->GetStringID(XPCJSRuntime::IDX_BASEURIOBJECT)) {
>+            desc->getter = isNsIContent
>+                           ? nsIContent_baseURIObject_getter
>+                           : nsIDocument_baseURIObject_getter;
>+        } else {
>+            desc->getter = nodePrincipal_getter;
>+        }
>         desc->setter = NULL;
>         desc->shortid = 0;
>         desc->value = JSVAL_VOID;
>         return true;
>     }
> 
>     desc->obj = NULL;
> 
>@@ -535,17 +646,17 @@ XrayWrapper<Base>::getPropertyDescriptor
>     if (!this->enter(cx, wrapper, id, action, &status))
>         return status;
> 
>     AutoLeaveHelper<Base> helper(*this, cx, wrapper);
> 
>     ResolvingId resolving(holder, id);
> 
>     // Redirect access straight to the wrapper if we should be transparent.
>-    if (Transparent(cx, wrapper)) {
>+    if (XrayUtils::IsTransparent(cx, wrapper)) {
>         JSObject *wnObject = GetWrappedNativeObjectFromHolder(holder);
> 
>         {
>             JSAutoEnterCompartment ac;
>             if (!ac.enter(cx, wnObject))
>                 return false;
> 
>             if (!JS_GetPropertyDescriptorById(cx, wnObject, id,
>@@ -606,17 +717,17 @@ XrayWrapper<Base>::getOwnPropertyDescrip
> 
>     AutoLeaveHelper<Base> helper(*this, cx, wrapper);
> 
>     ResolvingId resolving(holder, id);
> 
>     // NB: Nothing we do here acts on the wrapped native itself, so we don't
>     // enter our policy.
>     // Redirect access straight to the wrapper if we should be transparent.
>-    if (Transparent(cx, wrapper)) {
>+    if (XrayUtils::IsTransparent(cx, wrapper)) {
>         JSObject *wnObject = GetWrappedNativeObjectFromHolder(holder);
> 
>         {
>             JSAutoEnterCompartment ac;
>             if (!ac.enter(cx, wnObject))
>                 return false;
> 
>             if (!JS_GetPropertyDescriptorById(cx, wnObject, id,
>@@ -637,17 +748,17 @@ template <typename Base>
> bool
> XrayWrapper<Base>::defineProperty(JSContext *cx, JSObject *wrapper, jsid id,
>                                   js::PropertyDescriptor *desc)
> {
>     JSObject *holder = GetHolder(wrapper);
>     JSPropertyDescriptor *jsdesc = Jsvalify(desc);
> 
>     // Redirect access straight to the wrapper if we should be transparent.
>-    if (Transparent(cx, wrapper)) {
>+    if (XrayUtils::IsTransparent(cx, wrapper)) {
>         JSObject *wnObject = GetWrappedNativeObjectFromHolder(holder);
> 
>         JSAutoEnterCompartment ac;
>         if (!ac.enter(cx, wnObject))
>             return false;
> 
>         if (!cx->compartment->wrap(cx, desc))
>             return false;
>@@ -686,17 +797,17 @@ XrayWrapper<Base>::defineProperty(JSCont
> static bool
> EnumerateNames(JSContext *cx, JSObject *wrapper, uintN flags, js::AutoIdVector &props)
> {
>     JSObject *holder = GetHolder(wrapper);
> 
>     JSObject *wnObject = GetWrappedNativeObjectFromHolder(holder);
> 
>     // Redirect access straight to the wrapper if we should be transparent.
>-    if (Transparent(cx, wrapper)) {
>+    if (XrayUtils::IsTransparent(cx, wrapper)) {
>         JSAutoEnterCompartment ac;
>         if (!ac.enter(cx, wnObject))
>             return false;
> 
>         return js::GetPropertyNames(cx, wnObject, flags, &props);
>     }
> 
>     if (WrapperFactory::IsPartiallyTransparent(wrapper)) {
>@@ -743,17 +854,17 @@ template <typename Base>
> bool
> XrayWrapper<Base>::delete_(JSContext *cx, JSObject *wrapper, jsid id, bool *bp)
> {
>     JSObject *holder = GetHolder(wrapper);
>     jsval v;
>     JSBool b;
> 
>     // Redirect access straight to the wrapper if we should be transparent.
>-    if (Transparent(cx, wrapper)) {
>+    if (XrayUtils::IsTransparent(cx, wrapper)) {
>         JSObject *wnObject = GetWrappedNativeObjectFromHolder(holder);
> 
>         JSAutoEnterCompartment ac;
>         if (!ac.enter(cx, wnObject))
>             return false;
> 
>         if (!JS_DeletePropertyById2(cx, wnObject, id, &v) || !JS_ValueToBoolean(cx, v, &b))
>             return false;

Comment 18

[Andreas Gal :gal]

I am going to have to disable some mochitests for this (or turn them into chrome tests) since same-compartment can no longer see these properties.

Comment 19

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 557628 [details] [diff] [review]
patch

Since you're leaving documentURIObject be for now, you need to add WANT_SETPROPERTY to DOCUMENT_SCRIPTABLE_FLAGS, no?  If we don't have a test for this already, we need to add one.

And file a followup to nix this too?

Since you removed nsNodeSH::Get/SetProperty but subclasses call into it (either directly or via nsElementSH), those calls will now hit nsDOMClassInfo::Get/SetProperty and warn.  You probably need to fix the relevant callsites (at least the various document stuff that chains up to nsNodeSH::Get/SetProperty and the elements that chain up to nsElementSH::GetSetProperty: form, select, pluginobj).

nsHTMLSelectElement needs to have a WANT_SETPROPERTY flag added to its classinfo.  Once again, if we don't have a test for this already we need one.

The |false| returns in the various error handling codepaths you're adding should probably be JS_FALSE.  Similar for the |return true| at the end of the two getter functions.

You don't need a templated baseURIObject_getter.  GetBaseURI is on nsINode, which is implemented by both documents and content objects.  So just have a single getter that QIs to nsINode.  Then you'd also only need one Is() check there in the resolve hook and wouldn't need the isNsIContent boolean and so forth.

It would almost make more sense to template the principal and URI getters so they can share code, since they will be pretty close to identical at that point; we can assign the principal to an nsCOMPtr as well to make that easier (template on the nsINode::* function involved or its  return type or bot or something), though that may be too convoluted.  But the code duplication there really bothers me.  :(

I'm not really qualified to review the details of the isWrapper and IsXrayWrapper checks and the GetHolder/GetWrappedNativeFromHolder bits unless you want me to really dig into this code and understand how xraywrapper is set up.

I wouldn't copy the "Redirect access" comment into IsPrivilegedScript.

Your if condition in resolveOwnProperty seems to be overparenthesized around the WRAPPED_JSOBJECT equality check.  Like so:

        (id == rt->GetStringID(XPCJSRuntime::IDX_WRAPPED_JSOBJECT) ||
         ((id == rt->GetStringID(XPCJSRuntime::IDX_BASEURIOBJECT ||
           id == rt->GetStringID(XPCJSRuntime::IDX_NODEPRINCIPAL) &&
          Is<nsINode>(wrapper) && IsPrivilegedScript()))) {

or in the interests of paren-reduction even like so:

        (id == rt->GetStringID(XPCJSRuntime::IDX_WRAPPED_JSOBJECT) ||
         (id == rt->GetStringID(XPCJSRuntime::IDX_BASEURIOBJECT &&
          Is<nsINode>(wrapper) && IsPrivilegedScript()) ||
         (id == rt->GetStringID(XPCJSRuntime::IDX_NODEPRINCIPAL) &&
          Is<nsINode>(wrapper) && IsPrivilegedScript()))) {

but either one is more readable than what you have now.

Comment 20

[Andreas Gal :gal]

Will fix the stuff above. I can do documentURIObject too.

Comment 21

[Boris Zbarsky [:bzbarsky]]

If you do documentURIObject as well, then we want to nuke nsDocumentSH::Get/SetProperty, and change the flags for nsHTMLDocument to be DOCUMENT_SCRIPTABLE_FLAGS | WANT_GETPROPERTY.  And make sure that nsHTMLDocument::GetProperty doesn't call up to the superclass.

Comment 22

[Andreas Gal :gal]

JS engine is using true/false all over in newer code. JS_TRUE/JS_FALSE is very old-school. I will change them if you insist.

Comment 23

[Andreas Gal :gal]

Attachment: not ready yet

Comment 24

[Boris Zbarsky [:bzbarsky]]

I'm certainly not going to insist on the bool thing.  ;)

Comment 25

[Andreas Gal :gal]

I have a whatever-pleases-the-reviewer policy. If you prefered

#define BZ_TRUE true

return BZ_TRUE

I would totally do that to get the r+ I am after ;)

Comment 26

[Andreas Gal :gal]

Attachment: patch

Not tested yet, feel free to take a look while I try-server this.

Comment 27

[Boris Zbarsky [:bzbarsky]]

First thing... want to write this on top of bug 639720?  That makes the classinfo code simpler for nsDocumentSH, for one thing, and that patch will hopefully land to inbound tonight and actually stick...

Comment 28

[Boris Zbarsky [:bzbarsky]]

And don't you need to keep WANT_GETPROPERTY for nsHTMLDocumentSH ?

Comment 29

[Andreas Gal :gal]

Sounds good land and I will rebase. I will check the flag. The manual analysis is tedious.

Comment 30

[Boris Zbarsky [:bzbarsky]]

Yeah, I agree, re analysis.  It's a good thing we plan to make this code go away!

Comment 31

[Andreas Gal :gal]

Attachment: added WANT_GETPROPERTY for nsHTMLDocumentSH

Comment 32

[Boris Zbarsky [:bzbarsky]]

Andreas, rebase and post the result?

Comment 33

[Boris Zbarsky [:bzbarsky]]

Attachment: Move nodePrincipal, baseURIObject, and documentURIObject from classinfo to XrayWrapper.

Comment 34

[Boris Zbarsky [:bzbarsky]]

Attachment: Actually compiling and all

Comment 35

[Andreas Gal :gal]

Thanks

Comment 36

[Blake Kaplan (:mrbkap) (inactive)]

Comment on attachment 560673 [details] [diff] [review]
Actually compiling and all

Review of attachment 560673 [details] [diff] [review]:
-----------------------------------------------------------------

I have a couple of questions, but r=me with the answers.

::: js/src/xpconnect/wrappers/XrayWrapper.cpp
@@ +362,5 @@
> +    nsCOMPtr<nsIXPConnectJSObjectHolder> objectHolder;
> +    nsresult rv =
> +        nsXPConnect::FastGetXPConnect()->WrapNativeToJSVal(cx, scope, uri, nsnull,
> +                                                           &NS_GET_IID(nsIURI), PR_TRUE,
> +                                                           vp, getter_AddRefs(objectHolder));

Why do we need the objectHolder here? Isn't vp enough to root the value?

@@ +440,5 @@
> +    JSObject *scope = JS_GetGlobalForScopeChain(cx);
> +    nsresult rv =
> +        nsXPConnect::FastGetXPConnect()->WrapNativeToJSVal(cx, scope, node->NodePrincipal(), nsnull,
> +                                                           &NS_GET_IID(nsIPrincipal), PR_TRUE,
> +                                                           vp, getter_AddRefs(objectHolder));

Same question here.

@@ +558,5 @@
>      if (!WrapperFactory::IsPartiallyTransparent(wrapper) &&
> +        ((id == rt->GetStringID(XPCJSRuntime::IDX_WRAPPED_JSOBJECT)) ||
> +         (IsPrivilegedScript() &&
> +          ((Is<nsINode>(wrapper) &&
> +            (id == rt->GetStringID(XPCJSRuntime::IDX_BASEURIOBJECT) ||

I'm not sure if it'll actually make a difference, but it might make sense to invert these checks so that we avoid the QI in the more common case where we're not getting baseURIObject or nodePrincipal.

Comment 37

[Boris Zbarsky [:bzbarsky]]

> Why do we need the objectHolder here?

We probably don't.  I'll take it out.  Likewise for principals.

> it might make sense to invert these checks

Will do.

Comment 38

[Boris Zbarsky [:bzbarsky]]

http://hg.mozilla.org/integration/mozilla-inbound/rev/adceb7fb0fce

Comment 39

[Boris Zbarsky [:bzbarsky]]

And backed out, because all sorts of tests fail.

I can fix the tests, I bet, but it looks like we have lots of UI code that assumes that it can get these props for anything in a tab, which I think is just false with this xray wrapper approach.  Worse yet, extensions assume that too.

So in addition to fixing the tests that use this via UniversalXPConnect, I'll be adding getters on Node and Document prototypes in chrome only to handle these properties too.

Comment 40

[Boris Zbarsky [:bzbarsky]]

Attachment: Test fixes to make tests not assume that UniversalXPConnect will get them these properties

Comment 41

[Boris Zbarsky [:bzbarsky]]

Attachment: Test fixes as diff -w; may be easier to review

Comment 42

[Boris Zbarsky [:bzbarsky]]

Attachment: Add the Node and Document getters in chrome only. This applies on top of the other diff

Comment 43

[Boris Zbarsky [:bzbarsky]]

So those patches fail one and only one test in our test suite: content/media/test/test_bug495300.html 

That test doesn't use these properties.

In fact, that test will pass as long as NODE_SCRIPTABLE_FLAGS includes WANT_SETPROPERTY as far as I can tell.  Even if all the actual node class set hooks are no-ops.  What the heck?

Comment 44

[Boris Zbarsky [:bzbarsky]]

Found the problem.  It's a propcache bug.... Filed bug 688685.

Unfortunately, this might mean property _adds_ from jitcode will still be slow.  We'll see.  I have no idea whether our ICs handle this correctly now.

Comment 45

[Boris Zbarsky [:bzbarsky]]

    http://hg.mozilla.org/integration/mozilla-inbound/rev/d9cd2e3f0a9a
    http://hg.mozilla.org/integration/mozilla-inbound/rev/cd43848a0326

Comment 46

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/d9cd2e3f0a9a
https://hg.mozilla.org/mozilla-central/rev/cd43848a0326

Comment 47

[Jorge Villalobos [:jorgev] (he/him)]

If I understand this correctly, chrome DOM nodes should have baseURI and nodePrincipal properties that will no longer exist in Firefox 9. Is this correct?

I can see baseURI using DOM Inspector, but not nodePrincipal. I doubt either of them have much use, but I want to be sure.

Comment 48

[Boris Zbarsky [:bzbarsky]]

> Is this correct?

No.  I _think_ the only compat-affecting change from the final version of the patch is that the baseURIObject, nodePrincipal, and documentURIObject properties will no longer be available to otherwise-untrusted script using enablePrivilege("UniversalXPConnect") and tat content will no longer be able to detect that chrome has asked for those properties on a node.  Everything else should look as it did before this patch.

Note that baseURIObject and baseURI are totally different things.  None of the three properties affected by this bug showed up in DOM Inspector, since all were non-enumerable.

> I doubt either of them have much use

nodePrincipal is used a few dozen addons according to the addons mxr.

baseURIObject and documentURIObject likewise.

Comment 49

[Jorge Villalobos [:jorgev] (he/him)]

(In reply to Boris Zbarsky (:bz) from comment #48)
> > Is this correct?
> Note that baseURIObject and baseURI are totally different things.  None of
> the three properties affected by this bug showed up in DOM Inspector, since
> all were non-enumerable.

Got it, thanks.

> > I doubt either of them have much use
> 
> nodePrincipal is used a few dozen addons according to the addons mxr.
> 
> baseURIObject and documentURIObject likewise.

But for the most part they aren't affected, right? Given that those objects are only being removed for untrusted scripts that are using enablePrivilege to elevate their allowed access.

Comment 50

[Boris Zbarsky [:bzbarsky]]

> But for the most part they aren't affected, right?

That's the hope, yes.

Comment 51

[Eric Shepherd [:sheppy]]

This is mentioned here: https://developer.mozilla.org/en/Firefox/Updating_add-ons_for_Firefox_9#XPConnect_changes

I think that's all we're going to do for this unless someone knows of other places we should update. I don't see any obvious places for this to get mentioned otherwise though, since our existing docs on this stuff are pretty vague.