Tapas[:tkundu on #b2g/gaia/memshrink/gfx] (always NI me) Reporter
	Description • 10 years ago

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.

	Tapas[:tkundu on #b2g/gaia/memshrink/gfx] (always NI me) Reporter
	Comment 1 • 10 years ago

[Blocking Requested - why for this release]:

	Tapas[:tkundu on #b2g/gaia/memshrink/gfx] (always NI me) Reporter
	Comment 2 • 10 years ago

DMD memory report collected before starting video streaming stability test

	Tapas[:tkundu on #b2g/gaia/memshrink/gfx] (always NI me) Reporter
	Comment 3 • 10 years ago

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 :) .

	Kyle Huey (Exited; not receiving bugmail, old account, do not use)
	Comment 4 • 10 years ago

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

	Kyle Huey (Exited; not receiving bugmail, old account, do not use)
	Comment 5 • 10 years ago

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

	Kyle Huey (Exited; not receiving bugmail, old account, do not use)
	Comment 6 • 10 years ago

This one actually builds.

	Ethan Tseng [:ethan] Assignee
	Comment 7 • 10 years ago

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.

	Ethan Tseng [:ethan] Assignee
	Comment 8 • 10 years ago

Hi Steve,

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

	Kyle Huey (Exited; not receiving bugmail, old account, do not use)
	Comment 9 • 10 years ago

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

	Ethan Tseng [:ethan] Assignee
	Comment 10 • 10 years ago

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.

	Ethan Tseng [:ethan] Assignee
	Comment 12 • 10 years ago

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.

	Henry Chang [:hchang]
	Comment 13 • 10 years ago

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.

	Ethan Tseng [:ethan] Assignee
	Comment 14 • 10 years ago

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

	Kyle Huey (Exited; not receiving bugmail, old account, do not use)
	Comment 15 • 10 years ago

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.

	Ethan Tseng [:ethan] Assignee
	Comment 16 • 10 years ago

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

	Ethan Tseng [:ethan] Assignee
	Comment 17 • 10 years ago

Henry and Kyle,

Thanks for your review. Appreciated!

	Kyle Huey (Exited; not receiving bugmail, old account, do not use)
	Comment 18 • 10 years ago

Thanks for finishing this.

	Ethan Tseng [:ethan] Assignee
	Comment 19 • 10 years ago

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

	Carsten Book [:Tomcat]
	Comment 20 • 10 years ago

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

	Ryan VanderMeulen [:RyanVM]
	Comment 21 • 10 years ago

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

	Ethan Tseng [:ethan] Assignee
	Comment 22 • 10 years ago

(In reply to Kyle Huey [:khuey] (khuey@mozilla.com) 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.

	Ethan Tseng [:ethan] Assignee
	Comment 23 • 10 years ago

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

	Ryan VanderMeulen [:RyanVM]
	Comment 24 • 10 years ago

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

	Tapas[:tkundu on #b2g/gaia/memshrink/gfx] (always NI me) Reporter
	Comment 25 • 10 years ago

(In reply to Ethan Tseng [:ethan] from comment #22)
> (In reply to Kyle Huey [:khuey] (khuey@mozilla.com) 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

	bhavana bajaj [:bajaj]
	Comment 26 • 10 years ago

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

	Ryan VanderMeulen [:RyanVM]
	Comment 27 • 10 years ago

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

	Ryan VanderMeulen [:RyanVM]
	Comment 28 • 10 years ago

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