242183 - CreateNewStreamConv and NS_NewStreamConv are silly/leaky/...

Status: VERIFIED FIXED

(Core :: Networking, defect)

Description

[timeless]

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 }

Comment 1

[Christian :Biesinger (don't email me, ping me on IRC)]

> 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?

Comment 2

[timeless]

Attachment: fix silly code

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

Comment 3

[timeless]

mozilla/netwerk/streamconv/src/nsStreamConvServiceFactory.cpp 	1.14
mozilla/netwerk/streamconv/src/nsStreamConverterService.cpp 	1.63

Comment 4

[benc]

V/fixed.