Last Comment Bug 753334 - Audit code that deals with java.nio.Buffer
: Audit code that deals with java.nio.Buffer
Product: Firefox for Android
Classification: Client Software
Component: General (show other bugs)
: unspecified
: All Android
-- normal (vote)
: Firefox 15
Assigned To: (away until Feb21) Kartikaya Gupta (
: Sebastian Kaspari (:sebastian)
Depends on:
  Show dependency treegraph
Reported: 2012-05-09 07:58 PDT by (away until Feb21) Kartikaya Gupta (
Modified: 2012-05-15 10:34 PDT (History)
2 users (show)
See Also:
Crash Signature:
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---

Patch (16.56 KB, patch)
2012-05-09 07:58 PDT, (away until Feb21) Kartikaya Gupta ( review+
Details | Diff | Splinter Review

Description User image (away until Feb21) Kartikaya Gupta ( 2012-05-09 07:58:54 PDT
Created attachment 622372 [details] [diff] [review]

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 User image Chris Lord [:cwiiis] 2012-05-10 02:38:43 PDT
Comment on attachment 622372 [details] [diff] [review]

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

Looks good to me.

::: mobile/android/base/gfx/
@@ -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 User image (away until Feb21) Kartikaya Gupta ( 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.
Comment 3 User image Joe Drew (not getting mail) 2012-05-10 18:42:20 PDT
Comment 4 User image (away until Feb21) Kartikaya Gupta ( 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.