Bug 1526382 - Part 1: Split nsID& and nsID* in xpconnect, r=mccr8

Currently the [ref] and [ptr] types share the same underlying implementation. This is unfortunate, and can cause correctness problems with outparam refs (as an example). By using the same tools used to allow other larger objects (such as jsid, nsTArray, and nsString) to be stored directly in the nsXPTCVariant object, this patch directly stores the nsID in the nsXPTCVariant object when calling from JS into C++. Using this new strategy avoids an nsID* allocation every time we pass one over XPConnect, and should also allow us to simplify callers. In addition, some special casing is added to xpidl to make it possible to use the nsid reference type objects directly inside of Array<T>, which will allow us to remove `[array] nsIIDPtr` callers. Differential Revision: https://phabricator.services.mozilla.com/D19175
mozilla-spidermonkey · Feb 13, 2019 · 6adc106 · 6adc106
1 parent 7775cf3
commit 6adc106
Show file tree

Hide file tree

Showing 11 changed files with 68 additions and 34 deletions.
diff --git a/js/xpconnect/src/XPCConvert.cpp b/js/xpconnect/src/XPCConvert.cpp
@@ -175,7 +175,7 @@ bool XPCConvert::NativeData2JS(MutableHandleValue d, const void* s,
       XPC_LOG_ERROR(("XPCConvert::NativeData2JS : void* params not supported"));
       return false;
 
-    case nsXPTType::T_IID: {
+    case nsXPTType::T_NSIDPTR: {
       nsID* iid2 = *static_cast<nsID* const*>(s);
       if (!iid2) {
         d.setNull();
@@ -185,6 +185,9 @@ bool XPCConvert::NativeData2JS(MutableHandleValue d, const void* s,
       return xpc::ID2JSValue(cx, *iid2, d);
     }
 
+    case nsXPTType::T_NSID:
+      return xpc::ID2JSValue(cx, *static_cast<const nsID*>(s), d);
+
     case nsXPTType::T_ASTRING: {
       const nsAString* p = static_cast<const nsAString*>(s);
       if (!p || p->IsVoid()) {
@@ -542,13 +545,20 @@ bool XPCConvert::JSData2Native(JSContext* cx, void* d, HandleValue s,
       NS_ERROR("void* params not supported");
       return false;
 
-    case nsXPTType::T_IID:
+    case nsXPTType::T_NSIDPTR:
       if (Maybe<nsID> id = xpc::JSValue2ID(cx, s)) {
         *((const nsID**)d) = id.ref().Clone();
         return true;
       }
       return false;
 
+    case nsXPTType::T_NSID:
+      if (Maybe<nsID> id = xpc::JSValue2ID(cx, s)) {
+        *((nsID*)d) = id.ref();
+        return true;
+      }
+      return false;
+
     case nsXPTType::T_ASTRING: {
       nsAString* ws = (nsAString*)d;
       if (s.isUndefined() || s.isNull()) {
@@ -1615,7 +1625,7 @@ void xpc::InnerCleanupValue(const nsXPTType& aType, void* aValue,
       break;
 
     // Pointer Types
-    case nsXPTType::T_IID:
+    case nsXPTType::T_NSIDPTR:
     case nsXPTType::T_CHAR_STR:
     case nsXPTType::T_WCHAR_STR:
     case nsXPTType::T_PSTRING_SIZE_IS:
@@ -1647,6 +1657,11 @@ void xpc::InnerCleanupValue(const nsXPTType& aType, void* aValue,
       break;
     }
 
+    // Clear nsID& parameters to `0`
+    case nsXPTType::T_NSID:
+      ((nsID*)aValue)->Clear();
+      break;
+
     // Clear the JS::Value to `undefined`
     case nsXPTType::T_JSVAL:
       ((JS::Value*)aValue)->setUndefined();

diff --git a/js/xpconnect/src/XPCVariant.cpp b/js/xpconnect/src/XPCVariant.cpp
@@ -231,7 +231,7 @@ bool XPCArrayHomogenizer::GetTypeForArray(JSContext* cx, HandleObject array,
       *resultType = nsXPTType::MkArrayType(nsXPTType::Idx::PWSTRING);
       break;
     case tID:
-      *resultType = nsXPTType::MkArrayType(nsXPTType::Idx::PNSIID);
+      *resultType = nsXPTType::MkArrayType(nsXPTType::Idx::NSIDPTR);
       break;
     case tISup:
       *resultType = nsXPTType::MkArrayType(nsXPTType::Idx::INTERFACE_IS_TYPE);
@@ -466,7 +466,7 @@ bool XPCVariant::VariantDataToJS(nsIVariant* variant, nsresult* pErr,
         return false;
       }
       nsID* v = &iid;
-      return XPCConvert::NativeData2JS(pJSVal, (const void*)&v, {TD_PNSIID},
+      return XPCConvert::NativeData2JS(pJSVal, (const void*)&v, {TD_NSIDPTR},
                                        &iid, 0, pErr);
     }
     case nsIDataType::VTYPE_ASTRING: {
@@ -612,7 +612,7 @@ bool XPCVariant::VariantDataToJS(nsIVariant* variant, nsresult* pErr,
           xptIndex = nsXPTType::Idx::WCHAR;
           break;
         case nsIDataType::VTYPE_ID:
-          xptIndex = nsXPTType::Idx::PNSIID;
+          xptIndex = nsXPTType::Idx::NSIDPTR;
           break;
         case nsIDataType::VTYPE_CHAR_STR:
           xptIndex = nsXPTType::Idx::PSTRING;

diff --git a/js/xpconnect/src/XPCWrappedJSClass.cpp b/js/xpconnect/src/XPCWrappedJSClass.cpp
@@ -559,15 +559,17 @@ bool nsXPCWrappedJSClass::GetInterfaceTypeFromParam(
   } else if (inner.Tag() == nsXPTType::T_INTERFACE_IS) {
     // Get IID from a passed parameter.
     const nsXPTParamInfo& param = method->Param(inner.ArgNum());
-    if (param.Type().Tag() != nsXPTType::T_IID) {
+    if (param.Type().Tag() != nsXPTType::T_NSID &&
+        param.Type().Tag() != nsXPTType::T_NSIDPTR) {
       return false;
     }
 
     void* ptr = nativeParams[inner.ArgNum()].val.p;
 
-    // If the IID is passed indirectly (as an outparam), dereference by an
-    // extra level.
-    if (ptr && param.IsIndirect()) {
+    // If our IID is passed as a pointer outparameter, an extra level of
+    // dereferencing is required.
+    if (ptr && param.Type().Tag() == nsXPTType::T_NSIDPTR &&
+        param.IsIndirect()) {
       ptr = *(nsID**)ptr;
     }
 

diff --git a/js/xpconnect/src/XPCWrappedNative.cpp b/js/xpconnect/src/XPCWrappedNative.cpp
@@ -1262,13 +1262,23 @@ bool CallMethodHelper::GetInterfaceTypeFromParam(const nsXPTType& type,
 
     *result = inner.GetInterface()->IID();
   } else if (inner.Tag() == nsXPTType::T_INTERFACE_IS) {
-    nsID* id = (nsID*)GetDispatchParam(inner.ArgNum())->val.p;
-    if (!id) {
+    const nsXPTCVariant* param = GetDispatchParam(inner.ArgNum());
+    if (param->type.Tag() != nsXPTType::T_NSID &&
+        param->type.Tag() != nsXPTType::T_NSIDPTR) {
+      return Throw(NS_ERROR_UNEXPECTED, mCallContext);
+    }
+
+    const void* ptr = &param->val;
+    if (param->type.Tag() == nsXPTType::T_NSIDPTR) {
+      ptr = *static_cast<nsID* const*>(ptr);
+    }
+
+    if (!ptr) {
       return ThrowBadParam(NS_ERROR_XPC_CANT_GET_PARAM_IFACE_INFO,
                            inner.ArgNum(), mCallContext);
     }
 
-    *result = *id;
+    *result = *static_cast<const nsID*>(ptr);
   }
   return true;
 }
@@ -1499,9 +1509,9 @@ bool CallMethodHelper::ConvertIndependentParam(uint8_t i) {
   // the default value if IsOptional is true.
   if (i >= mArgc) {
     MOZ_ASSERT(paramInfo.IsOptional(), "missing non-optional argument!");
-    if (type.Tag() == nsXPTType::T_IID) {
-      // NOTE: 'const nsIID&' is supported, so it must be allocated.
-      dp->val.p = new nsIID();
+    if (type.Tag() == nsXPTType::T_NSID) {
+      // Use a default value of the null ID for optional NSID objects.
+      dp->ext.nsid.Clear();
       return true;
     }
   }

diff --git a/js/xpconnect/src/xpcprivate.h b/js/xpconnect/src/xpcprivate.h
@@ -2896,7 +2896,7 @@ nsIPrincipal* GetObjectPrincipal(JSObject* obj);
 // a pointer to a value described by the type `nsXPTType`.
 //
 // This method expects a value of the following types:
-//   TD_PNSIID
+//   TD_NSIDPTR
 //     value : nsID* (free)
 //   TD_ASTRING, TD_CSTRING, TD_UTF8STRING
 //     value : ns[C]String* (truncate)

diff --git a/xpcom/ds/nsIVariant.idl b/xpcom/ds/nsIVariant.idl
@@ -33,7 +33,7 @@ struct nsIDataType
         VTYPE_CHAR              = TD_CHAR             ,
         VTYPE_WCHAR             = TD_WCHAR            ,
         VTYPE_VOID              = TD_VOID             ,
-        VTYPE_ID                = TD_PNSIID           ,
+        VTYPE_ID                = TD_NSIDPTR          ,
         VTYPE_CHAR_STR          = TD_PSTRING          ,
         VTYPE_WCHAR_STR         = TD_PWSTRING         ,
         VTYPE_INTERFACE         = TD_INTERFACE_TYPE   ,

diff --git a/xpcom/idl-parser/xpidl/jsonxpt.py b/xpcom/idl-parser/xpidl/jsonxpt.py
@@ -37,7 +37,7 @@
     'wchar':              'TD_WCHAR',
     'wstring':            'TD_PWSTRING',
     # special types
-    'nsid':               'TD_PNSIID',
+    'nsid':               'TD_NSID',
     'astring':            'TD_ASTRING',
     'utf8string':         'TD_UTF8STRING',
     'cstring':            'TD_CSTRING',
@@ -93,7 +93,9 @@ def get_type(type, calltype, iid_is=None, size_is=None):
         }
 
     if isinstance(type, xpidl.Native):
-        if type.specialtype:
+        if type.specialtype == 'nsid' and type.isPtr(calltype):
+            return {'tag': 'TD_NSIDPTR'}
+        elif type.specialtype:
             return {
                 'tag': TypeMap[type.specialtype]
             }

diff --git a/xpcom/idl-parser/xpidl/xpidl.py b/xpcom/idl-parser/xpidl/xpidl.py
@@ -560,20 +560,21 @@ def nativeType(self, calltype, const=False, shared=False):
             const = True
 
         if calltype == 'element':
+            if self.specialtype == 'nsid':
+                if self.isPtr(calltype):
+                    raise IDLError("Array<nsIDPtr> not yet supported. "
+                                   "File an XPConnect bug if you need it.", self.location)
+
+                # ns[CI]?IDs should be held directly in Array<T>s
+                return self.nativename
+
             if self.isRef(calltype):
                 raise IDLError("[ref] qualified type unsupported in Array<T>", self.location)
 
             # Promises should be held in RefPtr<T> in Array<T>s
             if self.specialtype == 'promise':
                 return 'RefPtr<mozilla::dom::Promise>'
 
-            # We don't support nsIDPtr, in Array<T> currently, although
-            # this or support for Array<nsID> will be needed to replace
-            # [array] completely.
-            if self.specialtype == 'nsid':
-                raise IDLError("Array<nsIDPtr> not yet supported. "
-                               "File an XPConnect bug if you need it.", self.location)
-
         if self.isRef(calltype):
             m = '& '  # [ref] is always passed with a single indirection
         else:

diff --git a/xpcom/reflect/xptcall/xptcall.h b/xpcom/reflect/xptcall/xptcall.h
@@ -51,6 +51,7 @@ struct nsXPTCVariant {
     // can be used to store these types when IsIndirect() is true.
     nsXPTCMiniVariant mini;
 
+    nsID nsid;
     nsCString nscstr;
     nsString nsstr;
     JS::Value jsval;

diff --git a/xpcom/reflect/xptinfo/xptcodegen.py b/xpcom/reflect/xptinfo/xptcodegen.py
@@ -171,7 +171,7 @@ def splitint(i):
     {'tag': 'TD_BOOL'},
     {'tag': 'TD_CHAR'},
     {'tag': 'TD_WCHAR'},
-    {'tag': 'TD_PNSIID'},
+    {'tag': 'TD_NSIDPTR'},
     {'tag': 'TD_PSTRING'},
     {'tag': 'TD_PWSTRING'},
     {'tag': 'TD_INTERFACE_IS_TYPE', 'iid_is': 0},

diff --git a/xpcom/reflect/xptinfo/xptinfo.h b/xpcom/reflect/xptinfo/xptinfo.h
@@ -165,7 +165,7 @@ enum nsXPTTypeTag : uint8_t {
   //  - Outparams may be uninitialized by caller,
   //  - Supported in xptcall as raw pointer.
   TD_VOID = 13,
-  TD_PNSIID = 14,
+  TD_NSIDPTR = 14,
   TD_PSTRING = 15,
   TD_PWSTRING = 16,
   TD_INTERFACE_TYPE = 17,
@@ -185,8 +185,9 @@ enum nsXPTTypeTag : uint8_t {
   TD_UTF8STRING = 24,
   TD_CSTRING = 25,
   TD_ASTRING = 26,
-  TD_JSVAL = 27,
-  TD_ARRAY = 28,
+  TD_NSID = 27,
+  TD_JSVAL = 28,
+  TD_ARRAY = 29,
   _TD_LAST_COMPLEX = TD_ARRAY
 };
 
@@ -294,7 +295,7 @@ struct nsXPTType {
     BOOL,
     CHAR,
     WCHAR,
-    PNSIID,
+    NSIDPTR,
     PSTRING,
     PWSTRING,
     INTERFACE_IS_TYPE
@@ -335,7 +336,7 @@ struct nsXPTType {
   TD_ALIAS_(T_CHAR, TD_CHAR);
   TD_ALIAS_(T_WCHAR, TD_WCHAR);
   TD_ALIAS_(T_VOID, TD_VOID);
-  TD_ALIAS_(T_IID, TD_PNSIID);
+  TD_ALIAS_(T_NSIDPTR, TD_NSIDPTR);
   TD_ALIAS_(T_CHAR_STR, TD_PSTRING);
   TD_ALIAS_(T_WCHAR_STR, TD_PWSTRING);
   TD_ALIAS_(T_INTERFACE, TD_INTERFACE_TYPE);
@@ -346,6 +347,7 @@ struct nsXPTType {
   TD_ALIAS_(T_UTF8STRING, TD_UTF8STRING);
   TD_ALIAS_(T_CSTRING, TD_CSTRING);
   TD_ALIAS_(T_ASTRING, TD_ASTRING);
+  TD_ALIAS_(T_NSID, TD_NSID);
   TD_ALIAS_(T_JSVAL, TD_JSVAL);
   TD_ALIAS_(T_DOMOBJECT, TD_DOMOBJECT);
   TD_ALIAS_(T_PROMISE, TD_PROMISE);
@@ -635,7 +637,7 @@ inline const char* GetString(uint32_t aIndex) { return &sStrings[aIndex]; }
 
 #define XPT_FOR_EACH_POINTER_TYPE(MACRO)    \
   MACRO(TD_VOID, void*)                     \
-  MACRO(TD_PNSIID, nsID*)                   \
+  MACRO(TD_NSIDPTR, nsID*)                  \
   MACRO(TD_PSTRING, char*)                  \
   MACRO(TD_PWSTRING, wchar_t*)              \
   MACRO(TD_INTERFACE_TYPE, nsISupports*)    \
@@ -650,6 +652,7 @@ inline const char* GetString(uint32_t aIndex) { return &sStrings[aIndex]; }
   MACRO(TD_UTF8STRING, nsCString)        \
   MACRO(TD_CSTRING, nsCString)           \
   MACRO(TD_ASTRING, nsString)            \
+  MACRO(TD_NSID, nsID)                   \
   MACRO(TD_JSVAL, JS::Value)             \
   MACRO(TD_ARRAY, xpt::detail::UntypedTArray)