Closed Bug 1313758 Opened 3 years ago Closed 3 years ago

WebRTC getUserMedia mediaSource 'browser' broken: Cause: webrtcUI.jsm (listScreenShareDevices) ==> getString() NS_ERROR_FAILURE

Categories

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

49 Branch
x86_64
Linux
defect

Tracking

()

RESOLVED FIXED
mozilla53
Tracking Status
firefox53 --- fixed

People

(Reporter: mjacobsen, Assigned: mchiang)

Details

Attachments

(2 files)

Attempt to start a WebRTC screen share using mediaSource: 'browser' causes NS_ERROR_FAILURE when looking up browser window String in bundle.

Firefox 49.0.2
Build identifier: Mozilla/5.0 (X11; Ubuntu; Linux x86_64; rv:49.0) Gecko/20100101 Firefox/49.0
Target: x86_64-pc-linux-gnu (Ubuntu 14.04 64 bit).

Test scenario:

1) In about:config, add current https domain to media.getusermedia.screensharing.allowed_domains preference. Note that media.getusermedia.browser.enabled is true (by default).
2) Get the 'browser' media device via WebRTC API:
navigator.mediaDevices.getUserMedia({
  audio: false,
  video: { mediaSource: 'browser' }
}).then(
  function(stream) { alert('got stream!'); }
).catch(
  function(e) { alert('getUserMedia() error: ' + e.name); }
);
3) Wait for the screenshare security window chooser to display.


Screenshare security window chooser displays empty label in drop down selection (see attached screenshot) and selecting the only option and clicking "Share Selected Items" results in no callbacks to onSuccess or onFailure functions. The call to getUserMedia neither succeeds nor fails.

In the JS console the following describes the error:

[Exception... "Component returned failure code: 0x80004005 (NS_ERROR_FAILURE) [nsIStringBundle.GetStringFromName]"  nsresult: "0x80004005 (NS_ERROR_FAILURE)"  location: "JS frame :: XStringBundle :: getString :: line 21"  data: no]

getString()                                                                   XStringBundle
listScreenShareDevices()                                               webrtcUI.jsm:451
prompt/options.eventCallback()                                     webrtcUI.jsm:515
PopupNotifications_fireCallback()                                   PopupNotifications.jsm:1040
PopupNotifications_showPanel/notificationsToShow<()     PopupNotifications.jsm:720
filter()                                                                         self-hosted
PopupNotifications_showPanel()                                     PopupNotifications.jsm:719
PopupNotifications_update()                                          PopupNotifications.jsm:844
PopupNotifications_show()                                             PopupNotifications.jsm:394
prompt()                                                                      webrtcUI.jsm:582
this.webrtcUI.receiveMessage()                                      webrtcUI.jsm:216


Repeated tests will occasionally display the actual browser title bar text, instead of an empty String, in the screenshare security chooser. But the error is the same in the console and the behavior is otherwise identical insofar as the call to getUserMedia does not callback.


The call to getUserMedia should have succeeded or failed, calling back one of the onSuccess or onFailure functions.

NOTE: disabling the screenshare security chooser by setting about:config preference media.navigator.permission.disabled to true, avoids the issue and correctly provides the browser stream to the onSuccess callback. So it appears browser based WebRTC screensharing does work if the screenshare security chooser is avoided.
Line 451 in webrtcUI.jsm corresponds to the marked line below:

      function listScreenShareDevices(menupopup, devices) {
        while (menupopup.lastChild)
          menupopup.removeChild(menupopup.lastChild);

        let type = devices[0].mediaSource;
        let typeName = type.charAt(0).toUpperCase() + type.substr(1);

        let label = chromeDoc.getElementById("webRTC-selectWindow-label");
        let stringId = "getUserMedia.select" + typeName;
        label.setAttribute("value",
--->                       stringBundle.getString(stringId + ".label"));
        label.setAttribute("accesskey",
                           stringBundle.getString(stringId + ".accesskey"));

        // "No <type>" is the default because we can't pick a
        // 'default' window to share.
        addDeviceToList(menupopup,
                        stringBundle.getString("getUserMedia.no" + typeName + ".label"),
                        "-1");
        menupopup.appendChild(chromeDoc.createElement("menuseparator"));
Are we supporting tab sharing? I thought this was dropped when we removed Hello support (and that's most likely when we removed the strings) ?
Flags: needinfo?(mreavy)
I didn't realize the discontinuation of Hello also meant the discontinuation of mediaSource: browser (MediaEngineTabVideoSource). Can someone confirm?

If it is supported still, can we add these strings back?

If it's not supported anymore, we should at least throw an error (call the on onFailure callback) when mediaSource: 'browser' is specified. Especially, given that it seems to still be fully supported in all other respects.
Based on my testing, using mediaSource: 'browser' seems equivalent to using 'window' where the screenshare security chooser is simply limited to just the current browser window. This is really why I was using 'browser' to begin with. To avoid the confusion of selecting the current browser window in the chooser when using mediaSource: 'window'.

Can one pass in the browser's window id in the constraints using the experimental flag browserWindow to pre-select the correct window? Something like this seems like A) it'd be pretty helpful since there's no thumbnails like in Chrome, B) obviate the need for mediaSource: 'browser', and C) not too much work, maybe?
(In reply to Florian Quèze [:florian] [:flo] from comment #2)
> Are we supporting tab sharing? I thought this was dropped when we removed
> Hello support (and that's most likely when we removed the strings) ?

My reading of the spec for screen sharing (https://www.w3.org/TR/screen-capture) says for Firefox to be compliant we would need to support tab capturing.  If I'm reading the spec correctly, 'browser' can be a single tab (document) or a full browser window.  We can verify this with Martin Thomson, who is one of the draft authors.

Firefox does not have and never has had UI (other than in Hello) for tab sharing.  To comply with the spec we will need to have it at some point, but it does not have to happen and will not happen in the current release.  To be extra clear: for this release (Firefox 52) we should not support a mediasource of 'browser'.

We should file a follow up meta bug for screen sharing compliance.  I believe we will need additional work, beyond the UI, to be fully spec compliant, and I believe it makes sense to plan to do that work in H1 of 2017.
Flags: needinfo?(mreavy)
(In reply to Maire Reavy [:mreavy] from comment #5)
> To
> be extra clear: for this release (Firefox 52) we should not support a
> mediasource of 'browser'.

If I understand you correctly, we need a platform fix in 52 to call the failure callback immediately in this case without triggering the UI.
Component: Device Permissions → WebRTC: Audio/Video
Product: Firefox → Core
Maybe we just want to set media.getusermedia.browser.enabled to false by default for now.
Wfm, provided we keep the tests alive [1].

Note we currently implement an outdated draft [2] of the spec, so there's more work to do before we're spec compliant, but functionality wise it's the same.

[1] https://dxr.mozilla.org/mozilla-central/source/dom/media/tests/mochitest/test_getUserMedia_basicTabshare.html
[2] http://fluffy.github.io/w3c-screen-share/#screen-based-video-constraints
Status: UNCONFIRMED → NEW
Ever confirmed: true
To be clear, I'm agreeing with Florian we should set media.getusermedia.browser.enabled to false (and modify the tests to set it to true so they still work and test the code).
Rank: 21
Priority: -- → P2
Assignee: nobody → mchiang
Comment on attachment 8815612 [details]
Bug 1313758 - set media.getusermedia.browser.enabled to default false;

https://reviewboard.mozilla.org/r/96484/#review96978

Looks good
Attachment #8815612 - Flags: review+
Pushed by cbook@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/617cd94bd7c5
set media.getusermedia.browser.enabled to default false; r=jesup
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/617cd94bd7c5
Status: NEW → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla53
You need to log in before you can comment on or make changes to this bug.