1228064 - Find and uplift to 44 a fix to media shutdown leak in bug 1218799 comment 30

Status: RESOLVED DUPLICATE of bug 1227407

(Core :: WebRTC: Audio/Video, defect, P1)

Comment 1

[Jan-Ivar Bruaroey [:jib] (needinfo? me)]

After wasting some time trying to bisect these, I found they happen even in trunk when I test locally, so there's no fix to find and backport.

I suspect this a variation of Bug 1215265, which is apparently white-listed in the tree (Bug 1227347)??

Sadly, the patches in Bug 1215265 did not solve the leaks for me, so I'm including STRs here, in case it can help people find the leak.

STR:
1. Open browser in e10s and start camera with e.g. https://jsfiddle.net/srn9db4h/
2. Open a new tab.
3. Close gUM tab.
4. Close browser -> leak

Comment 2

[Andrew McCreight [:mccr8]]

From the leak from when you tried to land that other patch, you just need to check if the leak log message at the end includes a MediaManager.

Comment 3

[Andrew McCreight [:mccr8]]

(when doing a bisection for a fix)

Comment 4

[Jan-Ivar Bruaroey [:jib] (needinfo? me)]

Andrew is right in comment 2, reopening.

Update: Tried to bisect to find leak using my close-last-content-gum-tab STRs, but a shutdown hang appears using those STRs for a large period of the time between the good and bad version, which prevents leak output, making it hard to pinpoint when exactly it got fixed.

Leak is gone by the time Bug 1216972 rolls in and fixes this shutdown hang, but backporting those patches alone does not solve the leak.

Comment 5

[Jan-Ivar Bruaroey [:jib] (needinfo? me)]

I believe I have the MediaManager leak whipped on aurora. The remaining ones should all be bug 1215265:

== BloatView: ALL (cumulative) LEAK STATISTICS, tab process 54678
 
     |<----------------Class--------------->|<-----Bytes------>|<----Objects---->|
     |                                      | Per-Inst   Leaked|   Total      Rem|
   0 |TOTAL                                 |       18     4656| 3537631       32|
  17 |AsyncTransactionTrackersHolder        |       72       72|       8        1|
  76 |CompositorChild                       |      880      880|       1        1|
  78 |CondVar                               |       40      120|     551        3|
 225 |IPC::Channel                          |       16       32|       8        2|
 281 |MessagePump                           |       16       16|      29        1|
 287 |Mutex                                 |       32       96|    1155        3|
 308 |PCompositorChild                      |      776      776|       1        1|
 314 |PImageBridgeChild                     |      920      920|       1        1|
 367 |RefCountedMonitor                     |       80      160|       6        2|
 368 |RefCountedTask                        |       16       64|      12        4|
 418 |StoreRef                              |       16       32|       8        2|
 469 |WaitableEventKernel                   |       96       96|      33        1|
 474 |WeakReference<MessageListener>        |       16       32|     560        2|
 502 |base::Thread                          |       48       48|       4        1|
 543 |ipc::MessageChannel                   |      512     1024|       6        2|
 934 |nsTArray_base                         |        8       32|  678050        4|
 942 |nsThread                              |      256      256|      28        1|


It took four patches (basically Bug 1227407 and Bug 1216972):

2015-12-02 10:41:41 -0500 | gcp: Bug 1227407 - Ensure Cameras is alive before calling through it. r=jesup
2015-11-18 21:42:16 +0100 | dteller: Bug 1216972 - MediaManager AsyncShutdown for content processes;r?jesup
2015-11-19 00:11:14 +0100 | dteller: Bug 1216972 - OS.File AsyncShutdown for content processes;r?froydnj
2015-11-19 00:09:58 +0100 | dteller: Bug 1216972 - AsyncShutdown for content processes;r=froydnj
2015-11-17 06:21:47 -0500 | rjesup: Bug 1218799: Shutdown MediaManager engines from the MediaManager thread

Doing a try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=21820a7b54cc

Comment 6

[Jan-Ivar Bruaroey [:jib] (needinfo? me)]

Try looks green.

Comment 7

[Jan-Ivar Bruaroey [:jib] (needinfo? me)]

Aurora Approval Request Comment for Bug 1227407 and Bug 1216972

[Feature/regressing bug #]: Bug 1070216
[User impact if declined]:

Crash on shutdown in debug build after cam/mic use (wrong thread assert).

It's possible there may be fallout at shutdown in opt build as well in some cases, since parts of cam/mic cleanup is happening on the wrong thread, though we haven't seen this live. The reason we suspect this, is that an attempt to just remove the assert in debug build led to more asserts firing.

[Describe test coverage new/current, TreeHerder]:

Except for Bug 1227407 (which just fixes a leak), everything has been on m-c for a while (at least a week).

[Risks and why]: 

Changes alter media shutdown behavior only, i.e. no impact unless getUserMedia or enumerateDevices has been used.

The reason for the number of patches is that media shutdown behavior vastly improved in 45, in response to thread assumptions starting to crack in 44 with (Bug 1216972) where some behavior was switched to the right threads.

While we did investigate reverting to 43 behavior, this would most likely have meant backing out bug 1216972, a significant feature, which we considered not an option in #media yesterday. Other ideas about how to preserve that feature seemed riskier than merely advancing to the 45 behavior, which is the shutdown behavior the team is most comfortable with design-wise in spite of the shorter baking time it has had.

Add to this that Bug 1166293 already landed in 44, which means 44 has the most mixed-up shutdown behavior of 43, 44, and 45, adding to the risk of reverting back to 43 vs. moving forward to 45, by our assessment.

[String/UUID change made/needed]: none

Comment 8

[Jan-Ivar Bruaroey [:jib] (needinfo? me)]

All patches in comment 5 have been uplifted to 44.