Last Comment Bug 673154 - possible memory leak in WebM reader
: possible memory leak in WebM reader
Product: Core
Classification: Components
Component: Audio/Video (show other bugs)
: Trunk
: All Other
-- normal (vote)
: mozilla8
Assigned To: Matthew Gregan [:kinetik]
: Maire Reavy [:mreavy] Please needinfo me
Depends on:
Blocks: cppcheck
  Show dependency treegraph
Reported: 2011-07-21 10:30 PDT by David Volgyes
Modified: 2011-08-16 10:51 PDT (History)
3 users (show)
See Also:
Crash Signature:
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---

patch with my proposed solution (1.08 KB, patch)
2011-07-21 10:30 PDT, David Volgyes
no flags Details | Diff | Splinter Review
patch v0 (4.04 KB, patch)
2011-07-28 18:46 PDT, Matthew Gregan [:kinetik]
no flags Details | Diff | Splinter Review
patch v1 (4.05 KB, patch)
2011-07-28 18:55 PDT, Matthew Gregan [:kinetik]
cpearce: review+
Details | Diff | Splinter Review

Description User image David Volgyes 2011-07-21 10:30:48 PDT
Created attachment 547436 [details] [diff] [review]
patch with my proposed solution

User Agent: Mozilla/5.0 (X11; Linux x86_64; rv:5.0) Gecko/20100101 Firefox/5.0
Build ID: 20110622232440

Steps to reproduce:

I tested the code with cppcheck 1.49,
and I checked some warning from the result.

Actual results:

Cppcheck showed several warnings, but some of them false positive.
This seems a real bug:
content/media/webm/nsWebMReader.cpp:498]: (error) Memory leak: buffer

The code allocates memory for temporary storage.
However, it does not release the memory when error happens.

Expected results:

- Memory should be released before "return". 
(A possible fix is attached, but please, review it before use.)
- Mozilla should use cppcheck for automated testing.
Comment 1 User image Chris Pearce (:cpearce) 2011-07-22 12:34:00 PDT
Nice catch! Please make |buffer| an nsAutoArrayPtr<SoundDataValue>, so that it's automatically released on scope exit instead. You'll need to call buffer.forget() when you pass it to the SoundData constructor, as that takes over ownership.
Comment 2 User image Matthew Gregan [:kinetik] 2011-07-28 18:46:21 PDT
Created attachment 549287 [details] [diff] [review]
patch v0
Comment 3 User image Matthew Gregan [:kinetik] 2011-07-28 18:53:48 PDT
Comment on attachment 549287 [details] [diff] [review]
patch v0

Oops, this is missing a buffer.forget() in once place.
Comment 4 User image Matthew Gregan [:kinetik] 2011-07-28 18:55:29 PDT
Created attachment 549288 [details] [diff] [review]
patch v1
Comment 5 User image Chris Pearce (:cpearce) 2011-07-28 19:02:18 PDT
Comment on attachment 549288 [details] [diff] [review]
patch v1

Review of attachment 549288 [details] [diff] [review]:

Note You need to log in before you can comment on or make changes to this bug.