Last Comment Bug 660233 - Move nodePrincipal, documentURIObject, and baseURIObject onto XrayWrapper and remove from nsDOMClassInfo
: Move nodePrincipal, documentURIObject, and baseURIObject onto XrayWrapper and...
Status: RESOLVED FIXED
: addon-compat, dev-doc-complete
Product: Core
Classification: Components
Component: XPConnect (show other bugs)
: unspecified
: All All
: P1 normal (vote)
: mozilla9
Assigned To: Boris Zbarsky [:bz] (still a bit busy)
:
: Andrew Overholt [:overholt]
Mentors:
Depends on: 639720 659350 688685
Blocks: 517143 ParisBindings
  Show dependency treegraph
 
Reported: 2011-05-27 07:59 PDT by Andreas Gal :gal
Modified: 2011-12-06 14:37 PST (History)
9 users (show)
bzbarsky: in‑testsuite+
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
draft patch, untested (12.41 KB, patch)
2011-08-08 16:54 PDT, Andreas Gal :gal
no flags Details | Diff | Splinter Review
patch (18.77 KB, patch)
2011-08-09 08:00 PDT, Andreas Gal :gal
Ms2ger: feedback+
Details | Diff | Splinter Review
patch (18.76 KB, patch)
2011-08-09 08:19 PDT, Andreas Gal :gal
no flags Details | Diff | Splinter Review
patch (19.39 KB, patch)
2011-08-09 16:29 PDT, Andreas Gal :gal
mrbkap: review+
Details | Diff | Splinter Review
patch (21.84 KB, patch)
2011-09-01 13:43 PDT, Andreas Gal :gal
bzbarsky: review-
Details | Diff | Splinter Review
not ready yet (32.55 KB, patch)
2011-09-01 20:30 PDT, Andreas Gal :gal
no flags Details | Diff | Splinter Review
patch (33.84 KB, patch)
2011-09-01 20:45 PDT, Andreas Gal :gal
no flags Details | Diff | Splinter Review
added WANT_GETPROPERTY for nsHTMLDocumentSH (34.81 KB, patch)
2011-09-01 22:49 PDT, Andreas Gal :gal
no flags Details | Diff | Splinter Review
Move nodePrincipal, baseURIObject, and documentURIObject from classinfo to XrayWrapper. (34.21 KB, patch)
2011-09-15 16:29 PDT, Boris Zbarsky [:bz] (still a bit busy)
no flags Details | Diff | Splinter Review
Actually compiling and all (34.25 KB, patch)
2011-09-16 21:43 PDT, Boris Zbarsky [:bz] (still a bit busy)
mrbkap: review+
Details | Diff | Splinter Review
Test fixes to make tests not assume that UniversalXPConnect will get them these properties (35.08 KB, patch)
2011-09-22 11:03 PDT, Boris Zbarsky [:bz] (still a bit busy)
no flags Details | Diff | Splinter Review
Test fixes as diff -w; may be easier to review (25.09 KB, patch)
2011-09-22 11:04 PDT, Boris Zbarsky [:bz] (still a bit busy)
mrbkap: review+
Details | Diff | Splinter Review
Add the Node and Document getters in chrome only. This applies on top of the other diff (10.90 KB, patch)
2011-09-22 11:10 PDT, Boris Zbarsky [:bz] (still a bit busy)
mrbkap: review+
Details | Diff | Splinter Review

Description Andreas Gal :gal 2011-05-27 07:59:54 PDT

    
Comment 1 Andreas Gal :gal 2011-08-08 16:54:31 PDT
Created attachment 551618 [details] [diff] [review]
draft patch, untested
Comment 2 :Ms2ger (⌚ UTC+1/+2) 2011-08-09 01:52:45 PDT
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 2011-08-09 08:00:52 PDT
Created attachment 551767 [details] [diff] [review]
patch
Comment 4 Andreas Gal :gal 2011-08-09 08:03:03 PDT
> 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 (⌚ UTC+1/+2) 2011-08-09 08:08:36 PDT
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 2011-08-09 08:19:05 PDT
Created attachment 551769 [details] [diff] [review]
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 2011-08-09 08:20:25 PDT
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 2011-08-09 16:29:54 PDT
Created attachment 551925 [details] [diff] [review]
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 2011-08-09 16:30:37 PDT
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 2011-08-15 14:24:14 PDT
Comment on attachment 551925 [details] [diff] [review]
patch

Note the two questions below
Comment 11 Blake Kaplan (:mrbkap) 2011-08-17 16:26:40 PDT
(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) 2011-08-17 16:29:32 PDT
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 [:bz] (still a bit busy) 2011-08-25 08:47:01 PDT
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) 2011-08-31 14:45:06 PDT
(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) 2011-08-31 14:46:29 PDT
Comment on attachment 551925 [details] [diff] [review]
patch

r=me with my comment above addressed.
Comment 16 Andreas Gal :gal 2011-09-01 13:43:14 PDT
Created attachment 557628 [details] [diff] [review]
patch
Comment 17 Andreas Gal :gal 2011-09-01 13:43:39 PDT
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 2011-09-01 13:44:43 PDT
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 [:bz] (still a bit busy) 2011-09-01 15:26:15 PDT
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 2011-09-01 15:47:22 PDT
Will fix the stuff above. I can do documentURIObject too.
Comment 21 Boris Zbarsky [:bz] (still a bit busy) 2011-09-01 16:59:25 PDT
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 2011-09-01 20:29:17 PDT
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 2011-09-01 20:30:28 PDT
Created attachment 557757 [details] [diff] [review]
not ready yet
Comment 24 Boris Zbarsky [:bz] (still a bit busy) 2011-09-01 20:36:37 PDT
I'm certainly not going to insist on the bool thing.  ;)
Comment 25 Andreas Gal :gal 2011-09-01 20:38:47 PDT
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 2011-09-01 20:45:48 PDT
Created attachment 557761 [details] [diff] [review]
patch

Not tested yet, feel free to take a look while I try-server this.
Comment 27 Boris Zbarsky [:bz] (still a bit busy) 2011-09-01 21:16:55 PDT
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 [:bz] (still a bit busy) 2011-09-01 21:18:32 PDT
And don't you need to keep WANT_GETPROPERTY for nsHTMLDocumentSH ?
Comment 29 Andreas Gal :gal 2011-09-01 21:21:19 PDT
Sounds good land and I will rebase. I will check the flag. The manual analysis is tedious.
Comment 30 Boris Zbarsky [:bz] (still a bit busy) 2011-09-01 21:23:55 PDT
Yeah, I agree, re analysis.  It's a good thing we plan to make this code go away!
Comment 31 Andreas Gal :gal 2011-09-01 22:49:10 PDT
Created attachment 557770 [details] [diff] [review]
added WANT_GETPROPERTY for nsHTMLDocumentSH
Comment 32 Boris Zbarsky [:bz] (still a bit busy) 2011-09-07 20:06:03 PDT
Andreas, rebase and post the result?
Comment 33 Boris Zbarsky [:bz] (still a bit busy) 2011-09-15 16:29:43 PDT
Created attachment 560486 [details] [diff] [review]
Move nodePrincipal, baseURIObject, and documentURIObject from classinfo to XrayWrapper.
Comment 34 Boris Zbarsky [:bz] (still a bit busy) 2011-09-16 21:43:07 PDT
Created attachment 560673 [details] [diff] [review]
Actually compiling and all
Comment 35 Andreas Gal :gal 2011-09-16 23:35:56 PDT
Thanks
Comment 36 Blake Kaplan (:mrbkap) 2011-09-21 11:57:25 PDT
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 [:bz] (still a bit busy) 2011-09-21 12:23:10 PDT
> 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 [:bz] (still a bit busy) 2011-09-21 18:31:33 PDT
http://hg.mozilla.org/integration/mozilla-inbound/rev/adceb7fb0fce
Comment 39 Boris Zbarsky [:bz] (still a bit busy) 2011-09-21 21:47:40 PDT
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 [:bz] (still a bit busy) 2011-09-22 11:03:25 PDT
Created attachment 561795 [details] [diff] [review]
Test fixes to make tests not assume that UniversalXPConnect will get them these properties
Comment 41 Boris Zbarsky [:bz] (still a bit busy) 2011-09-22 11:04:42 PDT
Created attachment 561796 [details] [diff] [review]
Test fixes as diff -w; may be easier to review
Comment 42 Boris Zbarsky [:bz] (still a bit busy) 2011-09-22 11:10:02 PDT
Created attachment 561797 [details] [diff] [review]
Add the Node and Document getters in chrome only.  This applies on top of the other diff
Comment 43 Boris Zbarsky [:bz] (still a bit busy) 2011-09-22 19:33:10 PDT
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 [:bz] (still a bit busy) 2011-09-22 20:40:19 PDT
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 47 Jorge Villalobos [:jorgev] 2011-09-29 16:24:01 PDT
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 [:bz] (still a bit busy) 2011-09-29 22:40:15 PDT
> 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] 2011-09-30 07:53:53 PDT
(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 [:bz] (still a bit busy) 2011-09-30 11:35:48 PDT
> But for the most part they aren't affected, right?

That's the hope, yes.
Comment 51 Eric Shepherd [:sheppy] 2011-12-06 14:37:45 PST
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.

Note You need to log in before you can comment on or make changes to this bug.