Closed
Bug 678745
Opened 8 years ago
Closed 6 years ago
resource leak (not closed file) in js/src/shell/js.cpp
Categories
(Core :: JavaScript Engine, defect)
Not set
Tracking
()
RESOLVED
FIXED
mozilla29
People
(Reporter: david.volgyes, Assigned: sunfish)
References
Details
Attachments
(1 file, 1 obsolete file)
2.34 KB,
patch
|
jorendorff
:
review+
|
Details | Diff | Splinter Review |
User Agent: Mozilla/5.0 (X11; Linux x86_64; rv:5.0) Gecko/20100101 Firefox/5.0 Build ID: 20110622232440 Steps to reproduce: Cppcheck 1.49 found some resource (and memory) leaks, this is one of them. Actual results: In js/src/shell/js.cpp in this function: static JSObject * FileAsTypedArray(JSContext *cx, const char *pathname) there is an opened file, and it is not closed before "return" Expected results: You should close all opened file (and release all unnecessary allocated memory) before return. I attached my proposed patch. Please, review it.
Reporter | ||
Comment 1•8 years ago
|
||
I am so sorry, I did something wrong. I have some opened file for bug reports, and I mixed the texts. Skip the whole original report (and patch), they are mixed. In the next comment, I will do the fix. I am sorry.
Reporter | ||
Comment 2•8 years ago
|
||
Reporter | ||
Comment 3•8 years ago
|
||
So actually, I am a little bit tired. js.cpp does not close a file, and hopefully I attached the right solution. (First, it seemed that I attached a wrong patch. But somehow, the file attach process canceled, so I did not upload the wrong patch, but I thought that (comment #1). When I realized that, I attached the right solution (I hope). And now, I try to clarify my plenty of comments.) And finally, I marked the wrong version, because it was from trunk,and not from branch 5.) I am sorry for the lot of confusion I did.
Version: 5 Branch → Trunk
Updated•8 years ago
|
Assignee: nobody → general
Component: General → JavaScript Engine
Product: Firefox → Core
QA Contact: general → general
Hardware: x86_64 → x86
Comment 4•8 years ago
|
||
The patch looks correct. If you're up for it, you may want to refactor the function to use a C++ destructor to clean up the file--there's only one return now, but the destructor style is more robust to future changes. Anyway, whether you want to subit this patch or one with a destructor, the next step is to request review on the patch.
Status: UNCONFIRMED → NEW
Ever confirmed: true
Comment 5•8 years ago
|
||
Hey, I'm working on this bug. Don't officially assign me anything, but don't hand it off to a random volunteer please.
Assignee | ||
Comment 7•6 years ago
|
||
Thank you for finding this bug and submitting a patch! Sine the patch was submitted, js.cpp has gained an AutoCloseInputFile class which implements closing files in a destructor, which allows fixing this in the way suggested in comment 4. There hasn't been any activity here for a while, and I have an interest in getting this fixed, so I'm just going ahead and taking it.
Assignee: general → sunfish
Attachment #552890 -
Attachment is obsolete: true
Attachment #8344632 -
Flags: review?(acelists)
Comment on attachment 8344632 [details] [diff] [review] shell-leak.patch Review of attachment 8344632 [details] [diff] [review]: ----------------------------------------------------------------- Sorry, I am not a reviewer in JS module.
Attachment #8344632 -
Flags: review?(acelists)
Assignee | ||
Comment 9•6 years ago
|
||
Comment on attachment 8344632 [details] [diff] [review] shell-leak.patch Just a simple patch to use AutoCloseInputFile in a few more places, fixing some FILE leaks on error paths.
Attachment #8344632 -
Flags: review?(jorendorff)
Comment 10•6 years ago
|
||
Comment on attachment 8344632 [details] [diff] [review] shell-leak.patch Review of attachment 8344632 [details] [diff] [review]: ----------------------------------------------------------------- Thank you!
Attachment #8344632 -
Flags: review?(jorendorff) → review+
Assignee | ||
Comment 11•6 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/3b965f401c4f
Comment 12•6 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/3b965f401c4f
Status: NEW → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla29
You need to log in
before you can comment on or make changes to this bug.
Description
•