1087622 - b2g memleak during video streaming in v2.1

Status: RESOLVED FIXED

(Firefox OS Graveyard :: RTSP, defect, P1)

Description

[Tapas[:tkundu on #b2g/gaia/memshrink/gfx] (always NI me)]

We are seeing a memleak during video streaming stability test and b2g is hitting soon 465MB USS memory which is clear indication of memleak.

 [H [JEvery 2s: b2g-info                                          2014-10-18 10:45:54

                          |       megabytes      |
           NAME  PID PPID  CPU(s) NICE   USS   PSS   RSS SWAP VSIZE OOM_ADJ USER    
            b2g  223    1 22898.7    0 465.3 473.2 493.7  0.0 649.2       0 root    
         (Nuwa)  608  223   102.8    0   2.6   4.8  13.8  0.0  53.8     -16 root    
Built-in Keyboa 1133  608     9.1   18   7.4  10.3  22.3  0.0  72.0      11 u0_a1133
     Homescreen 1257  223  3263.7   18  16.2  21.5  35.8  0.0  83.2       8 u0_a1257
    VideoStream 3416  608     1.9   18   8.9  12.5  26.0  0.0  68.9      11 u0_a3416
(Preallocated a 5084  608     0.4   18   5.8   8.3  19.3  0.0  61.6       1 u0_a5084


We will collect and update gecko memory report soon.

Comment 1

[Tapas[:tkundu on #b2g/gaia/memshrink/gfx] (always NI me)]

[Blocking Requested - why for this release]:

Comment 2

[Tapas[:tkundu on #b2g/gaia/memshrink/gfx] (always NI me)]

Attachment: about-memory-0_no_memleak.tar.bz2

DMD memory report collected before starting video streaming stability test

Comment 3

[Tapas[:tkundu on #b2g/gaia/memshrink/gfx] (always NI me)]

Attachment: about-memory-2_with_memleak.tar.bz2

DMD Memory report collected after 10 hours video streaming stability test. 

I compared this with attachment 8511597 [details] and found that heap unclassified memory has increased 97MB. 

It also shows that all  unclassified heap allocations are coming from below stacktraces which is very huge :


Unreported {
  ~25,087 blocks in heap block record 1 of 886
  ~102,681,091 bytes (~102,681,091 requested / ~0 slop)
  75.68% of the heap (75.68% cumulative)
  89.63% of unreported (89.63% cumulative)
  Allocated at {
    replace_calloc /local/mnt/workspace/tkundu/AU110/gecko/memory/replace/dmd/DMD.cpp:1165 (libdmd.so+0x3d2c) 0xb6f0bd2c
    moz_xcalloc /local/mnt/workspace/tkundu/AU110/gecko/memory/mozalloc/mozalloc.cpp:69 (libxul.so+0x147dcaa) 0xb5e9acaa
    android::List<android::ARTPConnection::StreamInfo>::_Node::getNext() const /local/mnt/workspace/tkundu/AU110/system/core/include/utils/List.h:55 (libxul.so+0x433aee) 0xb4e50aee
    android::ARTPConnection::onMessageReceived(android::sp<android::AMessage> const&) /local/mnt/workspace/tkundu/AU110/gecko/netwerk/protocol/rtsp/rtsp/ARTPConnection.cpp:195 (libxul.so+0x4341b0) 0xb4e511b0
    android::ALooperRoster::deliverMessage(android::sp<android::AMessage> const&) /local/mnt/buildbot/sink/build/work/tmp/frameworks/av/media/libstagefright/foundation/ALooperRoster.cpp:148 (libstagefright_foundation.so+0x9436) 0xb6b09436
    android::ALooper::loop() /local/mnt/buildbot/sink/build/work/tmp/frameworks/av/media/libstagefright/foundation/ALooper.cpp:213 (libstagefright_foundation.so+0x8f12) 0xb6b08f12
    android::Thread::_threadLoop(void*) /local/mnt/buildbot/sink/build/work/tmp/system/core/libutils/Threads.cpp:770 (libutils.so+0xe9ee) 0xb6e8a9ee
    thread_data_t::trampoline(thread_data_t const*) /local/mnt/buildbot/sink/build/work/tmp/system/core/libutils/Threads.cpp:96 (libutils.so+0xe590) 0xb6e8a590
    __thread_entry /local/mnt/buildbot/sink/build/work/tmp/bionic/libc/bionic/pthread_create.cpp:107 (libc.so+0xd22c) 0xb6f3d22c
    pthread_create /local/mnt/buildbot/sink/build/work/tmp/bionic/libc/bionic/pthread_create.cpp:225 (libc.so+0xd3c4) 0xb6f3d3c4
  }
}


Kyle: Could you please give us a quick fix for this one :) .

Comment 4

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

Attachment: Patch

This is completely untested (I didn't even check if it compiles).

Comment 5

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

This is a regression from bug 996535, which means that this is in 2.0 ...

Comment 6

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

Attachment: Patch

This one actually builds.

Comment 7

[Ethan Tseng [:ethan]]

Comment on attachment 8511639 [details] [diff] [review]
Patch

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

Hi Kyle,

Thanks for adding this patch!

Apparently, ARTPConnection::onPollStreams() uses moz_xcalloc() without freeing memory,
which causes the leak.
I think your intent is to use nsTArray to perform memory allocation instead of using
moz_xcalloc() by ARTPConnection itself. It is reasonable to me.

However, when I tested your patch I found RTP stream cannot be transported over UDP
sockets. From tcpdump, RTP-over-UDP is received on the host, but our application does
not receive it.
The end effect is that our application will timeout (10 secs) and fail over to
RTP-over-TCP.

I know memory leak issue is critical. If it's urgent, could we add free() invocations as
workaround?
In the meanwhile, we will look into why your patch doesn't work.

Comment 8

[Ethan Tseng [:ethan]]

Hi Steve,

Do you have any idea on this issue and Kyle's patch?

Comment 9

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

Attachment: Patch

Yeah, we can do this.  I don't like not using RAII though :P

Comment 10

[Ethan Tseng [:ethan]]

Thanks for Henry and Benjamin's help. We know why nsTArray in the patch doesn't work.

pollList is defined as an nsTArray (containing PRPollDesc) and constructed with pollCount as its
capacity. However, the following manipulations to pollList do not use nsTArray methods such as 
AppendElements() to increase the length. And pollList.Length() always returns zero.

I'll update a patch soon.

Comment 11

[Ethan Tseng [:ethan]]

Attachment: Replace moz_xcalloc() by nsTArray to avoid memory leak

Comment 12

[Ethan Tseng [:ethan]]

Comment on attachment 8512494 [details] [diff] [review]
Replace moz_xcalloc() by nsTArray to avoid memory leak

Hi Henry,

Could you also help to review the patch?
Please supplement any comment if you would like to.

Comment 13

[Henry Chang [:hchang]]

Comment on attachment 8512494 [details] [diff] [review]
Replace moz_xcalloc() by nsTArray to avoid memory leak

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

Looks good! I am also surprised that nsTArray's ctor is designed 
differently from std::vector.

Comment 14

[Ethan Tseng [:ethan]]

BTW, pollList should be infallible.
Typically, pollCount = 4 and sizeof(PRPollDesc) = 8.

Comment 15

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

Comment on attachment 8512494 [details] [diff] [review]
Replace moz_xcalloc() by nsTArray to avoid memory leak

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

Nice catch, that's crazy.

::: netwerk/protocol/rtsp/rtsp/ARTPConnection.cpp
@@ +31,5 @@
>  #include <arpa/inet.h>
>  
>  #include "mozilla/NullPtr.h"
>  #include "mozilla/mozalloc.h"
> +#include "mozilla/UniquePtr.h"

You don't actually need the UniquePtr.h include, that was an artifact from an earlier iteration of my patch.

Comment 16

[Ethan Tseng [:ethan]]

Attachment: Replace moz_xcalloc by nsTArray to avoid memory leak

1. Remove include UniquePtr.h
2. Refresh comment "r=hchang, r=khuey"

Comment 17

[Ethan Tseng [:ethan]]

Henry and Kyle,

Thanks for your review. Appreciated!

Comment 18

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

Thanks for finishing this.

Comment 19

[Ethan Tseng [:ethan]]

The result of Treeherder:
https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=fb915e9f4e8b

Comment 20

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/integration/b2g-inbound/rev/10333db03e19

Comment 21

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/10333db03e19

Comment 22

[Ethan Tseng [:ethan]]

(In reply to Kyle Huey [:khuey] ([e-mail]) from comment #5)
> This is a regression from bug 996535, which means that this is in 2.0 ...

We have to uplift this patch to 2.0 and 2.1.

Comment 23

[Ethan Tseng [:ethan]]

Comment on attachment 8513212 [details] [diff] [review]
Replace moz_xcalloc by nsTArray to avoid memory leak

[Approval Request Comment]
Bug caused by (feature/regressing bug #): Bug 996535
User impact if declined: Memory leak from RTSP streaming
Testing completed: On m-c
Risk to taking this patch (and alternatives if risky): None
String or UUID changes made by this patch: None

Comment 24

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-b2g34_v2_1/rev/b06ffcfac0f1

Comment 25

[Tapas[:tkundu on #b2g/gaia/memshrink/gfx] (always NI me)]

(In reply to Ethan Tseng [:ethan] from comment #22)
> (In reply to Kyle Huey [:khuey] ([e-mail]) from comment #5)
> > This is a regression from bug 996535, which means that this is in 2.0 ...
> 
> We have to uplift this patch to 2.0 and 2.1.

We are also seeing this issue in v2.0. Please uplift asap :) thankx

Comment 26

[bhavana bajaj [:bajaj]]

Comment on attachment 8513212 [details] [diff] [review]
Replace moz_xcalloc by nsTArray to avoid memory leak

Uplifting to 2.1 to avoid the memleak and since the patch is deemed less risky

Comment 27

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-b2g32_v2_0/rev/093de6b632c5

Comment 28

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-b2g32_v2_0m/rev/093de6b632c5