919244 - ghost-windows appears on https://apprtc.appspot.com/ after execute "Minimize memory usage"

Status: VERIFIED FIXED

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

Description

[Alice0775 White]

Steps To Reproduce.
1. Open https://apprtc.appspot.com/ in new tab
2. Close the tab
3. Open about:memory and Click "Measure"
   --- observe ghost-windows
4. Click "GC" and Click "Measure"
   --- observe ghost-windows
5. Click "CC" and Click "Measure"
   --- observe ghost-windows
6. Click "Minimize memory usage" and Click "Measure"
   --- observe ghost-windows

Actual Results:
After Step 6, Two ghost-windows appears

Comment 1

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

Seems likely to be a webrtc issue.

Comment 2

[Nicholas Nethercote [inactive]]

I failed to reproduce on Linux (trunk build, where I don't have a camera) or Mac (FF 24 beta, where I do).  Maybe it's a Windows-only issue?

Comment 3

[Alice0775 White]

I can also reproduce on ubuntu 12.04.
ttp://hg.mozilla.org/mozilla-central/rev/8b4d14afc4f6
Mozilla/5.0 (X11; Linux i686; rv:27.0) Gecko/20100101 Firefox/27.0 ID:20130922030201

Comment 4

[Nicholas Nethercote [inactive]]

I didn't make a connection -- do you need to make a connection to reproduce?

Comment 5

[Alice0775 White]

(In reply to Nicholas Nethercote [:njn] from comment #4)
> I didn't make a connection -- do you need to make a connection to reproduce?

What is "connection"?

Just follow STR with new profile. 
It happens regardless of having camera or not.

Comment 6

[Adam Roach [:abr]]

I finally got this to repro on OS X also. One note on the STR is that the problem does not seem to arise if you acknowledge the microphone / camera doorhanger. You must close the page with that doorhanger open.

I also note that the issue will repro just fine using this test page, which uses gUM but not PeerConnection:

http://mozilla.github.io/webrtc-landing/gum_test.html

I used the "video-only" mode, but I suspect others have the same result. Assigning more specifically to "Audio/Video."

Comment 7

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

Ok, sounds like this is the cause of the leak that causes bug 917491 (same STR - don't answer a permission doorhanger; world leaks)

Comment 8

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

(In reply to Randell Jesup [:jesup] from comment #7)
> Ok, sounds like this is the cause of the leak that causes bug 917491 (same
> STR - don't answer a permission doorhanger; world leaks)

Sorry - not necessarily the same, but related

Comment 9

[Nicholas Nethercote [inactive]]

> One note on the STR is that the
> problem does not seem to arise if you acknowledge the microphone / camera
> doorhanger. You must close the page with that doorhanger open.

Ah, I acknowledged that before closing the tab.

Trying again, the doorhanger no long appears (even if I try a new profile?!) but I do get the 2 ghost windows.

Comment 10

[Nicholas Nethercote [inactive]]

jesup, ghost windows are pretty bad.  Any idea who might be able to work on this?

Comment 11

[Andrew McCreight [:mccr8]]

The same thing is happening on chat.meatspac.es.  I looked at a log, and there are two nsXPCWrappedJS (nsIDOMGetUserMediaSuccessCallback) objects holding everything alive.  The XPCWJS has two references, one the self reference, and one unknown reference that is holding everything alive.

Comment 12

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

Seems related to Bug 949907.

Comment 13

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

jib: can you investigate this one?  At least to the point of deciding if it's related to bug 949907

Comment 14

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

Yes I'm looking at this one.

Comment 15

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

Attachment: Leakproof gUMRunnable + OnNavigation cleanup of MediaManager::mActiveCallbacks

Two things needed to be fixed:

 1. OnNavigation now cleans up mActiveCallbacks (a coat-check for gUMRunnables)
 2. gUMRunnable now cleans up its JS callbacks unless it's "armed" for dispatch. 

I also added a comment on MediaManager's refcounting strategy for runnables off main:

 * GetUserMediaRunnable is meant for dispatch off main thread, where it would
 * be illegal to free JS callbacks. Therefore, it uses a rather risky strategy
 * of holding "already_AddRefed" references to the JS callbacks that must be
 * transfered and released in consequent dispatches arriving on main thread.
 *
 * A special Arm() method must be called before dispatch to enable this
 * behavior, because gUMRunnables are held in other circumstances.

Comment 17

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

Try - https://tbpl.mozilla.org/?tree=Try&rev=b2570e11d47a

Comment 18

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

Comment on attachment 8356574 [details] [diff] [review]
Leakproof gUMRunnable + OnNavigation cleanup of MediaManager::mActiveCallbacks

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

::: dom/media/MediaManager.cpp
@@ +814,5 @@
>      }
>    }
>  
> +  void
> +  Arm() {

Comment what Arm() is doing (I know it's discussed above).

@@ +1666,5 @@
>      return NS_OK;
>  
>    } else if (!strcmp(aTopic, "getUserMedia:response:allow")) {
>      nsString key(aData);
> +    nsRefPtr<GetUserMediaRunnable> gUMRunnable;

Lets change gUMRunnable to gum_runnable to avoid confusion with the gFoo naming convention for globals.  (and below as well).  I realize it predated this change, but since we need to expand it and duplicate it, let's rename.

Comment 19

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

Attachment: Leakproof gUMRunnable + OnNavigation cleanup of MediaManager::mActiveCallbacks (2) r=jesup

Added comments and renamed "gUMRunnable" to just "runnable" (which became available with this patch).

Carrying forward r+ from jesup.

Comment 20

[Olli Pettay [:smaug][[e-mail]]]

See also https://bugzilla.mozilla.org/show_bug.cgi?id=951491#c15
and 
https://bugzilla.mozilla.org/show_bug.cgi?id=951491#c12

Comment 21

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/cad9ce8315a4

Comment 22

[Ryan VanderMeulen [:RyanVM]]

Does this need backporting?

Comment 23

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

The leak has been there since the gUM permission prompt was introduced AFAIK, so it's not a regression per se. But clearly the sooner the fix appears in users' hands the better. What's customary?

Comment 24

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

Your call.

Comment 26

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

I'd say nominate it for Aurora.  Not a major problem if it's not accepted, as it's a memory leak issue.

Comment 27

[Olli Pettay [:smaug][[e-mail]]]

This kind of memory leak is increasing cycle collections times, so it may cause significant jank.

Comment 28

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

Comment on attachment 8357383 [details] [diff] [review]
Leakproof gUMRunnable + OnNavigation cleanup of MediaManager::mActiveCallbacks (2) r=jesup

[Approval Request Comment]
Bug caused by (feature/regressing bug #): gUM permission prompt feature.
User impact if declined: Window leak. Increased cycle collection times, jank.
Testing completed (on m-c, etc.): Landed on m-c two weeks ago. Verified locally.
Risk to taking this patch (and alternatives if risky): Low. Just MediaManager, no dependencies.
String or IDL/UUID changes made by this patch: None

Comment 29

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/29a536174b77

Comment 30

[Petruta Horea [:phorea], Desktop QA]

Verified as fixed on Firefox 28 beta 1 (20140205162153) and latest Aurora 29.0a2 (20140205004001) under Win 7 64-bit, Ubuntu 32-bit and Mac OSX 10.8.5.