Closed Bug 804073 Opened 8 years ago Closed 8 years ago

B2G RIL: use system message to notify telephony new calls

Categories

(Core :: DOM: Device Interfaces, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla19
blocking-basecamp +
Tracking Status
firefox18 --- fixed
firefox19 --- fixed

People

(Reporter: hsinyi, Assigned: hsinyi)

References

Details

(Whiteboard: [LOE:S])

Attachments

(3 files, 2 obsolete files)

+++ This bug was initially created as a clone of Bug 802745 +++

We might want to launch the dialer both on incoming and on outgoing calls.
bb? because this blocks bb+ bug 802745.
blocking-basecamp: --- → ?
Hsin, could you provide more information? I'm afraid that pointing to a security protected bug isn't enough.
As I see it, there is no point in notifying that an outgoing call is happening given that our model was that only the dialer should be making phone calls. I understood that you guys made the STK app able to make calls. Is that what is broken?
OS: Linux → All
Hardware: x86_64 → All
Version: unspecified → Trunk
Hi Mounir,

This is requested from our partner.

When making a call through AT command the call is connected and the notification of the outgoing call is sent to content process but the call screen is not shown.
Yeah, STK can initiate the call. Triggering the dialer seems like the right thing to do. The bug is not security protected, it just contains some partner information.
(In reply to Andreas Gal :gal from comment #4)
> Yeah, STK can initiate the call. Triggering the dialer seems like the right
> thing to do.

It scares me to provide enough APIs to simulate a phone call from A to Z without using the dialer. What was the reason why we didn't want to use a Web Activity here?

> The bug is not security protected, it just contains some partner information.

I thought I was only forbidden to access security protected bugs so I assumed it was one. Bugzilla obviously don't tell you why you are not allowed to access a bug :(
Think of STK as the SIM card controlling the phone, so it wants to dial a number. A web activity would wait for the user to confirm the dialing right? Thats probably not what we want. The SIM is trusted here in this context.
(In reply to Andreas Gal :gal from comment #6)
> Think of STK as the SIM card controlling the phone, so it wants to dial a
> number. A web activity would wait for the user to confirm the dialing right?
> Thats probably not what we want. The SIM is trusted here in this context.

Sure, the SIM card should be able to make calls but the STK wants to use the dialer UI which seems weird. We are introducing hacks that will allow apps to open the in-call dialer UI. I do not think this is really clean.

What about this: we introduce a new Web Activity named 'call' that requires dial permissions to be able to be used and requires dial permissions to be able to handle. The 'call' activity would place a direct call (I think some other apps would be interested by that). In our current model, it wouldn't show a UI because no third-party app can have dial permissions. However, any dialer app could replace the current dialer and be used instead.
How does that sound?
Sounds like a pretty clean and easy fix to me. Can you whip up a patch?
CC frsela who implements the gaia of STK.

In RIL, there is another RIL request called "RIL_REQUEST_HANDLE_CALL_SETUP_REQUESTED_FROM_SIM" which is dedicated to make an MO call requested by SIM. And the dialling number is already stored in modem, so STK app just needs to respond 'accept' or 'reject' to the call, it doesn't (and cannot) have to provide the number key-in by user.

so the behaviour is different from normal dialer, which uses "RIL_REQUEST_DIAL" with user-specified "number" needed.

(BTW, the behaviour here may be vendor-specific, I found the RIL on Galaxy S2 uses other hidden RIL request).
blocking-basecamp: ? → +
(In reply to Mounir Lamouri (:mounir) from comment #7)
> 
> Sure, the SIM card should be able to make calls but the STK wants to use the
> dialer UI which seems weird. We are introducing hacks that will allow apps
> to open the in-call dialer UI. I do not think this is really clean.
> 
> What about this: we introduce a new Web Activity named 'call' that requires
> dial permissions to be able to be used and requires dial permissions to be
> able to handle. The 'call' activity would place a direct call (I think some
> other apps would be interested by that). In our current model, it wouldn't
> show a UI because no third-party app can have dial permissions. However, any
> dialer app could replace the current dialer and be used instead.
> How does that sound?

Hi Mounir, the Web Activity idea looks great, but I am afraid that this issue isn't the case which 'call' Activity can support. As Yoshi commented, STK call is made by a different underlying request from that dialer uses. AT call's behaviour is different as well. Actually, AT call isn't made from content, from chrome instead.
Attached patch patchSplinter Review
use generic system message (telephony-new-call) to notify new calls, so we don't need 'telephony-incoming' and 'icc-dialing'
Attachment #674148 - Flags: review?(philipp)
Hsin-Yi: Am I right in saying that the way this works is that the sim card is the one actually placing the call. It's not that the sim card requests to make a call. It simply makes the call itself and then notifies the device that it has done so.

If so, a system message sounds like the right solution here. However, you shouldn't broadcast the message to all apps that are listening for it. You should only send the message to the dialer app.
(In reply to Jonas Sicking (:sicking) from comment #12)
> Hsin-Yi: Am I right in saying that the way this works is that the sim card
> is the one actually placing the call. It's not that the sim card requests to
> make a call. It simply makes the call itself and then notifies the device
> that it has done so.

Correct.

> If so, a system message sounds like the right solution here. However, you
> shouldn't broadcast the message to all apps that are listening for it. You
> should only send the message to the dialer app.

I agree. Holding off on the review for now. Hsin-Yi, does that sound like a sensible solution to you?
(In reply to Philipp von Weitershausen [:philikon] from comment #13)
> (In reply to Jonas Sicking (:sicking) from comment #12)
> > Hsin-Yi: Am I right in saying that the way this works is that the sim card
> > is the one actually placing the call. It's not that the sim card requests to
> > make a call. It simply makes the call itself and then notifies the device
> > that it has done so.
> 
> Correct.
> 

Yes, this is the way.

> > If so, a system message sounds like the right solution here. However, you
> > shouldn't broadcast the message to all apps that are listening for it. You
> > should only send the message to the dialer app.
> 
> I agree. Holding off on the review for now. Hsin-Yi, does that sound like a
> sensible solution to you?

Good point and that makes sense for me.
Attachment #674148 - Attachment is obsolete: true
Attachment #674148 - Flags: review?(philipp)
send system message "telephony-new-call" to Dialer notifying a new call (incoming/dialing)
Attachment #674563 - Flags: review?(philipp)
Attached patch Gaia test patch (won't submit) (obsolete) — Splinter Review
For gaia test
(In reply to Hsin-Yi Tsai [:hsinyi] from comment #16)
> Created attachment 674952 [details] [diff] [review]
> Gaia test patch (won't submit)
> 
> For gaia test

Do we still need https://github.com/mozilla-b2g/gaia/blob/master/apps/communications/dialer/js/dialer.js#L111 ?

Aren't we going through the "telephony-new-call" system message path for calls dialed from the Dialer app too?
Francisco, this is going to be of great interest to you :)
(In reply to Etienne Segonzac (:etienne) from comment #17)
> (In reply to Hsin-Yi Tsai [:hsinyi] from comment #16)
> > Created attachment 674952 [details] [diff] [review]
> > Gaia test patch (won't submit)
> > 
> > For gaia test
> 
> Do we still need
> https://github.com/mozilla-b2g/gaia/blob/master/apps/communications/dialer/
> js/dialer.js#L111 ?
> 
> Aren't we going through the "telephony-new-call" system message path for
> calls dialed from the Dialer app too?

'telephony-new-call' will be sent whenever a new call occurs, including 'incoming call', 'dialing call and made by the Dialer app', 'dialing call but not made by the Dialer.' (if the patch passes review)

I guess there would be more work that needs to be done than my gaia-test patch. You know, I am not the Dialer expert ;-) This patch is used just for verifying that the Dialer is launched at the right moment as I expect. I shall file a gaia bug as well. :)
We have to make sure those system messages can't be received by applications that do not have the 'dial' permissions.
(In reply to Jonas Sicking (:sicking) from comment #12)
> If so, a system message sounds like the right solution here. However, you
> shouldn't broadcast the message to all apps that are listening for it. You
> should only send the message to the dialer app.

I think I didn't think about the implications of the last sentence when writing comment 14. Did you mean to hard-code the dialer app in Gecko like Hsin-Yi did in her v2 patch? I'm not sure that's the right solution. In fact, I now wonder why we shouldn't broadcast this message among all subscribers. Remember, pages have to explicitly subscribe to these messages like in [1].

[1] https://github.com/mozilla-b2g/gaia/blob/master/apps/communications/manifest.webapp#L69


(In reply to Mounir Lamouri (:mounir) from comment #20)
> We have to make sure those system messages can't be received by applications
> that do not have the 'dial' permissions.

ITYM the 'telephony' permission? Or are you proposing to create a new permission just for this? In either case, why would a permission check add security here? AFAICT you need to explicitly subscribe to system messages in the manifest. The same place you also register for permissions. So the control is with the manifest which needs to be reviewed either way. Am I missing something?
(In reply to Philipp von Weitershausen [:philikon] from comment #21)
> (In reply to Jonas Sicking (:sicking) from comment #12)
> > If so, a system message sounds like the right solution here. However, you
> > shouldn't broadcast the message to all apps that are listening for it. You
> > should only send the message to the dialer app.
> 
> I think I didn't think about the implications of the last sentence when
> writing comment 14. Did you mean to hard-code the dialer app in Gecko like
> Hsin-Yi did in her v2 patch?

Isn't that what we're doing for incoming phone calls?

> I'm not sure that's the right solution. In
> fact, I now wonder why we shouldn't broadcast this message among all
> subscribers. Remember, pages have to explicitly subscribe to these messages
> like in [1].
> 
> [1]
> https://github.com/mozilla-b2g/gaia/blob/master/apps/communications/manifest.
> webapp#L69

With a broadcast we still deliver the message to all app processes. That means that if a process has been hacked it will get the message even if it isn't enumerated in the manifest I believe. Or is that not correct?

> (In reply to Mounir Lamouri (:mounir) from comment #20)
> > We have to make sure those system messages can't be received by applications
> > that do not have the 'dial' permissions.
> 
> ITYM the 'telephony' permission? Or are you proposing to create a new
> permission just for this? In either case, why would a permission check add
> security here? AFAICT you need to explicitly subscribe to system messages in
> the manifest. The same place you also register for permissions. So the
> control is with the manifest which needs to be reviewed either way. Am I
> missing something?

The problem is that if simply enumerating the messagehandler gives you additional permissions, then it means that reviewers will have to audit more things. One of the points of the "permissions" property was that all of the elevated permissions would be enumerated there, giving reviewers a single thing to audit.

Additionally, by tying into the permissions property it means that this will be tied in to the permissionmanager database which means that we can do things like allowing the settings app to revoke permissions. We've also talked about a blocklisting mechanism for apps which would allow the app to remain as-is but just revoking permissions for it.

So I definitely think we should make sure that a "telephony" permission is needed. Or that we hardcode the app for now (we don't support replacing the phone app anyway in v1, and it'll likely be a while before we allow it).
(In reply to Jonas Sicking (:sicking) from comment #22)
> (In reply to Philipp von Weitershausen [:philikon] from comment #21)
> > (In reply to Jonas Sicking (:sicking) from comment #12)
> > > If so, a system message sounds like the right solution here. However, you
> > > shouldn't broadcast the message to all apps that are listening for it. You
> > > should only send the message to the dialer app.
> > 
> > I think I didn't think about the implications of the last sentence when
> > writing comment 14. Did you mean to hard-code the dialer app in Gecko like
> > Hsin-Yi did in her v2 patch?
> 
> Isn't that what we're doing for incoming phone calls?

We never have. And I don't like it.

> > I'm not sure that's the right solution. In
> > fact, I now wonder why we shouldn't broadcast this message among all
> > subscribers. Remember, pages have to explicitly subscribe to these messages
> > like in [1].
> > 
> > [1]
> > https://github.com/mozilla-b2g/gaia/blob/master/apps/communications/manifest.
> > webapp#L69
> 
> With a broadcast we still deliver the message to all app processes. That
> means that if a process has been hacked it will get the message even if it
> isn't enumerated in the manifest I believe. Or is that not correct?

It is not correct if my understanding of nsISystemMessageInternal::broadcastMessage() [1] is correct. We explicitly only send the system message to processes that are registered for that type of message. Fabrice confirmed this over IRC. In fact, it was apparently introduced for this very use case.

[1] https://mxr.mozilla.org/mozilla-central/source/dom/messages/SystemMessageInternal.js#129

> > (In reply to Mounir Lamouri (:mounir) from comment #20)
> > > We have to make sure those system messages can't be received by applications
> > > that do not have the 'dial' permissions.
> > 
> > ITYM the 'telephony' permission? Or are you proposing to create a new
> > permission just for this? In either case, why would a permission check add
> > security here? AFAICT you need to explicitly subscribe to system messages in
> > the manifest. The same place you also register for permissions. So the
> > control is with the manifest which needs to be reviewed either way. Am I
> > missing something?
> 
> The problem is that if simply enumerating the messagehandler gives you
> additional permissions, then it means that reviewers will have to audit more
> things. One of the points of the "permissions" property was that all of the
> elevated permissions would be enumerated there, giving reviewers a single
> thing to audit.

Ok. If this is the goal (that reviewers only need to audit permissions), we must gate system messages by permission checks. Always. Because apps might sneak by DOM API permission reviews by simply registering themselves for system messages.

Thankfully Mounir filed bug 805655 to fix this. So maybe we can r+ and commit patch v1 in this bug, fix bug 805655, and then in a follow up bug fix all call sites to broadcastMessage() to require permissions.

> So I definitely think we should make sure that a "telephony" permission is
> needed.

Given what you said about the review process, I agree with you.

> Or that we hardcode the app for now (we don't support replacing the
> phone app anyway in v1, and it'll likely be a while before we allow it).

I would prefer not to do that but enforce the permission. Would the approach outline above work for you?
I still don't see any harm in directing system messages at a hardcoded application given that I'm fairly sure that we have lots of things that would break down if we actually installed multiple apps which tried to act as dialer.

But yes, I'm fine with the broadcast approach if we check permissions before sending system messages.
Updated app uri
Attachment #674952 - Attachment is obsolete: true
Updated app uri

(Will ask for review later once we come to an agreement about broadcastMessage or sendMessage)
Attachment #674563 - Attachment is obsolete: true
Attachment #674563 - Flags: review?(philipp)
Whiteboard: [LOE:S]
Attachment #676035 - Attachment description: Gaia test patch (won't submit) → Gaia test patch (won't submit) (v2)
(In reply to Jonas Sicking (:sicking) from comment #24)
> I still don't see any harm in directing system messages at a hardcoded
> application given that I'm fairly sure that we have lots of things that
> would break down if we actually installed multiple apps which tried to act
> as dialer.

Sure, but that breakage would be on the Gaia side of things more than on the Gecko side. Telephony events are supported across several apps. I see no good reason to restrict ourselves artificially in Gecko. Given the Gecko's release tree management and carrier requirements, it seems like a really bad idea to me to paint ourselves into a corner by hard-coding the dialer app URI.

> But yes, I'm fine with the broadcast approach if we check permissions before
> sending system messages.

Ok great. Hsinyi, can you tackle bug 805655 and make sure we require the right permissions for telephony-related system messages like the one in this bug here? Thanks!
Comment on attachment 674148 [details] [diff] [review]
patch

r+ on this patch, with the follow-ups mentioned in the previous comments.
Attachment #674148 - Attachment is obsolete: false
Attachment #674148 - Flags: review+
Comment on attachment 676036 [details] [diff] [review]
Patch (v2.1): sendMessage to Dialer

r- as per previous discussion: hardcoding the dialer URI in Gecko seems like a bad idea.
Attachment #676036 - Flags: review-
https://hg.mozilla.org/mozilla-central/rev/e7946c073343
Status: NEW → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla19
You need to log in before you can comment on or make changes to this bug.