Closed
Bug 1251213
Opened 8 years ago
Closed 8 years ago
[Static Analysis][Resource leak] In DoCommand::UpdateCallBack
Categories
(Firefox for Android Graveyard :: General, defect)
Firefox for Android Graveyard
General
Tracking
(firefox47 fixed)
RESOLVED
FIXED
Firefox 47
Tracking | Status | |
---|---|---|
firefox47 | --- | fixed |
People
(Reporter: andi, Assigned: andi)
References
(Blocks 1 open bug)
Details
(Keywords: coverity, Whiteboard: CID 81069)
Attachments
(1 file)
The Static Analysis tool Coverity added that variable |fis| can leak memory in the following context: >> FileInputStream fis = ctx.openFileInput(sFileName); >> int nBytes = fis.available(); >> if (nBytes > 0) >> { >> byte [] buffer = new byte [nBytes + 1]; >> int nRead = fis.read(buffer, 0, nBytes); >> fis.close(); >> ctx.deleteFile(sFileName); >> if (nRead > 0) >> { >> String sBuffer = new String(buffer); >> nEnd = sBuffer.indexOf(','); >> if (nEnd > 0) >> { >> sIP = (sBuffer.substring(0, nEnd)).trim(); >> nStart = nEnd + 1; >> nEnd = sBuffer.indexOf('\r', nStart); >> if (nEnd > 0) >> { >> sPort = (sBuffer.substring(nStart, nEnd)).trim(); >> Thread.sleep(5000); >> sRet = RegisterTheDevice(sIP, sPort, sBuffer.substring(nEnd + 1)); >> } >> } >> } >> } I think there are two possibilities here in order to have a memory leak: 1. nBytes is <= 0, as Coverity states 2. fis.available() raises an IOException For this i consider that we should add |fis| in a try-with-resources statement.If we target devices lower than api level 19 i can reupdate the patch with: >> finally >> { >> try >> { >> fis.close(); >> } >> }
Assignee | ||
Comment 1•8 years ago
|
||
Review commit: https://reviewboard.mozilla.org/r/36587/diff/#index_header See other reviews: https://reviewboard.mozilla.org/r/36587/
Attachment #8723527 -
Flags: review?(s.kaspari)
Comment 2•8 years ago
|
||
Comment on attachment 8723527 [details] MozReview Request: Bug 1251213 - release resources from fis. r?sebastian https://reviewboard.mozilla.org/r/36587/#review33699 Did you test this on an older device - preferably a Gingerbread one? It seems like all articles I found point to a minSdkVersion of 19 for try-with-resources. Feel free to reflag me if this work as is.
Attachment #8723527 -
Flags: review?(s.kaspari)
Assignee | ||
Comment 3•8 years ago
|
||
Comment on attachment 8723527 [details] MozReview Request: Bug 1251213 - release resources from fis. r?sebastian Review request updated; see interdiff: https://reviewboard.mozilla.org/r/36587/diff/1-2/
Attachment #8723527 -
Flags: review?(s.kaspari)
Assignee | ||
Comment 4•8 years ago
|
||
I've reverted my previous change since that was only valid for API Level 19+. I've added fis.close in a try() catch() block since close throws an exception.
Comment 5•8 years ago
|
||
Comment on attachment 8723527 [details] MozReview Request: Bug 1251213 - release resources from fis. r?sebastian https://reviewboard.mozilla.org/r/36587/#review34465 The format looks weird but it seems to follow what is used in this file.
Attachment #8723527 -
Flags: review?(s.kaspari) → review+
Assignee | ||
Comment 6•8 years ago
|
||
yes that's why i've used it, i didn't want to have any discrepancies from the rest of the format.
Comment 8•8 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/5b643385e0da
Status: NEW → RESOLVED
Closed: 8 years ago
status-firefox47:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 47
Updated•3 years ago
|
Product: Firefox for Android → Firefox for Android Graveyard
You need to log in
before you can comment on or make changes to this bug.
Description
•