Bug 683802 - Coalesce type-specific cleanup indicators. r=mrbkap

content/xslt/src/xpath/txXPCOMExtensionFunction.cpp

@@ -350,11 +350,16 @@ txParamArrayHolder::~txParamArrayHolder()
     PRUint8 i;
     for (i = 0; i < mCount; ++i) {
         nsXPTCVariant &variant = mArray[i];
-        if (variant.IsValInterface()) {
+        if (variant.DoesValNeedCleanup()) {
-            static_cast<nsISupports*>(variant.val.p)->Release();
+            if (variant.type.TagPart() == nsXPTType::T_DOMSTRING)
-        }
+                delete (nsAString*)variant.val.p;
-        else if (variant.IsValDOMString()) {
+            else {
-            delete (nsAString*)variant.val.p;
+                NS_ABORT_IF_FALSE(variant.type.TagPart() == nsXPTType::T_INTERFACE ||
+                                  variant.type.TagPart() == nsXPTType::T_INTERFACE_IS,
+                                  "We only support cleanup of strings and interfaces "
+                                  "here, and this looks like neither!");
+                static_cast<nsISupports*>(variant.val.p)->Release();
+            }
         }
     }
 }
@@ -419,7 +424,7 @@ txXPCOMExtensionFunctionCall::evaluate(txIEvalContext* aContext,
 
         nsXPTCVariant &invokeParam = invokeParams[0];
         invokeParam.type = paramInfo.GetType();
-        invokeParam.SetValIsInterface();
+        invokeParam.SetValNeedsCleanup();
         NS_ADDREF((txIFunctionEvaluationContext*&)invokeParam.val.p = context);
 
         // Skip first argument, since it's the context.
@@ -467,7 +472,7 @@ txXPCOMExtensionFunctionCall::evaluate(txIEvalContext* aContext,
                 rv = adaptor->Init();
                 NS_ENSURE_SUCCESS(rv, rv);
 
-                invokeParam.SetValIsInterface();
+                invokeParam.SetValNeedsCleanup();
                 nodeSet.swap((txINodeSet*&)invokeParam.val.p);
                 break;
             }
@@ -497,7 +502,7 @@ txXPCOMExtensionFunctionCall::evaluate(txIEvalContext* aContext,
                 rv = expr->evaluateToString(aContext, *value);
                 NS_ENSURE_SUCCESS(rv, rv);
 
-                invokeParam.SetValIsDOMString();
+                invokeParam.SetValNeedsCleanup();
                 invokeParam.val.p = value;
                 break;
             }
@@ -513,7 +518,7 @@ txXPCOMExtensionFunctionCall::evaluate(txIEvalContext* aContext,
                   return NS_ERROR_OUT_OF_MEMORY;
               }
 
-              invokeParam.SetValIsInterface();
+              invokeParam.SetValNeedsCleanup();
               adaptor.swap((txIXPathObject*&)invokeParam.val.p);
               break;
             }
@@ -540,13 +545,13 @@ txXPCOMExtensionFunctionCall::evaluate(txIEvalContext* aContext,
             return NS_ERROR_FAILURE;
         }
 
-        returnParam.SetValIsDOMString();
+        returnParam.SetValNeedsCleanup();
         returnParam.val.p = value;
     }
     else {
         returnParam.SetIndirect();
         if (returnType == eNODESET || returnType == eOBJECT) {
-            returnParam.SetValIsInterface();
+            returnParam.SetValNeedsCleanup();
         }
     }
 

js/src/xpconnect/src/xpcvariant.cpp

@@ -555,27 +555,27 @@ XPCVariant::VariantDataToJS(XPCLazyCallContext& lccx,
             if(NS_FAILED(variant->GetAsString((char**)&xpctvar.val.p)))
                 return JS_FALSE;
             xpctvar.type = (uint8)(TD_PSTRING | XPT_TDP_POINTER);
-            xpctvar.SetValIsAllocated();
+            xpctvar.SetValNeedsCleanup();
             break;
         case nsIDataType::VTYPE_STRING_SIZE_IS:
             if(NS_FAILED(variant->GetAsStringWithSize(&size, 
                                                       (char**)&xpctvar.val.p)))
                 return JS_FALSE;
             xpctvar.type = (uint8)(TD_PSTRING_SIZE_IS | XPT_TDP_POINTER);
-            xpctvar.SetValIsAllocated();
+            xpctvar.SetValNeedsCleanup();
             break;
         case nsIDataType::VTYPE_WCHAR_STR:        
             if(NS_FAILED(variant->GetAsWString((PRUnichar**)&xpctvar.val.p)))
                 return JS_FALSE;
             xpctvar.type = (uint8)(TD_PWSTRING | XPT_TDP_POINTER);
-            xpctvar.SetValIsAllocated();
+            xpctvar.SetValNeedsCleanup();
             break;
         case nsIDataType::VTYPE_WSTRING_SIZE_IS:        
             if(NS_FAILED(variant->GetAsWStringWithSize(&size, 
                                                       (PRUnichar**)&xpctvar.val.p)))
                 return JS_FALSE;
             xpctvar.type = (uint8)(TD_PWSTRING_SIZE_IS | XPT_TDP_POINTER);
-            xpctvar.SetValIsAllocated();
+            xpctvar.SetValNeedsCleanup();
             break;
         case nsIDataType::VTYPE_INTERFACE:        
         case nsIDataType::VTYPE_INTERFACE_IS:        
@@ -589,7 +589,7 @@ XPCVariant::VariantDataToJS(XPCLazyCallContext& lccx,
 
             xpctvar.type = (uint8)(TD_INTERFACE_IS_TYPE | XPT_TDP_POINTER);
             if(xpctvar.val.p)
-                xpctvar.SetValIsInterface();
+                xpctvar.SetValNeedsCleanup();
             break;
         }
         case nsIDataType::VTYPE_ARRAY:
@@ -710,10 +710,15 @@ VARIANT_DONE:
                                             &iid, pErr);
     }
 
-    if(xpctvar.IsValAllocated())
+    // We may have done something in the above code that requires cleanup.
-        nsMemory::Free((char*)xpctvar.val.p);
+    if (xpctvar.DoesValNeedCleanup())
-    else if(xpctvar.IsValInterface())
+    {
-        ((nsISupports*)xpctvar.val.p)->Release();
+        if (type == nsIDataType::VTYPE_INTERFACE ||
+            type == nsIDataType::VTYPE_INTERFACE_IS)
+            ((nsISupports*)xpctvar.val.p)->Release();
+        else
+            nsMemory::Free((char*)xpctvar.val.p);
+    }
 
     return success;
 }

js/src/xpconnect/src/xpcwrappednative.cpp

@@ -2240,6 +2240,8 @@ class CallMethodHelper
     JS_ALWAYS_INLINE JSBool ConvertIndependentParam(uint8 i);
     JS_ALWAYS_INLINE JSBool ConvertDependentParams();
 
+    JS_ALWAYS_INLINE void CleanupParam(nsXPTCMiniVariant& param, nsXPTType& type);
+
     JS_ALWAYS_INLINE JSBool HandleDipperParam(nsXPTCVariant* dp,
                                               const nsXPTParamInfo& paramInfo);
 
@@ -2391,69 +2393,49 @@ CallMethodHelper::~CallMethodHelper()
         for(uint8 i = 0; i < paramCount; i++)
         {
             nsXPTCVariant* dp = GetDispatchParam(i);
+            const nsXPTParamInfo& paramInfo = mMethodInfo->GetParam(i);
 
-            if(dp->IsValArray())
+            if(paramInfo.GetType().IsArray())
             {
                 void* p = dp->val.p;
                 if(!p)
                     continue;
 
-                // going to have to cleanup the array and perhaps its contents
+                // Clean up the array contents if necessary.
-                if(dp->IsValAllocated() || dp->IsValInterface())
+                if(dp->DoesValNeedCleanup())
                 {
-                    // we need to figure out how many elements are present.
+                    // We need some basic information to properly destroy the array.
                     JSUint32 array_count;
-
+                    nsXPTType datum_type;
-                    if(!GetArraySizeFromParam(i, &array_count))
+                    if(!GetArraySizeFromParam(i, &array_count) ||
+                       !NS_SUCCEEDED(mIFaceInfo->GetTypeForParam(mVTableIndex,
+                                                                 &paramInfo,
+                                                                 1, &datum_type)))
                     {
-                        NS_ERROR("failed to get array length, we'll leak here");
+                        // XXXbholley - I'm not convinced that the above calls will
+                        // ever fail.
+                        NS_ERROR("failed to get array information, we'll leak here");
                         continue;
                     }
-                    if(dp->IsValAllocated())
+
+                    // Loop over the array contents. For each one, we create a
+                    // dummy 'val' and pass it to the cleanup helper.
+                    for(JSUint32 k = 0; k < array_count; k++)
                     {
-                        void** a = (void**)p;
+                        nsXPTCMiniVariant v;
-                        for(JSUint32 k = 0; k < array_count; k++)
+                        v.val.p = static_cast<void**>(p)[k];
-                        {
+                        CleanupParam(v, datum_type);
-                            void* o = a[k];
-                            if(o) nsMemory::Free(o);
-                        }
-                    }
-                    else // if(dp->IsValInterface())
-                    {
-                        nsISupports** a = (nsISupports**)p;
-                        for(JSUint32 k = 0; k < array_count; k++)
-                        {
-                            nsISupports* o = a[k];
-                            NS_IF_RELEASE(o);
-                        }
                     }
                 }
+
                 // always free the array itself
                 nsMemory::Free(p);
             }
             else
             {
-                if(dp->IsValJSRoot())
+                // Clean up single parameters (if requested).
-                {
+                if (dp->DoesValNeedCleanup())
-                    NS_ASSERTION(!dp->IsValAllocated() && !dp->IsValInterface(),
+                    CleanupParam(*dp, dp->type);
-                                 "jsvals are their own class of values");
-                    JS_RemoveValueRoot(mCallContext, (jsval*)dp->ptr);
-                    continue;
-                }
-
-                void* p = dp->val.p;
-                if(!p)
-                    continue;
-                if(dp->IsValAllocated())
-                    nsMemory::Free(p);
-                else if(dp->IsValInterface())
-                    ((nsISupports*)p)->Release();
-                else if(dp->IsValDOMString())
-                    mCallContext.DeleteString((nsAString*)p);
-                else if(dp->IsValUTF8String())
-                    delete (nsCString*) p;
-                else if(dp->IsValCString())
-                    delete (nsCString*) p;
             }
         }
     }
@@ -2836,7 +2818,7 @@ CallMethodHelper::ConvertIndependentParam(uint8 i)
 
     if(type_tag == nsXPTType::T_INTERFACE)
     {
-        dp->SetValIsInterface();
+        dp->SetValNeedsCleanup();
     }
 
     jsval src;
@@ -2852,7 +2834,7 @@ CallMethodHelper::ConvertIndependentParam(uint8 i)
         dp->val.j = JSVAL_VOID;
         if (!JS_AddValueRoot(mCallContext, &dp->val.j))
             return JS_FALSE;
-        dp->SetValIsJSRoot();
+        dp->SetValNeedsCleanup();
     }
 
     if(paramInfo.IsOut())
@@ -2862,7 +2844,7 @@ CallMethodHelper::ConvertIndependentParam(uint8 i)
            !paramInfo.IsShared())
         {
             useAllocator = JS_TRUE;
-            dp->SetValIsAllocated();
+            dp->SetValNeedsCleanup();
         }
 
         if(!paramInfo.IsIn())
@@ -2875,11 +2857,11 @@ CallMethodHelper::ConvertIndependentParam(uint8 i)
             switch(type_tag)
             {
             case nsXPTType::T_IID:
-                dp->SetValIsAllocated();
+                dp->SetValNeedsCleanup();
                 useAllocator = JS_TRUE;
                 break;
             case nsXPTType::T_CHAR_STR:
-                dp->SetValIsAllocated();
+                dp->SetValNeedsCleanup();
                 useAllocator = JS_TRUE;
                 break;
             case nsXPTType::T_ASTRING:
@@ -2890,14 +2872,14 @@ CallMethodHelper::ConvertIndependentParam(uint8 i)
                 // Is an 'in' DOMString. Set 'useAllocator' to indicate
                 // that JSData2Native should allocate a new
                 // nsAString.
-                dp->SetValIsDOMString();
+                dp->SetValNeedsCleanup();
                 useAllocator = JS_TRUE;
                 break;
 
             case nsXPTType::T_UTF8STRING:
                 // Fall through to the C string case for now...
             case nsXPTType::T_CSTRING:
-                dp->SetValIsCString();
+                dp->SetValNeedsCleanup();
                 useAllocator = JS_TRUE;
                 break;
             }
@@ -2968,8 +2950,6 @@ CallMethodHelper::ConvertDependentParams()
 
         if(isArray)
         {
-            dp->SetValIsArray();
-
             if(NS_FAILED(mIFaceInfo->GetTypeForParam(mVTableIndex, &paramInfo, 1,
                                                      &datum_type)))
             {
@@ -2982,7 +2962,7 @@ CallMethodHelper::ConvertDependentParams()
 
         if(datum_type.IsInterfacePointer())
         {
-            dp->SetValIsInterface();
+            dp->SetValNeedsCleanup();
         }
 
         jsval src;
@@ -2997,7 +2977,7 @@ CallMethodHelper::ConvertDependentParams()
                (isArray || !paramInfo.IsShared()))
             {
                 useAllocator = JS_TRUE;
-                dp->SetValIsAllocated();
+                dp->SetValNeedsCleanup();
             }
 
             if(!paramInfo.IsIn())
@@ -3015,7 +2995,7 @@ CallMethodHelper::ConvertDependentParams()
                (isArray && datum_type.TagPart() == nsXPTType::T_CHAR_STR))
             {
                 useAllocator = JS_TRUE;
-                dp->SetValIsAllocated();
+                dp->SetValNeedsCleanup();
             }
         }
 
@@ -3075,6 +3055,45 @@ CallMethodHelper::ConvertDependentParams()
     return JS_TRUE;
 }
 
+// Performs all necessary teardown on a parameter after method invocation.
+//
+// This method should only be called if the value in question was flagged
+// for cleanup (ie, if dp->DoesValNeedCleanup()).
+void
+CallMethodHelper::CleanupParam(nsXPTCMiniVariant& param, nsXPTType& type)
+{
+    // We handle array elements, but not the arrays themselves.
+    NS_ABORT_IF_FALSE(type.TagPart() != nsXPTType::T_ARRAY, "Can't handle arrays.");
+
+    // Pointers may sometimes be null even if cleanup was requested. Combine
+    // the null checking for all the different types into one check here.
+    if (type.TagPart() != nsXPTType::T_JSVAL && param.val.p == nsnull)
+        return;
+
+    switch(type.TagPart())
+    {
+        case nsXPTType::T_JSVAL:
+            JS_RemoveValueRoot(mCallContext, (jsval*)&param.val);
+            break;
+        case nsXPTType::T_INTERFACE:
+        case nsXPTType::T_INTERFACE_IS:
+            ((nsISupports*)param.val.p)->Release();
+            break;
+        case nsXPTType::T_ASTRING:
+        case nsXPTType::T_DOMSTRING:
+            mCallContext.DeleteString((nsAString*)param.val.p);
+            break;
+        case nsXPTType::T_UTF8STRING:
+        case nsXPTType::T_CSTRING:
+            delete (nsCString*) param.val.p;
+            break;
+        default:
+            NS_ABORT_IF_FALSE(type.IsPointer(), "Cleanup requested on unexpected type.");
+            nsMemory::Free(param.val.p);
+            break;
+    }
+}
+
 // Handle parameters with dipper types.
 //
 // Dipper types are one of the more inscrutable aspects of xpidl. In a
@@ -3117,15 +3136,9 @@ CallMethodHelper::HandleDipperParam(nsXPTCVariant* dp,
     // ASTRING and DOMSTRING are very similar, and both use nsAutoString.
     // UTF8_STRING and CSTRING are also quite similar, and both use nsCString.
     if(type_tag == nsXPTType::T_ASTRING || type_tag == nsXPTType::T_DOMSTRING)
-    {
-        dp->SetValIsDOMString();
         dp->val.p = new nsAutoString();
-    }
     else
-    {
-        dp->SetValIsCString();
         dp->val.p = new nsCString();
-    }
 
     // Check for OOM, in either case.
     if(!dp->val.p)
@@ -3134,6 +3147,9 @@ CallMethodHelper::HandleDipperParam(nsXPTCVariant* dp,
         return JS_FALSE;
     }
 
+    // We allocated, so we need to deallocate after the method call completes.
+    dp->SetValNeedsCleanup();
+
     return JS_TRUE;
 }
 

xpcom/reflect/xptcall/public/xptcall.h

@@ -114,33 +114,21 @@ struct nsXPTCVariant : public nsXPTCMiniVariant
         // the way they are.
         PTR_IS_DATA    = 0x1,
 
-        VAL_IS_ALLOCD  = 0x2,  // val.p holds alloc'd ptr that must be freed
+        // Indicates that the value we hold requires some sort of cleanup (memory
-        VAL_IS_IFACE   = 0x4,  // val.p holds interface ptr that must be released
+        // deallocation, interface release, jsval unrooting, etc). The precise
-        VAL_IS_ARRAY   = 0x8,  // val.p holds a pointer to an array needing cleanup
+        // cleanup that is performed depends on the 'type' field above.
-        VAL_IS_DOMSTR  = 0x10, // val.p holds a pointer to domstring needing cleanup
+        // If the value is an array, this flag specifies whether the elements
-        VAL_IS_UTF8STR = 0x20, // val.p holds a pointer to utf8string needing cleanup
+        // within the array require cleanup (we always clean up the array itself,
-        VAL_IS_CSTR    = 0x40, // val.p holds a pointer to cstring needing cleanup
+        // so this flag would be redundant for that purpose).
-        VAL_IS_JSROOT  = 0x80  // val.p holds a pointer to a jsval that must be unrooted
+        VAL_NEEDS_CLEANUP = 0x2
     };
 
     void ClearFlags()         {flags = 0;}
     void SetIndirect()        {ptr = &val; flags |= PTR_IS_DATA;}
-    void SetValIsAllocated()  {flags |= VAL_IS_ALLOCD;}
+    void SetValNeedsCleanup() {flags |= VAL_NEEDS_CLEANUP;}
-    void SetValIsInterface()  {flags |= VAL_IS_IFACE;}
-    void SetValIsArray()      {flags |= VAL_IS_ARRAY;}
-    void SetValIsDOMString()  {flags |= VAL_IS_DOMSTR;}
-    void SetValIsUTF8String() {flags |= VAL_IS_UTF8STR;}
-    void SetValIsCString()    {flags |= VAL_IS_CSTR;}
-    void SetValIsJSRoot()     {flags |= VAL_IS_JSROOT;}
 
-    PRBool IsIndirect()      const  {return 0 != (flags & PTR_IS_DATA);}
+    PRBool IsIndirect()         const  {return 0 != (flags & PTR_IS_DATA);}
-    PRBool IsValAllocated()  const  {return 0 != (flags & VAL_IS_ALLOCD);}
+    PRBool DoesValNeedCleanup() const  {return 0 != (flags & VAL_NEEDS_CLEANUP);}
-    PRBool IsValInterface()  const  {return 0 != (flags & VAL_IS_IFACE);}
-    PRBool IsValArray()      const  {return 0 != (flags & VAL_IS_ARRAY);}
-    PRBool IsValDOMString()  const  {return 0 != (flags & VAL_IS_DOMSTR);}
-    PRBool IsValUTF8String() const  {return 0 != (flags & VAL_IS_UTF8STR);}
-    PRBool IsValCString()    const  {return 0 != (flags & VAL_IS_CSTR);}    
-    PRBool IsValJSRoot()     const  {return 0 != (flags & VAL_IS_JSROOT);}
 
     // Internal use only. Use IsIndirect() instead.
     PRBool IsPtrData()       const  {return 0 != (flags & PTR_IS_DATA);}