Closed
Bug 211174
Opened 21 years ago
Closed 21 years ago
Calling Get() on an nsSupportsHashtable and casting leaks
Categories
(Core :: Security: CAPS, defect)
Core
Security: CAPS
Tracking
()
RESOLVED
FIXED
People
(Reporter: bzbarsky, Assigned: bzbarsky)
References
Details
(Keywords: memory-leak)
Attachments
(1 file)
9.26 KB,
patch
|
caillon
:
review+
benjamin
:
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...
Assignee | ||
Comment 1•21 years ago
|
||
Assignee | ||
Updated•21 years ago
|
Attachment #126793 -
Flags: superreview?(alecf)
Attachment #126793 -
Flags: review?(mstoltz)
Comment 2•21 years ago
|
||
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•21 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•21 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•21 years ago
|
||
> if (mPrinicipals.IsInitialized() && !mPrincipals.Init(31))
oops, that's supposed to read (!mPrinicipals.IsInitialized() &&
!mPrincipals.Init(31))
Comment 6•21 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 7•21 years ago
|
||
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+
Assignee | ||
Updated•21 years ago
|
Summary: [FIX]Calling Get() on an nsSupportsHashtable and casting leaks → Calling Get() on an nsSupportsHashtable and casting leaks
Comment 8•21 years ago
|
||
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.
Description
•