813339 - dom/media/tests/crashtests/791330.html is leaking memory

Status: VERIFIED DUPLICATE of bug 802538

(Core :: WebRTC, defect, P1)

Description

[Henrik Skupin [:whimboo][⌚️UTC+1] (away from Feb 12th - Feb 22nd)]

The referenced crashtest is leaking memory as shown below.

Steps:
1. Enable XPCOM_MEM_LEAK_LOG
2. Start Firefox 
3. Open the referenced URL
4. Close Firefox

Leak log:

> == BloatView: ALL (cumulative) LEAK STATISTICS, default process 53283
> 
>      |<----------------Class--------------->|<-----Bytes------>|<----------------Objects---------------->|<--------------References-------------->|
>                                               Per-Inst   Leaked    Total      Rem      Mean       StdDev     Total      Rem      Mean       StdDev
>    0 TOTAL                                          30     1475   578538       42 ( 2853.60 +/-  4104.89)   960907       11 ( 2917.16 +/-  5265.69)
>   40 CondVar                                        32       32       15        1 (    7.14 +/-     3.49)        0        0 (    0.00 +/-     0.00)
>  116 MediaEngineDefaultAudioSource                  56       56        1        1 (    1.00 +/-     0.00)      467        1 (    3.17 +/-     0.71)
>  120 MediaStream                                   192      192        1        1 (    1.00 +/-     0.00)      502        1 (    4.43 +/-     8.30)
>  121 MediaStreamGraph                               16       16        1        1 (    1.00 +/-     0.00)        0        0 (    0.00 +/-     0.00)
>  123 MediaStreamListener                            16       16        1        1 (    1.00 +/-     0.00)        3        1 (    1.80 +/-     0.84)
>  159 Mutex                                          24       96      546        4 (   96.72 +/-    28.40)        0        0 (    0.00 +/-     0.00)
>  195 ReentrantMonitor                               32       64      192        2 (   32.67 +/-    13.08)        0        0 (    0.00 +/-     0.00)
>  219 StreamBuffer                                   24       24        1        1 (    1.00 +/-     0.00)        0        0 (    0.00 +/-     0.00)
>  231 TimeVaryingBase                                 1        3        3        3 (    2.00 +/-     1.00)        0        0 (    0.00 +/-     0.00)
>  323 nsAuthURLParser                                24       24        2        1 (    1.33 +/-     0.58)    10882        1 (  294.59 +/-   193.87)
>  334 nsBasePrincipal                                32       32       96        1 (   35.93 +/-    23.96)     1389        1 (  241.90 +/-   126.76)
>  434 nsDOMLocalMediaStream                          56       56        1        1 (    1.00 +/-     0.00)        8        1 (    4.27 +/-     2.25)
>  693 nsPrincipal                                    56       56       96        1 (   35.93 +/-    23.96)     1389        1 (  241.90 +/-   126.76)
>  761 nsStandardURL                                 248      248     3509        1 (  342.61 +/-   198.85)    32632        1 ( 1300.79 +/-   686.34)
>  771 nsStringBuffer                                  8        8    42021        1 ( 8739.39 +/-  3854.80)    89789        1 (15787.08 +/-  6412.35)
>  812 nsTArray_base                                   8      152    80713       19 (10369.38 +/-  4002.97)        0        0 (    0.00 +/-     0.00)
>  821 nsThread                                      200      400       29        2 (    9.39 +/-     4.44)     2530        2 (   97.79 +/-    30.96)

Comment 1

[Henrik Skupin [:whimboo][⌚️UTC+1] (away from Feb 12th - Feb 22nd)]

Attachment: testcase

Simple testcase which simply calls gUM but leaks a lot of stuff. It's a stripped down version of the referenced crashtest. Comparing both bloatlogs shows that the crashtest even leaks more but we should probably start with this leak. Can anyone have a look at?

> == BloatView: ALL (cumulative) LEAK STATISTICS, default process 13351
> 
>      |<----------------Class--------------->|<-----Bytes------>|<----------------Objects---------------->|<--------------References-------------->|
>                                               Per-Inst   Leaked    Total      Rem      Mean       StdDev     Total      Rem      Mean       StdDev
>    0 TOTAL                                          32      448   420646        6 ( 2698.00 +/-  3936.74)   796912        2 ( 2619.74 +/-  4626.37)
>  109 MediaManager                                  160      160        2        1 (    0.67 +/-     0.58)       16        1 (    3.61 +/-     1.82)
>  145 Mutex                                          24       48      479        2 (   87.02 +/-    24.09)        0        0 (    0.00 +/-     0.00)
>  173 ReentrantMonitor                               32       32      136        1 (   33.69 +/-    14.07)        0        0 (    0.00 +/-     0.00)
>  742 nsTArray_base                                   8        8    63385        1 ( 9903.73 +/-  3831.53)        0        0 (    0.00 +/-     0.00)
>  751 nsThread                                      200      200       18        1 (    8.34 +/-     4.32)     1075        1 (   77.76 +/-    23.62)

Comment 2

[Randell Jesup [:jesup] (needinfo me)]

It appears MediaManager (singleton) is leaking itself; I'll check if Anant included delete-on-shutdown code (I don't think so).

Comment 3

[Randell Jesup [:jesup] (needinfo me)]

I could find no instances of MediaManager leaking.

What's more, just opening the browser and closing it often leaks a ton of stuff according to this test.

I seem to get more consistent results if I use "-P webrtc -no-remote URL" instead of "-P webrtc -no-remote" and then opening the testcase - but what leaks are MediaDevices and a MediaStream (plus all the attendant things one would expect if one leaks), not a MediaManager.

However, it's hard to trust the results when "./firefox -P webrtc -no-remote http://mozilla.github.com/webrtc-landing/" leaks 133 objects in like 30 or 50 classes.

Comment 4

[Henrik Skupin [:whimboo][⌚️UTC+1] (away from Feb 12th - Feb 22nd)]

Randell, you can also remove all the crashtests except that one from the crashtests.list file and run it throught he crashtest suite like:

TEST_PATH=dom/media/tests/crashtests/crashtests.list make -C {OBJ_DIR} crashtest

That should not leak that many objects which I also have faced before.

Comment 5

[Jason Smith [:jsmith]]

Duping on bug 802538 as that bug is likely fixing the leaks for this bug.

If we end up finding out this is incorrect after it lands, feel free to reopen.

Comment 6

[Henrik Skupin [:whimboo][⌚️UTC+1] (away from Feb 12th - Feb 22nd)]

I'm against duping everything against bugs with possible fixes. Things like those have to end-up on the dependency list instead. Only in those cases you will get a notification for this bug that a dependent bug has been fixed and a verification is necessary here. If it doesn't fix the problem no-one is likely going to reopen this one.

Comment 7

[Jason Smith [:jsmith]]

(In reply to Henrik Skupin (:whimboo) [away 12/21 - 01/01] from comment #6)
> I'm against duping everything against bugs with possible fixes. Things like
> those have to end-up on the dependency list instead. Only in those cases you
> will get a notification for this bug that a dependent bug has been fixed and
> a verification is necessary here. If it doesn't fix the problem no-one is
> likely going to reopen this one.

Actually, bugzilla does notify you when a dupe gets closed. The policy typically is dupe and do qawanted to reopen if the patch doesn't fix the problem. Too many bugs open that have the same symptom is generally not a good idea. 

So reopen it if the problem still reproduces after that patch.

Comment 8

[Henrik Skupin [:whimboo][⌚️UTC+1] (away from Feb 12th - Feb 22nd)]

(In reply to Jason Smith [:jsmith] from comment #7)
> Actually, bugzilla does notify you when a dupe gets closed. The policy

No, it does not notify you for duped bugs. It only happens for dependent bugs. I'm working with Bugzilla since 2002 and never have seen that. So you should point me to a (hidden) setting which enables that.

> typically is dupe and do qawanted to reopen if the patch doesn't fix the

No-one is watching qawanted on already closed bugs. I also don't think that this idea is a good one. verifyme might be a candidate.

Comment 9

[Jason Smith [:jsmith]]

(In reply to Henrik Skupin (:whimboo) from comment #8)
> (In reply to Jason Smith [:jsmith] from comment #7)
> > Actually, bugzilla does notify you when a dupe gets closed. The policy
> 
> No, it does not notify you for duped bugs. It only happens for dependent
> bugs. I'm working with Bugzilla since 2002 and never have seen that. So you
> should point me to a (hidden) setting which enables that.
> 
> > typically is dupe and do qawanted to reopen if the patch doesn't fix the
> 
> No-one is watching qawanted on already closed bugs. I also don't think that
> this idea is a good one. verifyme might be a candidate.

Uhh...actually that's not true. verifyme is actually less consistently used - qawanted used more, including on closed bugs. 

Anyways, this was verified as part of the try run you just did.