Closed Bug 617965 Opened 9 years ago Closed 9 years ago

IPC::ParamTraits<nsIDOMGeoPosition*>::Read leaks GeoPositionCoords when ReadParam(... address) fails

Categories

(Core :: DOM: Geolocation, defect)

defect
Not set

Tracking

()

VERIFIED FIXED
Tracking Status
fennec 2.0+ ---

People

(Reporter: timeless, Unassigned)

References

(Blocks 1 open bug, )

Details

(Keywords: coverity, memory-leak)

Attachments

(1 file, 1 obsolete file)

46 typedef nsIDOMGeoPositionCoords   *GeoPositionCoords;
47 typedef nsIDOMGeoPosition         *GeoPosition;

225 struct ParamTraits<GeoPosition>
226 {
227   typedef GeoPosition paramType;
228 

252   // Function to de-serialize a geoposition
253   static bool Read(const Message* aMsg, void **aIter, paramType* aResult)
254   {

264     DOMTimeStamp timeStamp;
265     GeoPositionCoords coords;
266     GeoPositionAddress address;
267 
268     // It's not important to us where it fails, but rather if it fails
269     if (!(   ReadParam(aMsg, aIter, &timeStamp)
270           && ReadParam(aMsg, aIter, &coords   )
you leak coords when ReadParam address fails, so your comment about where not being important is inaccurate.
271           && ReadParam(aMsg, aIter, &address  ))) return false;
272 
273     *aResult = new nsGeoPosition(coords, address, timeStamp);
mike, can you take a look?
note that the code also tries to use nsRefPtr<nsIFoo> if nsIFoo is a typical nsISupports, this is probably wrong -- it should be using nsCOMPtr<nsIFoo>...
(In reply to comment #0)
> 264     DOMTimeStamp timeStamp;
> 265     GeoPositionCoords coords;
> 266     GeoPositionAddress address;
> 267 
> 268     // It's not important to us where it fails, but rather if it fails
> 269     if (!(   ReadParam(aMsg, aIter, &timeStamp)
> 270           && ReadParam(aMsg, aIter, &coords   )
> you leak coords when ReadParam address fails, so your comment about where not
> being important is inaccurate.

How/what would the coords instance leak in that case? - it is an automatic variable?
Attached patch nsCOMPtr instead of nsRefPtr (obsolete) — Splinter Review
Attachment #497518 - Flags: review?(doug.turner)
(In reply to comment #3)
> (In reply to comment #0)
> > 264     DOMTimeStamp timeStamp;
> > 265     GeoPositionCoords coords;
> > 266     GeoPositionAddress address;
> > 267 
> > 268     // It's not important to us where it fails, but rather if it fails
> > 269     if (!(   ReadParam(aMsg, aIter, &timeStamp)
> > 270           && ReadParam(aMsg, aIter, &coords   )
> > you leak coords when ReadParam address fails, so your comment about where not
> > being important is inaccurate.
> 
> How/what would the coords instance leak in that case? - it is an automatic
> variable?

GeoPositionCoords/GeoPositionAddress are typedefs for nsIDOMGeoPositionCoords* and nsIDOMGeoPositionAddress*, and their Read functions assign new objects into them.
Attached patch Leak handled tooSplinter Review
I see the problem now, thanks for pointing it out :)
Attachment #497518 - Attachment is obsolete: true
Attachment #497762 - Flags: review?(doug.turner)
Attachment #497518 - Flags: review?(doug.turner)
yeah, that should work.

the reason i punted on fixing it was in fact that part, i don't like the typedefs and didn't want to be responsible for asking to change them.
Comment on attachment 497762 [details] [diff] [review]
Leak handled too

bent, i think you reviewed this at one point.  is this an okay change?
Attachment #497762 - Flags: review?(doug.turner)
Attachment #497762 - Flags: review?(bent.mozilla)
Attachment #497762 - Flags: review+
Comment on attachment 497762 [details] [diff] [review]
Leak handled too

Hm, this is sorta ugly, but it looks correct.

Maybe a better way to handle this is to not serialize raw nsIDOMGeoPositionCoords, nsIDOMGeoPositionAddress, or nsIDOMGeoPosition pointers but to instead serialize nsCOMPtr<nsIDOMGeoPosition>& references? Then you could make this all a lot cleaner.

r=me for this fix, though. Maybe file a followup to clean this up a bit?
Attachment #497762 - Flags: review?(bent.mozilla) → review+
blocking2.0: --- → ?
blocking2.0: ? → ---
tracking-fennec: --- → 2.0+
http://hg.mozilla.org/mozilla-central/rev/c9bf08a67f28
Status: NEW → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
Is there something I can do to verify this is fixed?
Nope.
Marking this verified on faith.
Status: RESOLVED → VERIFIED
You need to log in before you can comment on or make changes to this bug.