getUserMedia UI doesn't work with e10s (no permission prompt appears)

VERIFIED FIXED in Firefox 35

Status

()

Firefox
Device Permissions
P1
normal
VERIFIED FIXED
4 years ago
3 months ago

People

(Reporter: jib, Assigned: florian)

Tracking

(Blocks: 1 bug)

Trunk
Firefox 35
x86
Mac OS X
Points:
8
Dependency tree / graph
Bug Flags:
firefox-backlog +
qe-verify +

Firefox Tracking Flags

(e10sm3+)

Details

Attachments

(4 attachments, 4 obsolete attachments)

Comment hidden (empty)
Blocks: 879538
Workaround: set media.navigator.permission.disabled=true in about:config
Assignee: jib → nobody
Summary: getUserMedia doesn't work in e10s window (no prompt appears) → getUserMedia doesn't work in e10s window (no permission prompt appears)
tracking-e10s: --- → +
Blocks: 997462
Created attachment 8423921 [details] [diff] [review]
webrtc

This is an unfinished patch that's been sitting on my laptop for a few months. Just posting it so I don't lose it.
Created attachment 8423922 [details] [diff] [review]
webrtc v2

Oops, I guess I needed to qref first.
Attachment #8423921 - Attachment is obsolete: true
Target Milestone: --- → mozilla33
Brad -- Do you have any problems if we address this in Fx34 instead of Fx33?
Flags: needinfo?(blassey.bugs)
Target Milestone: mozilla33 → mozilla34
(In reply to Maire Reavy [:mreavy] (Please needinfo me) from comment #6)
> Brad -- Do you have any problems if we address this in Fx34 instead of Fx33?

No, that's fine
Flags: needinfo?(blassey.bugs)
Assignee: nobody → wmccloskey
Priority: -- → P1
Yoink.
Assignee: wmccloskey → mconley

Updated

4 years ago
Blocks: 849746
Assignee: mconley → felipc

Updated

4 years ago
Status: NEW → ASSIGNED
Iteration: --- → 35.1
Points: --- → 8
Flags: qe-verify+
Flags: firefox-backlog+
Created attachment 8483754 [details] [diff] [review]
webrtc-globalindicators

First part (the easy one) is to handle the global indicators. This should be a good code starting point to base the other parts too.

The other two parts to this bug are: the per-browser indicators, and the request UI (which may or may not be broken down further, we'll figure it out soon)
Attachment #8423922 - Attachment is obsolete: true
Attachment #8483754 - Flags: review?(florian)
(Assignee)

Comment 10

4 years ago
Comment on attachment 8483754 [details] [diff] [review]
webrtc-globalindicators

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

::: browser/base/content/content.js
@@ +21,5 @@
>  XPCOMUtils.defineLazyModuleGetter(this, "UITour",
>    "resource:///modules/UITour.jsm");
>  XPCOMUtils.defineLazyModuleGetter(this, "FormSubmitObserver",
>    "resource:///modules/FormSubmitObserver.jsm");
> +XPCOMUtils.defineLazyModuleGetter(this, "ContentWebRTC",

Do you still want to make this lazy if we are using it unconditionally?

::: browser/modules/ContentWebRTC.jsm
@@ +23,5 @@
> +    }
> +    this._initialized = true;
> +
> +    Services.obs.addObserver(updateIndicators, "recording-device-events", false);
> +  },

nit: trailing comma.

@@ +55,5 @@
> +    else if (app.value && !state.showScreenSharingIndicator)
> +      state.showScreenSharingIndicator = "Application";
> +
> +    let mm = getMessageManagerForWindow(contentWindow);
> +    mm.sendAsyncMessage("webrtc:UpdateIndicators", state);

You want to send the message outside of the for loop.

Is there a way to send the message to the parent process without associating it with a specific browser?

::: browser/modules/webrtcUI.jsm
@@ +31,5 @@
> +               .getService(Ci.nsIMessageListenerManager);
> +    mm.addMessageListener("webrtc:UpdateIndicators", this);
> +  },
> +
> +  receiveMessage: function (message) {

nit: other methods in this file don't have a space between 'function' and '(' (init and uninit are the exceptions here).

@@ +34,5 @@
> +
> +  receiveMessage: function (message) {
> +    switch (message.name) {
> +      case "webrtc:UpdateIndicators":
> +        updateIndicators2(message.data, message.target)

You haven't defined an updateIndicators2 function, and the updateIndicators function only takes one 'data' parameter.

@@ +44,5 @@
>      Services.obs.removeObserver(handleRequest, "getUserMedia:request");
>      Services.obs.removeObserver(updateIndicators, "recording-device-events");
>      Services.obs.removeObserver(removeBrowserSpecificIndicator, "recording-window-ended");
>      Services.obs.removeObserver(maybeAddMenuIndicator, "browser-delayed-startup-finished");
> +    mm.removeMessageListener("webrtc:UpdateIndicators", this);

This won't work if you don't define mm here.
Attachment #8483754 - Flags: review?(florian) → review-
Moving old M2 P1 bugs to M3.
tracking-e10s: + → m3+

Updated

4 years ago
QA Contact: drno

Updated

4 years ago
Assignee: felipc → florian
Iteration: 35.1 → 35.2
Created attachment 8489455 [details] [diff] [review]
Updated webrtc-globalindicators
Attachment #8483754 - Attachment is obsolete: true
(Assignee)

Comment 13

4 years ago
Created attachment 8491492 [details] [diff] [review]
Part 1 - fix urlbar indicators, show global indicators

This patch fixes the url bar icons (including the "Stop sharing" feature), and shows the global indicators.

To try this patch with e10s enabled, you still need to set media.navigator.permission.disabled to true. I'm working on the second part that will fix the permission prompts.

To limit the scope of this bug, I'll file follow-up bugs for:
- make the global indicators actually list the streams with e10s enabled (with the attached patch they work with e10s off, and show empty lists with e10s on).
- handle removing the indicators if the child process crashed
- make the browser_devices_get_user_media.js test pass with e10s.
Attachment #8489455 - Attachment is obsolete: true
Attachment #8491492 - Flags: review?(felipc)
(Assignee)

Comment 14

4 years ago
Created attachment 8491695 [details] [diff] [review]
Part 2 - handle permission requests

Note: the browser_devices_get_user_media.js test is broken and will need a fix. I haven't finished debugging, but I've debugged enough to know the problem is in the test rather than in the patch I'm attaching. Part 3 will fix the test :).
Attachment #8491695 - Flags: review?(felipc)
(Assignee)

Updated

4 years ago
Summary: getUserMedia doesn't work in e10s window (no permission prompt appears) → getUserMedia UI doesn't work with e10s (no permission prompt appears)
(Assignee)

Comment 15

4 years ago
Created attachment 8492222 [details] [diff] [review]
Part 3 - fix test

Both audio and video devices are now stored in the same array, so hardcoding 0 as the device index didn't work anymore to re-enable a device type.
Attachment #8492222 - Flags: review?(felipc)
Comment on attachment 8491492 [details] [diff] [review]
Part 1 - fix urlbar indicators, show global indicators

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

::: browser/base/content/content.js
@@ +638,5 @@
>  };
>  DOMFullscreenHandler.init();
>  
> +ContentWebRTC.init();
> +addMessageListener("webrtc:StopSharing", ContentWebRTC);

let's verify that this does not need to be removed
Attachment #8491492 - Flags: review?(felipc) → review+
Attachment #8492222 - Flags: review?(felipc) → review+
Attachment #8491695 - Flags: review?(felipc) → review+
(Assignee)

Comment 17

4 years ago
Created attachment 8493047 [details] [diff] [review]
Part 4 - fix devtools test

I pushed the patches to try, and try seems happy except for a few existing intermitents and a devtools test that attachment 8491492 [details] [diff] [review] breaks (fix attached).
Attachment #8493047 - Flags: review?(paul)

Updated

4 years ago
Attachment #8493047 - Flags: review?(paul) → review+
(Assignee)

Comment 18

4 years ago
(In reply to :Felipe Gomes from comment #16)

> ::: browser/base/content/content.js

> > +addMessageListener("webrtc:StopSharing", ContentWebRTC);
> 
> let's verify that this does not need to be removed

Most of the addMessageListener calls from content.js don't have associated removeMessageListener calls, and the try push is green (except for a failure that now has an r+'ed fix), so I don't think we are leaking.

https://hg.mozilla.org/integration/fx-team/rev/846759ed5d7c
https://hg.mozilla.org/integration/fx-team/rev/ba660b435bb5
https://hg.mozilla.org/integration/fx-team/rev/7917631e88a4
https://hg.mozilla.org/integration/fx-team/rev/3a96b277afac
(Assignee)

Updated

4 years ago
Depends on: 1071626
(Assignee)

Updated

4 years ago
Depends on: 1071627
(Assignee)

Comment 20

4 years ago
(In reply to Florian Quèze [:florian] [:flo] from comment #13)

> To limit the scope of this bug, I'll file follow-up bugs for:

Done.

> - make the global indicators actually list the streams with e10s enabled
> (with the attached patch they work with e10s off, and show empty lists with
> e10s on).

Bug 1071626

> - handle removing the indicators if the child process crashed

Bug 1071627

> - make the browser_devices_get_user_media.js test pass with e10s.

Bug 1071623
(Assignee)

Updated

4 years ago
Depends on: 1074202
Depends on: 1098437
(Assignee)

Updated

4 years ago
Depends on: 1104054
(Assignee)

Updated

4 years ago
Depends on: 1107967
We tested getUserMedia using latest Nightly across platforms and found two issues that are not related with getUserMedia. More details about our testing can be found in the etherpad we created: https://etherpad.mozilla.org/getUserMedia-e10s

Marking this as verified fixed.
Status: RESOLVED → VERIFIED
(Assignee)

Updated

4 years ago
Component: WebRTC: Audio/Video → Device Permissions
Product: Core → Firefox
Target Milestone: mozilla35 → Firefox 35
(Assignee)

Updated

3 years ago
Depends on: 1126231
You need to log in before you can comment on or make changes to this bug.