Closed Bug 211174 Opened 21 years ago Closed 21 years ago

Calling Get() on an nsSupportsHashtable and casting leaks

Categories

(Core :: Security: CAPS, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED

People

(Reporter: bzbarsky, Assigned: bzbarsky)

References

Details

(Keywords: memory-leak)

Attachments

(1 file)

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

Attaching patch that converts mPrincipals to an nsInterfaceHashtable, fixing the
leaks...
Attached patch fixSplinter Review
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 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 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)
> if (mPrinicipals.IsInitialized() && !mPrincipals.Init(31))

oops, that's supposed to read (!mPrinicipals.IsInitialized() &&
!mPrincipals.Init(31))
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+
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
Closed: 21 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: