CreateNewStreamConv and NS_NewStreamConv are silly/leaky/...

VERIFIED FIXED

Status

()

Core
Networking
VERIFIED FIXED
14 years ago
14 years ago

People

(Reporter: timeless, Assigned: timeless)

Tracking

Trunk
x86
Windows 2000
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

(URL)

Attachments

(1 attachment)

2.06 KB, patch
Darin Fisher
: review+
Darin Fisher
: superreview+
Details | Diff | Splinter Review
(Assignee)

Description

14 years ago
72 static NS_IMETHODIMP                 
 73 CreateNewStreamConv(nsISupports* aOuter, REFNSIID aIID, void **aResult) 
 74 {
 75     if (!aResult) {                                                  
 76         return NS_ERROR_INVALID_POINTER;                             
 77     }
 78     if (aOuter) {                                                    
 79         *aResult = nsnull;                                           
 80         return NS_ERROR_NO_AGGREGATION;                              
 81     }   
 82     nsStreamConverterService* inst = nsnull;
 83     nsresult rv = NS_NewStreamConv(&inst);
this block leaks because of the very messed up ownership (or lack thereof in
NS_NewStreamConv):
 84     if (NS_FAILED(rv)) {                                             
 85         *aResult = nsnull;                                           
 86         return rv;                                                   
 87     } 
/block
 88     rv = inst->QueryInterface(aIID, aResult);
I don't see a reason for this block:
 89     if (NS_FAILED(rv)) {                                             
 90         *aResult = nsnull;                                           
 91     }                                                                
/block
 92     NS_RELEASE(inst);             /* get rid of extra refcnt */      
 93     return rv;              
 94 }



690 NS_NewStreamConv(nsStreamConverterService** aStreamConv)
691 {
692     NS_PRECONDITION(aStreamConv != nsnull, "null ptr");
693     if (!aStreamConv) return NS_ERROR_NULL_POINTER;
694 
695     *aStreamConv = new nsStreamConverterService();
696     if (!*aStreamConv) return NS_ERROR_OUT_OF_MEMORY;
697 
698     NS_ADDREF(*aStreamConv);
This returns failure for some OOM case:
699     return (*aStreamConv)->Init();
But what it should do is hold rv, and release *aStreamConv on failure.
700 }
> 83     nsresult rv = NS_NewStreamConv(&inst);
>this block leaks because of the very messed up ownership (or lack thereof in
>NS_NewStreamConv):

that is NS_NewStreamConv's bug, not the one of this function.

>I don't see a reason for this block:
> 89     if (NS_FAILED(rv)) {                                             
> 90         *aResult = nsnull;                                           
> 91     }

To make sure that the caller gets a null result when the operation failed?
(Assignee)

Comment 2

14 years ago
Created attachment 147473 [details] [diff] [review]
fix silly code

the second bit is silly because QI will out null by contract
(Assignee)

Updated

14 years ago
Assignee: darin → timeless
Status: NEW → ASSIGNED
(Assignee)

Updated

14 years ago
Attachment #147473 - Flags: superreview?(darin)
Attachment #147473 - Flags: review?(darin)

Updated

14 years ago
Attachment #147473 - Flags: superreview?(darin)
Attachment #147473 - Flags: superreview+
Attachment #147473 - Flags: review?(darin)
Attachment #147473 - Flags: review+
(Assignee)

Comment 3

14 years ago
mozilla/netwerk/streamconv/src/nsStreamConvServiceFactory.cpp 	1.14
mozilla/netwerk/streamconv/src/nsStreamConverterService.cpp 	1.63
Status: ASSIGNED → RESOLVED
Last Resolved: 14 years ago
Resolution: --- → FIXED

Comment 4

14 years ago
V/fixed.
Status: RESOLVED → VERIFIED
You need to log in before you can comment on or make changes to this bug.