Closed Bug 242183 Opened 20 years ago Closed 20 years ago

CreateNewStreamConv and NS_NewStreamConv are silly/leaky/...

Categories

(Core :: Networking, defect)

x86
Windows 2000
defect
Not set
normal

Tracking

()

VERIFIED FIXED

People

(Reporter: timeless, Assigned: timeless)

References

()

Details

Attachments

(1 file)

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?
Attached patch fix silly codeSplinter Review
the second bit is silly because QI will out null by contract
Assignee: darin → timeless
Status: NEW → ASSIGNED
Attachment #147473 - Flags: superreview?(darin)
Attachment #147473 - Flags: review?(darin)
Attachment #147473 - Flags: superreview?(darin)
Attachment #147473 - Flags: superreview+
Attachment #147473 - Flags: review?(darin)
Attachment #147473 - Flags: review+
mozilla/netwerk/streamconv/src/nsStreamConvServiceFactory.cpp 	1.14
mozilla/netwerk/streamconv/src/nsStreamConverterService.cpp 	1.63
Status: ASSIGNED → RESOLVED
Closed: 20 years ago
Resolution: --- → FIXED
V/fixed.
Status: RESOLVED → VERIFIED
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: