1007817 - WebRTC fails with error in a Panel created from the add-on sdk

Status: NEW

(Firefox :: Site Permissions, defect, P3)

Description

[mroberts]

User Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10_9_2) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/34.0.1847.131 Safari/537.36

Steps to reproduce:

I created a Firefox add-on that places a button in the toolbar. Clicking this button shows a panel which loads apprtc.appspot.com.

I've uploaded the add-on and instructions to reproduce here: https://github.com/markandrus/firefox-webrtc-addon-bug


Actual results:

Depending on how early or late you click the button (i.e. whether the panel content has loaded, triggering call to getUserMedia?), Firefox 29 throws the following error

************************************************************
* Call to xpconnect wrapped JSObject produced this error:  *
[Exception... "[JavaScript Error: "stringBundle is undefined" {file:
"resource://app/modules/webrtcUI.jsm" line: 128}]'[JavaScript Error:
"stringBundle is undefined" {file: "resource://app/modules/webrtcUI.jsm" line:
128}]' when calling method: [nsIGetUserMediaDevicesSuccessCallback::onSuccess]"
 nsresult: "0x80570021 (NS_ERROR_XPC_JAVASCRIPT_ERROR_WITH_DETAILS)"  location:
"native frame :: <unknown filename> :: <TOP_LEVEL> :: line 0"  data: yes]
************************************************************

This may be reproducible in other versions, I have not checked.


Expected results:

Error should not be shown? getUserMedia dialog appearance should be delayed until panel is visible?

Comment 1

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

My guess it's related to invoking webrtc from an add-on....

The errors come from webrtcUI.jsm

Comment 2

[Florian Quèze [:florian]]

I've just looked into this a bit, what's going on here is:
The add-on uses a Panel from the add-on SDK, which according to my initial guess AND the documentation at https://developer.mozilla.org/en-US/Add-ons/SDK/High-Level_APIs/panel creates a new window (dialog), not just a panel in the existing browser window.

This means that when detecting the chrome window owning the browser where getUserMedia has been called, the webrtcUI.jsm code finds a chrome window that isn't a regular browser window.

The JS error in comment 0 is because chromeWin.gNavigatorBundle is not defined. To fix this specific error, it would be easy to either to make webrtcUI.jsm fetch the bundle directly instead of using it from the navigator window, or have the add-on define gNavigatorBundle.

However, fixing this specific error wouldn't really help. The code will fail a few lines later when using chromeWin.PopupNotifications.


I think we should fix webrtcUI.jsm to not cause errors when getUserMedia is used in windows it can't handle.

I see two possible ways to do this:
1. Check that the chrome window for the gUM request is of the "navigator:browser" (or "Social:Chat") type and return early if it isn't.
2. Stop using chromeWin.gNavigatorBundle (use the bundle directly), and return early if chromeWin.PopupNotifications is null.

Not sure which solution I prefer. 2. has the advantage that the only thing add-ons would have to do to handle getUserMedia is to import PopupNotifications (and initialize it) in custom windows they create.

In both cases, add-ons have work to do to have working gUM UI in the custom windows they create.

For the specific case in comment 0 of using a Panel from the add-on sdk, maybe it could be handled in the SDK.

Dave, who would be the right person to have a look at this on the add-on sdk side?

Comment 3

[Dave Townsend [:mossop]]

I'm not really sure what you're looking for from the SDK side, this looks like getUserMedia could be fixed to just work in non-content environments. But I know Matteo has been using this in an add-on already.

Comment 4

[Matteo Ferretti [:zer0] [:matteo]]

I used WebRTC to get access to the Mic and it works quite fine. I got a PopupNotifications error, 'cause I obtain the WebRTC from our `sdk/addon/window` module, that basically is a frame in the `hiddenWindow`; however that doesn't prevent the code from working.

As Dave said, not sure what you're looking exactly from the SDK side. I think too that `getUserMedia` should works when the window is not a content window (in a tab).

Comment 5

[Jeff Griffiths (:canuckistani) (:⚡︎)]

Hi Maire, 

This came up recently on Stackoverflow[1] - I noticed this bug is marked mozilla35 which has come and gone. Can it be re-triaged again? I think getUserMedia will be something web devs will want to use when building devtools extensions.

[1] http://stackoverflow.com/questions/27195812/display-camera-in-firefox-addon

Comment 6

[Maire Reavy [:mreavy]]

Thanks for the poke, Jeff.   I'll re-triage this for Fx38 and look for an owner now.

Comment 7

[Maire Reavy [:mreavy]]

Florian -- Is this something you could help with?

Comment 8

[Florian Quèze [:florian]]

I can likely take another look in the Firefox 38 cycle, yes.

Comment 9

[Maire Reavy [:mreavy]]

Thanks, Florian.  I'm going to assign this to you with a target of Fx38.  If you need to reassign it or can't find the time in Fx38 to work on it, please let me know.  Thanks!

Comment 10

[keithlarrimore]

Before 35.0 I was getting around the "stringBundle is undefined" error by making sure to open the Panel before getUserMedia().  With 35.0 a new bug crept up though: https://bugzilla.mozilla.org/show_bug.cgi?id=1122036

Comment 11

[noitidart]

Hi all this situation came up again on Stackoverflow: http://stackoverflow.com/questions/30608803/how-to-get-webcam-video-feed-in-a-firefox-addon?noredirect=1#comment49454312_30608803

Comment 12

[viraniac]

(In reply to noitidart from comment #11)
> Hi all this situation came up again on Stackoverflow:
> http://stackoverflow.com/questions/30608803/how-to-get-webcam-video-feed-in-
> a-firefox-addon?noredirect=1#comment49454312_30608803

I went across it by using the same work around as keithlarrimore. if the panel is made to be visible on the screen, then we are not getting this error. But even then the issue is not solved. Even if the user gives permission to share webcam, the request seems to remain in pending state until the addon is unloaded in which case it gives "mm not defined" error as described in https://bugzilla.mozilla.org/show_bug.cgi?id=1122036

Comment 13

[Maire Reavy [:mreavy]]

Moving this to the proper category (which was created after this bug was filed).

Florian -- What's required to resolve this?

Comment 14

[Florian Quèze [:florian]]

(In reply to Maire Reavy [:mreavy] (Plz needinfo me) from comment #13)
> Moving this to the proper category (which was created after this bug was
> filed).
> 
> Florian -- What's required to resolve this?

Not really sure. To fix the initial problem, my suggestion from comment 2 should be enough:

(In reply to Florian Quèze [:florian] [:flo] from comment #2)

> 2. Stop using chromeWin.gNavigatorBundle (use the bundle directly), and
> return early if chromeWin.PopupNotifications is null.

However, comment 5, comment 10 and comment 12 seem to indicate there are new additional issues. I'm not sure what's causing these.

Comment 15

[i]

As Flash dies, the importance of this issue rises. A lot of us are using Flash for microphone/etc access because we can't use WebRTC in this scenario. Can we get this assigned to the right person and addressed?

Comment 16

[Shane Caraveo (:mixedpuppy)]

I could see this potentially affecting webextensions.  With the new sidebar, I imagine someone will end up trying to use webrtc in it.  The sidebar would have the same basic problems as a custom window in regards to a permission prompt.  For a webextension, we could provide an additional permission similar to geolocation that could simply give permanent permission to use webrtc in the addon.  Or we can document that an addon can only use webrtc in a tab (unfortunate, but maybe necessary).

Comment 17

[kirillstyopkin]

Hi. I have the same problem, trying to resolve getUserMedia() from the add-on, the promise never resolves. What's the progress with this issue?

Comment 18

[kirillstyopkin]

it works in chrome, but fails in firefox 54 OS X

Comment 19

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

Florian (and others): in the new, webextension-only world, where should we go with this?  There are a few issues: what it takes to get working gUM (and peerconnections) in an extension, and what permissions (if any) should be needed for use of peerconnections (and gUM) in an extension?  What does "save" for permissions mean in this case?  What issues might there be with cross-origin tests?

And how does this interact with screen capture permissions?  Would an extension be able to capture the screen and bypass the user permissions dialog for screen/window/etc sharing?

Extensions typically have more default "permission" to do things; what limits can (or should) we put on that?  Should extensions using this API require special notice to users before/when installing?  Do they need special review?

Quite possibly others from the extension team(?) should be involved...

Comment 20

[Adam Roach [:abr]]

(In reply to Randell Jesup [:jesup] from comment #19)
> Florian (and others): in the new, webextension-only world, where should we
> go with this?  There are a few issues: what it takes to get working gUM (and
> peerconnections) in an extension, and what permissions (if any) should be
> needed for use of peerconnections (and gUM) in an extension?  What does
> "save" for permissions mean in this case?  What issues might there be with
> cross-origin tests?

Each WebExtension has its own origin, of the form "moz-extension://{{UUID}}". Given that things like gUM permissions are stored on a per-origin basis, I'm not sure we need any semantic change here from how we handle permissions on normal https:// origins. There may well be some code changes to make this work, but I think the model is fine (prompt the user on use, store permissions persistently if the user asks us to). I recognize this is different than the WebExtensions "permissions" model; but, given the sensitivity of camera and microphone access, I think allowing users to keep access to a "per use" model (if they so choose) is entirely appropriate.

 
> And how does this interact with screen capture permissions?  Would an
> extension be able to capture the screen and bypass the user permissions
> dialog for screen/window/etc sharing?

I'd give the same answer: treat screen capture for extensions the same permission behavior as web pages. This is a very powerful feature that many users will reasonably want to have per-use veto power over.

> Extensions typically have more default "permission" to do things; what
> limits can (or should) we put on that?  Should extensions using this API
> require special notice to users before/when installing?  Do they need
> special review?

If we adopt the proposal I make above, I think no additional review is really necessary: users are given full, informed agency over the use of these extraordinary permissions.

> Quite possibly others from the extension team(?) should be involved...

Yes. I've copied Andy and Kev, as I suspect they have really well-informed opinions here.

Comment 21

[Jan-Ivar Bruaroey [:jib] (needinfo? me)]

I agree with Adam that the existing model should be able to handle this, and that it's a reasonable model, at least for now, maybe for a while even. This should be enough to unblock this issue about getting it working.

Once things work, we can open new bugs for future improvements, such as perhaps more informative prompts in this case, and ...

> Would an extension be able to capture the screen and bypass the user permissions dialog for screen/window/etc sharing?

Not initially, maybe never? The permission doubles as the selection prompt for screen-sharing, where end-users select what to share atm. It is not optional. Exploring letting users remember screen-sharing permission, would require giving extensions a way to select sources themselves subsequently. We actually have something for that: We might expose our internal deviceId(rawId) API which right now is privileged (chrome) only (used by the preview in the permission prompts themselves, which is pulled off using getUserMedia internally). But we should open a new issue to explore if we even want to allow that.

FYI we also have remnants of internal chrome-only APIs such as tab-sharing from Firefox Hello, where a window-id is passed in, which might be desirable instead of/in addition to indiscriminate screensharing access. Again separate issues I think.

My answers assume that web extensions do not have chrome privileges, which I believe is the case now.

Comment 22

[Martin Thomson [:mt:]]

I think that abr's opinion is mine as well.

I'm of the opinion that we should be making permissions requests just in time.  The hazard is that we need to properly attribute the request to the extension.  That's a UX issue that might depend on the nature of the thing that is asking.  We don't want the permissions request to be associated with a site.

UX issues that need thought and consideration: Hypothetical extension that takes a voice memo when you press a toolbar button or hotkey (it might attach context and uploads it somewhere): where does the permissions prompt appear?  Does the extension have some control over where the question is presented?  What does it say?  Does it have the "remember my decision" checkbox?  Is that checkbox enabled or disabled to start?

Please don't give away permanent screen-sharing permission - we should stop thinking of that as having permissions at all, and think of it more as the browser having "get me a screen share" UX.

Comment 23

[Kev Needham [:kev]]

Just to make sure I understand, the issue with creation is that the origin isn't a website, and is instead owned by the extension. From an assumptions standpoint, there's already an on-demand permission UI built into the browser proper, which would be invoked when a session is (attempted to be) initiated.

Main question I have is if the WebRTC connection is initiated by an extension, is that initiation limited to creating a new content window that then pulls up the connection setup and permission prompts?

I don't think a separate extension permission for invoking WebRTC connections is necessary provided there's no way to end-around, which shouldn't be possible without a WebExtensions API for it (which doesn't exist, and I don't think should).

I do think there should be a limit on what an extension can be used for in the creation of a connection per above, given the extension itself can have access to other info, and I wouldn't want it to be able to initiate the connection inheriting the same perms the webextension has.

Apologies if this is unclear. Basically if all we're asking for here is to allow a separate document to be setup to establish the connection outside the extension content, the "use what's in the browser" makes sense. If it's more than that, I'd want to review a) if we'd want to do that and b) what limits we'd put in place if we did. While extensions don't have chrome privs, they can have access to browser info, and I'd want to mitigate the risk there.

Comment 24

[Martin Thomson [:mt:]]

To clarify for Kev, in case there is any confusion: WebRTC doesn't require permissions on the web and wouldn't here either.

(For those read in on the minutiae, this assertion should extend to access to all local candidates in the case of a web extension; installing an extension should be sufficient.  That's probably not something we get for free.  An action for jib, methinks :)

It's the access to camera, mic, and screen sharing that generate prompts.

I don't think that we should require creation of a content frame for these prompts, but might be prepared to accept that as a stop-gap until we work out UX.

Comment 25

[Kev Needham [:kev]]

(In reply to Martin Thomson [:mt:] from comment #24)
> To clarify for Kev, in case there is any confusion: WebRTC doesn't require
> permissions on the web and wouldn't here either.

kk
 
> I don't think that we should require creation of a content frame for these
> prompts, but might be prepared to accept that as a stop-gap until we work
> out UX.

My biggest concern would be around whether we'd inadvertently expose info the extension may have outside it's scope via, say, a data channel set up from within the extension. I confess I don't know enough about the internals, so apologies if it's a dumb question. My wondering is less around hw access, and more whether there'd be any potential for shenanigans with data the extension has rights/perms to.

thanks for the quick reply!

Comment 26

[Martin Thomson [:mt:]]

This is probably a little off-topic, but the sorts of things you are talking about (media streams or tracks and data channels) aren't currently cross-origin portable, even intentionally.  We may decide to make them portable, but it would be intentional (Transferable via postMessage, or that sort of thing).

What could leave the origin is the actual bytes of messages or content of media.  I would assume that this would be fully intentional though.  For media in particular, it takes a non-trivial effort to extract data in a way that would allow it to leave the origin.

(I separately learned that we have an audit of WebRTC use of origin attributes scheduled, so I'd be more confident in this statement after that completes.)

Comment 27

[Kev Needham [:kev]]

Ok, thanks. Nothing further from me. From what it sounds like, the existing permissions sound like all that's needed.

Will wait for Andy to weigh in as well.

Comment 28

[Adam Roach [:abr]]

(In reply to Kev Needham [:kev] from comment #27)
> Ok, thanks. Nothing further from me. From what it sounds like, the existing
> permissions sound like all that's needed.

Semantically, yes. There's still the challenge that Martin mentions above of presenting the permissions UX to the user. If the Extension has an obvious visible thing (e.g., a button), it's *probably*  straightforward to do something like hanging a doorhanger off of that element. If it doesn't, we need to find some way to ask the user for (e.g.) microphone access in a way that makes it clear that it is an Extension asking (rather than the content), and also makes it clear *which* Extension is asking for permission.

Comment 29

[Martin Thomson [:mt:]]

It's possible that we could force extensions to give us somewhere to draw the dialog, and automatically deny the request if they don't.  That is also work.

Comment 30

[kirillstyopkin]

Could you please clarify, is this discussion about make WebRTC method getUserMedia() work from firefox extension ?

Comment 31

[keithlarrimore]

(In reply to kirillstyopkin from comment #30)
> Could you please clarify, is this discussion about make WebRTC method
> getUserMedia() work from firefox extension ?

It was but it seems to have gone in a different direction than what I would have liked.  I think this bug I submitted is squarely on getUserMedia() in add-ons (specifically the panel).  It hasn't gotten much love though https://bugzilla.mozilla.org/show_bug.cgi?id=1122036

Comment 32

[kirillstyopkin]

(In reply to keithlarrimore from comment #31)
> (In reply to kirillstyopkin from comment #30)
> > Could you please clarify, is this discussion about make WebRTC method
> > getUserMedia() work from firefox extension ?
> 
> It was but it seems to have gone in a different direction than what I would
> have liked.  I think this bug I submitted is squarely on getUserMedia() in
> add-ons (specifically the panel).  It hasn't gotten much love though
> https://bugzilla.mozilla.org/show_bug.cgi?id=1122036

Yeah, tree years passed, but we're still struggling with it :(

Comment 33

[Andy McKay]

Please see also bug 1363854 and bug 1278100, where we've discussed it as well as WebExtension APIs. Because this bug is in the SDK we haven't really looked at it (and the SDK is now dead). I think we've got all the concerns in this bug, its pretty clear that access to things like the video or mic should: prompt for user input and then give a clear indication that its doing so until its completed.

The prompt could be an optional permissions prompt. Currently a website displays a visual indicator in the tab (if I remember). As discussed in bug 1363854 comment 5, we aren't really sure to put that long running indicator yet.

Comment 35

[Florian Quèze [:florian]]

I had assigned this to myself before the switch to WebExtension, and I'm not actively working on this so unassigning. The needinfo in comment 19 has already been answered by the other comments, so removing it too.