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

take it as first bug

On desktop, webrtcUI.jsm will call mozGetUserMediaDevices to list available devices on the prompt dialog, but there is no this module on B2G so far.

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

Current B2G ContentPermissionPrompt.js, it allows one permission request per time but webrtc could need audio and video permission at the same time. It also needs to list the front/back camera for user to choose as well.

(In reply to Randy Lin [:rlin] from comment #3) > On B2G platform, could we use Manifest permission checking for this? On ap, yes. On web content, it needs a prompt for user. The behavior should be similar to geolocation.

FWIW, the original discussion on the security model for the permission prompt in the context of B2G according to https://wiki.mozilla.org/WebAPI/Security/Camera implies the following permission rules: * Web Content - PROMPT_ACTION * Hosted App - PROMPT_ACTION * Privileged App - PROMPT_ACTION * Certified App - ALLOW_ACTION Good question about the manifest property though - we're going to need that defined here in order to fall in line with the requirement that a manifest permission must be specified in order to use a WebAPI within an app. The doc I'm referencing though was written up a while back, so a quick review might be worthwhile. Paul - Can you give input here on what manifest permission + permission model we should use here for getUserMedia on B2G?

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.

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

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.

7. browser app needs to add permission 'camera' and 'microphone'. When the URL iframe opens, it will popup permission request dialog to user. I'll add browser app manifest patch after this patch is reviewed.

(In reply to Alfredo Yang from comment #11) > Created attachment 740222 [details] [diff] [review] > popup prompt dialog when request camera/microphone via getUserMedia on B2G > > 1. fix the miss part of getUserMedia prompt permisson requests on B2G, both > camera and microphone. > 2. add new permission 'microphone' for audio recording > 3. add a new jsm MediaPermission.jsm to handle camera/microphone premission > requets. > 4. it will pop 2 prompts if asking camera/microphone at the same time. This doesn't sound right. Desktop only prompts once, not twice. I'd think B2G needs to follow in suit. > 5. it doesn't take front/back camera into cosideration since W3C spec > doesn't define it so far. > 6. it also fixed bug 845727. (In reply to Alfredo Yang from comment #12) > 7. browser app needs to add permission 'camera' and 'microphone'. When the > URL iframe opens, it will popup permission request dialog to user. I'll add > browser app manifest patch after this patch is reviewed. Why two app manifest permissions? Why not just have one? Things get more messy for the app developer experience if we introduce complexity here. FWIW, we should really hold on doing a review here and as Paul suggests, discuss this on dev-webapi to remove the UI and permission contention points. For me, there's some things I agree with here, and others I'm questioning. UX also needs to add some input here to understand the recording icon issue.

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.

(In reply to Alfredo Yang from comment #9) > > * We need to have a different permission name for 'web-rtc camera' than the > > 'b2g camera' control API (or change the b2g camera permission name) > That's my thought at the beginning; however, after checking > webrtc/getusermedia spec, > there is no extra permission for webrtc. > On another proposal spec "Media Capture and Streams Settings API", > 3.2.2 Track Source Types: > > enum SourceTypeEnum { > "none", > "camera", > "microphone", > "photo-camera", > "readonly", > "remote" > }; Yes. I don't think it's that useful to distinguish in this prompt between different uses that the site might make of the camera/microphone (at least not at this time). The reason is that from a technical perspective, the permissions are pretty interchangeable. I.e., - If I can get the raw bits from the camera and microphone, I can arrange to RTP encode them and thus make a calling application. - If I can make a call and send the video/audio to the other side, then I can call myself and record the media. Now it might be the case that you think that this is a statement of the application's *intent*, but it's not really technically enforceable. In terms of indicating to the user that the devices are in use, a recording bar sounds pretty good.

> FWIW, we should really hold on doing a review here and as Paul suggests, > discuss this on dev-webapi to remove the UI and permission contention > points. For me, there's some things I agree with here, and others I'm > questioning. UX also needs to add some input here to understand the > recording icon issue. FYI, I started a thread on dev-b2g on this (sorry if webapi was more appropriate, i wasnt sure, and didnt want to crosspost). https://groups.google.com/d/msg/mozilla.dev.b2g/05LEV43lVNI/3QGZuIfF4twJ > Yes. I don't think it's that useful to distinguish in this > prompt between different uses that the site might make of the > camera/microphone (at least not at this time). The reason > is that from a technical perspective, the permissions are > pretty interchangeable. +1 for not distinguishing. > > I.e., > - If I can get the raw bits from the camera and microphone, > I can arrange to RTP encode them and thus make a calling > application. > > - If I can make a call and send the video/audio to the > other side, then I can call myself and record the media. > > Now it might be the case that you think that this is a statement > of the application's *intent*, but it's not really technically > enforceable. > > In terms of indicating to the user that the devices are in use, > a recording bar sounds pretty good. + 1 for a recording bar - perhaps using similar code to the existing ongoing call indicator, but red or something.

Asking FirefoxOS-UX for feedback on Comments 8 and 9 and to respond on https://groups.google.com/d/msg/mozilla.dev.b2g/05LEV43lVNI/3QGZuIfF4twJ (which Ian has already started). I think we all agree that we want to avoid 2 permissions prompts, and I agree with Ekr that we don't want to distinguish between the different uses that the site might make of the camera/microphone. A recording bar is also a good idea IMO. Android doesn't currently have a recording bar. Can we add it to both (Android and FxOS)? This feature is listed as "must have" for v1.2.

(In reply to Eric Rescorla (:ekr) from comment #15) > Now it might be the case that you think that this is a statement > of the application's *intent*, but it's not really technically > enforceable. B2G's app security model enforces that PROMPT_ACTION & ALLOW_ACTION WebAPIs have to specify a permission in order for apps to make use of the WebAPI. So there is a requirement for a web application to specify an intent that they intend to use getUserMedia with the appropriate manifest permission. That's what's being discussed earlier on in the permissions space. Web content doesn't need to do this, however. (In reply to Maire Reavy [:mreavy] from comment #17) > I think we all agree that we want to avoid 2 permissions prompts, and I > agree with Ekr that we don't want to distinguish between the different uses > that the site might make of the camera/microphone. A recording bar is also a > good idea IMO. Android doesn't currently have a recording bar. Can we add > it to both (Android and FxOS)? Don't forget that you need to pay attention to the app use cases here too. That's a core feature to B2G's operating system. Web content use cases are only one small part of the permission model you need to solve here. FWIW - I believe Gaia's UI already has a recording indicator that is show in the notification bar when recording is active in the camera. Although the design of the recording bar should be out of scope in this bug and discussed separately - this bug is talking about the permissions UX.

(In reply to Jason Smith [:jsmith] from comment #18) > (In reply to Eric Rescorla (:ekr) from comment #15) > > Now it might be the case that you think that this is a statement > > of the application's *intent*, but it's not really technically > > enforceable. > > B2G's app security model enforces that PROMPT_ACTION & ALLOW_ACTION WebAPIs > have to specify a permission in order for apps to make use of the WebAPI. So > there is a requirement for a web application to specify an intent that they > intend to use getUserMedia with the appropriate manifest permission. That's > what's being discussed earlier on in the permissions space. Web content > doesn't need to do this, however. > Yes, but once the app has access to the camera and microphone it can do whatever it wants and there's no existing mechanism to technically enforce whatever is put here. Unless you have some idea that we are going to punish people who lie about their intent in apps, I'm not sure I see how this is useful. > (In reply to Maire Reavy [:mreavy] from comment #17) > > I think we all agree that we want to avoid 2 permissions prompts, and I > > agree with Ekr that we don't want to distinguish between the different uses > > that the site might make of the camera/microphone. A recording bar is also a > > good idea IMO. Android doesn't currently have a recording bar. Can we add > > it to both (Android and FxOS)? > > Don't forget that you need to pay attention to the app use cases here too. > That's a core feature to B2G's operating system. Web content use cases are > only one small part of the permission model you need to solve here. I think we're aware of that, but I don't really see that it changes things much.

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

(In reply to Jason Smith [:jsmith] from comment #20) > (In reply to Eric Rescorla (:ekr) from comment #19) > > (In reply to Jason Smith [:jsmith] from comment #18) > > > (In reply to Eric Rescorla (:ekr) from comment #15) > > > > Now it might be the case that you think that this is a statement > > > > of the application's *intent*, but it's not really technically > > > > enforceable. > > > > > > B2G's app security model enforces that PROMPT_ACTION & ALLOW_ACTION WebAPIs > > > have to specify a permission in order for apps to make use of the WebAPI. So > > > there is a requirement for a web application to specify an intent that they > > > intend to use getUserMedia with the appropriate manifest permission. That's > > > what's being discussed earlier on in the permissions space. Web content > > > doesn't need to do this, however. > > > > > > > Yes, but once the app has access to the camera > > and microphone it can do whatever it wants and there's no existing > > mechanism to technically enforce whatever is put here. Unless you > > have some idea that we are going to punish people who lie about > > their intent in apps, I'm not sure I see how this is useful. > > The app review process would handle the case where a developer lies about > their intent of how an app intends to use the camera and microphone for apps > intending to be in our marketplace, given that: > > 1. This is the primary location users shall install apps (95%+ use case) This strikes me as rather speculative. > 2. If the developer did lie, the app would fail review. There's two quality > checks that ensure that depending on the type of app: > 2a. If the app is hosted or packaged, an app reviewer does exploratory > testing of every app that comes in to verify that the app stays in alignment > with app review guidelines, which includes checking to make sure that the > app is making sane use of APIs called out in the app manifest > 2b. If the app is packaged, an app reviewer additionally does a code review > to ensure sane use of the API that's called out in the manifest It strikes me as extremely unlikely that the level of code review will be anything like that required to detect malicious applications. That's simply not practical to do any any scale. Perhaps, the review will be sufficient to detect *mistakes*, but that's not an argument that the information needs to be reflected up to the browser chrome but rather that applications need to accurately represent themselves to the user in the application *content*. Consider the case where an application has two apparent modes: 1. A photobooth mode where it privately manipulates photos. 2. A mode where it uploads your photo to a public site. The app manifest permissions clearly are going to need to list the more expansive intent, because the application may need that permission. Now, consider the case where the application routinely claims (in the content) to be in photobooth mode but actually always uploads your picture. This isn't sensibly enforceable by the browser, because it's purely a matter of the content accurately representing itself. Given that we have this problem in any case, I really don't see a lot of value in trying to push a small part of the problem into some manifest information that most people ignore anyway (see the Felt citation below). > I would suggest following up with Lucas, Jonas, and Paul about this more if > you have more questions. Lucas was the original designer around why intent > has to be specified in the app manifest, so he could give you a deep > rationale on this. All the available evidence suggests that users are extremely insensitive to any sort of fine-grained permissions at application install time. See, for instance: http://www.cs.berkeley.edu/~afelt/felt-androidpermissions-soups.pdf Can you point to somewhere where someone has actually done a real analysis of this issue in the context of B2G taking into account the human factors questions? I.e., a written document, not something in someone's head.

Eric, You could read https://wiki.mozilla.org/Apps/Security Also note that the Felt paper you cite is mostly inapplicable to b2g where we choosed to *not* ask the user to make security decisions as much as possible. I'm pretty sure we want to PROMPT for web content and non-privileged apps, and ALLOW for privileged apps.

(In reply to Fabrice Desré [:fabrice] from comment #22) > Eric, > > You could read https://wiki.mozilla.org/Apps/Security I have. It seems to just state the outcome of the decisions, not provide any analysis that considers the human factors issues. > Also note that the Felt paper you cite is mostly inapplicable to b2g where > we choosed to *not* ask the user to make security decisions as much as > possible. > > I'm pretty sure we want to PROMPT for web content and non-privileged apps, > and ALLOW for privileged apps. As I understan Jason's comment here, it is that we need to state an intention (i.e., a string) that is attached to the manifest and shown to the user at the time of installation. I.e., https://wiki.mozilla.org/Apps/Security#Data_Usage_Intention Isn't this precisely the experience that the paper I am citing refers to?

(In reply to Eric Rescorla (:ekr) from comment #23) > > As I understan Jason's comment here, it is that we need to state an > intention (i.e., a string) that is attached to the manifest and > shown to the user at the time of installation. I.e., > > https://wiki.mozilla.org/Apps/Security#Data_Usage_Intention > > Isn't this precisely the experience that the paper I am citing refers > to? Well, try it for yourself. You'll see that we never show a "I allow this app to use x/y/z" at install time. If a PROMPT is needed, it will popup at first use (an subsequent ones if the user doesn't toggle the "Remember me" checkbox).

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

(In reply to Eric Rescorla (:ekr) from comment #25) > In that case, any kind of intent string in the manifest seems entirely > pointless. Why? It still could be displayed in permission prompts and in the settings app that let the user manage permissions granted to each app.

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

(In reply to Eric Rescorla (:ekr) from comment #21) > (In reply to Jason Smith [:jsmith] from comment #20) > > (In reply to Eric Rescorla (:ekr) from comment #19) > > > > The app review process would handle the case where a developer lies about > > their intent of how an app intends to use the camera and microphone for apps > > intending to be in our marketplace, given that: > > > > 1. This is the primary location users shall install apps (95%+ use case) > > This strikes me as rather speculative. That's based on the target 1.01 plan and resulting testing that's been done on the apps security model for 1.01. There's very little evidence at all from testing done (security, QA, apps review, etc) that sites are setting up their own site to allow apps to be installed onto the user's device. The primary use case users are discovering their apps are from within the Firefox Marketplace for 1.01. That doesn't mean that the use case doesn't exist to allow web developers to build sites to allow users to install apps. But that isn't typical use case at all right now for apps discovery in 1.01. However, that will change in future releases when we move to a more established multiple marketplace model. > > > > 2. If the developer did lie, the app would fail review. There's two quality > > checks that ensure that depending on the type of app: > > 2a. If the app is hosted or packaged, an app reviewer does exploratory > > testing of every app that comes in to verify that the app stays in alignment > > with app review guidelines, which includes checking to make sure that the > > app is making sane use of APIs called out in the app manifest > > 2b. If the app is packaged, an app reviewer additionally does a code review > > to ensure sane use of the API that's called out in the manifest > > It strikes me as extremely unlikely that the level of code review > will be anything like that required to detect malicious applications. > That's simply not practical to do any any scale. Perhaps, the > review will be sufficient to detect *mistakes*, but that's not > an argument that the information needs to be reflected up to the > browser chrome but rather that applications need to accurately > represent themselves to the user in the application *content*. We have two app reviewers hired full time working with one person in security who specializes in mobile malware on detection of malicious applications during the app review process. At the moment, we have been able to handle the incoming rate of apps with the appropriate code review needed for privileged app content. Even in cases where we had something slip past app review, we've had dogfooders raise issues on questionable apps, whether that be from direct filing of tech evangelism bugs, bad reviews on the app page, reporting abuse via marketplace, etc. We even have an escalation path to QA when more analysis is needed (which I work on). So I don't think the scalability problem is an issue right now. As for long term scalability, I'd have to look into what our addons process looks like as a point of comparison, as they deal with this problem at a larger scale of incoming addons (including dealing with malicious add-ons). I can followup with Lisa (app review lead) to find out what she's planning for long term scalability. > > Consider the case where an application has two apparent modes: > > 1. A photobooth mode where it privately manipulates photos. > 2. A mode where it uploads your photo to a public site. > > The app manifest permissions clearly are going to need to list the > more expansive intent, because the application may need that > permission. > > Now, consider the case where the application routinely claims > (in the content) to be in photobooth mode but actually always uploads > your picture. This isn't sensibly enforceable by the browser, > because it's purely a matter of the content accurately > representing itself. > > Given that we have this problem in any case, I really don't > see a lot of value in trying to push a small part of the problem > into some manifest information that most people ignore anyway > (see the Felt citation below). I don't follow this analysis. We don't grant content access to APIs by default on app install for the getUserMedia case. As Paul cites above, not providing the permission would always deny access to getUserMedia. Providing the permission means that the app can prompt the user within the app when access to the camera/mic is needed. So even in the case the user does install a malicious getUserMedia application, they would have to: 1. Have a discovery mechanism - marketplace is going to be hard to use here, because existing app review processes do check for malicious apps. And even if they do get into the marketplace, a post-discovery mechanism for flagging the app moderation could get the app caught for doing malicious activity. 2. After being installed with the permission specified, try to call getUserMedia before doing the malicious activity. Given that we're prompting the user here, we do give the user explicit warning when camera/mic access is being asked for. If they grant it, then at this point, the malicious activity could happen. Then again, remember the same prompting workflow happens on general web content as well in the browser. > > > > I would suggest following up with Lucas, Jonas, and Paul about this more if > > you have more questions. Lucas was the original designer around why intent > > has to be specified in the app manifest, so he could give you a deep > > rationale on this. > > All the available evidence suggests that users are extremely insensitive > to any sort of fine-grained permissions at application install time. > See, for instance: > > http://www.cs.berkeley.edu/~afelt/felt-androidpermissions-soups.pdf We currently don't do any of what you are describing at app install time from the user's perspective - it's a simple app install prompt for asking to install the app with a name, developer name & url, app size, etc. The user would find this out during runtime at the contextual point that the app needs access to the API if PROMPT_ACTION is provided. What happens instead at install time is that we are installing the permissions provided in the manifest for the particular app. That then allows the app to request access to specific WebAPIs against the permissions matrix based on their app type. In getUserMedia's case for a third-party use case, providing a manifest permission means that the app's content will be able to request access with a prompt for access to the camera/mic based on the matrix provided above. But it certainly doesn't mean that app content gets access without the prompt. So the only difference in the app use case is that we're additionally requiring a manifest property to be specified, but after that, we're doing the same thing that would happen in web content in the browser for the 3rd party use case. (In reply to Eric Rescorla (:ekr) from comment #23) > (In reply to Fabrice Desré [:fabrice] from comment #22) > > Eric, > > > > You could read https://wiki.mozilla.org/Apps/Security > > > I have. It seems to just state the outcome of the decisions, not > provide any analysis that considers the human factors issues. The deeper details of this I'd probably query Jonas & Paul directly about. > > > > > > Also note that the Felt paper you cite is mostly inapplicable to b2g where > > we choosed to *not* ask the user to make security decisions as much as > > possible. > > > > I'm pretty sure we want to PROMPT for web content and non-privileged apps, > > and ALLOW for privileged apps. > > As I understan Jason's comment here, it is that we need to state an > intention (i.e., a string) that is attached to the manifest and > shown to the user at the time of installation. I.e., > > https://wiki.mozilla.org/Apps/Security#Data_Usage_Intention > > Isn't this precisely the experience that the paper I am citing refers > to? Actually that's not what I mean. When an app specifies a permission in the app manifest, we don't show that prompt at app install time. What instead happens in our app security model is that the permission is installed for that particular app to allow the app to access the targeted WebAPI against the specified permissions matrix based on the app type. When access to that WebAPI happens within the web app, then we follow the permissions matrix on what to do here. In the 3rd party use case for getUserMedia, it's PROMPT_ACTION. That means a user would be prompted to grant access to the camera and microphone. If they grant access to the camera and microphone, then app content can proceed to use the camera and microphone as such. Note - the data usage intention field isn't even implemented right now in the UI. But when it does get implemented, the data usage intention would likely be shown contextually at the time the API request happens within the app and potentially in the app permissions UI in the Settings app in Gaia. If you really have more concerns more around the data usage intention string issue showing in the UI or not, I'd followup on bug 823385, which is tracking the UI work.

I missed a lot of posts, but I will try to provide some brief context on the app security model so we can get back on track here. - For web apps, all permissions the app intends to use must be declared in the manifest. - For PROMPT permissions, we also user approval before granting access (i.e. at runtime, the first time the API is called) - For ALLOW permissions, permission is just granted by virtue of being declared in the manifest. For security sensitive permissions, these are generally restricted to privileged apps, which are reviewed by the marketplace. For reference see Permission list here: https://developer.mozilla.org/en-US/docs/Web/Apps/App_permissions Or the code here: https://mxr.mozilla.org/mozilla-central/source/dom/apps/src/PermissionsTable.jsm#35 Notes: * Permissions are declared in the app manifest so we have a precise enumeration of the permissions an app will use.One place this is used is in the settings app so that a user can modify permissions on a per-app basis. (Note that only PROMPT permissions are shown to the user in the settings app) * Permissions are never shown to the user at install time * Usage intentions are a feature quite separate to this discussion (bug 823385, or the web api list is probably a better place to discuss any unresolved questions about usage intentions) * App reviews are seperate to this discussion since I don't think anyone is saying that we should restrict the webrtc permission to privileged apps only. (FWIW, only privileged apps go through the marketplace review process, normal web apps are not reviewed, nor could they be since they are dynamic) Further documentation: https://developer.mozilla.org/en-US/docs/Web/Apps/App_permissions https://developer.mozilla.org/en-US/docs/Mozilla/Firefox_OS/Security/Security_model For the history of the app security model see these two threads (there are others, but these are the biggest ones): https://groups.google.com/forum/#!msg/mozilla.dev.webapps/LdAid6uLQbo/sJ5W-rT-VWUJ https://groups.google.com/d/msg/mozilla.dev.webapps/UthgJMv9DaY/_s-IScrVnXQJ FWIW ekr, the author of the paper you cite was an active contributor to these threads, and part of the reason why we went with the model we went with. Hopefully that helps.

Now as for what model I propose, I think we can follow the geolocation model exactly for Firefox OS. That is: Web Content: Prompt to access Web App: Declare in manifest, prompt to access Privileged App: Declare in manifest, prompt to access Certified App: Declare in manifest, prompt to access Justification: * this is a web api, it needs to be exposed to web, but it needs user permisssion prior to enabling * permission is declared in manifest for consistency with app security model (allowing features like settings app permission enumeration) * permission should always be prompted for (even certified apps) due to the strong privacy implications (and also so that this is a permission that the user can disable, since only prompted permissions are exposed to the user currently). There are other questions, such as do we remember the permission decision, how long do we grant access for etc. But I think these are secondary. I'll set up a security review later this week to go through these questions.

Final note to disagree with my earlier post about not distinguishing between the permission type. I think https://bugzilla.mozilla.org/show_bug.cgi?id=853356#c14 was suggesting this so that gaia know what permission prompt to show in Gaia. FWIW Firefox for Android shows different prompts: https://bug862377.bugzilla.mozilla.org/attachment.cgi?id=750068 Fabrice, are we going to need separate permission names (ie webrtc:*) in order to show a custom dialog based on what resource is being requested/what is available? Do we also need some kind of capabilities API so Gaia can know the list of cameras/microphones to display?

(In reply to Paul Theriault [:pauljt] from comment #29) > * Usage intentions are a feature quite separate to this discussion (bug > 823385, or the web api list is probably a better place to discuss any > unresolved questions about usage intentions) OK, that's all I'm complaining about. > * App reviews are seperate to this discussion since I don't think anyone is > saying that we should restrict the webrtc permission to privileged apps > only. (FWIW, only privileged apps go through the marketplace review process, > normal web apps are not reviewed, nor could they be since they are dynamic) I don't think it's orthogonal to the question of usage intentions, since, as I said, they aren't mechanically enforced. > For the history of the app security model see these two threads (there are > others, but these are the biggest ones): > https://groups.google.com/forum/#!msg/mozilla.dev.webapps/LdAid6uLQbo/sJ5W- > rT-VWUJ > https://groups.google.com/d/msg/mozilla.dev.webapps/UthgJMv9DaY/_s-IScrVnXQJ > > FWIW ekr, the author of the paper you cite was an active contributor to > these threads, and part of the reason why we went with the model we went > with. Is there something in that thread that analyzes te intention model?

(In reply to Paul Theriault [:pauljt] from comment #30) > Certified App: Declare in manifest, prompt to access I would observe that the existing installed camera app does not prompt to access. Do we really want the installed app to be better than any app anyone can write? > There are other questions, such as do we remember the permission decision, > how long do we grant access for etc. But I think these are secondary. For any Web app, these need to be consistent with the desktop browser experience.

(In reply to Paul Theriault [:pauljt] from comment #31) > Final note to disagree with my earlier post about not distinguishing between > the permission type. > > I think https://bugzilla.mozilla.org/show_bug.cgi?id=853356#c14 was > suggesting this so that gaia know what permission prompt to show in Gaia. > FWIW Firefox for Android shows different prompts: > https://bug862377.bugzilla.mozilla.org/attachment.cgi?id=750068 Wait, let's distinguish between two things: 1. What device I am asking for. 2. What I intend to do with it. I think that having the prompt indicate the former is imperative. It's having it indicate the latter I have a problem with, and I would note that it won't be indicated by any web app. > Fabrice, are we going to need separate permission names (ie webrtc:*) in > order to show a custom dialog based on what resource is being requested/what > is available? Do we also need some kind of capabilities API so Gaia can > know the list of cameras/microphones to display?

(In reply to Eric Rescorla (:ekr) from comment #32) > (In reply to Paul Theriault [:pauljt] from comment #29) > > * Usage intentions are a feature quite separate to this discussion (bug > > 823385, or the web api list is probably a better place to discuss any > > unresolved questions about usage intentions) > > OK, that's all I'm complaining about. Note that usage intentions are not shown to the user yet (for various reasons, UI space mainly, but also there was concern about coercing users) Afaik, the intentions are only read by marketplace reviewers when doing reviews of _privileged_ apps. > > > > * App reviews are seperate to this discussion since I don't think anyone is > > saying that we should restrict the webrtc permission to privileged apps > > only. (FWIW, only privileged apps go through the marketplace review process, > > normal web apps are not reviewed, nor could they be since they are dynamic) > > I don't think it's orthogonal to the question of usage intentions, since, > as I said, they aren't mechanically enforced. I think there are crossed wires above - in c15 you were talking about having one permission since flagging intention via different permissions names was not useful from a security perspective since the permissions ultimately have a similar impact, due the attacks you cite. Which I totally agree with. I think since you used the word 'intentions' the discussion c16 then got onto talking about manifest usage intentions, which is something separate I think. Manifest usage intentions are not currently shown to the user, but ultimately we would like to show these once we can figure out an appropriate place for these to be shown. The only place these usage intentions are currently used is by marketplace reviewers, when they are reviewing _privileged_ apps only. Hence why I don't think the manifest usage intentions are important to this particular discussion. Bottom line is usage intentions will not be relevant for web content, since there is no manifest. So we cant base any security control for webrtc on this. > > > > For the history of the app security model see these two threads (there are > > others, but these are the biggest ones): > > https://groups.google.com/forum/#!msg/mozilla.dev.webapps/LdAid6uLQbo/sJ5W- > > rT-VWUJ > > https://groups.google.com/d/msg/mozilla.dev.webapps/UthgJMv9DaY/_s-IScrVnXQJ > > > > FWIW ekr, the author of the paper you cite was an active contributor to > > these threads, and part of the reason why we went with the model we went > > with. > > Is there something in that thread that analyzes te intention model?

(In reply to Eric Rescorla (:ekr) from comment #33) > (In reply to Paul Theriault [:pauljt] from comment #30) > > Certified App: Declare in manifest, prompt to access > I would observe that the existing installed camera app > does not prompt to access. Do we really want the installed > app to be better than any app anyone can write? Certified apps (ie those that come with the phone) in general dont prompt for APIs since we wrote them, and we trust them. The only permission which we actually prompt for in certified apps is geolocation, because of the privacy implications. As per c30, I am proposing we adopt the same model for webrtc, ie even certified apps have to prompt for, and the user has control over this permission. FWIW the camera app doesn't use webrtc, it uses the camera control API. This API that allows much deeper control than webrtc, such as control of aperture, flash etc. Unfortunately because of this, currently this API needs direct device access, which is insecure to expose to third party apps. The original intention was to expose this API to third party apps, but we weren't able to achieve this securely for version 1. Having the camera prompt for the Camera API isnt useful since if it can't work without the API, and since no other apps can use this app, there wasnt any point in considering equality between third-party and the camera app in this case. or at least not yet, anyways. > > > > There are other questions, such as do we remember the permission decision, > > how long do we grant access for etc. But I think these are secondary. > > For any Web app, these need to be consistent with the desktop > browser experience.

UX will be providing a spec for this in the near future. We're coordinating with Maire. Clearing the needinfo since this work is now in progress.

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

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

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.

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

Comment on attachment 781609 [details] [diff] [review] Permissions for gUM Review of attachment 781609 [details] [diff] [review]: ----------------------------------------------------------------- ::: dom/apps/src/PermissionsTable.jsm @@ +274,5 @@ > app: DENY_ACTION, > privileged: DENY_ACTION, > certified: ALLOW_ACTION > }, > + "webrtc": { If audio-capture & video-capture already exist here, why do we need a third permission here? You could handle the audio & video case by just checking if both audio-capture & video-capture both exist. Point of comparison - Google did this in two permissions.

Marcos - There's a lot of debate here on what manifest permissions to use for getUserMedia. The patch shows you one approach. My comment indicates an alternative approach. From the spec owner's side for the app manifest - what do you manifest permission properties would you prefer to be used?

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.

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

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

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

I still don't understand how we will show only one prompt if we have two permissions. I would be more comfortable r+ this if you also had the UI pieces done that implement that.

On current Gaia implementation, all the requests (including geolocation, desktop notification, gUM...) will be queued and prompted one by one. To merge multiple gUM requests, Gaia needs to check the the queue if there are gUM resuests from the same app/url [1]. For audio+video, Gecko will send audio and video request to Gaia respectively just like they are requested by two gUM calls separately in the same script. Implementation plan: 1.2 will support single gUM request prompt. 1.3 will support merge multiple requests gUM request to one prompt [2]. [1] https://bugzilla.mozilla.org/show_bug.cgi?id=898343#c18 [2] https://bugzilla.mozilla.org/show_bug.cgi?id=898343#c10

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.

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.

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.

(In reply to Doug Turner (:dougt) from comment #53) > I tend to think we need a app facing permission request API that allows the > developer to pre-grant permissions. They could ask for the set of all > things they might be interested in (geo, camera, mic, ect.) in one API call. > I tend to think this 'only ask for permission as a direct result of user > interaction' has problems (such as this). I tend to disagree, so if you go down that route please loop me in :-).

WRT comment #51, part 3, I tend to agree with Doug here. You should consolidate requests received in immediate succession from the JS (i.e., before it returns control to the system) but not those received asynchrounously. This has the advantage of being easier, as well.

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.

Split patch, part 1.

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

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

(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #54) > (In reply to Doug Turner (:dougt) from comment #53) > > I tend to think we need a app facing permission request API that allows the > > developer to pre-grant permissions. They could ask for the set of all > > things they might be interested in (geo, camera, mic, ect.) in one API call. > > I tend to think this 'only ask for permission as a direct result of user > > interaction' has problems (such as this). > > I tend to disagree, so if you go down that route please loop me in :-). I go that way, but only the new add permissions "video-capture" and "audio-capture" are allowed in one API call.

Comment on attachment 795911 [details] [diff] [review] gUM_Implementation_idl_pidl Review of attachment 795911 [details] [diff] [review]: ----------------------------------------------------------------- ::: dom/interfaces/base/nsIContentPermissionPrompt.idl @@ +40,5 @@ > + * The type of the permission requests, such as > + * "geolocation". > + */ > + void types([optional] out unsigned long aLength, > + [array, size_is(aLength), retval] out wstring aTypes); Using nsIArray for array type of attribute in IDL will be better. (see example in nsIMIMEInfo http://mxr.mozilla.org/mozilla-central/source/netwerk/mime/nsIMIMEInfo.idl#188)

Add new permissions and prompt for mutiple permissions request.

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

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

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

Add new permissions and prompt for mutiple permissions request. 1. Handle one permission deny and one permission allow condition. 2. Some minor codes improvement.

Add MediaPermissionGonk.h and MediaPermissionGonk.cpp to implement ContentPermissionPrompt. 1. Return correct message in permission deny case.

Pass a string array instead of a single string to support multiple permissions request. 1. minor coding style improvement

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.

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.

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 on attachment 797032 [details] [diff] [review] gUM_part_1 Review of attachment 797032 [details] [diff] [review]: ----------------------------------------------------------------- ::: dom/apps/src/PermissionsTable.jsm @@ +293,5 @@ > }, > + "audio-capture": { > + app: PROMPT_ACTION, > + privileged: PROMPT_ACTION, > + certified: ALLOW_ACTION Per talking with pauljt, this needs to prompt, not allow by default.

Comment on attachment 797032 [details] [diff] [review] gUM_part_1 Review of attachment 797032 [details] [diff] [review]: ----------------------------------------------------------------- That mostly looks good, but I want to see a new version with: - more comments - various nits fixed - do we have tests? ::: b2g/components/ContentPermissionPrompt.js @@ +30,5 @@ > "PermSettings", > "@mozilla.org/permissionSettings;1", > "nsIDOMPermissionSettings"); > > +function rememberPermission(aTypesInfo, aPrincipal, aSession) what is aTypesInfo structure? We need a lot more comments in this patch, things are getting tricky. @@ +72,5 @@ > > ContentPermissionPrompt.prototype = { > > + handleExistingPermission: function handleExistingPermission(request, typesInfo) { > + var getTypePermission = function (type) { s/var/let, here and everywhere else you used |var| @@ +73,5 @@ > ContentPermissionPrompt.prototype = { > > + handleExistingPermission: function handleExistingPermission(request, typesInfo) { > + var getTypePermission = function (type) { > + type.permission = Services.perms.testExactPermissionFromPrincipal(request.principal, type.access); nit: 80 char per line maximum. @@ +91,5 @@ > } > + > + var checkDenyPermission = function (type) { > + if (type.permission == Ci.nsIPermissionManager.DENY_ACTION || > + type.permission == Ci.nsIPermissionManager.UNKNOWN_ACTION && PROMPT_FOR_UNKNOWN.indexOf(type.access) < 0) { Why did you remove the request.cancel() here? @@ +113,5 @@ > + var checkIfAllowMultiRequest = function (type) { > + if (ALLOW_MULTIPLE_REQUESTS.indexOf(type.access) == -1) { > + return false; > + } > + return true; Replace by: return ALLOW_MULTIPLE_REQUESTS.indexOf(type.access) !== -1 @@ +114,5 @@ > + if (ALLOW_MULTIPLE_REQUESTS.indexOf(type.access) == -1) { > + return false; > + } > + return true; > + } Nit: add blank line @@ +190,1 @@ > return; nit: use {} even for single line ifs. @@ +195,5 @@ > + if (typesInfo[0].permission == Ci.nsIPermissionManager.PROMPT_ACTION && !typesInfo[0].deny) { > + typesInfo.push(typesInfo[0]); > + } > + typesInfo.shift(); > + } I think that we can do something a bit simpler like: typesInfo.forEach((aType, aIndex) => { if (typesInfo[0].permission != Ci.nsIPermissionManager.PROMPT_ACTION || typesInfo[0].deny) { typesInfo.splice(aIndex); } });

Comment on attachment 797033 [details] [diff] [review] gUM_part_2 Review of attachment 797033 [details] [diff] [review]: ----------------------------------------------------------------- ::: dom/media/MediaManager.cpp @@ +907,5 @@ > } > LOG(("%s: default prefs: %dx%d @%dfps (min %d)", __FUNCTION__, > mPrefs.mWidth, mPrefs.mHeight, mPrefs.mFPS, mPrefs.mMinFPS)); > > +#if defined(MOZ_WIDGET_GONK) MOZ_WIDGET_GONK is only defined on device. Don't we want that to work on b2g desktop also? That would then be MOZ_B2G instead.

Comment on attachment 797034 [details] [diff] [review] gUM_part_3 Review of attachment 797034 [details] [diff] [review]: ----------------------------------------------------------------- there are other call sites to update, like https://mxr.mozilla.org/mozilla-central/source/dom/permission/PermissionPromptHelper.jsm#82 ::: dom/interfaces/base/nsIContentPermissionPrompt.idl @@ +12,5 @@ > * Interface allows access to a content to request > * permission to perform a privileged operation such as > * geolocation. > */ > [scriptable, uuid(1de67000-2de8-11e2-81c1-0800200c9a66)] you need to update the uuid

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

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

Update build flag to MOZ_B2G according to comment 74.

Update UUID according to comment 75.

Fabrice wrote: >MOZ_WIDGET_GONK is only defined on device. Don't we want that to work on b2g desktop also? That would then be MOZ_B2G instead. If we're going to support b2g desktop webrtc/getusermedia, there are three other ifdefs in MediaManager.h/cpp, and one in media/webrtc/trunk (that one should likely change to WEBRTC_GONK, which is defined by the common.gypi file)

Comment on attachment 797741 [details] [diff] [review] gUM_part_2 Review of attachment 797741 [details] [diff] [review]: ----------------------------------------------------------------- I assume when "Allow" happens, the user will have been given the choice to select a device or no video (or no audio, depending on which dropdown it is) and it will have modified the mRequest->mDevices array, so you can accept audio and reject video (for example). r+ with the following nits addressed (especially the mDevices->Count() issue and making sure the error text makes sense.) ::: dom/media/MediaPermissionGonk.cpp @@ +116,5 @@ > + } > + } > + > + if (!mDevices.Length()) { > + mErrorMsg.Assign(NS_LITERAL_STRING("NO AVAILABLE DEVICE")); Consider aligning the text for this error with the text for the desktop error (NO_DEVICES_FOUND or HARDWARE_UNAVAILABLE depending on the error). Also, normally MediaManager.cpp will dispatch those errors back; if this string is used for something else (user-visible, etc) then never mind. @@ +150,5 @@ > +{ > + *aLength = mTypes.Length(); > + PRUnichar** ret = > + static_cast<PRUnichar**>(NS_Alloc(*aLength * sizeof(PRUnichar*))); > + NS_ENSURE_TRUE(ret, NS_ERROR_OUT_OF_MEMORY); Is there a reason to use a fallible allocator here for a <20 byte string? If so, fine, otherwise switch to an infallible allocator and get rid of the NS_ENSURE_TRUE. @@ +153,5 @@ > + static_cast<PRUnichar**>(NS_Alloc(*aLength * sizeof(PRUnichar*))); > + NS_ENSURE_TRUE(ret, NS_ERROR_OUT_OF_MEMORY); > + > + for (uint32_t i = 0; i < *aLength; ++i) { > + ret[i] = UTF8ToNewUnicode(mTypes[i]); We don't have a standard converter for this? @@ +302,5 @@ > +MediaPermissionMgr::stateClose() > +{ > + nsresult rv; > + rv = NS_NewISupportsArray(getter_AddRefs(mDevices)); > + NS_ASSERTION(NS_SUCCEEDED(rv), "Out of memory"); No need for a string here @@ +310,5 @@ > + mDevices->AppendElement(mRequest->mDevices[i]); > + } > + } > + > + // If there is granted device, it returns "allow" to script. minor nit: "is a granted" and "to the script" @@ +315,5 @@ > + nsCOMPtr<nsIObserverService> obs = mozilla::services::GetObserverService(); > + if (obs) { > + uint32_t itemCount; > + rv = mDevices->Count(&itemCount); > + NS_ASSERTION(NS_SUCCEEDED(rv), "Fail to get array count"); Do what nsDocShell.cpp does: uint32_t itemCount = 0; mDevices->Count(&itemCount); This lets the normal no-items case handle errors, and if that had failed (in reality it can't!), this code would have been wrong anyways and caused it to use an uninitialized value for itemCount. @@ +326,5 @@ > + obs->NotifyObservers(supportsString, "getUserMedia:response:deny", mCallID.get()); > + } > + } > + > + // Stop sate machine and free requests nit: state

Comment on attachment 797744 [details] [diff] [review] gUM_part_3 Review of attachment 797744 [details] [diff] [review]: ----------------------------------------------------------------- ::: dom/interfaces/base/nsIContentPermissionPrompt.idl @@ +20,4 @@ > * The access of the permission request, such as > * "read". > */ > readonly attribute ACString access; access is per permission type. Maybe we should have a new type is the union of both access and type @@ +40,5 @@ > + * The type of the permission requests, such as > + * "geolocation". > + */ > + void types([optional] out unsigned long aLength, > + [array, size_is(aLength), retval] out wstring aTypes); I think roc has some concern about doing it this way (supporting multiple permission requests in one go). I support this. Could you take this question to the platfrom newsgroup and cc both of us?

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

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

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

(In reply to Doug Turner (:dougt) from comment #82) > Comment on attachment 797744 [details] [diff] [review] > gUM_part_3 > > Review of attachment 797744 [details] [diff] [review]: > ----------------------------------------------------------------- > > ::: dom/interfaces/base/nsIContentPermissionPrompt.idl > @@ +20,4 @@ > > * The access of the permission request, such as > > * "read". > > */ > > readonly attribute ACString access; > > access is per permission type. > > Maybe we should have a new type is the union of both access and type IMHO, it probably needs an array of a permission object like [{type1, acess1}, {type2, access2}...]. It also needs to restrict the allowed multiple permissions request somewhere in Gecko. For example, only audio-capture and video-capture can be requested together so far.

Comment on attachment 800564 [details] [diff] [review] gUM_test Review of attachment 800564 [details] [diff] [review]: ----------------------------------------------------------------- ::: dom/media/tests/mochitest/test_getUserMedia_permissions.html @@ +29,5 @@ > +var gTests = [ > + { > + type: 'audio-capture', > + perm: Ci.nsIPermissionManager.ALLOW_ACTION, > + constraints: {audio: true}, You cannot use direct constraints in a mochitest planning to be ran on buildbot, as we do not have real or simulated devices right now for camera/mic on B2G. You would need to implement support for fake devices on Firefox OS to do this - see https://bugzilla.mozilla.org/show_bug.cgi?id=815002 which represents the desktop version of what would be needed here.

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 on attachment 800564 [details] [diff] [review] gUM_test Review of attachment 800564 [details] [diff] [review]: ----------------------------------------------------------------- ::: dom/media/tests/mochitest/test_getUserMedia_permissions.html @@ +29,5 @@ > +var gTests = [ > + { > + type: 'audio-capture', > + perm: Ci.nsIPermissionManager.ALLOW_ACTION, > + constraints: {audio: true}, Disregard this comment - saw the fake reference below. @@ +41,5 @@ > + testEnd: true > + } > +]; > + > +function getUserMedia(constraints, onSuccess, onError) { Note - this is code duplication. I think you should just include head.js to automatically get access to this function. @@ +59,5 @@ > + SimpleTest.finish(); > + }); > +} > + > +function testTracks(constraints, stream) { Note - the basic gUM tests which are already running on b2g emulators already cover this.

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

Update according to comment 89. 1. use head.js 2. change track test codes

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?

(In reply to Doug Turner (:dougt) from comment #90) > regarding comment #86, do we need another patch here? Ok, I'll write a patch for it. This change probably needs to take the requirements of bug 898949 into consideration.

IPDL modification for multiple permission request

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

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.

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

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

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

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

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

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.

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)

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

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

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

Updated patch according to comment 109.

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

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

Forgot to mention: review+ as long as this mochitest is confirmed to successfully run in a try run You should kick off a try run and verify that your mochitest ran successfully.

Comment on attachment 815220 [details] [diff] [review] gUM_part_1 Review of attachment 815220 [details] [diff] [review]: ----------------------------------------------------------------- r=me with nits addressed for the b2g parts and dom/apps part. The other changes look ok also, but you need to get them reviewed by their respective module owners. ::: b2g/components/ContentPermissionPrompt.js @@ +4,5 @@ > > "use strict" > > function debug(str) { > + dump("-*- ContentPermissionPrompt: " + str + "\n"); Put the comment back before landing ;) @@ +13,5 @@ > const Cu = Components.utils; > const Cc = Components.classes; > > +const PROMPT_FOR_UNKNOWN = ["geolocation", "desktop-notification", > + "video-capture", "audio-capture"]; nit: can you order them alphabetically? @@ +22,1 @@ > here too. ::: browser/components/nsBrowserGlue.js @@ +1970,5 @@ > }, > > prompt: function CPP_prompt(request) { > > + // only allow exactly one permission rquest here Nit: // Only allow exactly one permission request here. ::: browser/metro/components/ContentPermissionPrompt.js @@ +69,5 @@ > return false; > }, > > prompt: function(request) { > + // only allow exactly one permission rquest here nit: // Only allow exactly one permission request here. ::: mobile/android/components/ContentPermissionPrompt.js @@ +61,5 @@ > > prompt: function(request) { > let isApp = request.principal.appId !== Ci.nsIScriptSecurityManager.NO_APP_ID && request.principal.appId !== Ci.nsIScriptSecurityManager.UNKNOWN_APP_ID; > > + // only allow exactly one permission rquest here nit: same as in other files... ::: webapprt/ContentPermission.js @@ +29,5 @@ > .QueryInterface(Ci.nsIDOMChromeWindow); > }, > > prompt: function(request) { > + // only allow exactly one permission rquest here nit: here also..

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

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 on attachment 815220 [details] [diff] [review] gUM_part_1 Hi wesj, Could you please review mobile/android/components/ContentPermissionPrompt.js? Thank you.

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 on attachment 815220 [details] [diff] [review] gUM_part_1 Review of attachment 815220 [details] [diff] [review]: ----------------------------------------------------------------- r=felipe on the nsBrowserGlue.js changes

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.

(In reply to Wesley Johnston (:wesj) from comment #122) > Comment on attachment 815220 [details] [diff] [review] > gUM_part_1 > > Review of attachment 815220 [details] [diff] [review]: > ----------------------------------------------------------------- > > ::: mobile/android/components/ContentPermissionPrompt.js > @@ +63,5 @@ > > let isApp = request.principal.appId !== Ci.nsIScriptSecurityManager.NO_APP_ID && request.principal.appId !== Ci.nsIScriptSecurityManager.UNKNOWN_APP_ID; > > > > + // only allow exactly one permission rquest here > > + let types = request.types.QueryInterface(Ci.nsIArray); > > + if (types.length != 1) { > > We support multiple prompts at once on Android. Is there a good reason to > bail here? I'd be fine with this, but you should file something in our > component to update this. Thanks for feedback. The original concern is security. From my understanding, the only case allows multiple request is audio+video in getUserMedia. But thing looks different on Android, I will filed a bug for it.

(In reply to Alfredo Yang from comment #118) > Hi jimm > Could you please review browser/metro/base/content/helperui/IndexedDB.js and > browser/metro/components/ContentPermissionPrompt.js in gUM_part_1? > Thank you. For future reference, patches like this should be broken down into individual patches for each platform and posted individually so you can set review requests on each for the appropriate reviewer.

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

merged to tip 150690:608de8acc55f.

(In reply to Jim Mathies [:jimm] from comment #125) > Comment on attachment 815220 [details] [diff] [review] > gUM_part_1 > > Alfredo can you please post a rollup of this work merged to tip so I can > test it in metro? Sure, please try https://bugzilla.mozilla.org/attachment.cgi?id=817092. Thanks.

Updated according to review comment 126.

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

try build showing the build errors you'll need to fix to land - https://tbpl.mozilla.org/?tree=Try&showall=0&rev=a78aaa253d59

(In reply to Jim Mathies [:jimm] from comment #133) > try build showing the build errors you'll need to fix to land - > > https://tbpl.mozilla.org/?tree=Try&showall=0&rev=a78aaa253d59 Yes, the windows build failure can be fixed with following modifications in nsDocuemtn.cpp. 70 #ifdef CreateEvent 71 #undef CreateEvent 72 #endif 73 74 #ifdef LoadImage 75 #undef LoadImage 76 #endif 77 78 #ifdef ALTERNATE 79 #undef ALTERNATE 80 #endif https://tbpl.mozilla.org/?tree=Try&rev=e1d6d4475488

Comment on attachment 817093 [details] [diff] [review] gUM_part_3 Review of attachment 817093 [details] [diff] [review]: ----------------------------------------------------------------- This looks good, but I would like to see it again before giving an r+ Please fix the commit message. It appears to be two separate messages, and one is not for this patch. Also update the reviewer ;-) ::: content/base/src/nsDocument.cpp @@ +10643,5 @@ > +{ > + nsresult rv = CreatePermissionArray(NS_LITERAL_CSTRING("pointerLock"), > + NS_LITERAL_CSTRING("unused"), > + aTypes); > + return rv; just "return CreatePermissionArray(...". No need for the intermediate variable. ::: content/base/src/nsDocument.h @@ +67,5 @@ > #include "nsDataHashtable.h" > #include "mozilla/TimeStamp.h" > #include "mozilla/Attributes.h" > #include "nsIDOMXPathEvaluator.h" > +#include "nsIContentPermissionPrompt.h" Do you need to include this in the header? You didn't change anything else in nsDocument.h ... can you get away with just including it in nsDocument.cpp? ::: dom/base/nsContentPermissionHelper.cpp @@ +56,3 @@ > { > + nsCOMPtr<nsIMutableArray> types = do_CreateInstance(NS_ARRAY_CONTRACTID); > + if(ConvertPermissionRequestToArray(mPermissionRequests, types)) { nit: space between 'if' and '(' @@ +56,4 @@ > { > + nsCOMPtr<nsIMutableArray> types = do_CreateInstance(NS_ARRAY_CONTRACTID); > + if(ConvertPermissionRequestToArray(mPermissionRequests, types)) { > + NS_IF_ADDREF(*aTypes = types); I think you can just do types.forget(aTypes); @@ +184,5 @@ > +} > + > +uint32_t > +ConvertArrayToPermissionRequest(nsCOMPtr<nsIArray>& aSrcArray, > + nsTArray<PermissionRequest>& aDesArray) Nothing uses this function. Get rid of it? @@ +205,5 @@ > + nsCOMPtr<nsIMutableArray>& aDesArray) > +{ > + uint32_t len = aSrcArray.Length(); > + for (uint32_t i = 0; i < len; i++) { > + nsCOMPtr<ContentPermissionType> cpt = nsRefPtr<ContentPermissionType> We use nsRefPtr for concrete classes and nsCOMPtr for interfaces. @@ +212,5 @@ > + } > + return len; > +} > + > +nsresult CreatePermissionArray(const nsACString& aType, nit: nsresult\n CreatePermissionArray(... @@ +217,5 @@ > + const nsACString& aAccess, > + nsIArray** aTypesArray) > +{ > + nsCOMPtr<nsIMutableArray> types = do_CreateInstance(NS_ARRAY_CONTRACTID); > + nsCOMPtr<ContentPermissionType> permType = new ContentPermissionType(aType, nsRefPtr<ContentPermissionType> ::: dom/base/nsContentPermissionHelper.h @@ +50,5 @@ > + nsCString mType; > + nsCString mAccess; > +}; > + > +uint32_t ConvertArrayToPermissionRequest(nsCOMPtr<nsIArray>& aSrcArray, This function appears to be unused. Remove it? @@ +54,5 @@ > +uint32_t ConvertArrayToPermissionRequest(nsCOMPtr<nsIArray>& aSrcArray, > + nsTArray<PermissionRequest>& aDesArray); > + > +uint32_t ConvertPermissionRequestToArray(nsTArray<PermissionRequest>& aSrcArray, > + nsCOMPtr<nsIMutableArray>& aDesArray); Please line up the arguments, and make aDesArray an nsIMutableArray**. We try not to pass nsCOMPtr&. ::: dom/devicestorage/nsDeviceStorage.cpp @@ +1700,5 @@ > + DeviceStorageTypeChecker::GetPermissionForType(mFile->mStorageType, type); > + NS_ENSURE_SUCCESS(rv, rv); > + > + rv = CreatePermissionArray(type, NS_LITERAL_CSTRING("read"), aTypes); > + return rv; Same thing here about just returning CreatePermissionArray @@ +2209,5 @@ > return rv; > } > + > + rv = CreatePermissionArray(type, access, aTypes); > + return rv; And here. ::: dom/interfaces/base/nsIContentPermissionPrompt.idl @@ +14,2 @@ > */ > +[scriptable, uuid(384b6cc4-a66b-4bea-98e0-eb10562a9ba4)] please mark this as builtinclass too. ::: dom/ipc/PBrowser.ipdl @@ +210,5 @@ > * Initiates an asynchronous request for permission for the > * provided principal. > * > + * @param aRequests > + * The array of permission to requests. super-nit: 'permissions' and 'request' ::: dom/src/notification/DesktopNotification.cpp @@ +359,5 @@ > { > + nsresult rv = CreatePermissionArray(NS_LITERAL_CSTRING("desktop-notification"), > + NS_LITERAL_CSTRING("unused"), > + aTypes); > + return rv; and here ::: dom/src/notification/Notification.cpp @@ +233,5 @@ > { > + nsresult rv = CreatePermissionArray(NS_LITERAL_CSTRING("desktop-notification"), > + NS_LITERAL_CSTRING("unused"), > + aTypes); > + return rv; and here

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

(In reply to Alfredo Yang from comment #136) > Thanks for review. > This function actually used in gUM_part_1, or I move this function to > gUM_part_1? Yes, please move it to part 1 then. The same thing I said about ConvertPermissionRequestToArray applies here, that function should take an nsIArray* instead of an nsCOMPtr&.

(In reply to Jim Mathies [:jimm] from comment #132) > Comment on attachment 815220 [details] [diff] [review] > gUM_part_1 > > This doesn't build on win32 successfully, your changes to nsDocument > apparently pulled in the windows.h header creating a couple conflicts with > native apis. I was able to fix this by adding: > > #ifdef XP_WIN > #ifdef CreateEvent > #undef CreateEvent > #endif > #ifdef LoadImage > #undef LoadImage > #endif > #endif > > to the top of nsDocument.cpp. With that it builds. I'm not sure if the > inclusion of windows.h in nsDocument is an issue but since there are content > folks cc'd in here I'll leave it to them to complain if they feel it is. > > Tested with a local build on metro and found that the geolocation prompting > is working as expected with these patches applied. Thanks Kyle's help, windows.h is included by followings <khuey> so nsContentPermissionHelper.h <khuey> includes PermissionMessageUtils.h <khuey> which includes IPCMessageUtils.h <khuey> which includes a bunch of chromium ipc crap <khuey> which includes windows.h A forward declaration IPC::Principal can fix this problem.

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.

(In reply to Kyle Huey [:khuey] (khuey@mozilla.com) from comment #140) > > > > +class ContentPermissionType : public nsIContentPermissionType > > This class is only used in nsContentPermissionHelper.cpp (everyone else just > sees nsIContentPermissionType), so move this declaration into the .cpp file. Thanks for review. MediaPermissionGonk.cpp in gUM_part_2 uses this class, could we keep it in header? Others are fixed.

Ah, ok, yeah. It has to stay in the header then. It's confusing to break patches up in ways where things added in part X are already used in part X - 1. Please try not to do that in the future.

(In reply to Kyle Huey [:khuey] (khuey@mozilla.com) from comment #142) > Ah, ok, yeah. It has to stay in the header then. > > It's confusing to break patches up in ways where things added in part X are > already used in part X - 1. Please try not to do that in the future. Ok. Btw, for the windows.h header problem, I found all the auto-generated headers need to be removed. TabParent.cpp and nsContentPermissionHelper.h, and nsContentPermissionHelper.cpp are updated for this change.

Comment on attachment 820252 [details] [diff] [review] gUM_part_3 Carry r+ from comment 140.

Carry r+ from comment 122~123.

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

Carry r+ from comment 114

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

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