Closed Bug 1087622 Opened 8 years ago Closed 8 years ago

b2g memleak during video streaming in v2.1

Categories

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

ARM
Gonk (Firefox OS)
defect

Tracking

(blocking-b2g:2.0+, firefox34 wontfix, firefox35 wontfix, firefox36 fixed, b2g-v1.4 unaffected, b2g-v2.0 fixed, b2g-v2.0M fixed, b2g-v2.1 fixed, b2g-v2.2 fixed)

RESOLVED FIXED
2.1 S8 (7Nov)
blocking-b2g 2.0+
Tracking Status
firefox34 --- wontfix
firefox35 --- wontfix
firefox36 --- fixed
b2g-v1.4 --- unaffected
b2g-v2.0 --- fixed
b2g-v2.0M --- fixed
b2g-v2.1 --- fixed
b2g-v2.2 --- fixed

People

(Reporter: tkundu, Assigned: ethan)

References

Details

(Keywords: regression, Whiteboard: [caf priority: p2][CR 743682])

Attachments

(3 files, 4 obsolete files)

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.
[Blocking Requested - why for this release]:
blocking-b2g: --- → 2.1?
Whiteboard: [CR 743682]
Whiteboard: [CR 743682] → [caf priority: p2][CR 743682]
DMD memory report collected before starting video streaming stability test
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 :) .
Flags: needinfo?(khuey)
Component: Stability → RTSP
Flags: needinfo?(khuey)
Attached patch Patch (obsolete) — Splinter Review
This is completely untested (I didn't even check if it compiles).
Attachment #8511623 - Flags: review?(ettseng)
This is a regression from bug 996535, which means that this is in 2.0 ...
Attached patch Patch (obsolete) — Splinter Review
This one actually builds.
Assignee: nobody → khuey
Attachment #8511623 - Attachment is obsolete: true
Attachment #8511623 - Flags: review?(ettseng)
Attachment #8511639 - Flags: review?(ettseng)
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.
Hi Steve,

Do you have any idea on this issue and Kyle's patch?
Flags: needinfo?(sworkman)
Attached patch Patch (obsolete) — Splinter Review
Yeah, we can do this.  I don't like not using RAII though :P
Attachment #8512077 - Flags: review?(ettseng)
blocking-b2g: 2.1? → 2.1+
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.
Flags: needinfo?(sworkman)
Attachment #8511639 - Attachment is obsolete: true
Attachment #8512077 - Attachment is obsolete: true
Attachment #8511639 - Flags: review?(ettseng)
Attachment #8512077 - Flags: review?(ettseng)
Attachment #8512494 - Flags: review?(khuey)
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.
Attachment #8512494 - Flags: review?(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.
Attachment #8512494 - Flags: review?(hchang) → review+
BTW, pollList should be infallible.
Typically, pollCount = 4 and sizeof(PRPollDesc) = 8.
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.
Attachment #8512494 - Flags: review?(khuey) → review+
1. Remove include UniquePtr.h
2. Refresh comment "r=hchang, r=khuey"
Attachment #8512494 - Attachment is obsolete: true
Henry and Kyle,

Thanks for your review. Appreciated!
https://hg.mozilla.org/mozilla-central/rev/10333db03e19
Status: NEW → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Target Milestone: --- → 2.1 S8 (7Nov)
(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.
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
Attachment #8513212 - Flags: approval-mozilla-b2g34?
Attachment #8513212 - Flags: approval-mozilla-b2g32?
Attachment #8513212 - Flags: approval-mozilla-b2g34? → approval-mozilla-b2g34+
(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
Flags: needinfo?(bbajaj)
blocking-b2g: 2.1+ → 2.0+
Flags: needinfo?(bbajaj)
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
Attachment #8513212 - Flags: approval-mozilla-b2g32? → approval-mozilla-b2g32+
You need to log in before you can comment on or make changes to this bug.