Last Comment Bug 753334 - Audit code that deals with java.nio.Buffer
: Audit code that deals with java.nio.Buffer
Status: RESOLVED FIXED
:
Product: Firefox for Android
Classification: Client Software
Component: General (show other bugs)
: unspecified
: All Android
: -- normal (vote)
: Firefox 15
Assigned To: away[Nov24,Dec5) Kartikaya Gupta (email:kats@mozilla.com)
:
: Sebastian Kaspari (:sebastian)
Mentors:
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2012-05-09 07:58 PDT by away[Nov24,Dec5) Kartikaya Gupta (email:kats@mozilla.com)
Modified: 2012-05-15 10:34 PDT (History)
2 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---
wontfix
fixed


Attachments
Patch (16.56 KB, patch)
2012-05-09 07:58 PDT, away[Nov24,Dec5) Kartikaya Gupta (email:kats@mozilla.com)
chrislord.net: review+
Details | Diff | Splinter Review

Description away[Nov24,Dec5) Kartikaya Gupta (email:kats@mozilla.com) 2012-05-09 07:58:54 PDT
Created attachment 622372 [details] [diff] [review]
Patch

We might be leaking buffers somewhere, see bug 751343 and bug 750965. I audited the relevant code and found some possible double-frees (ScrollbarLayer.finalize(), TextLayer.finalize()) and possible leaks (BufferedCairoImage.setBuffer).
Comment 1 Chris Lord [:cwiiis] 2012-05-10 02:38:43 PDT
Comment on attachment 622372 [details] [diff] [review]
Patch

Review of attachment 622372 [details] [diff] [review]:
-----------------------------------------------------------------

Looks good to me.

::: mobile/android/base/gfx/TileLayer.java
@@ -198,5 @@
> -
> -            GLES20.glTexImage2D(GLES20.GL_TEXTURE_2D, 0, glInfo.internalFormat, mSize.width,
> -                                mSize.height, 0, glInfo.format, glInfo.type, tempBuffer);
> -            GeckoAppShell.freeDirectBuffer(tempBuffer);
> -            */

Unrelated removal, but ok :)
Comment 2 away[Nov24,Dec5) Kartikaya Gupta (email:kats@mozilla.com) 2012-05-10 06:51:32 PDT
(In reply to Chris Lord [:cwiiis] from comment #1)
> 
> Unrelated removal, but ok :)

Showed up in my grepping for buffer-related code :)

Also, sewardj ran valgrind on fennec with this patch applied, and found no real difference in behaviour. He did see a crash on shutdown but turns out that was pre-existing; it is tracked by bug 753665.

Although it seems like this patch doesn't actually fix the memory leak valgrind was possibly finding, it seems like an improvement in the code so I landed it on inbound anyway.

https://hg.mozilla.org/integration/mozilla-inbound/rev/1259212fa0f7
Comment 3 Joe Drew (not getting mail) 2012-05-10 18:42:20 PDT
https://hg.mozilla.org/mozilla-central/rev/1259212fa0f7
Comment 4 away[Nov24,Dec5) Kartikaya Gupta (email:kats@mozilla.com) 2012-05-15 10:34:42 PDT
Marking this as wontfix for 14 because I'm not convinced it actually helped anything, and without being convinced of that it's not worth the risk.

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