804073 - B2G RIL: use system message to notify telephony new calls

Status: RESOLVED FIXED

(Core :: DOM: Device Interfaces, defect)

Description

[Hsin-Yi Tsai (she/her) | away [:hsinyi]]

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

Comment 1

[Hsin-Yi Tsai (she/her) | away [:hsinyi]]

bb? because this blocks bb+ bug 802745.

Comment 2

[Mounir Lamouri (:mounir)]

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?

Comment 3

[Hsin-Yi Tsai (she/her) | away [:hsinyi]]

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.

Comment 4

[Andreas Gal :gal]

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.

Comment 5

[Mounir Lamouri (:mounir)]

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

Comment 6

[Andreas Gal :gal]

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.

Comment 7

[Mounir Lamouri (:mounir)]

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

Comment 8

[Andreas Gal :gal]

Sounds like a pretty clean and easy fix to me. Can you whip up a patch?

Comment 9

[Yoshi Cheng-Hao Huang [:allstars.chh][:allstarschh][:yoshi]]

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

Comment 10

[Hsin-Yi Tsai (she/her) | away [:hsinyi]]

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

Comment 11

[Hsin-Yi Tsai (she/her) | away [:hsinyi]]

Attachment: patch

use generic system message (telephony-new-call) to notify new calls, so we don't need 'telephony-incoming' and 'icc-dialing'

Comment 12

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

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.

Comment 13

[Philipp von Weitershausen [:philikon]]

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

Comment 14

[Hsin-Yi Tsai (she/her) | away [:hsinyi]]

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

Comment 15

[Hsin-Yi Tsai (she/her) | away [:hsinyi]]

Attachment: Patch (v2): sendMessage to Dialer

send system message "telephony-new-call" to Dialer notifying a new call (incoming/dialing)

Comment 16

[Hsin-Yi Tsai (she/her) | away [:hsinyi]]

Attachment: Gaia test patch (won't submit)

For gaia test

Comment 17

[Etienne Segonzac (:etienne)]

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

Comment 18

[Etienne Segonzac (:etienne)]

Francisco, this is going to be of great interest to you :)

Comment 19

[Hsin-Yi Tsai (she/her) | away [:hsinyi]]

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

Comment 20

[Mounir Lamouri (:mounir)]

We have to make sure those system messages can't be received by applications that do not have the 'dial' permissions.

Comment 21

[Philipp von Weitershausen [:philikon]]

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

Comment 22

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

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

Comment 23

[Philipp von Weitershausen [:philikon]]

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

Comment 24

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

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.

Comment 25

[Hsin-Yi Tsai (she/her) | away [:hsinyi]]

Attachment: Gaia test patch (won't submit) (v2)

Updated app uri

Comment 26

[Hsin-Yi Tsai (she/her) | away [:hsinyi]]

Attachment: Patch (v2.1): sendMessage to Dialer

Updated app uri

(Will ask for review later once we come to an agreement about broadcastMessage or sendMessage)

Comment 27

[Philipp von Weitershausen [:philikon]]

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

[Philipp von Weitershausen [:philikon]]

Comment on attachment 674148 [details] [diff] [review]
patch

r+ on this patch, with the follow-ups mentioned in the previous comments.

Comment 29

[Philipp von Weitershausen [:philikon]]

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.

Comment 30

[Hsin-Yi Tsai (she/her) | away [:hsinyi]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/e7946c073343

Comment 31

[Ed Morley [:emorley]]

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

Comment 32

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/5e70aa88e163