945614 - [B2G getUserMedia] fake video should not initialize camera

Status: RESOLVED INCOMPLETE

(Core :: WebRTC, defect, P4)

Description

[Alfredo Yang (:alfredo)]

Steps:
1. http://people.mozilla.org/~ayang/gum/gUM%20Test%20Page.html
2. press FakeVideo
3. press the link "Main webrtc demo page" to open another gUM test page.
4. press video

The camera can't be initialize due the the fake video will create the camera manager.
And the new WindowID won't be added into the WindowTable in DOMCameraManager::Register.

Comment 1

[Alfredo Yang (:alfredo)]

Attachment: gUM_fake_perm_donot_init_camera

Comment 2

[Alfredo Yang (:alfredo)]

After investigation, windowID among MediaEngine, CameraManager and MediaManager are not the sync.

I print window id at:
http://dxr.mozilla.org/mozilla-central/source/dom/media/MediaManager.cpp#1288
http://dxr.mozilla.org/mozilla-central/source/content/media/webrtc/MediaEngineWebRTCVideo.cpp#499
http://dxr.mozilla.org/mozilla-central/source/dom/camera/DOMCameraManager.cpp#144

Condition 1, in normal video gUM:

                            1st gUM(video)   2nd gUM(video)  3rd gUM(video)
--------------------------------------------------------------------------
MediaManager                 6                7               8
MediaEngineWebRTC\
VideoSource                  6                6               6
DOMCameraManager             6                6               6

It works because MediaEngine and DOMCameraManager use the same windowID. DOMCameraControl is created by MediaEngine and has the same windowID. There windowId test in nsDOMCameraManager::IsWindowStillActive [1] will be success, although both modules use the wrong windowID actually.



Condition 2, in mochitest test:

                                1st gUM(fake)          2nd gUM(video)  3rd gUM(video)
----------------------------------------------------------------------------------
MediaManager                     6                      7               8
MediaEngine (MediaEngineDefault)      (MediaEngineWeb\
                                 null  RTCVideoSource)  7               7
DOMCameraManager                 6                      6               6


The 2nd/3rd gUM don't work because MediaEngine and DOMCameraManager use different windowID and it will fail at [1].
My patch will disable the DOMCameraManger at 1st gUM which will sync the windowID on MediaEngine and DOMCameraManager at 2nd/3rd, that'd make situation same as condition 1.
To fix the problem in condition 1; IMHO, it probbably needs to avoid using static DOMCameraManager and remove mBackend from static MediaManager.


[1] http://dxr.mozilla.org/mozilla-central/source/dom/camera/DOMCameraControl.cpp#524

Comment 3

[u459114]

Hi Randell, need your valuable input here, thanks

Comment 4

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

I think the difference may be between InnerWindow IDs and OuterWindow IDs; for example see MediaManager::MediaCaptureWindowStateInternal() which finds all GUM streams for a given OuterWindow.

See https://developer.mozilla.org/en-US/docs/Inner_and_outer_windows

Note that the MediaManager's list is not all InnerWindows - see MediaManager::RemoveFromWindowList() for an example of how they're handled.  MediaManager keeps a hash table keyed by WindowID of active gUM streams, and it uses the windowID it was given by Navigator.mozGetUserMedia() - there's no GetInnerWindow/etc in the code for the MediaManager.cpp spot where you dump the WindowID.

(Iframes complicate things a bit, but mostly for the UI, not for MediaManager - see above about MediaCaptureWindowStateInternal() for something that deals with iframes.)  You need to be very careful that you compare windows correctly and deal with their tree-like structure - simple ID comparisons are often wrong.

I think the problem (per above) comes from looking up a window id in DOMCameraManager that might not be an inner id; DOMCameraManager appears to store inner windows only.


Needinfo to roc & smaug to verify my assessment or give suggestions (since we need to get this resolved ASAP).  FYI, from IRC with alfredo, I believe the issue is that if we init DOMCameraManager for fake streams, it's not cleaning up correctly (apparently due to the WindowID mismatch).  It may be that significant rework of how WindowIDs are handling between MediaEngine and DOMCameraManager is needed, or perhaps just some judicious use of Inner/OuterWindow calls.  (Note DOMCameraManager is B2G-only, and I don't know all the details of how it uses WindowIDs - just what I saw from inspection, and WindowIDs can be confusing.)

Concerning the patch here: I had told alfredo I didn't want to make "fake" streams avoid normal initialization (as much as possible) because I want us to test as much of the normal flow as possible, and also because adding a new state of "we're running but the camera manager isn't inited" makes things more complex and may hide issues (like this WindowID issue).  His message I'm responding to was the result of his diving into why DOMCameraManager wasn't cleaning up for fake streams.

Comment 5

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

(In reply to Randell Jesup [:jesup] from comment #4)
> I think the difference may be between InnerWindow IDs and OuterWindow IDs;
> for example see MediaManager::MediaCaptureWindowStateInternal() which finds
> all GUM streams for a given OuterWindow.

Hmm, sounds odd that it plays with outerwindow IDs. inner window ID is the one which maps better to
a web page. outer window is mapped to docshell lifetime.

Comment 6

[Alfredo Yang (:alfredo)]

(In reply to Randell Jesup [:jesup] from comment #4)
> Concerning the patch here: I had told alfredo I didn't want to make "fake"
> streams avoid normal initialization (as much as possible) because I want us
> to test as much of the normal flow as possible, and also because adding a
> new state of "we're running but the camera manager isn't inited" makes
> things more complex and may hide issues (like this WindowID issue).  His
> message I'm responding to was the result of his diving into why
> DOMCameraManager wasn't cleaning up for fake streams.

I think there is another way to make 'fake' with camera initialization working. I will upload another patch later. Could you please feedback it? Thanks.

Comment 7

[Alfredo Yang (:alfredo)]

Attachment: gUM_Set_WindowID

Comment 8

[Alfredo Yang (:alfredo)]

Attachment: gUM_Set_WindowID

Comment 9

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

(In reply to Randell Jesup [:jesup] from comment #4)
> I think the difference may be between InnerWindow IDs and OuterWindow IDs;
> for example see MediaManager::MediaCaptureWindowStateInternal() which finds
> all GUM streams for a given OuterWindow.
> 
> See https://developer.mozilla.org/en-US/docs/Inner_and_outer_windows
> 
> Note that the MediaManager's list is not all InnerWindows - see
> MediaManager::RemoveFromWindowList() for an example of how they're handled. 
> MediaManager keeps a hash table keyed by WindowID of active gUM streams, and
> it uses the windowID it was given by Navigator.mozGetUserMedia() - there's
> no GetInnerWindow/etc in the code for the MediaManager.cpp spot where you
> dump the WindowID.
> 
> (Iframes complicate things a bit, but mostly for the UI, not for
> MediaManager - see above about MediaCaptureWindowStateInternal() for
> something that deals with iframes.)  You need to be very careful that you
> compare windows correctly and deal with their tree-like structure - simple
> ID comparisons are often wrong.
> 
> I think the problem (per above) comes from looking up a window id in
> DOMCameraManager that might not be an inner id; DOMCameraManager appears to
> store inner windows only.

My explanation here is not right (I checked more of the code):


1) all windows in the MediaManager list are inner windows obtained from Navigator and verified by mWindow->GetOuterWindow()->getCurrentInnerWindow() == mWindow.  This is then used to get the WindowId and that's used for the hashtable entry/chain.

2) Note that per the diagram in the mdn page, an inner window can have iframes with their own outer and inner windows.

3) Note that MediaCaptureWindowStateInternal() does in fact find all streams for an outerwindow, but by iterating the window tree and finding all the innerwindows, and then asking the hashtable for any gUM() captures for those innerwindows.


The *real* issue here is that MediaEngineWebRTCVideoSource's (which are tied to the DOMCameraControl objects) are used to provide the WindowID, but MediaEngineWebRTCVideoSources are long-lived; they do not die when the page does, so having them hold a WindowID is a bad idea.  That's why the WindowIDs are the value of the first time the VideoSource was created and never change.  (They don't use WindowID for non-B2G code.)

In MediaEngineWebRTC::EnumerateVideoDevices(), in the B2G-specific ifdef, it does "if (mVideoSources.Get(...))" and if the source already exists, appends it tot he list of devices, otherwise it creates it.

The VideoSource, when Allocate() is called on it to activate it, dispatches an AllocImpl() to mainthread, and that creates the DOMCameraControl using the saved mWindowId - but note this is the window used for the *first* call to Enumerate the video devices, not the *current* getUserMedia() call!

The question is: for DOMCameraControls created from a gUM call, which are tied to an allocated VideoSource:  Do the need to know the window at all?  Normally it's used for DOMCameraManager::OnNavigation to clear the DOMCameraControls out - but the gUM mediaStreams will get hit with MediaManager::OnNavigation as well, and will Deallocate() any VideoSources, which will release the DOMCameraControls.

So, I'd posit the best solution is to let MediaManager handle freeing up the DOMCameraControls associated with it, and not add them to the main DOMCameraWindow table.  The only complication is that as DOM objects, DOMCameraControls must be freed on MainThread, but the AllocImpl/DeallocImpl calls already run there, so it's not a problem.

The other use of the table is to free all the DOMCameraControls on shutdown.  Similarly, all MediaManager captures get shut down, so the DOMCameraControls should get shut down via MediaManager anyways.

Comment 10

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

Attachment: Don't track windowIDs in MediaEngine for B2G

Builds clean (and fixes some B2G-specific warnings)

Comment 11

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

Comment on attachment 8343604 [details] [diff] [review]
Don't track windowIDs in MediaEngine for B2G

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

Michael - Not sure who's the best to review this (normally I would), but you've touched this code, and schient wrote some of it.

I'll take a review from either mwu or schien.  schien/alfredo/etc - can you give the a try (and submit a Try?)  I'll put one up too, but I'm not sure I have the right set of patches.

Comment 12

[Shih-Chiang Chien [:schien] (UTC+8) (use ni? plz)]

Comment on attachment 8343604 [details] [diff] [review]
Don't track windowIDs in MediaEngine for B2G

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

lgtm for the webrtc part, but I'm not confident to review DOMCameraManager. Add @mikeh for reviewing this part.

::: content/media/webrtc/MediaEngineWebRTC.h
@@ +51,5 @@
>  #include "GonkCameraControl.h"
>  #include "ImageContainer.h"
>  #include "nsGlobalWindow.h"
>  #include "prprf.h"
> +#include "nsProxyRelease.h"

nit: Except for the comment in line 402, I didn't see any code that really need this #include.

Comment 13

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

Attachment: Don't track windowIDs in MediaEngine for B2G

Probably need to Register() the CameraControl always to avoid problems with IsWindowStillActive() (thanks alfredo)

Comment 14

[Mike Habicher [:mikeh] (high bugzilla latency)]

Comment on attachment 8343470 [details] [diff] [review]
gUM_Set_WindowID

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

r+ on the DOMCameraManager changes; though bug 909542 (when it lands) will remove the MediaEngine's interactions with DOM layers in the b2g camera code. Instead, everything will be through a narrowly-defined ICameraControl that doesn't involve windows, JSContexts, etc.

Comment 15

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

https://hg.mozilla.org/integration/b2g-inbound/rev/1d72742584ad
r+ from mikeh in IRC for the correct patch

Comment 16

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/1d72742584ad

Comment 17

[Carsten Book [:Tomcat]]

backedout as part of https://tbpl.mozilla.org/?rev=df82be9d89a5 because of https://bugzilla.mozilla.org/show_bug.cgi?id=853356#c209

Comment 18

[Ryan VanderMeulen [:RyanVM]]

Backed out from Aurora along with bug 853356.

Comment 19

[Ivan Tsay (:ITsay)]

Hi Randell,

Because of https://bugzilla.mozilla.org/show_bug.cgi?id=853356#c209, this patch is also backed out. I wonder if this one will be just re-landed after the other test cases are passed in bug 853356. Thank you!

Comment 20

[Maire Reavy [:mreavy]]

(In reply to Ivan Tsay (:ITsay) from comment #19)
> Hi Randell,
> 
> Because of https://bugzilla.mozilla.org/show_bug.cgi?id=853356#c209, this
> patch is also backed out. I wonder if this one will be just re-landed after
> the other test cases are passed in bug 853356. Thank you!

Yes, Ivan.  That is correct.  Once the new test cases for bug 853356 have been added (see bug 948826) and are passing, we will re-land the patch on this bug together with the patches on bug 853356.

Comment 21

[Maire Reavy [:mreavy]]

Randell -- Please unbitrot and reland

Comment 22

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

Major refactors have made this bug basically moot.