853356 - [B2G getUserMedia] Display camera/ microphone permission acquisition prompt

Status: RESOLVED FIXED

(Core :: WebRTC, defect)

Description

[StevenLee[:slee]]

When apps try to get the microphone, we need to check if they have the permission to do so.

Comment 1

[Alfredo Yang (:alfredo)]

take it as first bug

Comment 2

[Alfredo Yang (:alfredo)]

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.

Comment 3

[Randy Lin [:rlin] - Be Mozillian on 2015/01/26]

On B2G platform, could we use Manifest permission checking for this?

Comment 4

[Alfredo Yang (:alfredo)]

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.

Comment 5

[Alfredo Yang (:alfredo)]

(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.

Comment 6

[Alfredo Yang (:alfredo)]

Attachment: draft for b2g design and the flow of current desktop design.

Comment 7

[Jason Smith [:jsmith]]

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?

Comment 8

[Paul Theriault [:pauljt] (no longer reading bugmail)]

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.

Comment 9

[Alfredo Yang (:alfredo)]

> * 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.

Comment 10

[Alfredo Yang (:alfredo)]

Attachment: Permission check for microphone acquisition

Comment 11

[Alfredo Yang (:alfredo)]

Attachment: 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.
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.

Comment 12

[Alfredo Yang (:alfredo)]

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.

Comment 13

[Jason Smith [:jsmith]]

(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.

Comment 14

[[:fabrice] Fabrice Desré]

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.

Comment 15

[Eric Rescorla (:ekr)]

(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.

Comment 16

[Paul Theriault [:pauljt] (no longer reading bugmail)]

> 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.

Comment 17

[Maire Reavy [:mreavy]]

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.

Comment 18

[Jason Smith [:jsmith]]

(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.

Comment 19

[Eric Rescorla (:ekr)]

(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.

Comment 20

[Jason Smith [:jsmith]]

(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.

Comment 21

[Eric Rescorla (:ekr)]

(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.

Comment 22

[[:fabrice] Fabrice Desré]

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.

Comment 23

[Eric Rescorla (:ekr)]

(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?

Comment 24

[[:fabrice] Fabrice Desré]

(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).

Comment 25

[Eric Rescorla (:ekr)]

In that case, any kind of intent string in the manifest seems entirely pointless.

Comment 26

[[:fabrice] Fabrice Desré]

(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.

Comment 27

[Eric Rescorla (:ekr)]

But both of those cases seem to me to directly run into Felt's results.

Comment 28

[Jason Smith [:jsmith]]

(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.

Comment 29

[Paul Theriault [:pauljt] (no longer reading bugmail)]

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.

Comment 30

[Paul Theriault [:pauljt] (no longer reading bugmail)]

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.

Comment 31

[Paul Theriault [:pauljt] (no longer reading bugmail)]

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?

Comment 32

[Eric Rescorla (:ekr)]

(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?

Comment 33

[Eric Rescorla (:ekr)]

(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.

Comment 34

[Eric Rescorla (:ekr)]

(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?

Comment 35

[Paul Theriault [:pauljt] (no longer reading bugmail)]

(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?

Comment 36

[Paul Theriault [:pauljt] (no longer reading bugmail)]

(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.

Comment 37

[Stephany Wilkes]

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.

Comment 38

[[:fabrice] Fabrice Desré]

(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.

Comment 39

[Jason Smith [:jsmith]]

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

Comment 40

[Alfredo Yang (:alfredo)]

Attachment: Permissions for gUM

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.

Comment 41

[Alfredo Yang (:alfredo)]

Create bug 898343 for detail implementations. Keep this one to discuss permission problem.

Comment 42

[Jason Smith [:jsmith]]

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.

Comment 43

[Jason Smith [:jsmith]]

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?

Comment 44

[Marcos Caceres [:marcosc]]

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.

Comment 45

[Jason Smith [:jsmith]]

(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.

Comment 46

[Marcos Caceres [:marcosc]]

(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 :)

Comment 47

[Alfredo Yang (:alfredo)]

Attachment: gUM_Permissions

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.

Comment 48

[Alfredo Yang (:alfredo)]

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.

Comment 49

[[:fabrice] Fabrice Desré]

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.

Comment 50

[Alfredo Yang (:alfredo)]

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

Comment 51

[Alfredo Yang (:alfredo)]

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.

Comment 52

[Alfredo Yang (:alfredo)]

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.

Comment 53

[Doug Turner (:dougt)]

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.

Comment 54

[Robert O'Callahan (:roc) (email my personal email if necessary)]

(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 :-).

Comment 55

[Eric Rescorla (:ekr)]

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.

Comment 56

[Alfredo Yang (:alfredo)]

Attachment: gUM_Implementation

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.

Comment 57

[Alfredo Yang (:alfredo)]

Attachment: gUM_Permissions

Split patch, part 1.

Comment 58

[Alfredo Yang (:alfredo)]

Attachment: gUM_Implementation_idl_pidl

Split patch, part 2.
Add string array in PBrowser.ipdl and nsIContentPermissionPrompt.idl.

Comment 59

[Alfredo Yang (:alfredo)]

Attachment: gUM_Implementation

Split patch, part 3.
Add MediaPermissionGonk.h and MediaPermissionGonk.cpp under dom/media.

Comment 60

[Alfredo Yang (:alfredo)]

(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 61

[Shih-Chiang Chien [:schien] (UTC+8) (use ni? plz)]

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)

Comment 62

[Alfredo Yang (:alfredo)]

Attachment: gUM_part_1

Add new permissions and prompt for mutiple permissions request.

Comment 63

[Alfredo Yang (:alfredo)]

Attachment: gUM_part_2

Add MediaPermissionGonk.h and MediaPermissionGonk.cpp to implement ContentPermissionPrompt.

Comment 64

[Alfredo Yang (:alfredo)]

Attachment: gUM_part_3

Pass a string array instead of a single string to support multiple permissions request.

Comment 65

[Alfredo Yang (:alfredo)]

Attachment: gUM_part_1

draft version.
Add MediaPermissionGonk.h and MediaPermissionGonk.cpp to implement ContentPermissionPrompt.

Comment 66

[Alfredo Yang (:alfredo)]

Attachment: gUM_part_1

Add new permissions and prompt for mutiple permissions request.

1. Handle one permission deny and one permission allow condition.
2. Some minor codes improvement.

Comment 67

[Alfredo Yang (:alfredo)]

Attachment: gUM_part_2

Add MediaPermissionGonk.h and MediaPermissionGonk.cpp to implement ContentPermissionPrompt.

1. Return correct message in permission deny case.

Comment 68

[Alfredo Yang (:alfredo)]

Attachment: gUM_part_3

Pass a string array instead of a single string to support multiple permissions request.

1. minor coding style improvement

Comment 69

[Alfredo Yang (:alfredo)]

Attachment: gUM_part_1

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.

Comment 70

[Alfredo Yang (:alfredo)]

Attachment: gUM_part_2

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.

Comment 71

[Alfredo Yang (:alfredo)]

Attachment: gUM_part_3

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.

Comment 72

[Jason Smith [:jsmith]]

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 73

[[:fabrice] Fabrice Desré]

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);
}
});

Comment 74

[[:fabrice] Fabrice Desré]

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 75

[[:fabrice] Fabrice Desré]

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

Comment 76

[Alfredo Yang (:alfredo)]

Attachment: gUM_part_1

1. Update default permission type according to comment 72.
2. Add comments, coding nit according to review feedback.

Comment 77

[Alfredo Yang (:alfredo)]

(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.

Comment 78

[Alfredo Yang (:alfredo)]

Attachment: gUM_part_2

Update build flag to MOZ_B2G according to comment 74.

Comment 79

[Alfredo Yang (:alfredo)]

Attachment: gUM_part_3

Update UUID according to comment 75.

Comment 80

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

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 81

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

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

Comment 82

[Doug Turner (:dougt)]

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?

Comment 83

[Alfredo Yang (:alfredo)]

(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

Comment 84

[Alfredo Yang (:alfredo)]

Attachment: gUM_test

Test case.
1. Change SpecialPowers.MochPermissionPrompt for multiple permissions request.
2. Check track type of audio+video gUM request.

Comment 85

[Alfredo Yang (:alfredo)]

Attachment: gUM_part_2

(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.

Comment 86

[Alfredo Yang (:alfredo)]

(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 87

[Jason Smith [:jsmith]]

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.

Comment 88

[Jason Smith [:jsmith]]

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.

Comment 89

[Jason Smith [:jsmith]]

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.

Comment 90

[Doug Turner (:dougt)]

regarding comment #86, do we need another patch here?

Comment 91

[Alfredo Yang (:alfredo)]

Attachment: gUM_test

Update according to comment 89.

1. use head.js
2. change track test codes

Comment 92

[Jason Smith [: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?

Comment 93

[Alfredo Yang (:alfredo)]

(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.

Comment 94

[Shih-Chiang Chien [:schien] (UTC+8) (use ni? plz)]

Attachment: multi-permission-request-ipdl.patch

IPDL modification for multiple permission request

Comment 95

[Alfredo Yang (:alfredo)]

Attachment: ContentPermissionPrompt_IDL

Hi :dougt,
The WIP of ContentPermissionRequest. Do you think it's ok?

Comment 96

[Shih-Chiang Chien [:schien] (UTC+8) (use ni? plz)]

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.

Comment 97

[Alfredo Yang (:alfredo)]

Attachment: gUM_part_1

WIP.
Update for new idl ContentPermissionType in ContentPermissionRequest and ipdl.

Comment 98

[Alfredo Yang (:alfredo)]

Attachment: gUM_part_2

WIP.
Update for new idl ContentPermissionType in ContentPermissionRequest and ipdl.

Comment 99

[Alfredo Yang (:alfredo)]

Attachment: gUM_part_3

WIP.
1. Add ContentPermissionType in ContentPermissionRequest.idl
2. Pass array of struct PermissionRequest between parent/child process

Comment 100

[Jason Smith [:jsmith]]

This isn't a blocker for release. We only need to support gUM audio for 1.2 and we're past feature complete.

Comment 101

[Jason Smith [:jsmith]]

This will be needed however for 1.3, as 1.3 is intended to support full webrtc support.

Comment 102

[Alfredo Yang (:alfredo)]

Attachment: gUM_part_1

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.

Comment 103

[Alfredo Yang (:alfredo)]

Attachment: gUM_part_2

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)

Comment 104

[Alfredo Yang (:alfredo)]

Attachment: gUM_part_3

Update summary:

1. Add nsIContentPermissionType in nsIContentPermissionRequest
2. Add helper functions in nsContentPermissionHelper which will be used among geolocation, devicestorage, media...etc.

Comment 105

[Alfredo Yang (:alfredo)]

Attachment: gUM_Test

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.

Comment 106

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

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.

Comment 107

[Jason Smith [:jsmith]]

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?

Comment 108

[Jason Smith [:jsmith]]

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.

Comment 109

[[:fabrice] Fabrice Desré]

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.

Comment 110

[Alfredo Yang (:alfredo)]

Attachment: gUM_part_1

Updated patch according to comment 109.

Comment 111

[Alfredo Yang (:alfredo)]

Attachment: gUM_part_2

Keep not prompting for fake=true according to comment 106.

Comment 112

[Alfredo Yang (:alfredo)]

Attachment: gUM_part_3

Comment 113

[Alfredo Yang (:alfredo)]

Attachment: gUM_Test

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

Comment 114

[Jason Smith [: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

Comment 115

[Jason Smith [:jsmith]]

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 116

[[:fabrice] Fabrice Desré]

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..

Comment 117

[Alfredo Yang (:alfredo)]

(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.

Comment 118

[Alfredo Yang (:alfredo)]

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.

Comment 119

[Alfredo Yang (:alfredo)]

Comment on attachment 815220 [details] [diff] [review]
gUM_part_1

Hi wesj,
Could you please review mobile/android/components/ContentPermissionPrompt.js?
Thank you.

Comment 120

[Alfredo Yang (:alfredo)]

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.

Comment 121

[:Felipe Gomes (needinfo for replies!)]

Comment on attachment 815220 [details] [diff] [review]
gUM_part_1

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

r=felipe on the nsBrowserGlue.js changes

Comment 122

[Wesley Johnston (:wesj)]

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.

Comment 123

[Alfredo Yang (:alfredo)]

(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.

Comment 124

[Jim Mathies [:jimm]]

(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.

Comment 125

[Jim Mathies [:jimm]]

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?

Comment 126

[Doug Turner (:dougt)]

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|).

Comment 127

[Alfredo Yang (:alfredo)]

Attachment: gUM_merged_patch

merged to tip 150690:608de8acc55f.

Comment 128

[Alfredo Yang (:alfredo)]

(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.

Comment 129

[Alfredo Yang (:alfredo)]

Attachment: gUM_part_3

Updated according to review comment 126.

Comment 130

[Alfredo Yang (:alfredo)]

(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.

Comment 131

[Eric Rescorla (:ekr)]

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+

Comment 132

[Jim Mathies [:jimm]]

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.

Comment 133

[Jim Mathies [:jimm]]

try build showing the build errors you'll need to fix to land -

https://tbpl.mozilla.org/?tree=Try&showall=0&rev=a78aaa253d59

Comment 134

[Alfredo Yang (:alfredo)]

(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 135

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

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

Comment 136

[Alfredo Yang (:alfredo)]

(In reply to Kyle Huey [:khuey] ([e-mail]) 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?

Comment 137

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

(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&.

Comment 138

[Alfredo Yang (:alfredo)]

(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.

Comment 139

[Alfredo Yang (:alfredo)]

Attachment: gUM_part_3

Comment 140

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

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.

Comment 141

[Alfredo Yang (:alfredo)]

(In reply to Kyle Huey [:khuey] ([e-mail]) 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.

Comment 142

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

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.

Comment 143

[Alfredo Yang (:alfredo)]

(In reply to Kyle Huey [:khuey] ([e-mail]) 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.

Comment 144

[Alfredo Yang (:alfredo)]

Attachment: gUM_part_3

Comment 145

[Alfredo Yang (:alfredo)]

Comment on attachment 820252 [details] [diff] [review]
gUM_part_3

Carry r+ from comment 140.

Comment 146

[Alfredo Yang (:alfredo)]

Attachment: gUM_part_2

Carry r+ from comment 122~123.

Comment 147

[Alfredo Yang (:alfredo)]

Attachment: gUM_part_1

Carry r+ from comment 116, 121, 122, 130~131, 132.

Comment 148

[Alfredo Yang (:alfredo)]

Attachment: gUM_Test

Carry r+ from comment 114

Comment 149

[Alfredo Yang (:alfredo)]

Pass all test on tryserver.
https://tbpl.mozilla.org/?tree=Try&rev=183f501a2452

The new added test also be tested.
https://tbpl.mozilla.org/php/getParsedLog.php?id=29489636&tree=Try&full=1

Comment 150

[Mark Finkle (:mfinkle) (use needinfo?)]

(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.

Comment 151

[Alfredo Yang (:alfredo)]

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

Comment 152

[Mark Finkle (:mfinkle) (use needinfo?)]

(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.

Comment 153

[Alfredo Yang (:alfredo)]

(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 <[e-mail]>
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.

Comment 154

[Alfredo Yang (:alfredo)]

https://tbpl.mozilla.org/?tree=Try&rev=c970add5f82a

Comment 155

[Alfredo Yang (:alfredo)]

Attachment: gUM_part_1

Rebase, carry r+ from comment 116, 121, 122, 130~131, 132.

Comment 156

[Alfredo Yang (:alfredo)]

Attachment: gUM_part_2

Rebase, carry r+ from comment 122~123.

Comment 157

[Alfredo Yang (:alfredo)]

Attachment: gUM_part_3

Rebase, carry r+ from comment 140.

Comment 158

[Alfredo Yang (:alfredo)]

Attachment: gUM_Test

Rebase, carry r+ from comment 114

Comment 159

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/7fe2c77ebbf2
https://hg.mozilla.org/integration/mozilla-inbound/rev/0f687f920370
https://hg.mozilla.org/integration/mozilla-inbound/rev/987b3ec24f68
https://hg.mozilla.org/integration/mozilla-inbound/rev/c7ea3c8c8375

Comment 160

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/7fe2c77ebbf2
https://hg.mozilla.org/mozilla-central/rev/0f687f920370
https://hg.mozilla.org/mozilla-central/rev/987b3ec24f68
https://hg.mozilla.org/mozilla-central/rev/c7ea3c8c8375

Comment 161

[Doug Turner (:dougt)]

may have caused regression bug 933075.

Comment 162

[Doug Turner (:dougt)]

yes, this caused the regression.  Either fix it, or back out asap.

Comment 163

[Doug Turner (:dougt)]

Sounds like Alfredo isn't in

Backing these out:

https://hg.mozilla.org/mozilla-central/rev/a8c00f1dfc77
https://hg.mozilla.org/mozilla-central/rev/6a2e7988d9d9
https://hg.mozilla.org/mozilla-central/rev/17097a9650b4
https://hg.mozilla.org/mozilla-central/rev/abe6790a5dd8 

https://hg.mozilla.org/releases/mozilla-aurora/rev/dfff1f403d29
https://hg.mozilla.org/releases/mozilla-aurora/rev/be8a1371032b
https://hg.mozilla.org/releases/mozilla-aurora/rev/175925fa023b
https://hg.mozilla.org/releases/mozilla-aurora/rev/cd77e66d77f0

Comment 164

[Ivan Tsay (:ITsay)]

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?

Comment 165

[Ivan Tsay (:ITsay)]

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?

Comment 166

[Alfredo Yang (:alfredo)]

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.

Comment 167

[[:fabrice] Fabrice Desré]

What prevents us from changing gaia as well on the branches this gecko patch is applied?

Comment 168

[Alfredo Yang (:alfredo)]

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.

Comment 169

[Kan-Ru Chen [:kanru] (UTC+9)]

https://hg.mozilla.org/integration/b2g-inbound/rev/3903cb4b779f
https://hg.mozilla.org/integration/b2g-inbound/rev/2baeee8eb114
https://hg.mozilla.org/integration/b2g-inbound/rev/6d33ac3a2b14
https://hg.mozilla.org/integration/b2g-inbound/rev/dc03ef5cd3fd

Comment 170

[Kan-Ru Chen [:kanru] (UTC+9)]

Red tree. backout'ed

https://hg.mozilla.org/integration/b2g-inbound/rev/76d25a2ce826
https://hg.mozilla.org/integration/b2g-inbound/rev/05337da4ed5b
https://hg.mozilla.org/integration/b2g-inbound/rev/39cf6f2f6c61
https://hg.mozilla.org/integration/b2g-inbound/rev/5de4d0c361ae

Comment 171

[Alfredo Yang (:alfredo)]

Attachment: gUM_part_1

Rebase to tip.

Comment 172

[Alfredo Yang (:alfredo)]

Attachment: gUM_part_2

rebase to tip

Comment 173

[Alfredo Yang (:alfredo)]

Attachment: gUM_part_3

rebase to tip

Comment 174

[Alfredo Yang (:alfredo)]

Attachment: gUM_Test

Comment 175

[Kan-Ru Chen [:kanru] (UTC+9)]

Reland:
remote:   https://hg.mozilla.org/integration/b2g-inbound/rev/4354fa962cbd
remote:   https://hg.mozilla.org/integration/b2g-inbound/rev/17a932d2d42a
remote:   https://hg.mozilla.org/integration/b2g-inbound/rev/a170a926c0fb
remote:   https://hg.mozilla.org/integration/b2g-inbound/rev/5e0c6b71a9ac

Comment 176

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/4354fa962cbd
https://hg.mozilla.org/mozilla-central/rev/17a932d2d42a
https://hg.mozilla.org/mozilla-central/rev/a170a926c0fb
https://hg.mozilla.org/mozilla-central/rev/5e0c6b71a9ac

Comment 177

[Reuben Morais [:reuben]]

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

Comment 178

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

Can we get someone to test this thoroughly before we land it a fourth time?

Comment 179

[Ryan VanderMeulen [:RyanVM]]

(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

Comment 180

[Ivan Tsay (:ITsay)]

Plus this one for v1.3 to complete gUM video part.

Comment 181

[Alfredo Yang (:alfredo)]

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.

Comment 182

[Jason Smith [:jsmith]]

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.

Comment 183

[Alfredo Yang (:alfredo)]

Test case depends on https://bugzilla.mozilla.org/show_bug.cgi?id=945614 and https://bugzilla.mozilla.org/show_bug.cgi?id=926746.

Comment 184

[Alfredo Yang (:alfredo)]

Hi fabrice, the testcase dependes on other bugs. I plan to disable the testcase and land it.
What do you think?

Comment 185

[Jason Smith [:jsmith]]

(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.

Comment 186

[[:fabrice] Fabrice Desré]

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.

Comment 187

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

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.

Comment 188

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

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.

Comment 189

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/3fd07d5561b7
https://hg.mozilla.org/mozilla-central/rev/5a019e26c6b2
https://hg.mozilla.org/mozilla-central/rev/a72c03a07567

Comment 190

[Alfredo Yang (:alfredo)]

Attachment: gUM_Test

Hi sheriff, bug 944615 pulled in, please add test case, gUM_Test.
Thanks.

https://tbpl.mozilla.org/?tree=Try&rev=3a958a7e29cc

Comment 191

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/b2g-inbound/rev/144cec97320a

Comment 192

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/144cec97320a

Comment 193

[Gregor Wagner [:gwagner]]

I think this broke the permission prompt for contacts again :(

STR: Download Con Backup app from Marketplace and try to backup your contacts.

Comment 194

[Jason Smith [:jsmith]]

(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.

Comment 195

[Jason Smith [:jsmith]]

Yup - Gregor is right. The contacts API permission prompt is busted. Device storage API permission prompt & geolocation permission prompt are fine though.

Comment 196

[Jason Smith [:jsmith]]

Spoke w/Gregor & Fabrice in IRC - we're backing this out & punting this feature to 1.4.

Comment 197

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

(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.

Comment 198

[Gregor Wagner [:gwagner]]

Lets also add an integration test for this prompt before landing again.

Comment 199

[Jason Smith [:jsmith]]

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.

Comment 200

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

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)

Comment 201

[[:fabrice] Fabrice Desré]

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.

Comment 202

[cmtalbert]

(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.

Comment 203

[Shih-Chiang Chien [:schien] (UTC+8) (use ni? plz)]

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

Comment 204

[Alfredo Yang (:alfredo)]

(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.

Comment 205

[[:fabrice] Fabrice Desré]

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.

Comment 206

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

Attachment: backout p1

Comment 207

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

Attachment: backout p2

Comment 208

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

Attachment: backout p3

Comment 209

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

To backout, apply these backouts in this order:
backout from bug 945614
backout p3
backout p2
backout p1

Comment 210

[Carsten Book [:Tomcat]]

(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

Comment 211

[Alfredo Yang (:alfredo)]

Attachment: auroa_backout

backout patch for 1.3 branch aurora.

Comment 212

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/a1acfd0cc6b9

Comment 213

[Shih-Chiang Chien [:schien] (UTC+8) (use ni? plz)]

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.

Comment 214

[Jason Smith [:jsmith]]

(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.

Comment 215

[Ivan Tsay (:ITsay)]

Update target milestone

Comment 216

[Alfredo Yang (:alfredo)]

Attachment: gUM_other_permissions

Comment 217

[Alfredo Yang (:alfredo)]

Attachment: gUM_sandbox_testcase

Comment 218

[Alfredo Yang (:alfredo)]

Attachment: gUM_video_prompt

Carry r+

Comment 219

[Ivan Tsay (:ITsay)]

Hi Fabrice,

Would you please help on the patch review? Thank you very much!

Comment 220

[[:fabrice] Fabrice Desré]

No need to micro-manage my review queue Ivan. I know it's there...

Comment 221

[[:fabrice] Fabrice Desré]

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.

Comment 222

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

https://hg.mozilla.org/integration/b2g-inbound/rev/b1e246595663

Comment 223

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/b1e246595663

Comment 224

[Kevin Grandon :kgrandon]

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.

Comment 225

[Jason Smith [:jsmith]]

(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.

Comment 226

[Gregor Wagner [:gwagner]]

(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.

Comment 227

[Gregor Wagner [:gwagner]]

Backout https://hg.mozilla.org/integration/b2g-inbound/rev/cb4960e3af99

Comment 228

[Carsten Book [:Tomcat]]

(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

Comment 229

[Alfredo Yang (:alfredo)]

Attachment: gUM_video_prompt

rebase, carry r+.

Comment 230

[Gregor Wagner [:gwagner]]

(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.

Comment 231

[Alfredo Yang (:alfredo)]

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.

Comment 232

[Jason Smith [:jsmith]]

(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?

Comment 233

[Alfredo Yang (:alfredo)]

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.

Comment 234

[Jason Smith [:jsmith]]

(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?

Comment 235

[Alfredo Yang (:alfredo)]

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

Comment 236

[u459114]

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.

Comment 237

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

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

Comment 238

[Alfredo Yang (:alfredo)]

(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.

Comment 239

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

(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?

Comment 240

[Alfredo Yang (:alfredo)]

(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.

Comment 241

[Jason Smith [:jsmith]]

That sounds fine to me.

Comment 242

[Alfredo Yang (:alfredo)]

Attachment: gUM_prompt_test

Comment 243

[Jason Smith [:jsmith]]

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?

Comment 244

[Alfredo Yang (:alfredo)]

(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.

Comment 245

[Alfredo Yang (:alfredo)]

Attachment: gUM_prompt_test

Comment 246

[Alfredo Yang (:alfredo)]

Attachment: gUM_video_prompt

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.

Comment 247

[Alfredo Yang (:alfredo)]

Comment on attachment 8372212 [details] [diff] [review]
gUM_prompt_test

Move testcase to bug 969977.

Comment 248

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

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?

Comment 249

[Alfredo Yang (:alfredo)]

> 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.

Comment 250

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

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

Comment 251

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/b2g-inbound/rev/b987d99d861a
https://hg.mozilla.org/integration/b2g-inbound/rev/572b469e40b6
https://hg.mozilla.org/integration/b2g-inbound/rev/7a823e498a9b

Comment 252

[Eric Rescorla (:ekr)]

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.

Comment 253

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/b987d99d861a
https://hg.mozilla.org/mozilla-central/rev/572b469e40b6
https://hg.mozilla.org/mozilla-central/rev/7a823e498a9b

Comment 254

[StevenLee[:slee]]

(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.

Comment 255

[Jason Smith [:jsmith]]

(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.

Comment 256

[StevenLee[:slee]]

(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.

Comment 257

[Jason Smith [:jsmith]]

(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.

Comment 258

[u459114]

(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.

Comment 259

[u459114]

(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.