Bug 1475409 - Part 3: Make the different categories of types in xptinfo more explicit, r=mccr8
☠☠ backed out by e4f654755cc5 ☠ ☠
authorNika Layzell <nika@thelayzells.com>
Wed, 11 Jul 2018 14:51:32 -0400
changeset 429251 9817819b7765b5bcb115c1a5537e9deff48c4ace
parent 429250 39fcebfe65295fa2f84e8e59734f38fa778bca20
child 429252 13c9626970e23b824edd496c4e56261724dc750a
push id67094
push userccoroiu@mozilla.com
push dateMon, 30 Jul 2018 22:02:32 +0000
treeherderautoland@397b4d841690 [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewersmccr8
bugs1475409
milestone63.0a1
first release with
nightly linux32
nightly linux64
nightly mac
nightly win32
nightly win64
last release without
nightly linux32
nightly linux64
nightly mac
nightly win32
nightly win64
Bug 1475409 - Part 3: Make the different categories of types in xptinfo more explicit, r=mccr8 Summary: This should make it more clear which types have which behaviours, and should make it easier to add new types without forgetting to handle a special case somewhere. Depends On D2114 Reviewers: mccr8! Tags: #secure-revision Bug #: 1475409 Differential Revision: https://phabricator.services.mozilla.com/D2115
js/xpconnect/src/XPCConvert.cpp
js/xpconnect/src/XPCInlines.h
js/xpconnect/src/XPCWrappedJSClass.cpp
xpcom/reflect/xptinfo/xptinfo.h
--- a/js/xpconnect/src/XPCConvert.cpp
+++ b/js/xpconnect/src/XPCConvert.cpp
@@ -1632,19 +1632,19 @@ xpc::InnerCleanupValue(const nsXPTType& 
         // Non-arithmetic types requiring no cleanup
         case nsXPTType::T_VOID:
             break;
 
         default:
             MOZ_CRASH("Unknown Type!");
     }
 
-    // Null out the pointer if we have it.
-    if (aType.HasPointerRepr()) {
-        *(void**)aValue = nullptr;
+    // Clear any non-complex values to the valid '0' state.
+    if (!aType.IsComplex()) {
+        aType.ZeroValue(aValue);
     }
 }
 
 /***************************************************************************/
 
 // Implementation of xpc::InitializeValue.
 
 void
@@ -1667,12 +1667,12 @@ xpc::InitializeValue(const nsXPTType& aT
             break;
 
         case nsXPTType::T_SEQUENCE:
             new (aValue) xpt::detail::UntypedSequence();
             break;
 
         // The remaining types all have valid states where all bytes are '0'.
         default:
-            memset(aValue, 0, aType.Stride());
+            aType.ZeroValue(aValue);
             break;
     }
 }
--- a/js/xpconnect/src/XPCInlines.h
+++ b/js/xpconnect/src/XPCInlines.h
@@ -525,17 +525,20 @@ void ThrowBadResult(nsresult result, XPC
 inline void
 xpc::CleanupValue(const nsXPTType& aType,
                   void* aValue,
                   uint32_t aArrayLen)
 {
     // Check if we can do a cheap early return, and only perform the inner call
     // if we can't. We never have to clean up null pointer types or arithmetic
     // types.
-    if (aType.IsArithmetic() || (aType.HasPointerRepr() && !*(void**)aValue)) {
+    //
+    // NOTE: We can skip zeroing arithmetic types in CleanupValue, as they are
+    // already in a valid state.
+    if (aType.IsArithmetic() || (aType.IsPointer() && !*(void**)aValue)) {
         return;
     }
     xpc::InnerCleanupValue(aType, aValue, aArrayLen);
 }
 
 /***************************************************************************/
 
 #endif /* xpcinlines_h___ */
--- a/js/xpconnect/src/XPCWrappedJSClass.cpp
+++ b/js/xpconnect/src/XPCWrappedJSClass.cpp
@@ -744,35 +744,37 @@ nsXPCWrappedJSClass::CleanupOutparams(co
                                       bool inOutOnly, uint8_t count) const
 {
     // clean up any 'out' params handed in
     for (uint8_t i = 0; i < count; i++) {
         const nsXPTParamInfo& param = info->GetParam(i);
         if (!param.IsOut())
             continue;
 
-        // Extract the array length so we can use it in CleanupValue.
-        uint32_t arrayLen = 0;
-        if (!GetArraySizeFromParam(info, param.Type(), nativeParams, &arrayLen))
-            continue;
-
         MOZ_ASSERT(param.IsIndirect(), "Outparams are always indirect");
 
-        // The inOutOnly flag is necessary because full outparams may contain
-        // uninitialized junk before the call is made, and we don't want to try
-        // to clean up uninitialized junk.
-        if (!inOutOnly || param.IsIn()) {
+        // Call 'CleanupValue' on parameters which we know to be initialized:
+        //  1. Complex parameters (initialized by caller)
+        //  2. 'inout' parameters (initialized by caller)
+        //  3. 'out' parameters when 'inOutOnly' is 'false' (initialized by us)
+        //
+        // We skip non-complex 'out' parameters before the call, as they may
+        // contain random junk.
+        if (param.Type().IsComplex() || param.IsIn() || !inOutOnly) {
+            uint32_t arrayLen = 0;
+            if (!GetArraySizeFromParam(info, param.Type(), nativeParams, &arrayLen))
+                continue;
+
             xpc::CleanupValue(param.Type(), nativeParams[i].val.p, arrayLen);
         }
 
-        // Even if we didn't call CleanupValue, null out any pointers. This is
-        // just to protect C++ callers which may read garbage if they forget to
-        // check the error value.
-        if (param.Type().HasPointerRepr()) {
-            *(void**)nativeParams[i].val.p = nullptr;
+        // Ensure our parameters are in a clean state. Complex values are always
+        // handled by CleanupValue, and others have a valid null representation.
+        if (!param.Type().IsComplex()) {
+            param.Type().ZeroValue(nativeParams[i].val.p);
         }
     }
 }
 
 nsresult
 nsXPCWrappedJSClass::CheckForException(XPCCallContext & ccx,
                                        AutoEntryScript& aes,
                                        const char * aPropertyName,
--- a/xpcom/reflect/xptinfo/xptinfo.h
+++ b/xpcom/reflect/xptinfo/xptinfo.h
@@ -149,50 +149,72 @@ static_assert(sizeof(nsXPTInterfaceInfo)
  * The following enum represents contains the different tag types which
  * can be found in nsXPTTypeInfo::mTag.
  *
  * WARNING: mTag is 5 bits wide, supporting at most 32 tags.
  */
 enum nsXPTTypeTag : uint8_t
 {
   // Arithmetic (POD) Types
+  //  - Do not require cleanup,
+  //  - All bit patterns are valid,
+  //  - Outparams may be uninitialized by caller,
+  //  - Directly supported in xptcall.
+  //
+  // NOTE: The name 'Arithmetic' comes from Harbison/Steele. Despite being a tad
+  // unclear, it is used frequently in xptcall, so is unlikely to be changed.
   TD_INT8              = 0,
   TD_INT16             = 1,
   TD_INT32             = 2,
   TD_INT64             = 3,
   TD_UINT8             = 4,
   TD_UINT16            = 5,
   TD_UINT32            = 6,
   TD_UINT64            = 7,
   TD_FLOAT             = 8,
   TD_DOUBLE            = 9,
   TD_BOOL              = 10,
   TD_CHAR              = 11,
   TD_WCHAR             = 12,
+  _TD_LAST_ARITHMETIC  = TD_WCHAR,
 
-  // Non-Arithmetic Types
+  // Pointer Types
+  //  - Require cleanup unless NULL,
+  //  - All-zeros (NULL) bit pattern is valid,
+  //  - Outparams may be uninitialized by caller,
+  //  - Supported in xptcall as raw pointer.
   TD_VOID              = 13,
   TD_PNSIID            = 14,
-  TD_DOMSTRING         = 15,
-  TD_PSTRING           = 16,
-  TD_PWSTRING          = 17,
-  TD_INTERFACE_TYPE    = 18,
-  TD_INTERFACE_IS_TYPE = 19,
-  TD_ARRAY             = 20,
-  TD_PSTRING_SIZE_IS   = 21,
-  TD_PWSTRING_SIZE_IS  = 22,
-  TD_UTF8STRING        = 23,
-  TD_CSTRING           = 24,
-  TD_ASTRING           = 25,
-  TD_JSVAL             = 26,
-  TD_DOMOBJECT         = 27,
-  TD_PROMISE           = 28,
-  TD_SEQUENCE          = 29
+  TD_PSTRING           = 15,
+  TD_PWSTRING          = 16,
+  TD_INTERFACE_TYPE    = 17,
+  TD_INTERFACE_IS_TYPE = 18,
+  TD_ARRAY             = 19,
+  TD_PSTRING_SIZE_IS   = 20,
+  TD_PWSTRING_SIZE_IS  = 21,
+  TD_DOMOBJECT         = 22,
+  TD_PROMISE           = 23,
+  _TD_LAST_POINTER     = TD_PROMISE,
+
+  // Complex Types
+  //  - Require cleanup,
+  //  - Always passed indirectly,
+  //  - Outparams must be initialized by caller,
+  //  - Supported in xptcall due to indirection.
+  TD_DOMSTRING         = 24,
+  TD_UTF8STRING        = 25,
+  TD_CSTRING           = 26,
+  TD_ASTRING           = 27,
+  TD_JSVAL             = 28,
+  TD_SEQUENCE          = 29,
+  _TD_LAST_COMPLEX     = TD_SEQUENCE
 };
 
+static_assert(_TD_LAST_COMPLEX < 32, "nsXPTTypeTag must fit in 5 bits");
+
 
 /*
  * A nsXPTType is a union used to identify the type of a method argument or
  * return value. The internal data is stored as an 5-bit tag, and two 8-bit
  * integers, to keep alignment requirements low.
  *
  * nsXPTType contains 3 extra bits, reserved for use by nsXPTParamInfo.
  */
@@ -234,60 +256,54 @@ public:
     return xpt::detail::GetInterface(Data16());
   }
 
   const nsXPTDOMObjectInfo& GetDOMObjectInfo() const {
     MOZ_ASSERT(Tag() == TD_DOMOBJECT);
     return xpt::detail::GetDOMObjectInfo(Data16());
   }
 
-  // 'Arithmetic' here roughly means that the value is self-contained and
-  // doesn't depend on anything else in memory (ie: not a pointer, not an
-  // XPCOM object, not a jsval, etc).
-  //
-  // Supposedly this terminology comes from Harbison/Steele, but it's still
-  // a rather crappy name. We'd change it if it wasn't used all over the
-  // place in xptcall. :-(
-  bool IsArithmetic() const { return Tag() <= TD_WCHAR; }
+  // See the comments in nsXPTTypeTag for an explanation as to what each of
+  // these categories mean.
+  bool IsArithmetic() const { return Tag() <= _TD_LAST_ARITHMETIC; }
+  bool IsPointer() const { return !IsArithmetic() && Tag() <= _TD_LAST_POINTER; }
+  bool IsComplex() const { return Tag() > _TD_LAST_POINTER; }
 
   bool IsInterfacePointer() const {
     return Tag() == TD_INTERFACE_TYPE || Tag() == TD_INTERFACE_IS_TYPE;
   }
 
-  bool IsArray() const { return Tag() == TD_ARRAY; }
-
   bool IsDependent() const {
     return (Tag() == TD_SEQUENCE && InnermostType().IsDependent()) ||
            Tag() == TD_INTERFACE_IS_TYPE || Tag() == TD_ARRAY ||
            Tag() == TD_PSTRING_SIZE_IS || Tag() == TD_PWSTRING_SIZE_IS;
   }
 
-  bool IsAlwaysIndirect() const {
-    return Tag() == TD_ASTRING || Tag() == TD_DOMSTRING ||
-           Tag() == TD_CSTRING || Tag() == TD_UTF8STRING ||
-           Tag() == TD_JSVAL || Tag() == TD_SEQUENCE;
-  }
-
   // Unwrap a nested type to its innermost value (e.g. through arrays).
   const nsXPTType& InnermostType() const {
     if (Tag() == TD_ARRAY || Tag() == TD_SEQUENCE) {
       return ArrayElementType().InnermostType();
     }
     return *this;
   }
 
-  // Helper methods for working with the type's native representation.
+  // In-memory size of native type in bytes.
   inline size_t Stride() const;
-  inline bool HasPointerRepr() const;
 
   // Offset the given base pointer to reference the element at the given index.
   void* ElementPtr(const void* aBase, uint32_t aIndex) const {
     return (char*)aBase + (aIndex * Stride());
   }
 
+  // Zero out a native value of the given type. The type must not be 'complex'.
+  void ZeroValue(void* aValue) const {
+    MOZ_RELEASE_ASSERT(!IsComplex(), "Cannot zero a complex value");
+    memset(aValue, 0, Stride());
+  }
+
   // Indexes into the extra types array of a small set of known types.
   enum class Idx : uint8_t
   {
     INT8 = 0,
     UINT8,
     INT16,
     UINT16,
     INT32,
@@ -389,20 +405,20 @@ struct nsXPTParamInfo
   bool IsOptional() const { return mType.mOptionalParam; }
   bool IsShared() const { return false; } // XXX remove (backcompat)
 
   // Get the type of this parameter.
   const nsXPTType& Type() const { return mType; }
   const nsXPTType& GetType() const { return Type(); } // XXX remove (backcompat)
 
   // Whether this parameter is passed indirectly on the stack. All out/inout
-  // params are passed indirectly, although some types are passed indirectly
-  // unconditionally.
+  // params are passed indirectly, and complex types are always passed
+  // indirectly.
   bool IsIndirect() const {
-    return IsOut() || Type().IsAlwaysIndirect();
+    return IsOut() || Type().IsComplex();
   }
 
   ////////////////////////////////////////////////////////////////
   // Ensure these fields are in the same order as xptcodegen.py //
   ////////////////////////////////////////////////////////////////
 
   nsXPTType mType;
 };
@@ -629,39 +645,16 @@ inline const char*
 GetString(uint32_t aIndex)
 {
   return &sStrings[aIndex];
 }
 
 } // namespace detail
 } // namespace xpt
 
-inline bool
-nsXPTType::HasPointerRepr() const
-{
-  // This method should return `true` if the given type would be represented as
-  // a pointer when not passed indirectly.
-  switch (Tag()) {
-    case TD_VOID:
-    case TD_PNSIID:
-    case TD_PSTRING:
-    case TD_PWSTRING:
-    case TD_INTERFACE_TYPE:
-    case TD_INTERFACE_IS_TYPE:
-    case TD_ARRAY:
-    case TD_PSTRING_SIZE_IS:
-    case TD_PWSTRING_SIZE_IS:
-    case TD_DOMOBJECT:
-    case TD_PROMISE:
-        return true;
-    default:
-        return false;
-  }
-}
-
 inline size_t
 nsXPTType::Stride() const
 {
   // Compute the stride to use when walking an array of the given type.
   switch (Tag()) {
     case TD_INT8:              return sizeof(int8_t);
     case TD_INT16:             return sizeof(int16_t);
     case TD_INT32:             return sizeof(int32_t);
@@ -670,31 +663,33 @@ nsXPTType::Stride() const
     case TD_UINT16:            return sizeof(uint16_t);
     case TD_UINT32:            return sizeof(uint32_t);
     case TD_UINT64:            return sizeof(uint64_t);
     case TD_FLOAT:             return sizeof(float);
     case TD_DOUBLE:            return sizeof(double);
     case TD_BOOL:              return sizeof(bool);
     case TD_CHAR:              return sizeof(char);
     case TD_WCHAR:             return sizeof(char16_t);
+
     case TD_VOID:              return sizeof(void*);
     case TD_PNSIID:            return sizeof(nsIID*);
     case TD_DOMSTRING:         return sizeof(nsString);
     case TD_PSTRING:           return sizeof(char*);
     case TD_PWSTRING:          return sizeof(char16_t*);
     case TD_INTERFACE_TYPE:    return sizeof(nsISupports*);
     case TD_INTERFACE_IS_TYPE: return sizeof(nsISupports*);
     case TD_ARRAY:             return sizeof(void*);
     case TD_PSTRING_SIZE_IS:   return sizeof(char*);
     case TD_PWSTRING_SIZE_IS:  return sizeof(char16_t*);
+    case TD_DOMOBJECT:         return sizeof(void*);
+    case TD_PROMISE:           return sizeof(void*);
+
     case TD_UTF8STRING:        return sizeof(nsCString);
     case TD_CSTRING:           return sizeof(nsCString);
     case TD_ASTRING:           return sizeof(nsString);
     case TD_JSVAL:             return sizeof(JS::Value);
-    case TD_DOMOBJECT:         return sizeof(void*);
-    case TD_PROMISE:           return sizeof(void*);
     case TD_SEQUENCE:          return sizeof(xpt::detail::UntypedSequence);
   }
 
   MOZ_CRASH("Unknown type");
 }
 
 #endif /* xptinfo_h */