Closed Bug 1249365 Opened 5 years ago Closed 5 years ago

Latest Nightly 47.0a1 breaks Hello - No camera/microphone found

Categories

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

defect

Tracking

()

VERIFIED FIXED
mozilla48
Tracking Status
firefox46 --- unaffected
firefox47 --- verified
firefox48 --- verified

People

(Reporter: RT, Assigned: gcp)

References

Details

(Keywords: regression)

Attachments

(4 files, 1 obsolete file)

STR:
1 Open Panel
2 Join a room
3 Get message "no camera or microphone found"

Console log:
Empty string passed to getElementById(). remote-browser.xml:222:7
unreachable code after return statement sdk.js:11464:4
mutating the [[Prototype]] of an object will cause your code to run very slowly; instead create the object with the correct initial [[Prototype]] value using Object.create sdk.js:12164:3
The Components object is deprecated. It will soon be removed. utils.js:9:15
No stores registered for event type  metricsLogJoinRoom dispatcher.js:88:9
Security Error: Content at chrome://browser/skin/identity-secure.svg attempted to load chrome://browser/skin/identity-secure.svg#mask-clasp-cutout, but may not load external data when being used as an image.
mutating the [[Prototype]] of an object will cause your code to run very slowly; instead create the object with the correct initial [[Prototype]] value using Object.create application-e4a4d1bb98f0e8b3c62fce2ce19244e3.js:58598:35
OT.Publisher.onStreamAvailableError SourceUnavailableError: Unknown Error while getting user media sdk.js:31778:5

OT.exception :: title: Unable to Publish (1500) msg: GetUserMedia sdk.js:28806:3

POST 
https://loop-dev.stage.mozaws.net/v0/rooms/rm35P5Sg-7A [HTTP/1.1 400 Bad Request 159ms]
POST 
https://loop-dev.stage.mozaws.net/v0/rooms/rm35P5Sg-7A [HTTP/1.1 400 Bad Request 329ms]
POST 
https://loop-dev.stage.mozaws.net/v0/rooms/rm35P5Sg-7A [HTTP/1.1 400 Bad Request 425ms]
Loop:Loop hawkRequest error: Object { code: 400, errno: 203, error: "Unable to leave a room you have not…" } MozLoopService.jsm:706
Loop:Object { code: 400, errno: 203, error: "Unable to leave a room you have not…" } LoopRooms.jsm:818
Loop:Loop hawkRequest error: Object { code: 400, errno: 203, error: "Can't update status for a room you …" } MozLoopService.jsm:706
Loop:Loop hawkRequest error: Object { code: 400, errno: 203, error: "Can't update status for a room you …" } MozLoopService.jsm:706
Looking at the timescales, this looks like a regression from bug 1177242.

There's no errors reported, we just don't get a camera/microphone.

Switching to fake streams works - which explains why our tests are still passing as they don't need to go the permissions route.
Blocks: 1177242
Severity: normal → blocker
Component: Client → WebRTC: Audio/Video
Flags: needinfo?(mreavy)
Flags: needinfo?(gpascutto)
Keywords: regression
Product: Hello (Loop) → Core
Summary: Latest Nightly 47.0a1 breaks Hello with beta add-on 1.2.0 → Latest Nightly 47.0a1 breaks Hello - No camera/microphone found
NSPR_LOG_MODULES=CamerasParent:5,CamerasChild:5 probably reveals what's up. I'm guessing Hello is bypassing regular permissions in MediaManager and so gets rejected because there's no recorded proof it got permission to use the camera.

Whatever grants the permission should store it in nsIPermissionManager, or we should special case Hello/access from chrome if we can detect it.

(It's also one of those cases where me might end up duplicating permission logic between MediaManager and CamerasParents that the original bug moans about)
Flags: needinfo?(gpascutto)
As you can see, gcp has already started to look at this, and we agree with Standard8 that it's due to bug 1177242 landing.   We (gcp, Standard8 and I) are starting to talk about it in #media now if anyone wants to join -- or read the irc logs later.
Flags: needinfo?(mreavy)
http://mxr.mozilla.org/mozilla-central/source/dom/media/MediaManager.cpp#1694

Maybe I can fix MediaManager to set a specific "priviledged" or "chrome" permission and then pick that up in CamerasParent.

I think it'd be more secure to check whether the originating IPC request is coming from a privileged process, but not sure we can full off something like that.
So this is really the problem:

"For tab sharing, Loop has implicit permissions within Firefox, as it is built-in, and will manage the active tab and provide appropriate UI."

https://dxr.mozilla.org/mozilla-central/rev/e1cf617a1f2813b6cd66f460313a61c223406c9b/dom/media/MediaManager.cpp#1949
Surprisingly, I'm not hitting that branch of the if() for my failure!

This check

https://dxr.mozilla.org/mozilla-central/rev/e1cf617a1f2813b6cd66f460313a61c223406c9b/dom/media/MediaManager.cpp#1858

fails, which means we never set the priviledged = false for Loop, and still access the camera.

Is it intentional that all that logic gets bypassed?
Flags: needinfo?(rjesup)
Flags: needinfo?(jib)
Straightforward fix. The way the Loop logic gets special cased in MediaManager OTOH...
Attachment #8722084 - Flags: review?(rjesup)
(In reply to Gian-Carlo Pascutto [:gcp] from comment #6)
> Surprisingly, I'm not hitting that branch of the if() for my failure!
> 
> This check
> 
> https://dxr.mozilla.org/mozilla-central/rev/
> e1cf617a1f2813b6cd66f460313a61c223406c9b/dom/media/MediaManager.cpp#1858
> 
> fails, which means we never set the priviledged = false for Loop, and still
> access the camera.
> 
> Is it intentional that all that logic gets bypassed?

Yes, it is intentional. Hello is privileged and accesses the camera without a user prompt, always has.

However, Hello punted on building its own selector for windows, applications and screens, and relies on the built-in permission prompt for this.
Flags: needinfo?(jib)
Clarified on IRC:

<Standard8> gcp: yes, effectively it picks a random one, I’ve been pushing for device selection since the beginning, but its never quite made priority (bug 1023930) - and last I heard the core code was going to implement something for device switching...

<Standard8> gcp: that comment should probably also change “selection of the device currently” to “selection of the window/screen currently”

So Loop also punted on selecting the Camera, which is why it can use the else {} part of that if: it never "needs" the permission prompt.
Flags: needinfo?(rjesup)
Attachment #8722084 - Flags: review?(rjesup) → review+
Assignee: nobody → gpascutto
Rank: 5
Priority: -- → P1
https://hg.mozilla.org/integration/mozilla-inbound/rev/99a0241ace6b08565e1b8b816e48bb59d39a91e2
Bug 1249365 - Store Loops permission to use the camera so we can check later. r=jesup
https://hg.mozilla.org/mozilla-central/rev/99a0241ace6b
Status: NEW → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla47
Fails in e10s documents:

[Child 90677] ###!!! ASSERTION: Cannot perform action in content process!: 'Error', file /Users/mikedeboer/Projects/fx-team/extensions/cookie/nsPermissionManager.cpp, line 1486
[Child 90677] WARNING: NS_ENSURE_SUCCESS(rv, rv) failed with result 0x80040111: file /Users/mikedeboer/Projects/fx-team/dom/media/MediaManager.cpp, line 2065
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
(In reply to Mike de Boer [:mikedeboer] from comment #12)
> Fails in e10s documents:

Can you be more specific what and how you tested?

The access to nsIPermissionManager that was added is done on the same thread as the non-Loop cases. I don't even get how for Loop that can end up being in *content* given that Loops' problem is that it runs in Chrome.
(In reply to Gian-Carlo Pascutto [:gcp] from comment #13)
> (In reply to Mike de Boer [:mikedeboer] from comment #12)
> > Fails in e10s documents:
> 
> Can you be more specific what and how you tested?

- Start up a browser in e10s mode with "loop.remote.autostart" set to true
- Open the Hello Panel
- Select "Browse this page with a friend".

=> Error happens almost straight away.

> The access to nsIPermissionManager that was added is done on the same thread
> as the non-Loop cases. I don't even get how for Loop that can end up being
> in *content* given that Loops' problem is that it runs in Chrome.

What we have is an about:loopconversation page. The files behind that are loaded from chrome code, but about:loopconversation is actually run in a content context.

With the switch to e10s, we'll therefore be running in the content process, rather than the main browser process.
Actually there seems to be something worse going on.

If I open up a tab to about:config or about:preferences, then opening a Hello conversation window works.

If I open up any other webpage, then we get the no camera/mic found...
gcp: not sure if you saw comment 14/15?
Flags: needinfo?(gpascutto)
I saw. I tried your steps in Nightly but things fail way before I get to Cameras code. Comment 15 seems to illustrate we have some issue with Loop code running on the wrong thread, and after my fix it not working in e10s is just a symptom of that.
Flags: needinfo?(gpascutto)
(In reply to Gian-Carlo Pascutto [:gcp] from comment #17)
> I saw. I tried your steps in Nightly but things fail way before I get to
> Cameras code. Comment 15 seems to illustrate we have some issue with Loop
> code running on the wrong thread, and after my fix it not working in e10s is
> just a symptom of that.

In e10s, Loop now runs in the content process. The basic problem is that nsPermissionManager::Add, which calls nsPermissionManager::AddFromPrincipal, can't be called from the content process. And that's what your patch is doing.

It's hard to test this right now because of an unrelated Loop regression (bug 1253688). If you back out the patch from bug 1229429 locally, you should be able to reproduce this yourself.

STR:
1. Back out patch from bug 1229429.
2. Set the loop.remote.autostart pref to true and restart.
3. Open loop.

You should get an error because the nsPermissionManager::AddFromPrincipal fails.

We recently discovered that this is blocking e10s, so we need a fix as soon as possible.
Flags: needinfo?(gpascutto)
(In reply to Bill McCloskey (:billm) from comment #18)
> In e10s, Loop now runs in the content process. The basic problem is that
> nsPermissionManager::Add, which calls nsPermissionManager::AddFromPrincipal,
> can't be called from the content process. And that's what your patch is
> doing.

I understand. But the patched code looks like it shouldn't be running in content to begin with, as I already said. That is, the code I added is in MediaManager::GetUserMedia and this is written as if it is supposed to run in Chrome: https://dxr.mozilla.org/mozilla-central/rev/e1cf617a1f2813b6cd66f460313a61c223406c9b/dom/media/MediaManager.cpp#2027

My patch covers the "else" of this condition. If the non-priviledged case is expecting to run in chrome and access the same nsIPermissionManager, then it makes no sense to me that the priviledged case has a problem because it's running in content.
Flags: needinfo?(gpascutto)
Clarified on IRC the existing code worked because it's only testing permissions and that's allowed from content. The patch adds permissions and that isn't.

We'll have to back this out and then try to add the permission from any relevant chrome JS that Loop might have. See bug 1250122 for an example.
I talked to gcp on IRC and we agree that we need a different fix here. The permission manager allows permissions to be tested in the content process, but not added. This is to prevent a rogue content process from adding all the permissions it wants.

We think it might work for the Loop Chrome code to add the camera permission from chrome JS before it does anything camera-related. Mark, does that seem reasonable to you? If so, could you write a patch for that? If that works, then we can back out gcp's patch.

As I said above, we want this ASAP for e10s.
Flags: needinfo?(standard8)
These were my attempts to move the permission check to the chrome process. Unfortunately neither putting it in bootstrap on application start up, nor putting it in MozLoopService when the chat window is opened were enough to get this to work at all.

I tried a few different things, but I'm not sure what's happening here.
Attachment #8727160 - Flags: feedback?(wmccloskey)
Attachment #8727160 - Flags: feedback?(gpascutto)
Flags: needinfo?(standard8)
From a quick look at the code only: the real URL isn't about:loopconversation but has some stuff appended to it IIRC.

I'll investigate more monday.
(In reply to Gian-Carlo Pascutto [:gcp] from comment #23)
> From a quick look at the code only: the real URL isn't
> about:loopconversation but has some stuff appended to it IIRC.
> 
> I'll investigate more monday.

I've just done a change where I inserted the real url, this seems to work from a quick check - I'll do some more intensive testing in an hour or two.
Depends on: 1243378
Attachment #8727160 - Flags: feedback?(wmccloskey)
Attachment #8727160 - Flags: feedback?(gpascutto)
Ok, I've done some more testing and this is working fine. I think it's good to go.

I did look at seeing if our permissions test works for this - however, it seems there's other issues with it for e10s mode, see bug 1243378, so I've left that for now and added that bug to our tracking.
Attachment #8727370 - Flags: review?(gpascutto)
Attachment #8727160 - Attachment is obsolete: true
Comment on attachment 8727370 [details] [diff] [review]
Fix regression in e10s mode to re-allow the camera by default in Loop's conversation window.

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

::: browser/extensions/loop/chrome/content/modules/MozLoopService.jsm
@@ +949,5 @@
>        let url = this.getChatURL(windowId);
>  
> +      // Ensure about:loopconversation has access to the camera.
> +      Services.perms.add(Services.io.newURI(url, null, null), "camera",
> +                     Services.perms.ALLOW_ACTION, Services.perms.EXPIRE_SESSION);

nit: indentation looks a bit weird?
Attachment #8727370 - Flags: review?(gpascutto) → review+
https://hg.mozilla.org/integration/fx-team/rev/1dc3cfa74e2b8a69e7ed81ed95fb72089f6e28a7
Bug 1249365 - Fix regression in e10s mode to re-allow the camera by default in Loop's conversation window. r=gcp
Thanks, I fixed the indent and pushed to fx-team, I also pushed the loop specific change to our repo so that the next mere should work fine:

https://github.com/mozilla/loop/commit/186a9919caf46688f8cb16eded8a72187a89c7c6
https://hg.mozilla.org/mozilla-central/rev/1dc3cfa74e2b
Status: REOPENED → RESOLVED
Closed: 5 years ago5 years ago
Resolution: --- → FIXED
Target Milestone: mozilla47 → mozilla48
Comment on attachment 8727370 [details] [diff] [review]
Fix regression in e10s mode to re-allow the camera by default in Loop's conversation window.

Approval Request Comment
[Feature/regressing bug #]: Bug 1177242
[User impact if declined]: In e10s mode, the user won't be able to use Hello as a link generator, as the camera access will be denied.
[Describe test coverage new/current, TreeHerder]: Landed in m-c. We're working on tests to cover this case in bug 1254132.
[Risks and why]: Low, minor change to permissions specific to Hello.
[String/UUID change made/needed]: None
Attachment #8727370 - Flags: approval-mozilla-aurora?
To clarify the flags: the first patch which fixed non-e10s mode landed in 47. The second patch did not.
Comment on attachment 8727370 [details] [diff] [review]
Fix regression in e10s mode to re-allow the camera by default in Loop's conversation window.

This is obviously a release blocker, let's uplift to Aurora47
Attachment #8727370 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
I verified this issue on: 
- Latest Nightly 49.0a1 (2016-05-06)
- Latest Aurora 48.0a2 (2016-05-06)
- 47.0b3 (20160505125249)
using:
- Windows 10 x64
- Ubuntu 14.04 x86 
- Mac OS X 10.11
and I still reproduced it on Ubuntu 14.04 x86 using latest Aurora and latest Nightly. The rest of platforms and builds are verified fixed.
Note: I have to specify that the test included e10s on / off and loop.server pref set on https://loop-dev.stage.mozaws.net/v0, too.
Iulia: thanks for testing this.

Can you try it again in Ubuntu with NSPR_LOG_MODULES=MediaManager:4,GetUserMedia:4 and upload the logs?  Also indicate exactly what prefs you modified and to what values, starting from a fresh profile if possible.  Also re-verify that Beta 47 is good on Ubuntu.

Thanks!
Flags: needinfo?(iulia.cristescu)
(In reply to Randell Jesup [:jesup] from comment #35)
> Iulia: thanks for testing this.
> 
> Can you try it again in Ubuntu with
> NSPR_LOG_MODULES=MediaManager:4,GetUserMedia:4 and upload the logs?  Also
> indicate exactly what prefs you modified and to what values, starting from a
> fresh profile if possible.  Also re-verify that Beta 47 is good on Ubuntu.
> 
> Thanks!

Hello! I tested again this issue on Ubuntu 14.04 x86. In every test I used a clean profile. These are the results:
- reproduced on 49.0a1 (2016-05-06) - default settings
- reproduced on 49.0a1 (2016-05-08) - default settings (not at the first try)
                                    - loop.server pref set on https://loop-dev.stage.mozaws.net/v0 and e10s on
                                    - loop.server pref set on https://loop-dev.stage.mozaws.net/v0 and e10s off
- reproduced on 48.0a2 (2016-05-09) - default settings
                                    - loop.server pref set on https://loop-dev.stage.mozaws.net/v0 and e10s on (not at the first try)
                                    - loop.server pref set on https://loop-dev.stage.mozaws.net/v0 and e10s off
- not reproduced on 47.0b3 build1 (20160505125249) - default settings
                                                   - loop.server pref set on https://loop-dev.stage.mozaws.net/v0 and e10s on
                                                   - loop.server pref set on https://loop-dev.stage.mozaws.net/v0 and e10s off
I also attached the corresponding log files (Log files.zip)
I'm back with updates.
I investigated again this bug on 
- 47.0 build3 (20160604131506)
- 48.0b2 build2 (20160620091522)
- latest Aurora 49.0a2 (2016-06-23)
- latest Nightly 50.0a1 (2016-06-23)
using 
- Windows 10 x64
- Ubuntu 14.04 x86 
- Mac OS X 10.11
The test included e10s on / off and loop.server pref set on https://loop-dev.stage.mozaws.net/v0, too.
The issue is not reproducible anymore. Therefore, I will set the flags accordingly.
Status: RESOLVED → VERIFIED
Version: unspecified → Trunk
You need to log in before you can comment on or make changes to this bug.