Calling Get() on an nsSupportsHashtable and casting leaks

RESOLVED FIXED

Status

()

Core
Security: CAPS
RESOLVED FIXED
15 years ago
15 years ago

People

(Reporter: bz, Assigned: bz)

Tracking

({mlk})

Trunk
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment)

fix
9.26 KB, patch
Christopher Aillon (sabbatical, not receiving bugmail)
: review+
Benjamin Smedberg
: superreview+
Details | Diff | Splinter Review
The basic problem is that Get() returns an addrefed pointer.

Attaching patch that converts mPrincipals to an nsInterfaceHashtable, fixing the
leaks...
Created attachment 126793 [details] [diff] [review]
fix
Attachment #126793 - Flags: superreview?(alecf)
Attachment #126793 - Flags: review?(mstoltz)
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.
Attachment #126793 - Flags: review?(mstoltz) → review?(caillon)

Comment 3

15 years ago
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.
Attachment #126793 - Flags: superreview?(alecf) → superreview+

Comment 4

15 years ago
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;
}
Attachment #126793 - Flags: superreview+ → superreview?(alecf)

Comment 5

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

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

Comment 6

15 years ago
Comment on attachment 126793 [details] [diff] [review]
fix

and the attachment-editing page doesn't do mid-air collision detection
Attachment #126793 - Flags: superreview?(alecf) → superreview+
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
Attachment #126793 - Flags: review?(caillon) → review+
Depends on: 83536
Summary: [FIX]Calling Get() on an nsSupportsHashtable and casting leaks → Calling Get() on an nsSupportsHashtable and casting leaks
Fixed with checkin to bug 83536.
Status: NEW → RESOLVED
Last Resolved: 15 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.