753334 - Audit code that deals with java.nio.Buffer

Status: RESOLVED FIXED

(Firefox for Android Graveyard :: General, defect)

Description

[Kartikaya Gupta (email:[e-mail])]

Attachment: 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]]

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

[Kartikaya Gupta (email:[e-mail])]

(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)]

https://hg.mozilla.org/mozilla-central/rev/1259212fa0f7

Comment 4

[Kartikaya Gupta (email:[e-mail])]

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.