[B2G getUserMedia] Display camera/ microphone permission acquisition prompt

RESOLVED FIXED in 1.3 C3/1.4 S3(31jan)

Status

()

defect
RESOLVED FIXED
6 years ago
5 years ago

People

(Reporter: slee, Assigned: ayang)

Tracking

(Blocks 2 bugs)

unspecified
1.3 C3/1.4 S3(31jan)
ARM
Gonk (Firefox OS)
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(firefox28 wontfix, b2g-v1.3 wontfix)

Details

(Whiteboard: [WebRTC][blocking-webrtc-][b2g-gum+][ft:multimedia-platform], [apps watch list])

Attachments

(3 attachments, 62 obsolete attachments)

2.84 KB, patch
fabrice
: review+
Details | Diff | Splinter Review
1.62 KB, patch
baku
: review+
Details | Diff | Splinter Review
90.64 KB, patch
ayang
: review+
Details | Diff | Splinter Review
When apps try to get the microphone, we need to check if they have the permission to do so.
Assignee: nobody → ayang
take it as first bug
On desktop, webrtcUI.jsm will call mozGetUserMediaDevices to list available devices on the prompt dialog, but there is no this module on B2G so far.
On B2G platform, could we use Manifest permission checking for this?
Current B2G ContentPermissionPrompt.js, it allows one permission request per time but webrtc could need audio and video permission at the same time.
It also needs to list the front/back camera for user to choose as well.
(In reply to Randy Lin [:rlin] from comment #3)
> On B2G platform, could we use Manifest permission checking for this?
On ap, yes.
On web content, it needs a prompt for user. The behavior should be similar to geolocation.
FWIW, the original discussion on the security model for the permission prompt in the context of B2G according to https://wiki.mozilla.org/WebAPI/Security/Camera implies the following permission rules:

* Web Content - PROMPT_ACTION
* Hosted App - PROMPT_ACTION
* Privileged App - PROMPT_ACTION
* Certified App - ALLOW_ACTION

Good question about the manifest property though - we're going to need that defined here in order to fall in line with the requirement that a manifest permission must be specified in order to use a WebAPI within an app.

The doc I'm referencing though was written up a while back, so a quick review might be worthwhile.

Paul - Can you give input here on what manifest permission + permission model we should use here for getUserMedia on B2G?
Flags: needinfo?(ptheriault)
I can provide my opinion for what we should use, but I think this needs a lot more input - Jonas and also UX (josh/larissa) at least, but I am going to start a thread on web-api list for this.

Some initial thoughts:
* We need to have a different permission name for 'web-rtc camera' than the 'b2g camera' control API (or change the b2g camera permission name)
* There must be a permission in the manifest for this API one way or another
* It would be nice if we didn't prompt twice ( I assume from the comments above that there will be an _audio_ and _camera_ permission, and on b2g we can only grant one permission per prompt, so we need two prompts?) 
* The permissions is time-based - that the user might be comfortable to grant this permission at some times but not others. We should be careful about choosing to persist permission choices for this API (i.e. I don't think the default privileged action of remember choice by default is appropriate for this API)
* Ideally the user would do the same action to enable the camera across web and all types of apps - that way they know exactly when they are turning the camera on.
* Probably the biggest threat here is surreptitious recording - we need to think about how to prevent this (e.g. forbid this API in the background, but how to do this without stomping on use cases, e.g. audio recorder ) 

If we had no other mechanism and just had to land it, I would suggest copying the geolocation model. That is:

https://mxr.mozilla.org/mozilla-central/source/dom/apps/src/PermissionsTable.jsm#36
{ geolocation: {
37                              app: PROMPT_ACTION,
38                              privileged: PROMPT_ACTION,
39                              certified: PROMPT_ACTION
40                            } 

To be honest though, I don't like the "prompt" on certified apps. To me certified apps should be trusted to be designed intuitively so as to inform the user of the consequences of their actions. But in the case of the camera app for example, there is no way to turn geolocation tagging off or on, so instead the only control the user has is to toggle the permission (which is buried deep in the settings app!)

Personally I would prefer:

app: PROMPT_ACTION,
privileged: PROMPT_ACTION,
certified: ALLOW_ACTION

And ensure that any certified app we develop is well designed in the way it informs the user when the camera is enabled. Others may see it differently though. I am going to start a thread on the webapi list to discuss this further since it needs broader attention I think.
Flags: needinfo?(ptheriault)
> * We need to have a different permission name for 'web-rtc camera' than the
> 'b2g camera' control API (or change the b2g camera permission name)
That's my thought at the beginning; however, after checking webrtc/getusermedia spec, 
there is no extra permission for webrtc.
On another proposal spec "Media Capture and Streams Settings API", 
3.2.2 Track Source Types:

enum SourceTypeEnum {
    "none",
    "camera",
    "microphone",
    "photo-camera",
    "readonly",
    "remote"
};

Although it's just draft now, would it better to add "microphone" and "photo-camera" to follow it?


> * It would be nice if we didn't prompt twice ( I assume from the comments
> above that there will be an _audio_ and _camera_ permission, and on b2g we
> can only grant one permission per prompt, so we need two prompts?) 
Yes, that's current UI design (858235).
Spec suggests to combine them into one (getusermedia, 8.5 Implementation Suggestions) 
but before the proper UI comes out, 2 prompts are unavoidable.

From my understanding, Maire is starting to discuss with UX/UI to to make sure that the 
permission design and behavior is similar across the products. Maybe she have more comments
about this.


> * The permissions is time-based - that the user might be comfortable to
> grant this permission at some times but not others. We should be careful
> about choosing to persist permission choices for this API (i.e. I don't
> think the default privileged action of remember choice by default is
> appropriate for this API)
> * Ideally the user would do the same action to enable the camera across web
> and all types of apps - that way they know exactly when they are turning the
> camera on.
Agreed, at least for web content and unauthorized apps.
For privileged app, there is a bug(823258) asked to have persist on.


> * Probably the biggest threat here is surreptitious recording - we need to
> think about how to prevent this (e.g. forbid this API in the background, but
> how to do this without stomping on use cases, e.g. audio recorder ) 
Agreed, actually in SettingsAPI_proposal, section 2 definition:

"armed: Implementations of this specification are advised to include some indicator
 that a device is armed in their UI so that users are aware that an application may 
 start the source at any time."

My first thought is a recording indicator on status bar and display which app or web content
is recording.


> If we had no other mechanism and just had to land it, I would suggest
> copying the geolocation model. That is:
> 
> https://mxr.mozilla.org/mozilla-central/source/dom/apps/src/PermissionsTable.
> jsm#36
> { geolocation: {
> 37                              app: PROMPT_ACTION,
> 38                              privileged: PROMPT_ACTION,
> 39                              certified: PROMPT_ACTION
> 40                            } 
There is a mechanism for desktop at m-c/dom/media/MediaManager.cpp and m-c/mobile/android/chrome/content/WebrtcUI.js
which uses several observers to communicate each others.

To avoid break it and uses the current B2G way, I plan to add a new jsm m-c/b2g/components/MediaPermission.jsm.
It will receive the observer notifications from MediaManager.cpp and convert the callback to
geolocation model (prompted by m-c/b2g/components/ContentPermissionPrompt.js).


> To be honest though, I don't like the "prompt" on certified apps. To me
> certified apps should be trusted to be designed intuitively so as to inform
> the user of the consequences of their actions. But in the case of the camera
> app for example, there is no way to turn geolocation tagging off or on, so
> instead the only control the user has is to toggle the permission (which is
> buried deep in the settings app!)
> 
> Personally I would prefer:
> 
> app: PROMPT_ACTION,
> privileged: PROMPT_ACTION,
> certified: ALLOW_ACTION
> 
> And ensure that any certified app we develop is well designed in the way it
> informs the user when the camera is enabled. Others may see it differently
> though. I am going to start a thread on the webapi list to discuss this
> further since it needs broader attention I think.
Flags: needinfo?(mreavy)
1. fix the miss part of getUserMedia prompt permisson requests on B2G, both camera and microphone.
2. add new permission 'microphone' for audio recording
3. add a new jsm MediaPermission.jsm to handle camera/microphone premission requets.
4. it will pop 2 prompts if asking camera/microphone at the same time.
5. it doesn't take front/back camera into cosideration since W3C spec doesn't define it so far.
6. it also fixed bug 845727.
Attachment #740213 - Attachment is obsolete: true
Attachment #740222 - Flags: superreview?
Attachment #740222 - Flags: review?(fabrice)
7. browser app needs to add permission 'camera' and 'microphone'. When the URL iframe opens, it will popup permission request dialog to user. I'll add browser app manifest patch after this patch is reviewed.
(In reply to Alfredo Yang from comment #11)
> Created attachment 740222 [details] [diff] [review]
> popup prompt dialog when request camera/microphone via getUserMedia on B2G
> 
> 1. fix the miss part of getUserMedia prompt permisson requests on B2G, both
> camera and microphone.
> 2. add new permission 'microphone' for audio recording
> 3. add a new jsm MediaPermission.jsm to handle camera/microphone premission
> requets.
> 4. it will pop 2 prompts if asking camera/microphone at the same time.

This doesn't sound right. Desktop only prompts once, not twice. I'd think B2G needs to follow in suit.

> 5. it doesn't take front/back camera into cosideration since W3C spec
> doesn't define it so far.
> 6. it also fixed bug 845727.
(In reply to Alfredo Yang from comment #12)
> 7. browser app needs to add permission 'camera' and 'microphone'. When the
> URL iframe opens, it will popup permission request dialog to user. I'll add
> browser app manifest patch after this patch is reviewed.

Why two app manifest permissions? Why not just have one? Things get more messy for the app developer experience if we introduce complexity here.

FWIW, we should really hold on doing a review here and as Paul suggests, discuss this on dev-webapi to remove the UI and permission contention points. For me, there's some things I agree with here, and others I'm questioning. UX also needs to add some input here to understand the recording icon issue.
Whiteboard: [WebRTC][blocking-webrtc-] → [WebRTC][blocking-webrtc-][b2g-gum+]
Comment on attachment 740222 [details] [diff] [review]
popup prompt dialog when request camera/microphone via getUserMedia on B2G

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

I agree with Jason that we can't go with 2 permissions prompts. Can we use a set of "webrtc:*" permissions here? I have the feeling that we need a custom permission dialog here in gaia.
Then we could have webrct:microphone, webrtc:camera-front, webrtc:camera-back, like we did for the various device-storage:* permissions.
Attachment #740222 - Flags: superreview?
Attachment #740222 - Flags: review?(fabrice)
Attachment #740222 - Flags: review-
(In reply to Alfredo Yang from comment #9)
> > * We need to have a different permission name for 'web-rtc camera' than the
> > 'b2g camera' control API (or change the b2g camera permission name)
> That's my thought at the beginning; however, after checking
> webrtc/getusermedia spec, 
> there is no extra permission for webrtc.
> On another proposal spec "Media Capture and Streams Settings API", 
> 3.2.2 Track Source Types:
> 
> enum SourceTypeEnum {
>     "none",
>     "camera",
>     "microphone",
>     "photo-camera",
>     "readonly",
>     "remote"
> };


Yes. I don't think it's that useful to distinguish in this
prompt between different uses that the site might make of the
camera/microphone (at least not at this time). The reason
is that from a technical perspective, the permissions are
pretty interchangeable.

I.e.,
- If I can get the raw bits from the camera and microphone,
I can arrange to RTP encode them and thus make a calling
application.

- If I can make a call and send the video/audio to the
other side, then I can call myself and record the media.

Now it might be the case that you think that this is a statement
of the application's *intent*, but it's not really technically
enforceable.

In terms of indicating to the user that the devices are in use,
a recording bar sounds pretty good.
> FWIW, we should really hold on doing a review here and as Paul suggests,
> discuss this on dev-webapi to remove the UI and permission contention
> points. For me, there's some things I agree with here, and others I'm
> questioning. UX also needs to add some input here to understand the
> recording icon issue.

FYI, I started a thread on dev-b2g on this (sorry if webapi was more appropriate, i wasnt sure, and didnt want to crosspost). https://groups.google.com/d/msg/mozilla.dev.b2g/05LEV43lVNI/3QGZuIfF4twJ


> Yes. I don't think it's that useful to distinguish in this
> prompt between different uses that the site might make of the
> camera/microphone (at least not at this time). The reason
> is that from a technical perspective, the permissions are
> pretty interchangeable.

+1 for not distinguishing. 

> 
> I.e.,
> - If I can get the raw bits from the camera and microphone,
> I can arrange to RTP encode them and thus make a calling
> application.
> 
> - If I can make a call and send the video/audio to the
> other side, then I can call myself and record the media.
> 
> Now it might be the case that you think that this is a statement
> of the application's *intent*, but it's not really technically
> enforceable.
> 
> In terms of indicating to the user that the devices are in use,
> a recording bar sounds pretty good.

+ 1 for a recording bar - perhaps using similar code to the existing ongoing call indicator, but red or something.
Asking FirefoxOS-UX for feedback on Comments 8 and 9 and to respond on https://groups.google.com/d/msg/mozilla.dev.b2g/05LEV43lVNI/3QGZuIfF4twJ (which Ian has already started).  

I think we all agree that we want to avoid 2 permissions prompts, and I agree with Ekr that we don't want to distinguish between the different uses that the site might make of the camera/microphone. A recording bar is also a good idea IMO.  Android doesn't currently have a recording bar. Can we add it to both (Android and FxOS)?

This feature is listed as "must have" for v1.2.
Flags: needinfo?(mreavy) → needinfo?(firefoxos-ux-bugzilla)
(In reply to Eric Rescorla (:ekr) from comment #15)
> Now it might be the case that you think that this is a statement
> of the application's *intent*, but it's not really technically
> enforceable.

B2G's app security model enforces that PROMPT_ACTION & ALLOW_ACTION WebAPIs have to specify a permission in order for apps to make use of the WebAPI. So there is a requirement for a web application to specify an intent that they intend to use getUserMedia with the appropriate manifest permission. That's what's being discussed earlier on in the permissions space. Web content doesn't need to do this, however.

(In reply to Maire Reavy [:mreavy] from comment #17)
> I think we all agree that we want to avoid 2 permissions prompts, and I
> agree with Ekr that we don't want to distinguish between the different uses
> that the site might make of the camera/microphone. A recording bar is also a
> good idea IMO.  Android doesn't currently have a recording bar. Can we add
> it to both (Android and FxOS)?

Don't forget that you need to pay attention to the app use cases here too. That's a core feature to B2G's operating system. Web content use cases are only one small part of the permission model you need to solve here.

FWIW - I believe Gaia's UI already has a recording indicator that is show in the notification bar when recording is active in the camera. Although the design of the recording bar should be out of scope in this bug and discussed separately - this bug is talking about the permissions UX.
(In reply to Jason Smith [:jsmith] from comment #18)
> (In reply to Eric Rescorla (:ekr) from comment #15)
> > Now it might be the case that you think that this is a statement
> > of the application's *intent*, but it's not really technically
> > enforceable.
> 
> B2G's app security model enforces that PROMPT_ACTION & ALLOW_ACTION WebAPIs
> have to specify a permission in order for apps to make use of the WebAPI. So
> there is a requirement for a web application to specify an intent that they
> intend to use getUserMedia with the appropriate manifest permission. That's
> what's being discussed earlier on in the permissions space. Web content
> doesn't need to do this, however.
>

Yes, but once the app has access to the camera
and microphone it can do whatever it wants and there's no existing
mechanism to technically enforce whatever is put here. Unless you
have some idea that we are going to punish people who lie about
their intent in apps, I'm not sure I see how this is useful.






> (In reply to Maire Reavy [:mreavy] from comment #17)
> > I think we all agree that we want to avoid 2 permissions prompts, and I
> > agree with Ekr that we don't want to distinguish between the different uses
> > that the site might make of the camera/microphone. A recording bar is also a
> > good idea IMO.  Android doesn't currently have a recording bar. Can we add
> > it to both (Android and FxOS)?
> 
> Don't forget that you need to pay attention to the app use cases here too.
> That's a core feature to B2G's operating system. Web content use cases are
> only one small part of the permission model you need to solve here.

I think we're aware of that, but I don't really see that it changes things
much.
(In reply to Eric Rescorla (:ekr) from comment #19)
> (In reply to Jason Smith [:jsmith] from comment #18)
> > (In reply to Eric Rescorla (:ekr) from comment #15)
> > > Now it might be the case that you think that this is a statement
> > > of the application's *intent*, but it's not really technically
> > > enforceable.
> > 
> > B2G's app security model enforces that PROMPT_ACTION & ALLOW_ACTION WebAPIs
> > have to specify a permission in order for apps to make use of the WebAPI. So
> > there is a requirement for a web application to specify an intent that they
> > intend to use getUserMedia with the appropriate manifest permission. That's
> > what's being discussed earlier on in the permissions space. Web content
> > doesn't need to do this, however.
> >
> 
> Yes, but once the app has access to the camera
> and microphone it can do whatever it wants and there's no existing
> mechanism to technically enforce whatever is put here. Unless you
> have some idea that we are going to punish people who lie about
> their intent in apps, I'm not sure I see how this is useful.

The app review process would handle the case where a developer lies about their intent of how an app intends to use the camera and microphone for apps intending to be in our marketplace, given that:

1. This is the primary location users shall install apps (95%+ use case)
2. If the developer did lie, the app would fail review. There's two quality checks that ensure that depending on the type of app:
2a. If the app is hosted or packaged, an app reviewer does exploratory testing of every app that comes in to verify that the app stays in alignment with app review guidelines, which includes checking to make sure that the app is making sane use of APIs called out in the app manifest
2b. If the app is packaged, an app reviewer additionally does a code review to ensure sane use of the API that's called out in the manifest

I would suggest following up with Lucas, Jonas, and Paul about this more if you have more questions. Lucas was the original designer around why intent has to be specified in the app manifest, so he could give you a deep rationale on this.
(In reply to Jason Smith [:jsmith] from comment #20)
> (In reply to Eric Rescorla (:ekr) from comment #19)
> > (In reply to Jason Smith [:jsmith] from comment #18)
> > > (In reply to Eric Rescorla (:ekr) from comment #15)
> > > > Now it might be the case that you think that this is a statement
> > > > of the application's *intent*, but it's not really technically
> > > > enforceable.
> > > 
> > > B2G's app security model enforces that PROMPT_ACTION & ALLOW_ACTION WebAPIs
> > > have to specify a permission in order for apps to make use of the WebAPI. So
> > > there is a requirement for a web application to specify an intent that they
> > > intend to use getUserMedia with the appropriate manifest permission. That's
> > > what's being discussed earlier on in the permissions space. Web content
> > > doesn't need to do this, however.
> > >
> > 
> > Yes, but once the app has access to the camera
> > and microphone it can do whatever it wants and there's no existing
> > mechanism to technically enforce whatever is put here. Unless you
> > have some idea that we are going to punish people who lie about
> > their intent in apps, I'm not sure I see how this is useful.
> 
> The app review process would handle the case where a developer lies about
> their intent of how an app intends to use the camera and microphone for apps
> intending to be in our marketplace, given that:
> 
> 1. This is the primary location users shall install apps (95%+ use case)

This strikes me as rather speculative.


> 2. If the developer did lie, the app would fail review. There's two quality
> checks that ensure that depending on the type of app:
> 2a. If the app is hosted or packaged, an app reviewer does exploratory
> testing of every app that comes in to verify that the app stays in alignment
> with app review guidelines, which includes checking to make sure that the
> app is making sane use of APIs called out in the app manifest
> 2b. If the app is packaged, an app reviewer additionally does a code review
> to ensure sane use of the API that's called out in the manifest

It strikes me as extremely unlikely that the level of code review
will be anything like that required to detect malicious applications.
That's simply not practical to do any any scale. Perhaps, the
review will be sufficient to detect *mistakes*, but that's not
an argument that the information needs to be reflected up to the
browser chrome but rather that applications need to accurately
represent themselves to the user in the application *content*.

Consider the case where an application has two apparent modes:

1. A photobooth mode where it privately manipulates photos.
2. A mode where it uploads your photo to a public site.

The app manifest permissions clearly are going to need to list the
more expansive intent, because the application may need that
permission.

Now, consider the case where the application routinely claims
(in the content) to be in photobooth mode but actually always uploads
your picture. This isn't sensibly enforceable by the browser,
because it's purely a matter of the content accurately
representing itself.

Given that we have this problem in any case, I really don't
see a lot of value in trying to push a small part of the problem
into some manifest information that most people ignore anyway
(see the Felt citation below).


> I would suggest following up with Lucas, Jonas, and Paul about this more if
> you have more questions. Lucas was the original designer around why intent
> has to be specified in the app manifest, so he could give you a deep
> rationale on this.

All the available evidence suggests that users are extremely insensitive
to any sort of fine-grained permissions at application install time.
See, for instance:

http://www.cs.berkeley.edu/~afelt/felt-androidpermissions-soups.pdf

Can you point to somewhere where someone has actually done a real
analysis of this issue in the context of B2G taking into account the
human factors questions? I.e., a written document, not something in
someone's head.
Eric,

You could read https://wiki.mozilla.org/Apps/Security

Also note that the Felt paper you cite is mostly inapplicable to b2g where we choosed to *not* ask the user to make security decisions as much as possible.

I'm pretty sure we want to PROMPT for web content and non-privileged apps, and ALLOW for privileged apps.
(In reply to Fabrice Desré [:fabrice] from comment #22)
> Eric,
> 
> You could read https://wiki.mozilla.org/Apps/Security


I have. It seems to just state the outcome of the decisions, not
provide any analysis that considers the human factors issues.




> Also note that the Felt paper you cite is mostly inapplicable to b2g where
> we choosed to *not* ask the user to make security decisions as much as
> possible.
> 
> I'm pretty sure we want to PROMPT for web content and non-privileged apps,
> and ALLOW for privileged apps.

As I understan Jason's comment here, it is that we need to state an
intention (i.e., a string) that is attached to the manifest and
shown to the user at the time of installation. I.e.,

https://wiki.mozilla.org/Apps/Security#Data_Usage_Intention

Isn't this precisely the experience that the paper I am citing refers
to?
(In reply to Eric Rescorla (:ekr) from comment #23)
> 
> As I understan Jason's comment here, it is that we need to state an
> intention (i.e., a string) that is attached to the manifest and
> shown to the user at the time of installation. I.e.,
> 
> https://wiki.mozilla.org/Apps/Security#Data_Usage_Intention
> 
> Isn't this precisely the experience that the paper I am citing refers
> to?

Well, try it for yourself. You'll see that we never show a "I allow this app to use x/y/z" at install time. If a PROMPT is needed, it will popup at first use (an subsequent ones if the user doesn't toggle the "Remember me" checkbox).
In that case, any kind of intent string in the manifest seems entirely pointless.
(In reply to Eric Rescorla (:ekr) from comment #25)
> In that case, any kind of intent string in the manifest seems entirely
> pointless.

Why? It still could be displayed in permission prompts and in the settings app that let the user manage permissions granted to each app.
But both of those cases seem to me to directly run into Felt's results.
(In reply to Eric Rescorla (:ekr) from comment #21)
> (In reply to Jason Smith [:jsmith] from comment #20)
> > (In reply to Eric Rescorla (:ekr) from comment #19)
> > 
> > The app review process would handle the case where a developer lies about
> > their intent of how an app intends to use the camera and microphone for apps
> > intending to be in our marketplace, given that:
> > 
> > 1. This is the primary location users shall install apps (95%+ use case)
> 
> This strikes me as rather speculative.

That's based on the target 1.01 plan and resulting testing that's been done on the apps security model for 1.01. There's very little evidence at all from testing done (security, QA, apps review, etc) that sites are setting up their own site to allow apps to be installed onto the user's device. The primary use case users are discovering their apps are from within the Firefox Marketplace for 1.01.

That doesn't mean that the use case doesn't exist to allow web developers to build sites to allow users to install apps. But that isn't typical use case at all right now for apps discovery in 1.01. However, that will change in future releases when we move to a more established multiple marketplace model.

> 
> 
> > 2. If the developer did lie, the app would fail review. There's two quality
> > checks that ensure that depending on the type of app:
> > 2a. If the app is hosted or packaged, an app reviewer does exploratory
> > testing of every app that comes in to verify that the app stays in alignment
> > with app review guidelines, which includes checking to make sure that the
> > app is making sane use of APIs called out in the app manifest
> > 2b. If the app is packaged, an app reviewer additionally does a code review
> > to ensure sane use of the API that's called out in the manifest
> 
> It strikes me as extremely unlikely that the level of code review
> will be anything like that required to detect malicious applications.
> That's simply not practical to do any any scale. Perhaps, the
> review will be sufficient to detect *mistakes*, but that's not
> an argument that the information needs to be reflected up to the
> browser chrome but rather that applications need to accurately
> represent themselves to the user in the application *content*.

We have two app reviewers hired full time working with one person in security who specializes in mobile malware on detection of malicious applications during the app review process. At the moment, we have been able to handle the incoming rate of apps with the appropriate code review needed for privileged app content. Even in cases where we had something slip past app review, we've had dogfooders raise issues on questionable apps, whether that be from direct filing of tech evangelism bugs, bad reviews on the app page, reporting abuse via marketplace, etc. We even have an escalation path to QA when more analysis is needed (which I work on). So I don't think the scalability problem is an issue right now.

As for long term scalability, I'd have to look into what our addons process looks like as a point of comparison, as they deal with this problem at a larger scale of incoming addons (including dealing with malicious add-ons). I can followup with Lisa (app review lead) to find out what she's planning for long term scalability.

> 
> Consider the case where an application has two apparent modes:
> 
> 1. A photobooth mode where it privately manipulates photos.
> 2. A mode where it uploads your photo to a public site.
> 
> The app manifest permissions clearly are going to need to list the
> more expansive intent, because the application may need that
> permission.
> 
> Now, consider the case where the application routinely claims
> (in the content) to be in photobooth mode but actually always uploads
> your picture. This isn't sensibly enforceable by the browser,
> because it's purely a matter of the content accurately
> representing itself.
> 
> Given that we have this problem in any case, I really don't
> see a lot of value in trying to push a small part of the problem
> into some manifest information that most people ignore anyway
> (see the Felt citation below).

I don't follow this analysis. We don't grant content access to APIs by default on app install for the getUserMedia case. As Paul cites above, not providing the permission would always deny access to getUserMedia. Providing the permission means that the app can prompt the user within the app when access to the camera/mic is needed. So even in the case the user does install a malicious getUserMedia application, they would have to:

1. Have a discovery mechanism - marketplace is going to be hard to use here, because existing app review processes do check for malicious apps. And even if they do get into the marketplace, a post-discovery mechanism for flagging the app moderation could get the app caught for doing malicious activity.

2. After being installed with the permission specified, try to call getUserMedia before doing the malicious activity. Given that we're prompting the user here, we do give the user explicit warning when camera/mic access is being asked for. If they grant it, then at this point, the malicious activity could happen. Then again, remember the same prompting workflow happens on general web content as well in the browser.

> 
> 
> > I would suggest following up with Lucas, Jonas, and Paul about this more if
> > you have more questions. Lucas was the original designer around why intent
> > has to be specified in the app manifest, so he could give you a deep
> > rationale on this.
> 
> All the available evidence suggests that users are extremely insensitive
> to any sort of fine-grained permissions at application install time.
> See, for instance:
> 
> http://www.cs.berkeley.edu/~afelt/felt-androidpermissions-soups.pdf

We currently don't do any of what you are describing at app install time from the user's perspective - it's a simple app install prompt for asking to install the app with a name, developer name & url, app size, etc. The user would find this out during runtime at the contextual point that the app needs access to the API if PROMPT_ACTION is provided.

What happens instead at install time is that we are installing the permissions provided in the manifest for the particular app. That then allows the app to request access to specific WebAPIs against the permissions matrix based on their app type. In getUserMedia's case for a third-party use case, providing a manifest permission means that the app's content will be able to request access with a prompt for access to the camera/mic based on the matrix provided above. But it certainly doesn't mean that app content gets access without the prompt. So the only difference in the app use case is that we're additionally requiring a manifest property to be specified, but after that, we're doing the same thing that would happen in web content in the browser for the 3rd party use case.

(In reply to Eric Rescorla (:ekr) from comment #23)
> (In reply to Fabrice Desré [:fabrice] from comment #22)
> > Eric,
> > 
> > You could read https://wiki.mozilla.org/Apps/Security
> 
> 
> I have. It seems to just state the outcome of the decisions, not
> provide any analysis that considers the human factors issues.

The deeper details of this I'd probably query Jonas & Paul directly about.

> 
> 
> 
> 
> > Also note that the Felt paper you cite is mostly inapplicable to b2g where
> > we choosed to *not* ask the user to make security decisions as much as
> > possible.
> > 
> > I'm pretty sure we want to PROMPT for web content and non-privileged apps,
> > and ALLOW for privileged apps.
> 
> As I understan Jason's comment here, it is that we need to state an
> intention (i.e., a string) that is attached to the manifest and
> shown to the user at the time of installation. I.e.,
> 
> https://wiki.mozilla.org/Apps/Security#Data_Usage_Intention
> 
> Isn't this precisely the experience that the paper I am citing refers
> to?

Actually that's not what I mean. When an app specifies a permission in the app manifest, we don't show that prompt at app install time. What instead happens in our app security model is that the permission is installed for that particular app to allow the app to access the targeted WebAPI against the specified permissions matrix based on the app type. When access to that WebAPI happens within the web app, then we follow the permissions matrix on what to do here. In the 3rd party use case for getUserMedia, it's PROMPT_ACTION. That means a user would be prompted to grant access to the camera and microphone. If they grant access to the camera and microphone, then app content can proceed to use the camera and microphone as such.

Note - the data usage intention field isn't even implemented right now in the UI. But when it does get implemented, the data usage intention would likely be shown contextually at the time the API request happens within the app and potentially in the app permissions UI in the Settings app in Gaia.

If you really have more concerns more around the data usage intention string issue showing in the UI or not, I'd followup on bug 823385, which is tracking the UI work.
I missed a lot of posts, but I will try to provide some brief context on the app security model so we can get back on track here.

- For web apps, all permissions the app intends to use must be declared in the manifest. 
- For PROMPT permissions, we also user approval before granting access (i.e. at runtime, the first time the API is called)
- For ALLOW permissions, permission is just granted by virtue of being declared in the manifest. For security sensitive permissions, these are generally restricted to privileged apps, which are reviewed by the marketplace.

For reference see Permission list here:  https://developer.mozilla.org/en-US/docs/Web/Apps/App_permissions
Or the code here: https://mxr.mozilla.org/mozilla-central/source/dom/apps/src/PermissionsTable.jsm#35

Notes: 
* Permissions are declared in the app manifest so we have a precise enumeration of the permissions an app will use.One place this is used is in the settings app so that a user can modify permissions on a per-app basis. (Note that only PROMPT permissions are shown to the user in the settings app)
* Permissions are never shown to the user at install time
* Usage intentions are a feature quite separate to this discussion (bug 823385, or the web api list is probably a better place to discuss any unresolved questions about usage intentions)
* App reviews are seperate to this discussion since I don't think anyone is saying that we should restrict the webrtc permission to privileged apps only. (FWIW, only privileged apps go through the marketplace review process, normal web apps are not reviewed, nor could they be since they are dynamic)

Further documentation:
https://developer.mozilla.org/en-US/docs/Web/Apps/App_permissions
https://developer.mozilla.org/en-US/docs/Mozilla/Firefox_OS/Security/Security_model

For the history of the app security model see these two threads (there are others, but these are the biggest ones):
https://groups.google.com/forum/#!msg/mozilla.dev.webapps/LdAid6uLQbo/sJ5W-rT-VWUJ
https://groups.google.com/d/msg/mozilla.dev.webapps/UthgJMv9DaY/_s-IScrVnXQJ

FWIW ekr, the author of the paper you cite was an active contributor to these threads, and part of the reason why we went with the model we went with.

Hopefully that helps.
Now as for what model I propose, I think we can follow the geolocation model exactly for Firefox OS. That is:

Web Content: Prompt to access
Web App: Declare in manifest, prompt to access
Privileged App: Declare in manifest, prompt to access
Certified App: Declare in manifest, prompt to access

Justification:
* this is a web api, it needs to be exposed to web, but it needs user permisssion prior to enabling
* permission is declared in manifest for consistency with app security model (allowing features like settings app permission enumeration)
* permission should always be prompted for (even certified apps) due to the strong privacy implications (and also so that this is a permission that the user can disable, since only prompted permissions are exposed to the user currently).

There are other questions, such as do we remember the permission decision, how long do we grant access for etc. But I think these are secondary. I'll set up a security review later this week to go through these questions.
Final note to disagree with my earlier post about not distinguishing between the permission type. 

I think https://bugzilla.mozilla.org/show_bug.cgi?id=853356#c14 was suggesting this so that gaia know what permission prompt to show in Gaia. FWIW Firefox for Android shows different prompts: https://bug862377.bugzilla.mozilla.org/attachment.cgi?id=750068

Fabrice, are we going to need separate permission names (ie webrtc:*) in order to show a custom dialog based on what resource is being requested/what is available?  Do we also need some kind of capabilities API so Gaia can know the list of cameras/microphones to display?
Flags: needinfo?(fabrice)
(In reply to Paul Theriault [:pauljt] from comment #29)
> * Usage intentions are a feature quite separate to this discussion (bug
> 823385, or the web api list is probably a better place to discuss any
> unresolved questions about usage intentions)

OK, that's all I'm complaining about.


> * App reviews are seperate to this discussion since I don't think anyone is
> saying that we should restrict the webrtc permission to privileged apps
> only. (FWIW, only privileged apps go through the marketplace review process,
> normal web apps are not reviewed, nor could they be since they are dynamic)

I don't think it's orthogonal to the question of usage intentions, since,
as I said, they aren't mechanically enforced.


> For the history of the app security model see these two threads (there are
> others, but these are the biggest ones):
> https://groups.google.com/forum/#!msg/mozilla.dev.webapps/LdAid6uLQbo/sJ5W-
> rT-VWUJ
> https://groups.google.com/d/msg/mozilla.dev.webapps/UthgJMv9DaY/_s-IScrVnXQJ
> 
> FWIW ekr, the author of the paper you cite was an active contributor to
> these threads, and part of the reason why we went with the model we went
> with.

Is there something in that thread that analyzes te intention model?
(In reply to Paul Theriault [:pauljt] from comment #30)
> Certified App: Declare in manifest, prompt to access
I would observe that the existing installed camera app
does not prompt to access. Do we really want the installed
app to be better than any app anyone can write?


> There are other questions, such as do we remember the permission decision,
> how long do we grant access for etc. But I think these are secondary.

For any Web app, these need to be consistent with the desktop
browser experience.
(In reply to Paul Theriault [:pauljt] from comment #31)
> Final note to disagree with my earlier post about not distinguishing between
> the permission type. 
> 
> I think https://bugzilla.mozilla.org/show_bug.cgi?id=853356#c14 was
> suggesting this so that gaia know what permission prompt to show in Gaia.
> FWIW Firefox for Android shows different prompts:
> https://bug862377.bugzilla.mozilla.org/attachment.cgi?id=750068

Wait, let's distinguish between two things:

1. What device I am asking for.
2. What I intend to do with it.

I think that having the prompt indicate the former is imperative.
It's having it indicate the latter I have a problem with, and I
would note that it won't be indicated by any web app.


> Fabrice, are we going to need separate permission names (ie webrtc:*) in
> order to show a custom dialog based on what resource is being requested/what
> is available?  Do we also need some kind of capabilities API so Gaia can
> know the list of cameras/microphones to display?
(In reply to Eric Rescorla (:ekr) from comment #32)
> (In reply to Paul Theriault [:pauljt] from comment #29)
> > * Usage intentions are a feature quite separate to this discussion (bug
> > 823385, or the web api list is probably a better place to discuss any
> > unresolved questions about usage intentions)
> 
> OK, that's all I'm complaining about.

Note that usage intentions are not shown to the user yet (for various reasons, UI space mainly, but also there was concern about coercing users) Afaik, the intentions are only read by marketplace reviewers when doing reviews of _privileged_ apps.

> 
> 
> > * App reviews are seperate to this discussion since I don't think anyone is
> > saying that we should restrict the webrtc permission to privileged apps
> > only. (FWIW, only privileged apps go through the marketplace review process,
> > normal web apps are not reviewed, nor could they be since they are dynamic)
> 
> I don't think it's orthogonal to the question of usage intentions, since,
> as I said, they aren't mechanically enforced.

I think there are crossed wires above - in c15 you were talking about having one permission since flagging intention via different permissions names was not useful from a security perspective since the permissions ultimately have a similar impact, due the attacks you cite. Which I totally agree with.

I think since you used the word 'intentions' the discussion c16 then got onto talking about manifest usage intentions, which is something separate I think. Manifest usage intentions are not currently shown to the user, but ultimately we would like to show these once we can figure out an appropriate place for these to be shown. The only place these usage intentions are currently used is by marketplace reviewers, when they are reviewing _privileged_ apps only. Hence why I don't think the manifest usage intentions are important to this particular discussion.

Bottom line is usage intentions will not be relevant for web content, since there is no manifest. So we cant base any security control for webrtc on this.

> 
> 
> > For the history of the app security model see these two threads (there are
> > others, but these are the biggest ones):
> > https://groups.google.com/forum/#!msg/mozilla.dev.webapps/LdAid6uLQbo/sJ5W-
> > rT-VWUJ
> > https://groups.google.com/d/msg/mozilla.dev.webapps/UthgJMv9DaY/_s-IScrVnXQJ
> > 
> > FWIW ekr, the author of the paper you cite was an active contributor to
> > these threads, and part of the reason why we went with the model we went
> > with.
> 
> Is there something in that thread that analyzes te intention model?
(In reply to Eric Rescorla (:ekr) from comment #33)
> (In reply to Paul Theriault [:pauljt] from comment #30)
> > Certified App: Declare in manifest, prompt to access
> I would observe that the existing installed camera app
> does not prompt to access. Do we really want the installed
> app to be better than any app anyone can write?

Certified apps (ie those that come with the phone) in general dont prompt for APIs since we wrote them, and we trust them. The only permission which we actually prompt for in certified apps is geolocation, because of the privacy implications. As per c30, I am proposing we adopt the same model for webrtc, ie even certified apps have to prompt for, and the user has control over this permission.


FWIW the camera app doesn't use webrtc, it uses the camera control API. This API that allows much deeper control than webrtc, such as control of aperture, flash etc. Unfortunately because of this, currently this API needs direct device access, which is insecure to expose to third party apps. The original intention was to expose this API to third party apps, but we weren't able to achieve this securely for version 1. Having the camera prompt for the Camera API isnt useful since if it can't work without the API, and since no other apps can use this app, there wasnt any point in considering equality between third-party and the camera app in this case. or at least not yet, anyways.

> 
> 
> > There are other questions, such as do we remember the permission decision,
> > how long do we grant access for etc. But I think these are secondary.
> 
> For any Web app, these need to be consistent with the desktop
> browser experience.
UX will be providing a spec for this in the near future. We're coordinating with Maire. Clearing the needinfo since this work is now in progress.
Flags: needinfo?(firefoxos-ux-bugzilla)
(In reply to Paul Theriault [:pauljt] from comment #31)
> Final note to disagree with my earlier post about not distinguishing between
> the permission type. 
> 
> I think https://bugzilla.mozilla.org/show_bug.cgi?id=853356#c14 was
> suggesting this so that gaia know what permission prompt to show in Gaia.
> FWIW Firefox for Android shows different prompts:
> https://bug862377.bugzilla.mozilla.org/attachment.cgi?id=750068

> Fabrice, are we going to need separate permission names (ie webrtc:*) in
> order to show a custom dialog based on what resource is being requested/what
> is available?  Do we also need some kind of capabilities API so Gaia can
> know the list of cameras/microphones to display?

It looks like we don't want separate permission names, but gecko will have to send to gaia enough information to build a nice dialog with the device list and let the user choose what it wants to use. I proposed webrtc:* permissions but I now think that it's different from device-storage:* where the app knows which kind of storage it needs. With webrtc we let the user choose the media source(s) so a single "webrtc" permission should be enough.
Flags: needinfo?(fabrice)
Summary: Permission check for microphone acquisition → [B2G getUserMedia] Display camera/ microphone permission acquisition prompt by ContentPermmissionRequest
FYI - Here's a doc for what Chrome decided to do here: http://developer.chrome.com/apps/declare_permissions.html.

Chrome has two permissions:

- audioCapture - microphone
- videoCapture - camera
Blocks: 894848
Summary: [B2G getUserMedia] Display camera/ microphone permission acquisition prompt by ContentPermmissionRequest → [B2G getUserMedia] Display camera/ microphone permission acquisition prompt
Posted patch Permissions for gUM (obsolete) — Splinter Review
Add 3 permission; 'audio-capture', 'video-capure', and 'webrtc'.

1. Add 'video-capture' because 'camera' doesn't include PROMPT_ACTION. Since 'camera' is for mozCamera, it may be better to define another one for gUM.
2. 'webrtc' is for both audio/video recording.

Please comments, thanks.
Attachment #736695 - Attachment is obsolete: true
Attachment #740222 - Attachment is obsolete: true
Depends on: 898343
Create bug 898343 for detail implementations. Keep this one to discuss permission problem.
Depends on: 898949
Comment on attachment 781609 [details] [diff] [review]
Permissions for gUM

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

::: dom/apps/src/PermissionsTable.jsm
@@ +274,5 @@
>                               app: DENY_ACTION,
>                               privileged: DENY_ACTION,
>                               certified: ALLOW_ACTION
>                             },
> +                           "webrtc": {

If audio-capture & video-capture already exist here, why do we need a third permission here? You could handle the audio & video case by just checking if both audio-capture & video-capture both exist.

Point of comparison - Google did this in two permissions.
Marcos - There's a lot of debate here on what manifest permissions to use for getUserMedia. The patch shows you one approach. My comment indicates an alternative approach.

From the spec owner's side for the app manifest - what do you manifest permission properties would you prefer to be used?
Flags: needinfo?(mcaceres)
If I understand correctly, the two permission model should work. 

As an aside: I share Eric's concerns about the usefulness of this permission in the manifest for hosted-applications. It only really seems applicable for packaged apps, as adding it for hosted apps shouldn't be necessary. However, if all new Web Platform APIs are going to be enabled in this way, then I guess that's water under the bridge now.
Flags: needinfo?(mcaceres)
(In reply to Marcos Caceres [:marcosc] from comment #44)
> If I understand correctly, the two permission model should work. 
> 
> As an aside: I share Eric's concerns about the usefulness of this permission
> in the manifest for hosted-applications. It only really seems applicable for
> packaged apps, as adding it for hosted apps shouldn't be necessary. However,
> if all new Web Platform APIs are going to be enabled in this way, then I
> guess that's water under the bridge now.

Right now all hosted apps require app manifest permissions to exist to allow usage of the particular WebAPI. So this is a more of a general design decision with each explicit prompt permission for WebAPIs - not specific to getUserMedia.

I think the main motivation was for app reviewers to gain knowledge of explicit permissions of the app when conducting reviews. If we're questioning this now, then we should start a conversation with Jonas about this.
(In reply to Jason Smith [:jsmith] from comment #45)
> (In reply to Marcos Caceres [:marcosc] from comment #44)
> > If I understand correctly, the two permission model should work. 
> > 
> > As an aside: I share Eric's concerns about the usefulness of this permission
> > in the manifest for hosted-applications. It only really seems applicable for
> > packaged apps, as adding it for hosted apps shouldn't be necessary. However,
> > if all new Web Platform APIs are going to be enabled in this way, then I
> > guess that's water under the bridge now.
> 
> Right now all hosted apps require app manifest permissions to exist to allow
> usage of the particular WebAPI. So this is a more of a general design
> decision with each explicit prompt permission for WebAPIs - not specific to
> getUserMedia.

yeah, hence water under the bridge. Anyway, this is off topic. 

> I think the main motivation was for app reviewers to gain knowledge of
> explicit permissions of the app when conducting reviews. If we're
> questioning this now, then we should start a conversation with Jonas about
> this.

Sure. Will take this up elsewhere - sorry about the noise. 

Let me know if you need anything else re: manifest stuff :)
Posted patch gUM_Permissions (obsolete) — Splinter Review
IMHO, I don't think a new permission for audio+video is required.
The original debet of this is to avoid 2 prompts on comment 14. On https://bugzilla.mozilla.org/show_bug.cgi?id=898343#c10, UX teams should be notified about merging multiple requests into one prompt which can be improved in future plan.
To keep thing going and meet the FxOS 1.2 scope, I removed the 'webrtc' in patch. It is still an option to add it back if the final conclusive is made.
Attachment #781609 - Attachment is obsolete: true
Comment on attachment 786118 [details] [diff] [review]
gUM_Permissions

It takes a while and it looks like no objection.

Hi Fabrice,
Could you please review the permission name?
Bug 898343 needs it before landing.
Thanks.
Attachment #786118 - Flags: review?(fabrice)
I still don't understand how we will show only one prompt if we have two permissions. I would be more comfortable r+ this if you also had the UI pieces done that implement that.
On current Gaia implementation, all the requests (including geolocation, desktop notification, gUM...) will be queued and prompted one by one. To merge multiple gUM requests, Gaia needs to check the the queue if there are gUM resuests from the same app/url [1].
For audio+video, Gecko will send audio and video request to Gaia respectively just like they are requested by two gUM calls separately in the same script.

Implementation plan:
1.2 will support single gUM request prompt.
1.3 will support merge multiple requests gUM request to one prompt [2].

[1] https://bugzilla.mozilla.org/show_bug.cgi?id=898343#c18
[2] https://bugzilla.mozilla.org/show_bug.cgi?id=898343#c10
Whiteboard: [WebRTC][blocking-webrtc-][b2g-gum+] → [WebRTC][blocking-webrtc-][b2g-gum+][FT: Media Recording, Sprint 2]
After discussing with :fabrice on IRC and :alive on other meeting.

1. Don't display 2 prompts for audio+video at all, even on 1.2 only.
2. If script calls audio gUM and then video gUM later, it will display 2 prompts instead of merging them into one.
3. If script calls one audio gUM following of another audio gUM request before the first audio prompt dismisses. It should be merged into one, bug 907075.
Attachment #786118 - Flags: review?(fabrice)
Hi :dougt,
To support audio+video permissions on the same ContentPermissionRequest instance. I plan to modify this idl as below.

~~~

interface nsIContentPermissionRequest : nsISupports {

  void types(out unsigned long aLength,
             [array, size_is(aLength), retval] out wstring aTypes);

~~~

There will be several modifications on Gecko for this change like geolocation, device-storage, nsContentPermissionHelper... etc. 

What do you think about it? Any comment is welcome.
Thanks.
Flags: needinfo?(doug.turner)
In comment 51, I think #3 is an unexpected ux.

Consider an app that does two requests -- a mic requests, then a camera request.

Users sees:
   "This app only wants to use my mic"

The user is about to click OK.

Then the user may see:
   "This app wants to use my mic and my camera"... 

Would the user notice?


I tend to think we need a app facing permission request API that allows the developer to pre-grant permissions.  They could ask for the set of all things they might be interested in (geo, camera, mic, ect.) in one API call.  I tend to think this 'only ask for permission as a direct result of user interaction' has problems (such as this).



Since I don't want to derail the conversation, I defer to fabrice on the API change you're making.
Flags: needinfo?(doug.turner)
(In reply to Doug Turner (:dougt) from comment #53)
> I tend to think we need a app facing permission request API that allows the
> developer to pre-grant permissions.  They could ask for the set of all
> things they might be interested in (geo, camera, mic, ect.) in one API call.
> I tend to think this 'only ask for permission as a direct result of user
> interaction' has problems (such as this).

I tend to disagree, so if you go down that route please loop me in :-).
WRT comment #51, part 3,

I tend to agree with Doug here. You should consolidate requests received in
immediate succession from the JS (i.e., before it returns control to
the system) but not those received asynchrounously. This has the advantage
of being easier, as well.
Posted patch gUM_Implementation (obsolete) — Splinter Review
Draft version.
1. Pass a string array insead of a single string in dom/ipc/PBrowser.ipdl and dom/interfaces/base/nsIContentPermissionPrompt.idl.
2. Add dom/media/MediaPermissionGonk.h and dom/media/MediaPermissionGonk.cpp.
3. Handle multiple requests in b2g/components/ContentPermissionPrompt.js.

TODO:
1. Separate this patch.
2. Clear up and add proper comment in the patch.
Posted patch gUM_Permissions (obsolete) — Splinter Review
Split patch, part 1.
Attachment #786118 - Attachment is obsolete: true
Posted patch gUM_Implementation_idl_pidl (obsolete) — Splinter Review
Split patch, part 2.
Add string array in PBrowser.ipdl and nsIContentPermissionPrompt.idl.
Attachment #795853 - Attachment is obsolete: true
Posted patch gUM_Implementation (obsolete) — Splinter Review
Split patch, part 3.
Add MediaPermissionGonk.h and MediaPermissionGonk.cpp under dom/media.
(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #54)
> (In reply to Doug Turner (:dougt) from comment #53)
> > I tend to think we need a app facing permission request API that allows the
> > developer to pre-grant permissions.  They could ask for the set of all
> > things they might be interested in (geo, camera, mic, ect.) in one API call.
> > I tend to think this 'only ask for permission as a direct result of user
> > interaction' has problems (such as this).
> 
> I tend to disagree, so if you go down that route please loop me in :-).

I go that way, but only the new add permissions "video-capture" and "audio-capture" are allowed in one API call.
Comment on attachment 795911 [details] [diff] [review]
gUM_Implementation_idl_pidl

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

::: dom/interfaces/base/nsIContentPermissionPrompt.idl
@@ +40,5 @@
> +   *  The type of the permission requests, such as
> +   *  "geolocation".
> +   */
> +  void types([optional] out unsigned long aLength,
> +             [array, size_is(aLength), retval] out wstring aTypes);

Using nsIArray for array type of attribute in IDL will be better. (see example in nsIMIMEInfo http://mxr.mozilla.org/mozilla-central/source/netwerk/mime/nsIMIMEInfo.idl#188)
Posted patch gUM_part_1 (obsolete) — Splinter Review
Add new permissions and prompt for mutiple permissions request.
Attachment #795910 - Attachment is obsolete: true
Attachment #795911 - Attachment is obsolete: true
Attachment #795912 - Attachment is obsolete: true
Posted patch gUM_part_2 (obsolete) — Splinter Review
Add MediaPermissionGonk.h and MediaPermissionGonk.cpp to implement ContentPermissionPrompt.
Posted patch gUM_part_3 (obsolete) — Splinter Review
Pass a string array instead of a single string to support multiple permissions request.
Attachment #796483 - Attachment is patch: true
Posted patch gUM_part_1 (obsolete) — Splinter Review
draft version.
Add MediaPermissionGonk.h and MediaPermissionGonk.cpp to implement ContentPermissionPrompt.
Attachment #796481 - Attachment is obsolete: true
Posted patch gUM_part_1 (obsolete) — Splinter Review
Add new permissions and prompt for mutiple permissions request.

1. Handle one permission deny and one permission allow condition.
2. Some minor codes improvement.
Attachment #796482 - Attachment is obsolete: true
Attachment #796483 - Attachment is obsolete: true
Attachment #796520 - Attachment is obsolete: true
Posted patch gUM_part_2 (obsolete) — Splinter Review
Add MediaPermissionGonk.h and MediaPermissionGonk.cpp to implement ContentPermissionPrompt.

1. Return correct message in permission deny case.
Posted patch gUM_part_3 (obsolete) — Splinter Review
Pass a string array instead of a single string to support multiple permissions request.

1. minor coding style improvement
Posted patch gUM_part_1 (obsolete) — Splinter Review
Add new permissions and prompt for mutiple permissions request.

1. Handle multple-permission request; currently, only allow audio-capture and video-capture are requested at the same request instance.
2. Send the PROMPT_ACTION request only to UI. Others will be filtered.
Attachment #796522 - Attachment is obsolete: true
Attachment #796523 - Attachment is obsolete: true
Attachment #796524 - Attachment is obsolete: true
Attachment #797032 - Flags: review?(fabrice)
Posted patch gUM_part_2 (obsolete) — Splinter Review
Add MediaPermissionGonk.h and MediaPermissionGonk.cpp to implement ContentPermissionPrompt.

Most part of this patch is from bug 898343 which has been reviewed before, the major difference is this patch will send 2 permissions in the same request instance.
Attachment #797033 - Flags: review?(rjesup)
Posted patch gUM_part_3 (obsolete) — Splinter Review
Pass a string array instead of a single string to support multiple permissions request.

Add string array "types" in nsIContentPermissionPrompt.idl and dom/ipc/PBrowser.ipdl.
Attachment #797034 - Flags: review?(doug.turner)
Comment on attachment 797032 [details] [diff] [review]
gUM_part_1

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

::: dom/apps/src/PermissionsTable.jsm
@@ +293,5 @@
>                             },
> +                           "audio-capture": {
> +                             app: PROMPT_ACTION,
> +                             privileged: PROMPT_ACTION,
> +                             certified: ALLOW_ACTION

Per talking with pauljt, this needs to prompt, not allow by default.
Comment on attachment 797032 [details] [diff] [review]
gUM_part_1

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

That mostly looks good, but I want to see a new version with:
- more comments
- various nits fixed
- do we have tests?

::: b2g/components/ContentPermissionPrompt.js
@@ +30,5 @@
>                                     "PermSettings",
>                                     "@mozilla.org/permissionSettings;1",
>                                     "nsIDOMPermissionSettings");
>  
> +function rememberPermission(aTypesInfo, aPrincipal, aSession)

what is aTypesInfo structure? We need a lot more comments in this patch, things are getting tricky.

@@ +72,5 @@
>  
>  ContentPermissionPrompt.prototype = {
>  
> +  handleExistingPermission: function handleExistingPermission(request, typesInfo) {
> +    var getTypePermission = function (type) {

s/var/let, here and everywhere else you used |var|

@@ +73,5 @@
>  ContentPermissionPrompt.prototype = {
>  
> +  handleExistingPermission: function handleExistingPermission(request, typesInfo) {
> +    var getTypePermission = function (type) {
> +      type.permission = Services.perms.testExactPermissionFromPrincipal(request.principal, type.access);

nit: 80 char per line maximum.

@@ +91,5 @@
>      }
> +
> +    var checkDenyPermission = function (type) {
> +      if (type.permission == Ci.nsIPermissionManager.DENY_ACTION ||
> +          type.permission == Ci.nsIPermissionManager.UNKNOWN_ACTION && PROMPT_FOR_UNKNOWN.indexOf(type.access) < 0) {

Why did you remove the request.cancel() here?

@@ +113,5 @@
> +      var checkIfAllowMultiRequest = function (type) {
> +        if (ALLOW_MULTIPLE_REQUESTS.indexOf(type.access) == -1) {
> +          return false;
> +        }
> +        return true;

Replace by: return ALLOW_MULTIPLE_REQUESTS.indexOf(type.access) !== -1

@@ +114,5 @@
> +        if (ALLOW_MULTIPLE_REQUESTS.indexOf(type.access) == -1) {
> +          return false;
> +        }
> +        return true;
> +      }

Nit: add blank line

@@ +190,1 @@
>         return;

nit: use {} even for single line ifs.

@@ +195,5 @@
> +      if (typesInfo[0].permission == Ci.nsIPermissionManager.PROMPT_ACTION && !typesInfo[0].deny) {
> +        typesInfo.push(typesInfo[0]);
> +      }
> +      typesInfo.shift();
> +    }

I think that we can do something a bit simpler like:
typesInfo.forEach((aType, aIndex) => {
  if (typesInfo[0].permission != Ci.nsIPermissionManager.PROMPT_ACTION || typesInfo[0].deny) {
  typesInfo.splice(aIndex);
}
});
Attachment #797032 - Flags: review?(fabrice) → feedback+
Comment on attachment 797033 [details] [diff] [review]
gUM_part_2

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

::: dom/media/MediaManager.cpp
@@ +907,5 @@
>    }
>    LOG(("%s: default prefs: %dx%d @%dfps (min %d)", __FUNCTION__,
>         mPrefs.mWidth, mPrefs.mHeight, mPrefs.mFPS, mPrefs.mMinFPS));
>  
> +#if defined(MOZ_WIDGET_GONK)

MOZ_WIDGET_GONK is only defined on device. Don't we want that to work on b2g desktop also? That would then be MOZ_B2G instead.
Comment on attachment 797034 [details] [diff] [review]
gUM_part_3

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

there are other call sites to update, like https://mxr.mozilla.org/mozilla-central/source/dom/permission/PermissionPromptHelper.jsm#82

::: dom/interfaces/base/nsIContentPermissionPrompt.idl
@@ +12,5 @@
>   * Interface allows access to a content to request
>   * permission to perform a privileged operation such as
>   * geolocation.
>   */
>  [scriptable, uuid(1de67000-2de8-11e2-81c1-0800200c9a66)]

you need to update the uuid
Posted patch gUM_part_1 (obsolete) — Splinter Review
1. Update default permission type according to comment 72.
2. Add comments, coding nit according to review feedback.
Attachment #797032 - Attachment is obsolete: true
Attachment #797740 - Flags: review?(fabrice)
(In reply to Fabrice Desré [:fabrice] from comment #73)
> Comment on attachment 797032 [details] [diff] [review]
> gUM_part_1
> 
> Review of attachment 797032 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> That mostly looks good, but I want to see a new version with:
> - more comments
> - various nits fixed
> - do we have tests?

Yes, I'm cooking tests now. I'll upload test patch later.

> @@ +91,5 @@
> >      }
> > +
> > +    var checkDenyPermission = function (type) {
> > +      if (type.permission == Ci.nsIPermissionManager.DENY_ACTION ||
> > +          type.permission == Ci.nsIPermissionManager.UNKNOWN_ACTION && PROMPT_FOR_UNKNOWN.indexOf(type.access) < 0) {
> 
> Why did you remove the request.cancel() here?

request.cancel() move to every() function immediately after checkDenyPermission defined.

    if (typesInfo.every(checkDenyPermission)) {
       debug("all permission requests are denied");
       request.cancel();
       return true;
    }


Others should be fixed. Thanks for feedbacks.
blocking-b2g: --- → koi+
Posted patch gUM_part_2 (obsolete) — Splinter Review
Update build flag to MOZ_B2G according to comment 74.
Attachment #797033 - Attachment is obsolete: true
Attachment #797033 - Flags: review?(rjesup)
Attachment #797741 - Flags: review?(rjesup)
Posted patch gUM_part_3 (obsolete) — Splinter Review
Update UUID according to comment 75.
Attachment #797034 - Attachment is obsolete: true
Attachment #797034 - Flags: review?(doug.turner)
Attachment #797744 - Flags: review?(doug.turner)
Fabrice wrote:
>MOZ_WIDGET_GONK is only defined on device. Don't we want that to work on b2g desktop also? That would then be MOZ_B2G instead.

If we're going to support b2g desktop webrtc/getusermedia, there are three other ifdefs in MediaManager.h/cpp, and one in media/webrtc/trunk (that one should likely change to WEBRTC_GONK, which is defined by the common.gypi file)
Comment on attachment 797741 [details] [diff] [review]
gUM_part_2

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

I assume when "Allow" happens, the user will have been given the choice to select a device or no video (or no audio, depending on which dropdown it is) and it will have modified the mRequest->mDevices array, so you can accept audio and reject video (for example).

r+ with the following nits addressed (especially the mDevices->Count() issue and making sure the error text makes sense.)

::: dom/media/MediaPermissionGonk.cpp
@@ +116,5 @@
> +    }
> +  }
> +
> +  if (!mDevices.Length()) {
> +    mErrorMsg.Assign(NS_LITERAL_STRING("NO AVAILABLE DEVICE"));

Consider aligning the text for this error with the text for the desktop error (NO_DEVICES_FOUND or HARDWARE_UNAVAILABLE depending on the error).  Also, normally MediaManager.cpp will dispatch those errors back; if this string is used for something else (user-visible, etc) then never mind.

@@ +150,5 @@
> +{
> +  *aLength = mTypes.Length();
> +  PRUnichar** ret =
> +    static_cast<PRUnichar**>(NS_Alloc(*aLength * sizeof(PRUnichar*)));
> +  NS_ENSURE_TRUE(ret, NS_ERROR_OUT_OF_MEMORY);

Is there a reason to use a fallible allocator here for a <20 byte string?  If so, fine, otherwise switch to an infallible allocator and get rid of the NS_ENSURE_TRUE.

@@ +153,5 @@
> +    static_cast<PRUnichar**>(NS_Alloc(*aLength * sizeof(PRUnichar*)));
> +  NS_ENSURE_TRUE(ret, NS_ERROR_OUT_OF_MEMORY);
> +
> +  for (uint32_t i = 0; i < *aLength; ++i) {
> +    ret[i] = UTF8ToNewUnicode(mTypes[i]);

We don't have a standard converter for this?

@@ +302,5 @@
> +MediaPermissionMgr::stateClose()
> +{
> +  nsresult rv;
> +  rv = NS_NewISupportsArray(getter_AddRefs(mDevices));
> +  NS_ASSERTION(NS_SUCCEEDED(rv), "Out of memory");

No need for a string here

@@ +310,5 @@
> +      mDevices->AppendElement(mRequest->mDevices[i]);
> +    }
> +  }
> +
> +  // If there is granted device, it returns "allow" to script.

minor nit: "is a granted" and "to the script"

@@ +315,5 @@
> +  nsCOMPtr<nsIObserverService> obs = mozilla::services::GetObserverService();
> +  if (obs) {
> +    uint32_t    itemCount;
> +    rv = mDevices->Count(&itemCount);
> +    NS_ASSERTION(NS_SUCCEEDED(rv), "Fail to get array count");

Do what nsDocShell.cpp does: uint32_t itemCount = 0; mDevices->Count(&itemCount);
This lets the normal no-items case handle errors, and if that had failed (in reality it can't!), this code would have been wrong anyways and caused it to use an uninitialized value for itemCount.

@@ +326,5 @@
> +      obs->NotifyObservers(supportsString, "getUserMedia:response:deny", mCallID.get());
> +    }
> +  }
> +
> +  // Stop sate machine and free requests

nit: state
Attachment #797741 - Flags: review?(rjesup) → review+
Comment on attachment 797744 [details] [diff] [review]
gUM_part_3

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

::: dom/interfaces/base/nsIContentPermissionPrompt.idl
@@ +20,4 @@
>     *  The access of the permission request, such as
>     *  "read".
>     */
>    readonly attribute ACString access;

access is per permission type.

Maybe we should have a new type is the union of both access and type

@@ +40,5 @@
> +   *  The type of the permission requests, such as
> +   *  "geolocation".
> +   */
> +  void types([optional] out unsigned long aLength,
> +             [array, size_is(aLength), retval] out wstring aTypes);

I think roc has some concern about doing it this way (supporting multiple permission requests in one go).  I support this.  Could you take this question to the platfrom newsgroup and cc both of us?
(In reply to Doug Turner (:dougt) from comment #82)
> 
> @@ +40,5 @@
> > +   *  The type of the permission requests, such as
> > +   *  "geolocation".
> > +   */
> > +  void types([optional] out unsigned long aLength,
> > +             [array, size_is(aLength), retval] out wstring aTypes);
> 
> I think roc has some concern about doing it this way (supporting multiple
> permission requests in one go).  I support this.  Could you take this
> question to the platfrom newsgroup and cc both of us?

Done.
https://groups.google.com/forum/?fromgroups#!topic/mozilla.dev.platform/jbxYU5FC27E
Posted patch gUM_test (obsolete) — Splinter Review
Test case.
1. Change SpecialPowers.MochPermissionPrompt for multiple permissions request.
2. Check track type of audio+video gUM request.
Attachment #800564 - Flags: review?(fabrice)
Posted patch gUM_part_2 (obsolete) — Splinter Review
(In reply to Randell Jesup [:jesup] from comment #81)
> Comment on attachment 797741 [details] [diff] [review]
> gUM_part_2
> 
> Review of attachment 797741 [details] [diff] [review]:
> -----------------------------------------------------------------

> @@ +153,5 @@
> > +    static_cast<PRUnichar**>(NS_Alloc(*aLength * sizeof(PRUnichar*)));
> > +  NS_ENSURE_TRUE(ret, NS_ERROR_OUT_OF_MEMORY);
> > +
> > +  for (uint32_t i = 0; i < *aLength; ++i) {
> > +    ret[i] = UTF8ToNewUnicode(mTypes[i]);
> 
> We don't have a standard converter for this?

I greped Gecko several times. There is another one used NS_strdup, but I don't think it better than UTF8ToNewUnicode.
Could you please let me know if there is other better way? I will change it, thanks.

Others are fixed.
Attachment #797741 - Attachment is obsolete: true
Flags: needinfo?(rjesup)
(In reply to Doug Turner (:dougt) from comment #82)
> Comment on attachment 797744 [details] [diff] [review]
> gUM_part_3
> 
> Review of attachment 797744 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: dom/interfaces/base/nsIContentPermissionPrompt.idl
> @@ +20,4 @@
> >     *  The access of the permission request, such as
> >     *  "read".
> >     */
> >    readonly attribute ACString access;
> 
> access is per permission type.
> 
> Maybe we should have a new type is the union of both access and type

IMHO, it probably needs an array of a permission object like [{type1, acess1}, {type2, access2}...]. It also needs to restrict the allowed multiple permissions request somewhere in Gecko. For example, only audio-capture and video-capture can be requested together so far.
Comment on attachment 800564 [details] [diff] [review]
gUM_test

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

::: dom/media/tests/mochitest/test_getUserMedia_permissions.html
@@ +29,5 @@
> +var gTests = [
> +  {
> +    type: 'audio-capture',
> +    perm: Ci.nsIPermissionManager.ALLOW_ACTION,
> +    constraints: {audio: true},

You cannot use direct constraints in a mochitest planning to be ran on buildbot, as we do not have real or simulated devices right now for camera/mic on B2G. You would need to implement support for fake devices on Firefox OS to do this - see https://bugzilla.mozilla.org/show_bug.cgi?id=815002 which represents the desktop version of what would be needed here.
Attachment #800564 - Flags: feedback-
Blocks: 913896
Comment on attachment 800564 [details] [diff] [review]
gUM_test

Actually, missed the fact that this fake parameter was included in a separate function call. Disregard my feedback statement.
Attachment #800564 - Flags: feedback-
Comment on attachment 800564 [details] [diff] [review]
gUM_test

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

::: dom/media/tests/mochitest/test_getUserMedia_permissions.html
@@ +29,5 @@
> +var gTests = [
> +  {
> +    type: 'audio-capture',
> +    perm: Ci.nsIPermissionManager.ALLOW_ACTION,
> +    constraints: {audio: true},

Disregard this comment - saw the fake reference below.

@@ +41,5 @@
> +    testEnd: true
> +  }
> +];
> +
> +function getUserMedia(constraints, onSuccess, onError) {

Note - this is code duplication. I think you should just include head.js to automatically get access to this function.

@@ +59,5 @@
> +      SimpleTest.finish();
> +    });
> +}
> +
> +function testTracks(constraints, stream) {

Note - the basic gUM tests which are already running on b2g emulators already cover this.
regarding comment #86, do we need another patch here?
Posted patch gUM_test (obsolete) — Splinter Review
Update according to comment 89.

1. use head.js
2. change track test codes
Attachment #800564 - Attachment is obsolete: true
Attachment #800564 - Flags: review?(fabrice)
Attachment #801976 - Flags: review?(jsmith)
Comment on attachment 801976 [details] [diff] [review]
gUM_test

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

Only including a review here for the mochitest piece. Someone else will need to handle reviewing the mock permission prompt area.

review- for some cleanup. I'm also putting r? on fabrice for reviewing the mock permission prompt pieces.

::: dom/media/tests/mochitest/Makefile.in
@@ +58,5 @@
>    pc.js \
>    templates.js \
>    $(NULL)
>  
> +ifdef MOZ_B2G

I'd double check with fabrice here on which MOZ_B2G value makes sense here.

::: dom/media/tests/mochitest/test_getUserMedia_permissions.html
@@ +4,5 @@
> +https://bugzilla.mozilla.org/show_bug.cgi?id=853356
> +-->
> +<head>
> +  <meta charset="utf-8">
> +  <title>Permission test for Device Storage</title>

Nit - mochitest title isn't right here

@@ +34,5 @@
> +    type: 'video-capture',
> +    perm: Ci.nsIPermissionManager.ALLOW_ACTION,
> +    constraints: {video: true, audio: true},
> +    testEnd: true
> +  }

We should probably also get a test for video only constraints.

@@ +37,5 @@
> +    testEnd: true
> +  }
> +];
> +
> +function testComplete() {

Why is setting of permission disabled to true necessary here? Why can't we just call SimpleTest.finish?

@@ +50,5 @@
> +
> +// check if the stream contains the requested track.
> +function checkTrackExist(constraints, stream) {
> +  var hasAudioTrack = stream.getAudioTracks().length > 0 ? true : false;
> +  var hasVideoTrack = stream.getVideoTracks().length > 0 ? true : false;

Nit - you shouldn't need the ? mark piece. Just do:

var hasAudioTrack = stream.getAudioTracks().length > 0;

@@ +57,5 @@
> +  is((constraints.video ? true : false), hasVideoTrack, "Contains video track: " + hasVideoTrack);
> +}
> +
> +function gUM(data) {
> +  SpecialPowers.addPermission(data.type, data.perm, document);

Each gTest that executes here should aim to operate independently of one another. However, right now, the 2nd test is dependent on the 1st executing. Can you fix this? That will help you have the ability to independently test video only vs. audio only vs. video + audio.

@@ +74,5 @@
> +  }
> +  getUserMedia(data.constraints, success, fail);
> +}
> +
> +SpecialPowers.pushPrefEnv({"set": [["media.navigator.permission.disabled", false]]},

Isn't this false by default?
Attachment #801976 - Flags: review?(jsmith)
Attachment #801976 - Flags: review?(fabrice)
Attachment #801976 - Flags: review-
(In reply to Doug Turner (:dougt) from comment #90)
> regarding comment #86, do we need another patch here?

Ok, I'll write a patch for it.
This change probably needs to take the requirements of bug 898949 into consideration.
IPDL modification for multiple permission request
Attachment #797744 - Flags: review?(doug.turner)
Attachment #797740 - Flags: review?(fabrice)
Posted patch ContentPermissionPrompt_IDL (obsolete) — Splinter Review
Hi :dougt,
The WIP of ContentPermissionRequest. Do you think it's ok?
Attachment #802852 - Flags: feedback?(doug.turner)
Comment on attachment 802852 [details] [diff] [review]
ContentPermissionPrompt_IDL

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

::: dom/interfaces/base/nsIContentPermissionPrompt.idl
@@ +38,5 @@
> +   *  The array will include the request types. Elements of this array are
> +   *  nsIContentPermissionType object. This array will conain at least one
> +   *  element.
> +   */
> +  readonly attribute nsIMutableArray types;

The types attribute should not be modifiable out side after the request object is created. We should change it to nsIArray.
No longer blocks: 894848
Posted patch gUM_part_1 (obsolete) — Splinter Review
WIP.
Update for new idl ContentPermissionType in ContentPermissionRequest and ipdl.
Attachment #797740 - Attachment is obsolete: true
Posted patch gUM_part_2 (obsolete) — Splinter Review
WIP.
Update for new idl ContentPermissionType in ContentPermissionRequest and ipdl.
Attachment #800570 - Attachment is obsolete: true
Posted patch gUM_part_3 (obsolete) — Splinter Review
WIP.
1. Add ContentPermissionType in ContentPermissionRequest.idl
2. Pass array of struct PermissionRequest between parent/child process
Attachment #797744 - Attachment is obsolete: true
Attachment #802276 - Attachment is obsolete: true
Attachment #802852 - Attachment is obsolete: true
Attachment #802852 - Flags: feedback?(doug.turner)
Blocks: 916524
This isn't a blocker for release. We only need to support gUM audio for 1.2 and we're past feature complete.
blocking-b2g: koi+ → koi?
blocking-b2g: koi? → 1.3?
This will be needed however for 1.3, as 1.3 is intended to support full webrtc support.
Posted patch gUM_part_1 (obsolete) — Splinter Review
Update summary:
Add nsIContentPermissionType interface in nsIContentPermissionPrompt.idl for multiple permissions request and an attribute types (array of nsIContentPermissionType) in nsIContentPermissionRequest. The new interface contain type and access.

1. Modified function prompt() in ContentPermissionPrompt.js for the new interface.
2. Modified MockPermissionPrompt for new interface and add try-catch for unregister exception during test.
Attachment #801976 - Attachment is obsolete: true
Attachment #804315 - Attachment is obsolete: true
Attachment #804317 - Attachment is obsolete: true
Attachment #804318 - Attachment is obsolete: true
Attachment #801976 - Flags: review?(fabrice)
Attachment #806545 - Flags: review?(fabrice)
Posted patch gUM_part_2 (obsolete) — Splinter Review
Update summary:

1. Update MediaPermissionGonk for new nsIContentPermissionType interface in nsIContentPermissionRequest.
2. Remove 'fake' ignore the permission check in MediaManager. ('fake' device will popup ask for permission if media.navigator.permission.disabled=false)
Attachment #806546 - Flags: review?(rjesup)
Flags: needinfo?(rjesup)
Posted patch gUM_part_3 (obsolete) — Splinter Review
Update summary:

1. Add nsIContentPermissionType in nsIContentPermissionRequest
2. Add helper functions in nsContentPermissionHelper which will be used among geolocation, devicestorage, media...etc.
Attachment #806548 - Flags: review?(doug.turner)
Posted patch gUM_Test (obsolete) — Splinter Review
1. Update according to comment 92.
2. Set media.navigator.permission.disabled=true in MediaRecorder tests. I remove the 'fake will pass permission check' in MediaManager for permission check in emulator.
Attachment #806555 - Flags: review?(jsmith)
Attachment #806546 - Flags: review?(rjesup) → review+
Comment on attachment 806546 [details] [diff] [review]
gUM_part_2

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

back to r? for now - does this remove the aspect of fake=true not prompting the user?  That's an important requirement for fake for (among other things) our mochitests/etc.
Attachment #806546 - Flags: review+ → review?
Comment on attachment 806555 [details] [diff] [review]
gUM_Test

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

review- mainly for the added preferences to the media recorder tests not requiring this change. There's also two sets of open questions here:

1. Why is media.navigator.permission.enabled required to be set now? If that's the case, why are some of the other tests in dom/media not getting updated by these changes?
2. Why is marionette required here over a mochitest?

We might kickoff a try run btw with this test.

::: content/media/test/test_mediarecorder_avoid_recursion.html
@@ +34,5 @@
>    });
>  }
>  
>  SimpleTest.waitForExplicitFinish();
> +SpecialPowers.pushPrefEnv({"set": [["media.navigator.permission.disabled", true]]}, startTest);

Why is this preference required to be set now? Fake getUserMedia never used to require this permission. If that's the case also, why aren't other tests needing to be updated in dom/media?

::: content/media/test/test_mediarecorder_creation.html
@@ +38,5 @@
>    manager.finished(token);
>  }
>  
> +SpecialPowers.pushPrefEnv({"set": [["media.navigator.permission.disabled", true]]},
> +                          manager.runTests(gMediaRecorderTests, startTest));

This shouldn't be necessary - this test has no dependencies on getUserMedia.

::: content/media/test/test_mediarecorder_record_audiocontext.html
@@ +56,5 @@
>      ok(false, 'Can t record audio context');
>    }
>  }
>  
> +SpecialPowers.pushPrefEnv({"set": [["media.navigator.permission.disabled", true]]}, startTest());

This isn't necessary - this test has no dependencies on getUserMedia.

::: content/media/test/test_mediarecorder_record_immediate_stop.html
@@ +106,5 @@
>    element.play();
>  }
>  
> +SpecialPowers.pushPrefEnv({"set": [["media.navigator.permission.disabled", true]]},
> +                          manager.runTests(gMediaRecorderTests, startTest));

Not needed - this test doesn't depend on getUserMedia.

::: content/media/test/test_mediarecorder_record_timeslice.html
@@ +95,5 @@
>    element.play();
>  }
>  
> +SpecialPowers.pushPrefEnv({"set": [["media.navigator.permission.disabled", true]]},
> +                          manager.runTests(gMediaRecorderTests, startTest));

Not needed - this test doesn't depend on getUserMedia.

::: dom/media/tests/marionette/test_getUserMedia_Permission.js
@@ +1,5 @@
> +/* Any copyright is dedicated to the Public Domain.
> + * http://creativecommons.org/publicdomain/zero/1.0/ */
> +
> +const Ci = SpecialPowers.Ci;
> +MARIONETTE_TIMEOUT = 60000;

Why is marionette required here over a mochitest?
Attachment #806555 - Flags: review?(jsmith) → review-
Got my answer to some of the questions I asked above upon re-reading the comments & discussing with jgriffin:

* Mochitests can't interact with Gaia, so we need Marionette to test prompts
* Checked the above comment which indicates that fake: true will now prompt in the emulator. Although there is the above question of why some of the other dom/media tests are not updated by these changes, as I would expect them to be affected by the change for fake: true to prompt.
Blocks: b2g-getusermedia
No longer blocks: b2g-webrtc
No longer blocks: b2g-multimedia
Comment on attachment 806545 [details] [diff] [review]
gUM_part_1

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

That's a good start, thanks Alfredo. Can you address my comments and flag me for another review?

You will also need to update all the other nsIContentPermissionPrompt implementations because of the changes to the idl (see browser/components/nsBrowserGlue.js, browser/metro/components/ContentPermissionPrompt.js, mobile/android/components/ContentPermissionPrompt.js and webapprt/ContentPermission.js)

::: b2g/components/ContentPermissionPrompt.js
@@ +36,5 @@
> + * aTypesInfo is an array of {type, access, permission, deny} which keeps
> + * the information of each permission. This arrary is initialized in
> + * ContentPermissionPrompt.prompt and used among functions.
> + *
> + * aTypesInfo[].type      : permission name

call that permission

@@ +38,5 @@
> + * ContentPermissionPrompt.prompt and used among functions.
> + *
> + * aTypesInfo[].type      : permission name
> + * aTypesInfo[].access    : permission name + request.access
> + * aTypesInfo[].permission: the default action of this permission

and call that action

@@ +109,5 @@
>      }
> +
> +    // If all permissions are DENY_ACTION or UNKNOWN_ACTION, call cancel()
> +    // without prompting.
> +    let checkDenyPermission = function (type) {

nit: function(type)

@@ +130,5 @@
> +  checkMultipleRequest: function checkMultipleRequest(typesInfo) {
> +    if (typesInfo.length == 1) {
> +      return true;
> +    } else if (typesInfo.length > 1) {
> +      let checkIfAllowMultiRequest = function (type) {

nit: function(type)

@@ -87,4 @@
>      if (request.principal.appId == Ci.nsIScriptSecurityManager.NO_APP_ID ||
>          request.principal.appId == Ci.nsIScriptSecurityManager.UNKNOWN_APP_ID) {
>        // This should not really happen
> -      request.cancel();

Why did you remove that?

@@ +153,5 @@
>      let appsService = Cc["@mozilla.org/AppsService;1"]
>                          .getService(Ci.nsIAppsService);
>      let app = appsService.getAppByLocalId(request.principal.appId);
>  
> +    // Check each permission if it's deined by permission manager with app's

s/deined/denied

@@ +155,5 @@
>      let app = appsService.getAppByLocalId(request.principal.appId);
>  
> +    // Check each permission if it's deined by permission manager with app's
> +    // URL.
> +    let checkIfDenyAppPrincipal = function (type) {

nit: function(type)

@@ +183,5 @@
>        request.allow();
>        return true;
>      }
>  
> +    // initialized the typesInfo and set the default value.

s/initialized/Initialize

@@ +218,1 @@
>         return;

hm, it looks like we sometimes return a boolean, and sometimes nothing. We need to fix that.

@@ +343,5 @@
>  
>      let details = {
>        type: type,
> +      permission: permissions[0], // XXX for compatiable, it can be removed
> +                                  // after Gaia udpated.

I would rather not do that, to be sure we don't keep outdated code in gaia.
Attachment #806545 - Flags: review?(fabrice) → feedback+
Posted patch gUM_part_1 (obsolete) — Splinter Review
Updated patch according to comment 109.
Attachment #806545 - Attachment is obsolete: true
Attachment #806546 - Attachment is obsolete: true
Attachment #806548 - Attachment is obsolete: true
Attachment #806555 - Attachment is obsolete: true
Attachment #806546 - Flags: review?
Attachment #806548 - Flags: review?(doug.turner)
Attachment #815220 - Flags: review?(fabrice)
Posted patch gUM_part_2 (obsolete) — Splinter Review
Keep not prompting for fake=true according to comment 106.
Attachment #815221 - Flags: review?(rjesup)
Posted patch gUM_part_3 (obsolete) — Splinter Review
Attachment #815222 - Flags: review?(doug.turner)
Posted patch gUM_Test (obsolete) — Splinter Review
Update patch according to comment 107.
1. Don't use "fake=true" in constraints since it's for many other tests and doesn't prompt permission. After discuss with :rjesup, it's not a good idea to remove it just for this testcase.
Instead of it, including this test with platform has "MOZ_B2G_CAMERA" flag. (emulator and device both have this "MOZ_B2G_CAMERA")
2. Use mochitest, marionette is not necessary here.

https://tbpl.mozilla.org/?tree=Try&rev=23c4b7f11e88
Attachment #815224 - Flags: review?(jsmith)
Comment on attachment 815224 [details] [diff] [review]
gUM_Test

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

::: dom/media/tests/mochitest/Makefile.in
@@ +9,5 @@
>  endif
> +
> +ifdef MOZ_B2G_CAMERA
> +MOCHITEST_FILES += \
> +		test_getUserMedia_permission.html \

Nit - spacing appears off

::: dom/media/tests/mochitest/test_getUserMedia_permission.html
@@ +38,5 @@
> +
> +    var hasAudioTrack = stream.getAudioTracks().length > 0;
> +    var hasVideoTrack = stream.getVideoTracks().length > 0;
> +
> +    SimpleTest.is(data.constraints.audio, hasAudioTrack, "Request audio track:" +

Nit - only is is needed, no SimpleTest needs to be included
Attachment #815224 - Flags: review?(jsmith) → review+
Forgot to mention:

review+ as long as this mochitest is confirmed to successfully run in a try run

You should kick off a try run and verify that your mochitest ran successfully.
Comment on attachment 815220 [details] [diff] [review]
gUM_part_1

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

r=me with nits addressed for the b2g parts and dom/apps part. The other changes look ok also, but you need to get them reviewed by their respective module owners.

::: b2g/components/ContentPermissionPrompt.js
@@ +4,5 @@
>  
>  "use strict"
>  
>  function debug(str) {
> +  dump("-*- ContentPermissionPrompt: " + str + "\n");

Put the comment back before landing ;)

@@ +13,5 @@
>  const Cu = Components.utils;
>  const Cc = Components.classes;
>  
> +const PROMPT_FOR_UNKNOWN = ["geolocation", "desktop-notification",
> +                            "video-capture", "audio-capture"];

nit: can you order them alphabetically?

@@ +22,1 @@
>  

here too.

::: browser/components/nsBrowserGlue.js
@@ +1970,5 @@
>    },
>  
>    prompt: function CPP_prompt(request) {
>  
> +    // only allow exactly one permission rquest here

Nit: // Only allow exactly one permission request here.

::: browser/metro/components/ContentPermissionPrompt.js
@@ +69,5 @@
>      return false;
>    },
>  
>    prompt: function(request) {
> +    // only allow exactly one permission rquest here

nit: // Only allow exactly one permission request here.

::: mobile/android/components/ContentPermissionPrompt.js
@@ +61,5 @@
>  
>    prompt: function(request) {
>      let isApp = request.principal.appId !== Ci.nsIScriptSecurityManager.NO_APP_ID && request.principal.appId !== Ci.nsIScriptSecurityManager.UNKNOWN_APP_ID;
>  
> +    // only allow exactly one permission rquest here

nit: same as in other files...

::: webapprt/ContentPermission.js
@@ +29,5 @@
>        .QueryInterface(Ci.nsIDOMChromeWindow);
>    },
>  
>    prompt: function(request) {
> +    // only allow exactly one permission rquest here

nit: here also..
Attachment #815220 - Flags: review?(fabrice) → review+
(In reply to Fabrice Desré [:fabrice] from comment #116)
> Comment on attachment 815220 [details] [diff] [review]
> gUM_part_1
> 
> Review of attachment 815220 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> r=me with nits addressed for the b2g parts and dom/apps part. The other
> changes look ok also, but you need to get them reviewed by their respective
> module owners.

Thanks for review.

Hi felipe,
Could you please review browser/components/nsBrowserGlue.js in gUM_part_1 patch?
Thank you.
Flags: needinfo?(felipc)
Hi jimm
Could you please review browser/metro/base/content/helperui/IndexedDB.js and browser/metro/components/ContentPermissionPrompt.js in gUM_part_1?
Thank you.
Flags: needinfo?(jmathies)
Comment on attachment 815220 [details] [diff] [review]
gUM_part_1

Hi wesj,
Could you please review mobile/android/components/ContentPermissionPrompt.js?
Thank you.
Attachment #815220 - Flags: feedback?(wjohnston)
Comment on attachment 815220 [details] [diff] [review]
gUM_part_1

Hi mrbkap,
Could you please review webapprt/ContentPermission.js in patch gUM_test_1?
Thank you.
Attachment #815220 - Flags: review?(mrbkap)
Attachment #815220 - Flags: feedback?
Comment on attachment 815220 [details] [diff] [review]
gUM_part_1

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

r=felipe on the nsBrowserGlue.js changes
Attachment #815220 - Flags: review+
Flags: needinfo?(felipc)
Comment on attachment 815220 [details] [diff] [review]
gUM_part_1

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

::: mobile/android/components/ContentPermissionPrompt.js
@@ +63,5 @@
>      let isApp = request.principal.appId !== Ci.nsIScriptSecurityManager.NO_APP_ID && request.principal.appId !== Ci.nsIScriptSecurityManager.UNKNOWN_APP_ID;
>  
> +    // only allow exactly one permission rquest here
> +    let types = request.types.QueryInterface(Ci.nsIArray);
> +    if (types.length != 1) {

We support multiple prompts at once on Android. Is there a good reason to bail here? I'd be fine with this, but you should file something in our component to update this.
Attachment #815220 - Flags: feedback?(wjohnston) → feedback+
blocking-b2g: 1.3? → ---
Attachment #815221 - Flags: review?(rjesup) → review+
Attachment #815220 - Flags: feedback?
(In reply to Wesley Johnston (:wesj) from comment #122)
> Comment on attachment 815220 [details] [diff] [review]
> gUM_part_1
> 
> Review of attachment 815220 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: mobile/android/components/ContentPermissionPrompt.js
> @@ +63,5 @@
> >      let isApp = request.principal.appId !== Ci.nsIScriptSecurityManager.NO_APP_ID && request.principal.appId !== Ci.nsIScriptSecurityManager.UNKNOWN_APP_ID;
> >  
> > +    // only allow exactly one permission rquest here
> > +    let types = request.types.QueryInterface(Ci.nsIArray);
> > +    if (types.length != 1) {
> 
> We support multiple prompts at once on Android. Is there a good reason to
> bail here? I'd be fine with this, but you should file something in our
> component to update this.

Thanks for feedback.

The original concern is security. From my understanding, the only case allows multiple request is audio+video in getUserMedia. But thing looks different on Android, I will filed a bug for it.
(In reply to Alfredo Yang from comment #118)
> Hi jimm
> Could you please review browser/metro/base/content/helperui/IndexedDB.js and
> browser/metro/components/ContentPermissionPrompt.js in gUM_part_1?
> Thank you.

For future reference, patches like this should be broken down into individual patches for each platform and posted individually so you can set review requests on each for the appropriate reviewer.
Flags: needinfo?(jmathies)
Comment on attachment 815220 [details] [diff] [review]
gUM_part_1

Alfredo can you please post a rollup of this work merged to tip so I can test it in metro?
Attachment #815220 - Flags: review?(jmathies)
Comment on attachment 815222 [details] [diff] [review]
gUM_part_3

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

looks fine.  one more patch to clean up the things here.

::: content/base/src/nsDocument.cpp
@@ +10603,5 @@
> +{
> +  nsCOMPtr<nsIMutableArray> types = do_CreateInstance(NS_ARRAY_CONTRACTID);
> +  nsCOMPtr<PointerLockPermissionType> permType =
> +    new PointerLockPermissionType(NS_LITERAL_CSTRING("pointerLock"),
> +                                  NS_LITERAL_CSTRING("unused"));

Probably best not to create a seperate specific Pointer Lock permission type here. Instead, just move this to nsContentPermissionHelper and keep it generic.

@@ +11373,5 @@
> +
> +NS_IMPL_ISUPPORTS1(PointerLockPermissionType, nsIContentPermissionType)
> +
> +PointerLockPermissionType::PointerLockPermissionType(const nsACString& aType,
> +                                             const nsACString& aAccess)

white space alignment is off here.

::: dom/base/nsContentPermissionHelper.cpp
@@ +25,5 @@
>    MOZ_COUNT_DTOR(nsContentPermissionRequestProxy);
>  }
>  
>  nsresult
> +nsContentPermissionRequestProxy::Init(const nsTArray<mozilla::dom::PermissionRequest>& requests,

Do you need to use mozilla::dom:: here?  I think you're in the dom namespace.

@@ +60,1 @@
>    return NS_OK;

Are you sure you want to return a NS_OK and a null here?  I think the IDL said something about you never returning an empty array.

@@ +200,5 @@
> +void
> +ConvertPermissionRequestToArray(nsTArray<PermissionRequest>& aSrcArray,
> +                                nsCOMPtr<nsIMutableArray>& aDesArray)
> +{
> +  for (uint32_t i = 0; i < aSrcArray.Length(); i++) {

Please move the .Length() outside of for() statement.

::: dom/base/nsContentPermissionHelper.h
@@ +77,4 @@
>   private:
>    // Non-owning pointer to the ContentPermissionRequestParent object which owns this proxy.
>    mozilla::dom::ContentPermissionRequestParent* mParent;
> +  nsTArray<mozilla::dom::PermissionRequest> mRequests;

'requests' is kind of a weird name for this member.  mPermissionRequests might be better.

::: dom/devicestorage/nsDeviceStorage.cpp
@@ +1702,5 @@
> +  nsCOMPtr<nsIMutableArray> types = do_CreateInstance(NS_ARRAY_CONTRACTID);
> +  nsCOMPtr<ContentPermissionType> permType =
> +    new ContentPermissionType(type, NS_LITERAL_CSTRING("read"));
> +  types->AppendElement(permType, false);
> +  NS_IF_ADDREF(*aTypes = types);

This is a pretty common pattern.  Have you thought about promoting it into nsCOntentPermissionHelper?

Something like:

nsresult CreatePermissionArray(nsACString, nsACString, nsIArray**)

::: dom/interfaces/base/nsIContentPermissionPrompt.idl
@@ +37,4 @@
>    /**
> +   *  The array will include the request types. Elements of this array are
> +   *  nsIContentPermissionType object. This array will conain at least one
> +   *  element.

I don't think I would say that this array will contain at least one element (not the spelling mistake in |contain|).
Posted patch gUM_merged_patch (obsolete) — Splinter Review
merged to tip 150690:608de8acc55f.
(In reply to Jim Mathies [:jimm] from comment #125)
> Comment on attachment 815220 [details] [diff] [review]
> gUM_part_1
> 
> Alfredo can you please post a rollup of this work merged to tip so I can
> test it in metro?

Sure, please try https://bugzilla.mozilla.org/attachment.cgi?id=817092.
Thanks.
Posted patch gUM_part_3 (obsolete) — Splinter Review
Updated according to review comment 126.
Attachment #815222 - Attachment is obsolete: true
Attachment #815222 - Flags: review?(doug.turner)
Attachment #817093 - Flags: review?(doug.turner)
(In reply to Doug Turner (:dougt) from comment #126)
> Comment on attachment 815222 [details] [diff] [review]
> gUM_part_3
> 
> Review of attachment 815222 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> looks fine.  one more patch to clean up the things here.

Thanks fore review, all updated.
Attachment #815220 - Flags: review?(mrbkap) → review+
Comment on attachment 817093 [details] [diff] [review]
gUM_part_3

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

Reassigning to khuey in the hopes of a faster r+
Attachment #817093 - Flags: review?(doug.turner) → review?(khuey)
Comment on attachment 815220 [details] [diff] [review]
gUM_part_1

This doesn't build on win32 successfully, your changes to nsDocument apparently pulled in the windows.h header creating a couple conflicts with native apis. I was able to fix this by adding:

#ifdef XP_WIN
#ifdef CreateEvent
#undef CreateEvent
#endif
#ifdef LoadImage
#undef LoadImage
#endif
#endif

to the top of nsDocument.cpp. With that it builds. I'm not sure if the inclusion of windows.h in nsDocument is an issue but since there are content folks cc'd in here I'll leave it to them to complain if they feel it is.

Tested with a local build on metro and found that the geolocation prompting is working as expected with these patches applied.
Attachment #815220 - Flags: review?(jmathies) → review+
try build showing the build errors you'll need to fix to land -

https://tbpl.mozilla.org/?tree=Try&showall=0&rev=a78aaa253d59
(In reply to Jim Mathies [:jimm] from comment #133)
> try build showing the build errors you'll need to fix to land -
> 
> https://tbpl.mozilla.org/?tree=Try&showall=0&rev=a78aaa253d59

Yes, the windows build failure can be fixed with following modifications in nsDocuemtn.cpp.

   70 #ifdef CreateEvent
   71 #undef CreateEvent
   72 #endif
   73 
   74 #ifdef LoadImage
   75 #undef LoadImage
   76 #endif
   77 
   78 #ifdef ALTERNATE
   79 #undef ALTERNATE
   80 #endif


https://tbpl.mozilla.org/?tree=Try&rev=e1d6d4475488
Comment on attachment 817093 [details] [diff] [review]
gUM_part_3

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

This looks good, but I would like to see it again before giving an r+

Please fix the commit message.  It appears to be two separate messages, and one is not for this patch.  Also update the reviewer ;-)

::: content/base/src/nsDocument.cpp
@@ +10643,5 @@
> +{
> +  nsresult rv = CreatePermissionArray(NS_LITERAL_CSTRING("pointerLock"),
> +                                      NS_LITERAL_CSTRING("unused"),
> +                                      aTypes);
> +  return rv;

just "return CreatePermissionArray(...".

No need for the intermediate variable.

::: content/base/src/nsDocument.h
@@ +67,5 @@
>  #include "nsDataHashtable.h"
>  #include "mozilla/TimeStamp.h"
>  #include "mozilla/Attributes.h"
>  #include "nsIDOMXPathEvaluator.h"
> +#include "nsIContentPermissionPrompt.h"

Do you need to include this in the header?  You didn't change anything else in nsDocument.h ... can you get away with just including it in nsDocument.cpp?

::: dom/base/nsContentPermissionHelper.cpp
@@ +56,3 @@
>  {
> +  nsCOMPtr<nsIMutableArray> types = do_CreateInstance(NS_ARRAY_CONTRACTID);
> +  if(ConvertPermissionRequestToArray(mPermissionRequests, types)) {

nit: space between 'if' and '('

@@ +56,4 @@
>  {
> +  nsCOMPtr<nsIMutableArray> types = do_CreateInstance(NS_ARRAY_CONTRACTID);
> +  if(ConvertPermissionRequestToArray(mPermissionRequests, types)) {
> +    NS_IF_ADDREF(*aTypes = types);

I think you can just do types.forget(aTypes);

@@ +184,5 @@
> +}
> +
> +uint32_t
> +ConvertArrayToPermissionRequest(nsCOMPtr<nsIArray>& aSrcArray,
> +                                nsTArray<PermissionRequest>& aDesArray)

Nothing uses this function.  Get rid of it?

@@ +205,5 @@
> +                                nsCOMPtr<nsIMutableArray>& aDesArray)
> +{
> +  uint32_t len = aSrcArray.Length();
> +  for (uint32_t i = 0; i < len; i++) {
> +    nsCOMPtr<ContentPermissionType> cpt =

nsRefPtr<ContentPermissionType>

We use nsRefPtr for concrete classes and nsCOMPtr for interfaces.

@@ +212,5 @@
> +  }
> +  return len;
> +}
> +
> +nsresult CreatePermissionArray(const nsACString& aType,

nit:

nsresult\n
CreatePermissionArray(...

@@ +217,5 @@
> +                               const nsACString& aAccess,
> +                               nsIArray** aTypesArray)
> +{
> +  nsCOMPtr<nsIMutableArray> types = do_CreateInstance(NS_ARRAY_CONTRACTID);
> +  nsCOMPtr<ContentPermissionType> permType = new ContentPermissionType(aType,

nsRefPtr<ContentPermissionType>

::: dom/base/nsContentPermissionHelper.h
@@ +50,5 @@
> +  nsCString mType;
> +  nsCString mAccess;
> +};
> +
> +uint32_t ConvertArrayToPermissionRequest(nsCOMPtr<nsIArray>& aSrcArray,

This function appears to be unused.  Remove it?

@@ +54,5 @@
> +uint32_t ConvertArrayToPermissionRequest(nsCOMPtr<nsIArray>& aSrcArray,
> +                                     nsTArray<PermissionRequest>& aDesArray);
> +
> +uint32_t ConvertPermissionRequestToArray(nsTArray<PermissionRequest>& aSrcArray,
> +                                     nsCOMPtr<nsIMutableArray>& aDesArray);

Please line up the arguments, and make aDesArray an nsIMutableArray**.  We try not to pass nsCOMPtr&.

::: dom/devicestorage/nsDeviceStorage.cpp
@@ +1700,5 @@
> +    DeviceStorageTypeChecker::GetPermissionForType(mFile->mStorageType, type);
> +  NS_ENSURE_SUCCESS(rv, rv);
> +
> +  rv = CreatePermissionArray(type, NS_LITERAL_CSTRING("read"), aTypes);
> +  return rv;

Same thing here about just returning CreatePermissionArray

@@ +2209,5 @@
>        return rv;
>      }
> +
> +    rv = CreatePermissionArray(type, access, aTypes);
> +    return rv;

And here.

::: dom/interfaces/base/nsIContentPermissionPrompt.idl
@@ +14,2 @@
>   */
> +[scriptable, uuid(384b6cc4-a66b-4bea-98e0-eb10562a9ba4)]

please mark this as builtinclass too.

::: dom/ipc/PBrowser.ipdl
@@ +210,5 @@
>       * Initiates an asynchronous request for permission for the
>       * provided principal.
>       *
> +     * @param aRequests
> +     *   The array of permission to requests.

super-nit: 'permissions' and 'request'

::: dom/src/notification/DesktopNotification.cpp
@@ +359,5 @@
>  {
> +  nsresult rv = CreatePermissionArray(NS_LITERAL_CSTRING("desktop-notification"),
> +                                      NS_LITERAL_CSTRING("unused"),
> +                                      aTypes);
> +  return rv;

and here

::: dom/src/notification/Notification.cpp
@@ +233,5 @@
>  {
> +  nsresult rv = CreatePermissionArray(NS_LITERAL_CSTRING("desktop-notification"),
> +                                      NS_LITERAL_CSTRING("unused"),
> +                                      aTypes);
> +  return rv;

and here
Attachment #817093 - Flags: review?(khuey) → review-
(In reply to Kyle Huey [:khuey] (khuey@mozilla.com) from comment #135)
> Comment on attachment 817093 [details] [diff] [review]
> gUM_part_3
> 
> Review of attachment 817093 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> This looks good, but I would like to see it again before giving an r+
> 
> Please fix the commit message.  It appears to be two separate messages, and
> one is not for this patch.  Also update the reviewer ;-)
> 
> @@ +184,5 @@
> > +}
> > +
> > +uint32_t
> > +ConvertArrayToPermissionRequest(nsCOMPtr<nsIArray>& aSrcArray,
> > +                                nsTArray<PermissionRequest>& aDesArray)
> 
> Nothing uses this function.  Get rid of it?

Thanks for review.
This function actually used in gUM_part_1, or I move this function to gUM_part_1?
(In reply to Alfredo Yang from comment #136)
> Thanks for review.
> This function actually used in gUM_part_1, or I move this function to
> gUM_part_1?

Yes, please move it to part 1 then.

The same thing I said about ConvertPermissionRequestToArray applies here, that function should take an nsIArray* instead of an nsCOMPtr&.
(In reply to Jim Mathies [:jimm] from comment #132)
> Comment on attachment 815220 [details] [diff] [review]
> gUM_part_1
> 
> This doesn't build on win32 successfully, your changes to nsDocument
> apparently pulled in the windows.h header creating a couple conflicts with
> native apis. I was able to fix this by adding:
> 
> #ifdef XP_WIN
> #ifdef CreateEvent
> #undef CreateEvent
> #endif
> #ifdef LoadImage
> #undef LoadImage
> #endif
> #endif
> 
> to the top of nsDocument.cpp. With that it builds. I'm not sure if the
> inclusion of windows.h in nsDocument is an issue but since there are content
> folks cc'd in here I'll leave it to them to complain if they feel it is.
> 
> Tested with a local build on metro and found that the geolocation prompting
> is working as expected with these patches applied.

Thanks Kyle's help, windows.h is included by followings
<khuey> so nsContentPermissionHelper.h
<khuey> includes PermissionMessageUtils.h
<khuey> which includes IPCMessageUtils.h
<khuey> which includes a bunch of chromium ipc crap
<khuey> which includes windows.h
A forward declaration IPC::Principal can fix this problem.
Posted patch gUM_part_3 (obsolete) — Splinter Review
Attachment #817093 - Attachment is obsolete: true
Attachment #819640 - Flags: review?(khuey)
Comment on attachment 819640 [details] [diff] [review]
gUM_part_3

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

r=me with those changes.

::: dom/base/nsContentPermissionHelper.h
@@ +12,5 @@
>  #include "mozilla/dom/PContentPermissionRequestParent.h"
>  
>  class nsContentPermissionRequestProxy;
>  
> +// Forward declares IPC::Principal here which is defined in

nit: declare (no 's')

@@ +13,5 @@
>  
>  class nsContentPermissionRequestProxy;
>  
> +// Forward declares IPC::Principal here which is defined in
> +// PermissionMessageUtils.h. Including this file will transitively includes

just 'include'

@@ +19,5 @@
> +//   #define CreateEvent CreateEventW
> +//   #define LoadImage LoadImageW
> +// That will mess up windows build.
> +namespace IPC {
> +  class Principal;

nit: no indentation.

@@ +45,5 @@
>    virtual bool Recvprompt();
>    virtual void ActorDestroy(ActorDestroyReason why);
>  };
>  
> +class ContentPermissionType : public nsIContentPermissionType

This class is only used in nsContentPermissionHelper.cpp (everyone else just sees nsIContentPermissionType), so move this declaration into the .cpp file.
Attachment #819640 - Flags: review?(khuey) → review+
(In reply to Kyle Huey [:khuey] (khuey@mozilla.com) from comment #140)
> >  
> > +class ContentPermissionType : public nsIContentPermissionType
> 
> This class is only used in nsContentPermissionHelper.cpp (everyone else just
> sees nsIContentPermissionType), so move this declaration into the .cpp file.

Thanks for review.
MediaPermissionGonk.cpp in gUM_part_2 uses this class, could we keep it in header? Others are fixed.
Ah, ok, yeah.  It has to stay in the header then.

It's confusing to break patches up in ways where things added in part X are already used in part X - 1.  Please try not to do that in the future.
(In reply to Kyle Huey [:khuey] (khuey@mozilla.com) from comment #142)
> Ah, ok, yeah.  It has to stay in the header then.
> 
> It's confusing to break patches up in ways where things added in part X are
> already used in part X - 1.  Please try not to do that in the future.

Ok.
Btw, for the windows.h header problem, I found all the auto-generated headers need to be removed. TabParent.cpp and nsContentPermissionHelper.h, and nsContentPermissionHelper.cpp are updated for this change.
Posted patch gUM_part_3 (obsolete) — Splinter Review
Attachment #819640 - Attachment is obsolete: true
Comment on attachment 820252 [details] [diff] [review]
gUM_part_3

Carry r+ from comment 140.
Attachment #820252 - Flags: review+
Posted patch gUM_part_2 (obsolete) — Splinter Review
Carry r+ from comment 122~123.
Attachment #815220 - Attachment is obsolete: true
Attachment #815221 - Attachment is obsolete: true
Attachment #815224 - Attachment is obsolete: true
Attachment #817092 - Attachment is obsolete: true
Attachment #820820 - Flags: review+
Posted patch gUM_part_1 (obsolete) — Splinter Review
Carry r+ from comment 116, 121, 122, 130~131, 132.
Attachment #820822 - Flags: review?
Attachment #820822 - Flags: review? → review+
Posted patch gUM_Test (obsolete) — Splinter Review
Carry r+ from comment 114
Attachment #820823 - Flags: review+
Attachment #820820 - Attachment is patch: true
Keywords: checkin-needed
(In reply to Alfredo Yang from comment #123)
> (In reply to Wesley Johnston (:wesj) from comment #122)
> > Comment on attachment 815220 [details] [diff] [review]
> > gUM_part_1
> > 
> > Review of attachment 815220 [details] [diff] [review]:
> > -----------------------------------------------------------------
> > 
> > ::: mobile/android/components/ContentPermissionPrompt.js
> > @@ +63,5 @@
> > >      let isApp = request.principal.appId !== Ci.nsIScriptSecurityManager.NO_APP_ID && request.principal.appId !== Ci.nsIScriptSecurityManager.UNKNOWN_APP_ID;
> > >  
> > > +    // only allow exactly one permission rquest here
> > > +    let types = request.types.QueryInterface(Ci.nsIArray);
> > > +    if (types.length != 1) {
> > 
> > We support multiple prompts at once on Android. Is there a good reason to
> > bail here? I'd be fine with this, but you should file something in our
> > component to update this.
> 
> Thanks for feedback.
> 
> The original concern is security. From my understanding, the only case
> allows multiple request is audio+video in getUserMedia. But thing looks
> different on Android, I will filed a bug for it.

You really can't land this if it breaks Android, with only a followup bug. I didn't see an r+ for the Android part either.
Hmm... it'd be probably a misunderstanding, these patches should not break Fennec.
ContentPermissionRequest actually supports "one" permission type [1]. So when Wesley said "support multiple prompts at once on Android", it should be Android UI collects several ContentPermissionRequest objects and combines the requests into one prompt. (I'm not familiar with Fennec, please correct me if I'm wrong.)
In this case, my work should not break this part. Instead, it adds ability to ContentPermissionRequest supporting multiple permission types [2].
So IMHO is to update the ContentPermissionRequest.idl on Gecko and keeps other platforms except B2G with the same behavior before my patches. Leave these parts for people who are familiar to udpate.
Does that make sense to you?

[1] http://dxr.mozilla.org/mozilla-central/source/dom/interfaces/base/nsIContentPermissionPrompt.idl#l23
[2] https://bugzilla.mozilla.org/attachment.cgi?id=820252&action=diff#a/dom/interfaces/base/nsIContentPermissionPrompt.idl_sec2
Keywords: checkin-needed
(In reply to Alfredo Yang from comment #151)

> So IMHO is to update the ContentPermissionRequest.idl on Gecko and keeps
> other platforms except B2G with the same behavior before my patches. Leave
> these parts for people who are familiar to udpate.
> Does that make sense to you?

Yes. This does make sense. I think a few test for gUM landed recently (after your Try push) for Fennec, so we should be able to tell if the changes break the way the permission UI is displayed.
(In reply to Mark Finkle (:mfinkle) from comment #152)
> (In reply to Alfredo Yang from comment #151)
> 
> Yes. This does make sense. I think a few test for gUM landed recently (after
> your Try push) for Fennec, so we should be able to tell if the changes break
> the way the permission UI is displayed.

Do you mean this changeset?

changeset:   151645:ececa191d8b5
user:        Gian-Carlo Pascutto <gpascutto@mozilla.com>
date:        Tue Oct 22 11:10:17 2013 +0200
summary:     Bug 928870 - Add test for Android gUM doorhanger. r=gbrown

I'll rebase my patches and have another try run to make sure they pass these tests.
Posted patch gUM_part_1 (obsolete) — Splinter Review
Rebase, carry r+ from comment 116, 121, 122, 130~131, 132.
Attachment #820252 - Attachment is obsolete: true
Attachment #820820 - Attachment is obsolete: true
Attachment #820822 - Attachment is obsolete: true
Attachment #820823 - Attachment is obsolete: true
Attachment #822769 - Flags: review+
Posted patch gUM_part_2 (obsolete) — Splinter Review
Rebase, carry r+ from comment 122~123.
Attachment #822770 - Flags: review+
Posted patch gUM_part_3 (obsolete) — Splinter Review
Rebase, carry r+ from comment 140.
Attachment #822771 - Flags: review+
Posted patch gUM_Test (obsolete) — Splinter Review
Rebase, carry r+ from comment 114
Attachment #822772 - Flags: review+
Keywords: checkin-needed
may have caused regression bug 933075.
yes, this caused the regression.  Either fix it, or back out asap.
Hi Alfredo,

Please check why your patch blocks the geolocation permission prompt. Does this one affect the audio permission prompt which is one of the main feature in v1.2?
Flags: needinfo?(ayang)
Thanks for the clarification from Jason and Shih-Chiang that this one won't affect audio for v1.2. But I think the conflict should be fixed for v1.3?
blocking-b2g: --- → 1.3?
Sorry for late reply, not in office yesterday.

The problem should be from the event.detail changing to support multiple types. My original patch is to keep the old one to avoid the old Gaia compatible problem, but reviewer doesn't like it.

From comment 109.
>
>@@ +343,5 @@
>>  
>>      let details = {
>>        type: type,
>> +      permission: permissions[0], // XXX for compatiable, it can be removed
>> +                                  // after Gaia udpated.
>
>I would rather not do that, to be sure we don't keep outdated code in gaia.

I can add it back but it needs to be granted by Fabrice.
What prevents us from changing gaia as well on the branches this gecko patch is applied?
After checking, the Gaia patch is almost ready, please check https://bugzilla.mozilla.org/show_bug.cgi?id=919927.
Both patches need to be checkin together to avoid break.
Flags: needinfo?(ayang)
Blocks: 919927
Posted patch gUM_part_1 (obsolete) — Splinter Review
Rebase to tip.
Attachment #822769 - Attachment is obsolete: true
Attachment #822770 - Attachment is obsolete: true
Attachment #822771 - Attachment is obsolete: true
Attachment #822772 - Attachment is obsolete: true
Attachment #827240 - Flags: review+
Posted patch gUM_part_2 (obsolete) — Splinter Review
rebase to tip
Attachment #827241 - Flags: review+
Posted patch gUM_part_3 (obsolete) — Splinter Review
rebase to tip
Attachment #827242 - Flags: review+
Posted patch gUM_Test (obsolete) — Splinter Review
Attachment #827243 - Flags: review+
Blocks: 935291
No longer blocks: 935316
Backed out for breaking permission prompts for "prompt for unknown" permissions (eg. notifications, geolocation coming from a browser tab. See bug 935291) and also breaking the prompt UI in B2G:

https://hg.mozilla.org/integration/b2g-inbound/rev/9ce66d058939
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Flags: in-testsuite+
Target Milestone: mozilla27 → ---
Can we get someone to test this thoroughly before we land it a fourth time?
(In reply to Reuben Morais [:reuben] from comment #177)
> Backed out for breaking permission prompts for "prompt for unknown"
> permissions (eg. notifications, geolocation coming from a browser tab. See
> bug 935291) and also breaking the prompt UI in B2G:
> 
> https://hg.mozilla.org/integration/b2g-inbound/rev/9ce66d058939

Merged to m-c:
https://hg.mozilla.org/mozilla-central/rev/9ce66d058939
Depends on: 935427
Blocks: 935557
Plus this one for v1.3 to complete gUM video part.
blocking-b2g: 1.3? → 1.3+
Target Milestone: --- → mozilla28
https://tbpl.mozilla.org/php/getParsedLog.php?id=31036036&tree=Try

Crash at mozilla::nsDOMCameraControl::nsDOMCameraControl] when running tryserver. Push another try with debug symbol again.
Per discussion offline - gUM camera support is a targeted feature, not committed, which means we don't need to block on this. We should try to get this in for 1.3 still, however.
blocking-b2g: 1.3+ → -
Blocks: 923361
Depends on: 945614
Depends on: 926746
Hi fabrice, the testcase dependes on other bugs. I plan to disable the testcase and land it.
What do you think?
Flags: needinfo?(fabrice)
(In reply to Alfredo Yang from comment #184)
> Hi fabrice, the testcase dependes on other bugs. I plan to disable the
> testcase and land it.
> What do you think?

I don't think that's a good idea. New feature landings need to come with tests. This feature also has regression risk per the above backouts, so having more test coverage is critical.

I think what we instead need to discuss here is if this should wait until 1.4 or not. I think this might be too risky to land for 1.3 at this point.
Depends on: 944615
Blocks: 898949
No longer depends on: 898949
Hm, I agree with Jason there. Disabling tests is the last thing you should ask a sheriff to approve ;)
So first land the dependencies, and we'll see where we are at this point.
Flags: needinfo?(fabrice)
https://hg.mozilla.org/integration/b2g-inbound/rev/3fd07d5561b7
https://hg.mozilla.org/integration/b2g-inbound/rev/5a019e26c6b2
https://hg.mozilla.org/integration/b2g-inbound/rev/a72c03a07567

I'll note that I didn't land the test; sorry - I was discussing it in #b2g with mikeh, and we agreed to land without tests until the emulator patch (bug 944615) gets pulled, then I saw the comments here.  After discussing with fabrice, we'll leave it this way.
Whiteboard: [WebRTC][blocking-webrtc-][b2g-gum+][FT: Media Recording, Sprint 2] → [WebRTC][blocking-webrtc-][b2g-gum+][FT: Media Recording, Sprint 2][leave-open]
Leave-open is until bug 944615 is pulled and installed on the test machines, then we can land the test patch here and close the bug.
Blocks: 945111
No longer depends on: 945111
Posted patch gUM_Test (obsolete) — Splinter Review
Hi sheriff, bug 944615 pulled in, please add test case, gUM_Test.
Thanks.

https://tbpl.mozilla.org/?tree=Try&rev=3a958a7e29cc
Attachment #827243 - Attachment is obsolete: true
Attachment #8344571 - Flags: review+
Keywords: checkin-needed
https://hg.mozilla.org/integration/b2g-inbound/rev/144cec97320a
Keywords: checkin-needed
Whiteboard: [WebRTC][blocking-webrtc-][b2g-gum+][FT: Media Recording, Sprint 2][leave-open] → [WebRTC][blocking-webrtc-][b2g-gum+][FT: Media Recording, Sprint 2]
https://hg.mozilla.org/mozilla-central/rev/144cec97320a
Status: REOPENED → RESOLVED
Closed: 6 years ago6 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Keywords: verifyme
Depends on: 948185
I think this broke the permission prompt for contacts again :(

STR: Download Con Backup app from Marketplace and try to backup your contacts.
(In reply to Gregor Wagner [:gwagner] from comment #193)
> I think this broke the permission prompt for contacts again :(
> 
> STR: Download Con Backup app from Marketplace and try to backup your
> contacts.

I'm going to look into this right now on device.
Flags: needinfo?(jsmith)
Yup - Gregor is right. The contacts API permission prompt is busted. Device storage API permission prompt & geolocation permission prompt are fine though.
Flags: needinfo?(jsmith)
Spoke w/Gregor & Fabrice in IRC - we're backing this out & punting this feature to 1.4.
(In reply to Ryan VanderMeulen [:RyanVM UTC-5] from comment #192)
> https://hg.mozilla.org/mozilla-central/rev/144cec97320a

I backed this commit out in https://hg.mozilla.org/mozilla-central/rev/75c0c92d7fa4 on the request of jsmith before I saw that there were three other commits in this bug that needed to be backed out on aurora and m-c. The backouts of those other three commits hit a few conflicts that I don't feel comfortable resolving myself, so that'll have to be done by someone else.
Lets also add an integration test for this prompt before landing again.
Alfredo - Can you provide a backout patch for the commits in comment 189 on m-c & aurora? The tree is orange right now & the contacts API is broken in privileged apps. Gregor, Fabrice, and I need this backed out.
Flags: needinfo?(ayang)
I don't see why we would back this out of nightly (and aurora) (since as mentioned it didn't just land and the backout becomes involved), instead of filing a bug and fixing it.  If we've decided to turn it off for 1.3, that has no impact on nightly or aurora.  (and we added a pref to allow such a turnoff)
We don't want to loose time with a new patch+review+testing here. Especially since that doesn't seem to work well so far. Please backout from everywhere this landed.
(In reply to Randell Jesup [:jesup] from comment #200)
> I don't see why we would back this out of nightly (and aurora) (since as
> mentioned it didn't just land and the backout becomes involved), instead of
> filing a bug and fixing it.  If we've decided to turn it off for 1.3, that
> has no impact on nightly or aurora.  (and we added a pref to allow such a
> turnoff)

Also, just turning this feature off doesn't fix the other permission prompts it broke.  It's breaking prompts for unrelated features, and that's another reason we can't leave this in.
The PermissionPromptRequest object for contacts is generated in JS, which is pretty hard to find out in first place during the development.
http://mxr.mozilla.org/mozilla-central/source/dom/permission/PermissionPromptHelper.jsm#76
(In reply to Jason Smith [:jsmith] from comment #199)
> Alfredo - Can you provide a backout patch for the commits in comment 189 on
> m-c & aurora? The tree is orange right now & the contacts API is broken in
> privileged apps. Gregor, Fabrice, and I need this backed out.

That looks like another prompt without testcase as bug 935291 again.
I'll add testcase for 935291 and contact prompt. If anyone knows any permission prompt without testcase, please let me know, I'll add it. I hope there will be enough testcases to land this bug and people who work about this part can get early wanring on try.

I'll provide backout patch today.
Flags: needinfo?(ayang)
QA Contact: jsmith
Alfredo, take a look at https://mxr.mozilla.org/mozilla-central/source/dom/apps/src/PermissionsTable.jsm to see all the APIs that can trigger a prompt.
To backout, apply these backouts in this order:
backout from bug 945614
backout p3
backout p2
backout p1
Whiteboard: [WebRTC][blocking-webrtc-][b2g-gum+][FT: Media Recording, Sprint 2] → [WebRTC][blocking-webrtc-][b2g-gum+][ft:media-recording, Sprint 2]
Whiteboard: [WebRTC][blocking-webrtc-][b2g-gum+][ft:media-recording, Sprint 2] → [WebRTC][blocking-webrtc-][b2g-gum+][ft:multimedia-platform, Sprint 2]
(In reply to Randell Jesup [:jesup] from comment #209)
> To backout, apply these backouts in this order:
> backout from bug 945614
> backout p3
> backout p2
> backout p1

(In reply to Randell Jesup [:jesup] from comment #209)
> To backout, apply these backouts in this order:
> backout from bug 945614
> backout p3
> backout p2
> backout p1

pushed to central as https://tbpl.mozilla.org/?rev=df82be9d89a5
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
blocking-b2g: - → ---
Whiteboard: [WebRTC][blocking-webrtc-][b2g-gum+][ft:multimedia-platform, Sprint 2] → [WebRTC][blocking-webrtc-][b2g-gum+][ft:multimedia-platform]
Posted patch auroa_backout (obsolete) — Splinter Review
backout patch for 1.3 branch aurora.
Keywords: verifyme
I have a quick chat with @atasi, he can help us (or at least guide us) to provide Gaia UI test case on contact permission prompt.
(In reply to Shih-Chiang Chien [:schien] from comment #213)
> I have a quick chat with @atasi, he can help us (or at least guide us) to
> provide Gaia UI test case on contact permission prompt.

This could be done in a Marionette JS test case or a Gaia UI Test case. You could implement this by doing:

1. Add a privileged app to the test apps directory with each PROMPT_ACTION permission included
2. Launch the app
3. Execute JS in the context of the app for a privileged PROMPT_ACTION permission that will trigger the prompt
4. Grant permissions
5. Verify the JS action was successful
6. Repeat 3 - 5 for the other privileged PROMPT_ACTION permissions

I'll file a bug for tracking development of this automated test case.
Depends on: 948826
Update target milestone
Target Milestone: --- → 1.3 C2/1.4 S2(17jan)
Depends on: 951997
Blocks: 952244
No longer blocks: 952244
Attachment #827240 - Attachment is obsolete: true
Attachment #827241 - Attachment is obsolete: true
Attachment #827242 - Attachment is obsolete: true
Attachment #8344571 - Attachment is obsolete: true
Attachment #8345125 - Attachment is obsolete: true
Attachment #8345127 - Attachment is obsolete: true
Attachment #8345128 - Attachment is obsolete: true
Attachment #8345195 - Attachment is obsolete: true
Attachment #8361117 - Flags: review?(fabrice)
Attachment #8361118 - Flags: review?(amarchesini)
Posted patch gUM_video_prompt (obsolete) — Splinter Review
Carry r+
Attachment #8361120 - Flags: review+
Attachment #8361118 - Flags: review?(amarchesini) → review+
Blocks: 961945
Hi Fabrice,

Would you please help on the patch review? Thank you very much!
Flags: needinfo?(fabrice)
No need to micro-manage my review queue Ivan. I know it's there...
Flags: needinfo?(fabrice)
Comment on attachment 8361117 [details] [diff] [review]
gUM_other_permissions

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

::: dom/permission/PermissionPromptService.js
@@ +72,5 @@
> +    let types = aRequest.types.QueryInterface(Ci.nsIArray);
> +    if (types.length != 1) {
> +      aRequest.cancel();
> +      return;
> +    }

nit: add blank line.
Attachment #8361117 - Flags: review?(fabrice) → review+
https://hg.mozilla.org/mozilla-central/rev/b1e246595663
Status: REOPENED → RESOLVED
Closed: 6 years ago6 years ago
Resolution: --- → FIXED
Target Milestone: 1.3 C2/1.4 S2(17jan) → 1.3 C3/1.4 S3(31jan)
Keywords: verifyme
QA Contact: jsmith
FYI - we now have a failed gaia tree due to contact test failures. These tests are not yet running on TBPL, but my hunch is that this bug is causing our failures. A bug is filed here: 966729.

I will try to see if I can run these tests locally to verify.
Depends on: 966729
(In reply to Kevin Grandon :kgrandon from comment #224)
> FYI - we now have a failed gaia tree due to contact test failures. These
> tests are not yet running on TBPL, but my hunch is that this bug is causing
> our failures. A bug is filed here: 966729.
> 
> I will try to see if I can run these tests locally to verify.

It looks we've manually confirmed that this patch causes the permission prompt to fail to come up for the contacts API - see https://bugzilla.mozilla.org/show_bug.cgi?id=966729#c11. I've pinged the sheriffs alias offline to ask for a backout of this patch on m-c.
(In reply to Jason Smith [:jsmith] from comment #225)
> (In reply to Kevin Grandon :kgrandon from comment #224)
> > FYI - we now have a failed gaia tree due to contact test failures. These
> > tests are not yet running on TBPL, but my hunch is that this bug is causing
> > our failures. A bug is filed here: 966729.
> > 
> > I will try to see if I can run these tests locally to verify.
> 
> It looks we've manually confirmed that this patch causes the permission
> prompt to fail to come up for the contacts API - see
> https://bugzilla.mozilla.org/show_bug.cgi?id=966729#c11. I've pinged the
> sheriffs alias offline to ask for a backout of this patch on m-c.

So this broke the permission prompt for contacts the 3rd time.
Backout https://hg.mozilla.org/integration/b2g-inbound/rev/cb4960e3af99
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Depends on: 966882
(In reply to Gregor Wagner [:gwagner] from comment #227)
> Backout https://hg.mozilla.org/integration/b2g-inbound/rev/cb4960e3af99

also backout landed on central and so now also on m-i and fx-team
Blocks: 931654
Blocks: 923038
No longer blocks: 923038
Depends on: 968144
Posted patch gUM_video_prompt (obsolete) — Splinter Review
rebase, carry r+.
Attachment #8361120 - Attachment is obsolete: true
Attachment #8371216 - Flags: review+
(In reply to Alfredo Yang from comment #229)
> Created attachment 8371216 [details] [diff] [review]
> gUM_video_prompt
> 
> rebase, carry r+.

Please ask for review again if you fixed the issues.
The backout is due to https://bugzilla.mozilla.org/show_bug.cgi?id=853356#c222 only submits 1 patch (gUM_other_permissions) but lack of other 2 patches (by accident?) during the CNY.

For contact permission, I've tried the testcase from bug 948826 2 weeks ago and confirmed it works.
(In reply to Alfredo Yang from comment #231)
> The backout is due to
> https://bugzilla.mozilla.org/show_bug.cgi?id=853356#c222 only submits 1
> patch (gUM_other_permissions) but lack of other 2 patches (by accident?)
> during the CNY.
> 
> For contact permission, I've tried the testcase from bug 948826 2 weeks ago
> and confirmed it works.

I had a feeling that was the problem here - I noticed only one patch land on inbound when this landed.

Can we try to reland this with all three patches here?
Keywords: verifymecheckin-needed
I'd suggest later.
The testcase in this bug is the first testcase which uses emulator's audio/camera driver and there are many bugs in it (bug 944615, 926746, 945614). The emulator's camera driver and audio hal could be deadlocked easy(about 10%) on try if invoking gUM calls in very short time.
I fixed the video one (bug 968144). Right now I'm checking audio hal.
(In reply to Alfredo Yang from comment #233)
> I'd suggest later.
> The testcase in this bug is the first testcase which uses emulator's
> audio/camera driver and there are many bugs in it (bug 944615, 926746,
> 945614). The emulator's camera driver and audio hal could be deadlocked
> easy(about 10%) on try if invoking gUM calls in very short time.
> I fixed the video one (bug 968144). Right now I'm checking audio hal.

I don't exactly follow this statement - are you stating that we should hold off landing this right now?
Keywords: checkin-needed
Landed it won't impact to device.
But there will be possibility that causes orange on try due to emulator HAL problem.
https://tbpl.mozilla.org/?tree=Try&rev=d26e9ed099fc
In a word:
We should land bug 968144 first, and then bug 853356.
Without the fix in bug 968144, the test case in bug 853356(gUM_sandbox_testcase) might fail.
Alfredo: this try does not look happy.  https://tbpl.mozilla.org/?tree=Try&rev=97d0b85f4c75

I retriggered several times for good measure.  Also note the Gu failure, not sure if that's related
Flags: needinfo?(ayang)
(In reply to Randell Jesup [:jesup] from comment #237)
> Alfredo: this try does not look happy. 
> https://tbpl.mozilla.org/?tree=Try&rev=97d0b85f4c75
> 
> I retriggered several times for good measure.  Also note the Gu failure, not
> sure if that's related

The mochitest failure is from emulator HAL (audio HAL busy loop) problem.
Flags: needinfo?(ayang)
(In reply to Alfredo Yang from comment #238)
> (In reply to Randell Jesup [:jesup] from comment #237)
> > Alfredo: this try does not look happy. 
> > https://tbpl.mozilla.org/?tree=Try&rev=97d0b85f4c75
> > 
> > I retriggered several times for good measure.  Also note the Gu failure, not
> > sure if that's related
> 
> The mochitest failure is from emulator HAL (audio HAL busy loop) problem.

So, is there a bug associated with that problem?  Is there an ETA for fixing it?
Flags: needinfo?(ayang)
(In reply to Randell Jesup [:jesup] from comment #239)
> > The mochitest failure is from emulator HAL (audio HAL busy loop) problem.
> 
> So, is there a bug associated with that problem?  Is there an ETA for fixing
> it?

I plan to write a new test case not to touching emulator HAL. If Jason agrees that, I'll complete it today.
Flags: needinfo?(ayang) → needinfo?(jsmith)
Attachment #8371216 - Flags: review+
That sounds fine to me.
Flags: needinfo?(jsmith)
Posted patch gUM_prompt_test (obsolete) — Splinter Review
Attachment #8372092 - Flags: review?(jsmith)
See Also: → 969272
Comment on attachment 8372092 [details] [diff] [review]
gUM_prompt_test

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

I'm probably not the best person to review a mochitest involving chrome interaction. I'll redirect this to Fabrice.

::: b2g/components/test/mochitest/test_getUserMedia_prompt.html
@@ +30,5 @@
> +    prompt: false
> +  }
> +  ,
> +  {
> +    type: "audio-video-capture",

Why are we using a permission here encompassing two permissions that already exist?
Attachment #8372092 - Flags: review?(jsmith) → review?(fabrice)
(In reply to Jason Smith [:jsmith] from comment #243)
> Comment on attachment 8372092 [details] [diff] [review]
> gUM_prompt_test
> 
> Review of attachment 8372092 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> I'm probably not the best person to review a mochitest involving chrome
> interaction. I'll redirect this to Fabrice.
> 
> ::: b2g/components/test/mochitest/test_getUserMedia_prompt.html
> @@ +30,5 @@
> > +    prompt: false
> > +  }
> > +  ,
> > +  {
> > +    type: "audio-video-capture",
> 
> Why are we using a permission here encompassing two permissions that already
> exist?

It is an event ID only to communicate between chrome and content. I can change its name if you prefer to use other name.
Posted patch gUM_prompt_test (obsolete) — Splinter Review
Attachment #8372092 - Attachment is obsolete: true
Attachment #8372092 - Flags: review?(fabrice)
Attachment #8372212 - Flags: review?(fabrice)
Remove the old testcase, carry r+.
The quick try test is ok https://tbpl.mozilla.org/?tree=Try&rev=452e88f0ee45.
Now wait for the full try result, https://tbpl.mozilla.org/?tree=Try&rev=7e038f50fc88.
Attachment #8371216 - Attachment is obsolete: true
Attachment #8372320 - Flags: review+
Comment on attachment 8372212 [details] [diff] [review]
gUM_prompt_test

Move testcase to bug 969977.
Attachment #8372212 - Attachment is obsolete: true
Attachment #8372212 - Flags: review?(fabrice)
Keywords: checkin-needed
ayang: I see it's checkin-needed now.  Do we know the cause (bug #) of the Gu failures on both of your Try pushes?  If so, please star the failures with the bug number.  If this is an unrelated problem, is there a way to avoid the problem and get a clean set of Try data?
Flags: needinfo?(ayang)
> ayang: I see it's checkin-needed now.  Do we know the cause (bug #) of the
> Gu failures on both of your Try pushes?  If so, please star the failures
> with the bug number.  If this is an unrelated problem, is there a way to
> avoid the problem and get a clean set of Try data?

Gu failure should be bug 948395.
There is another try with success Gu, https://tbpl.mozilla.org/?tree=Try&rev=452e88f0ee45.
Flags: needinfo?(ayang)
Yes; there's one success there.  Is Gu so hosed that 15 runs gets one that runs all the way to the end?

I needinfo'd ryan to see who can work on that, and why it got MUCH worse around 1/30 or 1/31
This does not work reliably for me.

1. Repeated attempts to get the camera don't seem to work.
2. Sometimes the prompt has a remember me toggle which we don't want
3. The video is sideways on the nexus 4.
(In reply to Eric Rescorla (:ekr) from comment #252)
> This does not work reliably for me.
> 1. Repeated attempts to get the camera don't seem to work.
How could we reproduce the problem? I tested it on gUM test page(http://mozilla.github.io/webrtc-landing/gum_test.html) and I cannot reproduce it. I was running on Browser app and the device is nexus 4.
> 2. Sometimes the prompt has a remember me toggle which we don't want
Do you mean the prompt doesn't show?
> 3. The video is sideways on the nexus 4.
This is because we don't tell camera hardware the correct angle. The output frame of camera is landscape by default. We may need a mechanism to tell camera about the rotation information.
Flags: needinfo?(ekr)
(In reply to StevenLee[:slee] from comment #254)
> > 2. Sometimes the prompt has a remember me toggle which we don't want
> Do you mean the prompt doesn't show?

Not that - Ekr is mentioning that gUM prompts aren't supposed to have a remember my choice option. We need a bug on file for this.

> > 3. The video is sideways on the nexus 4.
> This is because we don't tell camera hardware the correct angle. The output
> frame of camera is landscape by default. We may need a mechanism to tell
> camera about the rotation information.

Can we get a followup bug open for this? Note that this needs to be fixed for the MWC demo, as we can't demo webrtc on mwc with a landscape only setup.
Keywords: verifyme
(In reply to Jason Smith [:jsmith] from comment #255)
> (In reply to StevenLee[:slee] from comment #254)
> Not that - Ekr is mentioning that gUM prompts aren't supposed to have a
> remember my choice option. We need a bug on file for this.
Thanks, I misunderstood what he meant. But I tested on test landing page and cannot reproduce it. Which page should I tested on? Or do we have a testing page that is for MWC demo and we can do our test on it. It seems that some bugs cannot reproduce on testing landing page but I usually use it to test my device.
> 
> > > 3. The video is sideways on the nexus 4.
> > This is because we don't tell camera hardware the correct angle. The output
> > frame of camera is landscape by default. We may need a mechanism to tell
> > camera about the rotation information.
> 
> Can we get a followup bug open for this? Note that this needs to be fixed
> for the MWC demo, as we can't demo webrtc on mwc with a landscape only setup.
OK, I will file a bug for it.
(In reply to StevenLee[:slee] from comment #256)
> (In reply to Jason Smith [:jsmith] from comment #255)
> > (In reply to StevenLee[:slee] from comment #254)
> > Not that - Ekr is mentioning that gUM prompts aren't supposed to have a
> > remember my choice option. We need a bug on file for this.
> Thanks, I misunderstood what he meant. But I tested on test landing page and
> cannot reproduce it. Which page should I tested on? Or do we have a testing
> page that is for MWC demo and we can do our test on it. It seems that some
> bugs cannot reproduce on testing landing page but I usually use it to test
> my device.

It should technically reproduce on the landing page - you should see it immediately on the gUM video prompt. If it's not there, then maybe Ekr is running an old Gaia build?

I'll check this tomorrow to see if I can reproduce.
(In reply to StevenLee[:slee] from comment #256)

> > Can we get a followup bug open for this? Note that this needs to be fixed
> > for the MWC demo, as we can't demo webrtc on mwc with a landscape only setup.
> OK, I will file a bug for it.
Steven,
The bug you file should be a blocker of Bug 965140.
(In reply to Eric Rescorla (:ekr) from comment #252)
> This does not work reliably for me.
> 1. Repeated attempts to get the camera don't seem to work.
Can't reproduce. Please share your testing app or page
> 2. Sometimes the prompt has a remember me toggle which we don't want
Can't reproduce. Please share your testing app or page
> 3. The video is sideways on the nexus 4.
Bug 970183, Steven will work on this. We need to fix buffer rotation between camera and media manager, that bug is not relative to video prompt.
Whiteboard: [WebRTC][blocking-webrtc-][b2g-gum+][ft:multimedia-platform] → [WebRTC][blocking-webrtc-][b2g-gum+][ft:multimedia-platform], [apps watch list]
Flags: needinfo?(ekr)
Keywords: verifyme
QA Contact: jsmith
You need to log in before you can comment on or make changes to this bug.