Closed Bug 973001 Opened 6 years ago Closed 6 years ago

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

Categories

(Firefox :: Site Permissions, defect, P1)

x86
macOS
defect
Points:
8

Tracking

()

VERIFIED FIXED
Firefox 35
Iteration:
35.2
Tracking Status
e10s m3+ ---

People

(Reporter: jib, Assigned: florian)

References

(Blocks 1 open bug)

Details

Attachments

(4 files, 4 obsolete files)

No description provided.
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)
Attached patch webrtc (obsolete) — Splinter Review
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.
Attached patch webrtc v2 (obsolete) — Splinter Review
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
Blocks: e10s-webrtc
Assignee: mconley → felipc
Status: NEW → ASSIGNED
Iteration: --- → 35.1
Points: --- → 8
Flags: qe-verify+
Flags: firefox-backlog+
Attached patch webrtc-globalindicators (obsolete) — Splinter Review
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)
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.
QA Contact: drno
Assignee: felipc → florian
Iteration: 35.1 → 35.2
Attached patch Updated webrtc-globalindicators (obsolete) — Splinter Review
Attachment #8483754 - Attachment is obsolete: true
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)
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)
Summary: getUserMedia doesn't work in e10s window (no permission prompt appears) → getUserMedia UI doesn't work with e10s (no permission prompt appears)
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+
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)
Attachment #8493047 - Flags: review?(paul) → review+
(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
Depends on: 1071626
Depends on: 1071627
(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
Depends on: 1074202
Depends on: 1098437
Depends on: 1104054
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
Component: WebRTC: Audio/Video → Device Permissions
Product: Core → Firefox
Target Milestone: mozilla35 → Firefox 35
Depends on: 1126231
You need to log in before you can comment on or make changes to this bug.