211174 - Calling Get() on an nsSupportsHashtable and casting leaks

Status: RESOLVED FIXED

(Core :: Security: CAPS, defect)

Description

[Boris Zbarsky [:bzbarsky]]

The basic problem is that Get() returns an addrefed pointer.

Attaching patch that converts mPrincipals to an nsInterfaceHashtable, fixing the
leaks...

Comment 1

[Boris Zbarsky [:bzbarsky]]

Attachment: fix

Comment 2

[Mitchell Stoltz (not reading bugmail)]

Comment on attachment 126793 [details] [diff] [review]
fix

Looks OK to me, but caillon is in the process of rewriting some of this code,
so he should review.

Comment 3

[Alec Flett]

Comment on attachment 126793 [details] [diff] [review]
fix

nice cleanup! I wonder if mPrinciples should be an inline class, rather than a
pointer-to-a-class?

sr=alecf either way.

Comment 4

[Benjamin Smedberg]

Comment on attachment 126793 [details] [diff] [review]
fix

>+    typedef nsIPrincipal* KeyTypePointer;

this needs to be const nsIPrincipal*

Wondering about:
>-    nsSupportsHashtable* mPrincipals;
>+    nsInterfaceHashtable<nsIPrincipalKey, nsIPrincipal>* mPrincipals;
and
>         if (!mPrincipals)
>             return NS_ERROR_OUT_OF_MEMORY;
>+        PRBool inited = mPrincipals->Init(31);
>+        if (!inited) {
>+            delete mPrincipals;
>+            mPrincipals = nsnull;
>+            return NS_ERROR_OUT_OF_MEMORY;
>+        }

is mPrincipals normally used? if we can make it a class member instead of a
pointer, you can simplify this down a lot:

/** header */
nsInterfaceHashtable<nsIPrincipalKey, nsIPrincipal> mPrincipals;
/** code */
if (mPrinicipals.IsInitialized() && !mPrincipals.Init(31)) {
    return NS_ERROR_OUT_OF_MEMORY;
}

Comment 5

[Benjamin Smedberg]

> if (mPrinicipals.IsInitialized() && !mPrincipals.Init(31))

oops, that's supposed to read (!mPrinicipals.IsInitialized() &&
!mPrincipals.Init(31))

Comment 6

[Benjamin Smedberg]

Comment on attachment 126793 [details] [diff] [review]
fix

and the attachment-editing page doesn't do mid-air collision detection

Comment 7

[Christopher Aillon (sabbatical, not receiving bugmail)]

Comment on attachment 126793 [details] [diff] [review]
fix

>@@ -412,21 +417,21 @@ private:

>     nsCOMPtr<nsIPrefBranch> mPrefBranch;
>     nsCOMPtr<nsISecurityPref> mSecurityPref;
>     nsIPrincipal* mSystemPrincipal;
>     nsCOMPtr<nsIPrincipal> mSystemCertificate;
>-    nsSupportsHashtable* mPrincipals;
>+    nsInterfaceHashtable<nsIPrincipalKey, nsIPrincipal>* mPrincipals;

I have this chagned to a non pointer in my tree...  your call here.  I can fix
it with your merge conflicts you'll give me, or you can go all out and just
give me extra conflicts to resolve.  I will have conflicts no matter what, so
you may want to just go ahead and do this anyway.


>Index: caps/src/nsScriptSecurityManager.cpp

>@@ -1993,26 +1992,31 @@ nsScriptSecurityManager::SavePrincipal(n
>     nsresult rv;
>     nsCOMPtr<nsIPrincipal> persistent = aToSave;
>     nsCOMPtr<nsIAggregatePrincipal> aggregate(do_QueryInterface(aToSave, &rv));
>     if (NS_SUCCEEDED(rv))
>         if (NS_FAILED(aggregate->GetPrimaryChild(getter_AddRefs(persistent))))
>             return NS_ERROR_FAILURE;
> 
>     //-- Save to mPrincipals
>     if (!mPrincipals)
>     {
>-        mPrincipals = new nsSupportsHashtable(31);
>+        mPrincipals = new nsInterfaceHashtable<nsIPrincipalKey, nsIPrincipal>();

Your call here.  I have this changed to a 

>         if (!mPrincipals)
>             return NS_ERROR_OUT_OF_MEMORY;
>+        PRBool inited = mPrincipals->Init(31);

|inited| does not look right to me... And anyway you probably don't even need
the local var...  just put the fn call in the if.


>+        if (!inited) {
>+            delete mPrincipals;
>+            mPrincipals = nsnull;
>+            return NS_ERROR_OUT_OF_MEMORY;
>+        }



>@@ -3131,26 +3135,31 @@ nsScriptSecurityManager::InitPrincipals(
>                 NS_RELEASE(codebase);
>             }
>         }
>         PR_FREEIF(grantedList);
>         PR_FREEIF(deniedList);
> 
>         if (principal)
>         {
>             if (!mPrincipals)
>             {
>-                mPrincipals = new nsSupportsHashtable(31);
>+                mPrincipals = new nsInterfaceHashtable<nsIPrincipalKey, nsIPrincipal>();
>                 if (!mPrincipals)
>                     return NS_ERROR_OUT_OF_MEMORY;
>+                PRBool inited = mPrincipals->Init(31);

Same as above.

>+                if (!inited) {
>+                    delete mPrincipals;
>+                    mPrincipals = nsnull;
>+                    return NS_ERROR_OUT_OF_MEMORY;
>+                }
>             }

r=caillon

Comment 8

[Christopher Aillon (sabbatical, not receiving bugmail)]

Fixed with checkin to bug 83536.