849185 - Disable the airplane mode when an emergency number is dialed

Status: RESOLVED FIXED

(Core :: DOM: Device Interfaces, defect)

Description

[Etienne Segonzac (:etienne)]

After discussing on bug 829581, the plan when the airplane mode is on is:

- gaia .dial() the call
- the platform checks if the call _isEmergency
  - if yes the air plane mode is disabled and the call goes through
  - if no we get on onerror triggered on the call with a specific error

Comment 1

[Etienne Segonzac (:etienne)]

This block a gaia leo+ bug.

Comment 2

[Mounir Lamouri (:mounir)]

Vicamo, how does that sound to you? My plan is that the RIL would send a specific error (like "AirplaneMode") when the call can't be made because the phone is in airplane mode and would just disable the airplane mode if the number is an emergency number and place the call.

Comment 3

[Vicamo Yang [:vicamo][:vyang]]

(In reply to Mounir Lamouri (:mounir) from comment #2)
> Vicamo, how does that sound to you?

Sounds very reasonable.

HsinYi, could you take this or let's find another appropriate owner on Monday?

Comment 4

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

(In reply to Vicamo Yang [:vicamo][:vyang] from comment #3)
> (In reply to Mounir Lamouri (:mounir) from comment #2)
> > Vicamo, how does that sound to you?
> 
> Sounds very reasonable.
> 
> HsinYi, could you take this or let's find another appropriate owner on
> Monday?

Sure!

Comment 5

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

After investigating this issue more, I realized that it could be better that Gaia takes responsibility of disabling airplane mode automatically for making an emergency call. 

The main concern is that settings DB is involved. RIL can enable the radio, that's no problem. But while RIL successfully makes radio on, it finds out the current radio state doesn't match the settings value. So it will reset the radio again that causes the radio still not available for the emergency call.

Following are the potential solutions in gecko side:
Possible solution 1): RIL changes settings value internally first. When the radio is enabled due to the settings value which is on, RIL redials the call. 

Possible solution 2): RIL doesn't change the settings value at the first moment (but we need to have corresponding modification in handler function for setting radio so that RIL won't disable the radio due to the consistency of the settings value). RIL just enables the radio and make a call. However, eventually, RIL should change the settings value in DB. Otherwise, UI shows wrong information about the radio state. If we don't want to change the setting after this emergency call, then we need to disable radio again.

Since involvement of settings DB, I was thinking letting gaia taking care of this seems making more sense as gaia is the one who checks the airplane mode. Though, we will need to expose isEmergencyNumber in DOM API. Does that sound reasonable?

Comment 6

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

(In reply to Hsin-Yi Tsai [:hsinyi] from comment #5)
> Since involvement of settings DB, I was thinking letting gaia taking care of
> this seems making more sense as gaia is the one who checks the airplane
> mode. Though, we will need to expose isEmergencyNumber in DOM API. Does that
> sound reasonable?
B.t.w. Android have this feature done in app level.

Comment 7

[Anshul]

Hsin-Yi, you also need to consider the case of an emergency dialer from the phone lock screen. Currently emergency dialer does allow dialing an emergency call in airplane mode which modem fails anyways and is also inconsistent with the behavior of regular dialer where gaia blocks the call in airplane mode.

If Gaia can handle both the cases then I am okay with that approach, however I thought there was a concern with exposing emergency numbers to Gaia in the first place bug 840102.

Comment 8

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

(In reply to Anshul from comment #7)
> Hsin-Yi, you also need to consider the case of an emergency dialer from the
> phone lock screen. Currently emergency dialer does allow dialing an
> emergency call in airplane mode which modem fails anyways and is also
> inconsistent with the behavior of regular dialer where gaia blocks the call
> in airplane mode.
> 
> If Gaia can handle both the cases then I am okay with that approach, 

I think so, but maybe Etienne could help confirm or provide opinions?

> however
> I thought there was a concern with exposing emergency numbers to Gaia in the
> first place bug 840102.

According to the discussion thread, bug 840102 was marked as WONTFIX because we thought RIL disabling airplane mode automatically was a good idea there. I doubt now, as my above comment said. Maybe Mounir can also provide his concerns or comments here, thanks!

Comment 9

[Etienne Segonzac (:etienne)]

To be honest I don't really have a preference :) I'm really fine with both solutions.

Comment 10

[Mounir Lamouri (:mounir)]

I'm pretty sure that at any moment, from any application, if someone tries to make an emergency call, we would like (even are requested?) to disable the airplane mode. The best way to make sure that happens is by making this behaviour happening in the backend instead of the frontend.

So, unless it is technically impossible to have RIL taking care of disabling the airplane mode, we should do that.

Comment 11

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

(In reply to Mounir Lamouri (:mounir) from comment #10)
> I'm pretty sure that at any moment, from any application, if someone tries
> to make an emergency call, we would like (even are requested?) to disable
> the airplane mode. The best way to make sure that happens is by making this
> behaviour happening in the backend instead of the frontend.
> 
> So, unless it is technically impossible to have RIL taking care of disabling
> the airplane mode, we should do that.
Thanks, if you do think backend should do that, I will take your suggestion. I just wanted to raise my concern and make sure we take a good method. :)

Comment 12

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

Attachment: Part 1 - disable the airplane mode

As comment#0 said, the platform checks if the call _isEmergency
  - if yes the air plane mode is disabled and the call goes through
  - if no we get on onerror triggered on the call with a specific error 'AirplaneMode'

More, if we encounter error 3 times when trying to disable airplane mode, then we stop attempting and deliver callerror 'AirplaneMode'.

Comment 13

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

Attachment: Part2 - marionette test case

Comment 14

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

Comment on attachment 726495 [details] [diff] [review]
Part 1 - disable the airplane mode

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

::: dom/system/gonk/ril_worker.js
@@ +1603,5 @@
>    },
>  
> +  // Cache requests for making an emergency call until radio state is ready.
> +  _dialEmergencyQueue : [],
> +  _dialEmergencyRetryCount: 0,

Seems we could replace retryCount with dialEmergencyQueue.length?

@@ +1651,5 @@
> +        this.sendDOMMessage(options);
> +      }
> +      return;
> +    }
> +

In the original dial, it already has handlers for emergency call,
I suggest you move you code in there.
Otherwise you would make dial more complicated.

if (RADIO_OFF) {
   if (isEMERGENCY) {

    }
}
...

if (VOICE.emergencyOnly || isEMERGENCY) {

}
...

In your call path, isEmergecy would be called twice.

@@ +4569,5 @@
> +    }
> +
> +    this._dialEmergencyRetryCount = 0;
> +    queue.length = 0;
> +  }

Add a simple check outside, so normal callbacks won't spend too much time on this.

@@ +4623,5 @@
> +
> +        queue.length = 0;
> +        this._dialEmergencyRetryCount = 0;
> +      }
> +    }

4 levels of indents again.

Comment 15

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

Attachment: Part1 - v2

Addressed review comments

Comment 16

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

Attachment: Part2 - v2

Modified test case manifest

Comment 17

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

Comment on attachment 726549 [details] [diff] [review]
Part1 - v2

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

::: dom/system/gonk/ril_worker.js
@@ +1602,5 @@
>      Buf.simpleRequest(REQUEST_BASEBAND_VERSION);
>    },
>  
> +  // Cache requests for making an emergency call until radio state is ready.
> +  _dialEmergencyQueue : [],

A queue seems not neccesary.

@@ +1616,5 @@
>     *        Integer doing something XXX TODO
>     */
>    dial: function dial(options) {
> +    if (this._isEmergencyNumber(options.number)) {
> +      this._dialEmergency(options);

dialEmergencyNumber,
as we need to tell it from 'dial emergency call'

@@ +1618,5 @@
>    dial: function dial(options) {
> +    if (this._isEmergencyNumber(options.number)) {
> +      this._dialEmergency(options);
> +    } else {
> +      this._dial(options);

I found there are many error handling share the same code,
you could add an 'onerror' parameter to these two dial functions, and move the common code here.

@@ +1650,5 @@
> +  _dialEmergency: function _dialEmergency(options) {
> +    let request = REQUEST_DIAL;
> +    if (RILQUIRKS_REQUEST_USE_DIAL_EMERGENCY_CALL) {
> +      request = REQUEST_DIAL_EMERGENCY_CALL;
> +    }

let request = RILQUIRKS_REQUEST_USE_DIAL_EMERGENCY_CALL? REQUEST_DIAL_EMERGENCY_CALL : REQUEST_DIAL;

@@ +1660,3 @@
>          options.callIndex = -1;
>          options.rilMessageType = "callError";
> +        options.errorMsg = "AirplaneMode";

errorMsg should be provided by caller, also the message here seems not so right.

@@ +1682,5 @@
> +
> +    this._sendDialRequest(request, options);
> +  },
> +
> +  _sendDialRequest: function _sendDialRequest(request, options) {

We don't have to prefix it with _.
Also can we add request into options?

@@ +4562,5 @@
>    obj.rilMessageType = "signalstrengthchange";
>    this.sendDOMMessage(obj);
> +
> +  if (this._dialEmergencyQueue.length) {
> +    if (obj.gsmDBM != 0 && obj.gsmRelative != 0) {

nit, if (.. && .. && ..)

Comment 18

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

(In reply to Yoshi Huang[:allstars.chh][:yoshi] from comment #17)
> Comment on attachment 726549 [details] [diff] [review]
> Part1 - v2
> 
> Review of attachment 726549 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: dom/system/gonk/ril_worker.js
> @@ +1602,5 @@
> >      Buf.simpleRequest(REQUEST_BASEBAND_VERSION);
> >    },
> >  
> > +  // Cache requests for making an emergency call until radio state is ready.
> > +  _dialEmergencyQueue : [],
> 
> A queue seems not neccesary.
> 

Agree. Revision is coming soon.

> @@ +1616,5 @@
> >     *        Integer doing something XXX TODO
> >     */
> >    dial: function dial(options) {
> > +    if (this._isEmergencyNumber(options.number)) {
> > +      this._dialEmergency(options);
> 
> dialEmergencyNumber,
> as we need to tell it from 'dial emergency call'
> 

OK!

> @@ +1618,5 @@
> >    dial: function dial(options) {
> > +    if (this._isEmergencyNumber(options.number)) {
> > +      this._dialEmergency(options);
> > +    } else {
> > +      this._dial(options);
> 
> I found there are many error handling share the same code,
> you could add an 'onerror' parameter to these two dial functions, and move
> the common code here.
>

Sure.
 
> @@ +1650,5 @@
> > +  _dialEmergency: function _dialEmergency(options) {
> > +    let request = REQUEST_DIAL;
> > +    if (RILQUIRKS_REQUEST_USE_DIAL_EMERGENCY_CALL) {
> > +      request = REQUEST_DIAL_EMERGENCY_CALL;
> > +    }
> 
> let request = RILQUIRKS_REQUEST_USE_DIAL_EMERGENCY_CALL?
> REQUEST_DIAL_EMERGENCY_CALL : REQUEST_DIAL;
> 
> @@ +1660,3 @@
> >          options.callIndex = -1;
> >          options.rilMessageType = "callError";
> > +        options.errorMsg = "AirplaneMode";
> 
> errorMsg should be provided by caller, also the message here seems not so
> right.
> 

I will let errorMsg provided by caller as you suggested. I will also change the errorMsg from 'AirplaneMode' to 'RadioNotAvailable', since that is already defined in ril_const.js. 

Though the error happens in request_radio_power, but I think the errorMsg should be 'RadioNotAvailable' because this is the callback of dialing, so that we should give a specific message indicating why this call fails. And here, the call fail is caused by inavailability of radio power. 

Thank you!

Comment 19

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

Comment on attachment 726549 [details] [diff] [review]
Part1 - v2

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

::: dom/system/gonk/ril_worker.js
@@ +1618,5 @@
>    dial: function dial(options) {
> +    if (this._isEmergencyNumber(options.number)) {
> +      this._dialEmergency(options);
> +    } else {
> +      this._dial(options);

I would suggest rename _dial to dialNonEmergencyNumber to make it more clear.

Comment 20

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

Attachment: Part1 - v3

Comment 21

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

Comment on attachment 727029 [details] [diff] [review]
Part1 - v3

Addressed comment#17 and comment#19. Thanks!

Comment 22

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

Attachment: Part1 - v3

Addressed comment#17 and comment#19. Thanks!

Comment 23

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

Comment on attachment 727031 [details] [diff] [review]
Part1 - v3

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

::: dom/system/gonk/ril_worker.js
@@ +4551,5 @@
>    if (DEBUG) debug("Signal strength " + JSON.stringify(obj));
>    obj.rilMessageType = "signalstrengthchange";
>    this.sendDOMMessage(obj);
> +
> +  if (this.cachedDialRequest && obj.gsmDBM != 0 && obj.gsmRelative != 0) {

Do we still need != 0 check?

Comment 24

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

Attachment: Part1 - v4

Addressed comment#23. r=allstars.chh

Comment 25

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

https://hg.mozilla.org/integration/mozilla-inbound/rev/44b99b1c798c
https://hg.mozilla.org/integration/mozilla-inbound/rev/e34554f9ec37

Comment 26

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

Summary of the patch:
1) if the number is emergency number and radio is off, the backend tries to enable radio and make the call when the radio is ready. If it fails in turning on the radio, the backend sends 'RadioNotAvailable' callerror to DOM.
2) if we want to make a non-emergency call when radio is off, 'RadioNotAvailable' callerror will be sent.

Comment 27

[Edmund Wong (:ewong)]

(In reply to Hsin-Yi Tsai (afk till March 28) [:hsinyi] from comment #25)
> https://hg.mozilla.org/integration/mozilla-inbound/rev/44b99b1c798c
> https://hg.mozilla.org/integration/mozilla-inbound/rev/e34554f9ec37

I backed these out due to marionette oranges.

Comment 28

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

Attachment: Part2 - v3

Modified document permission. r=allstars.chh

Comment 29

[Lucas Adamski [:ladamski]]

Did you mean to remove the blocker?  Otherwise blocks a blocker.

Comment 31

[Anshul]

@Hsin-Yi, is there a corresponding Gaia change to not block the calls when in airplane mode?

Comment 32

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

(In reply to Anshul from comment #31)
> @Hsin-Yi, is there a corresponding Gaia change to not block the calls when
> in airplane mode?

Yes, there is! We should make some modifications in [1]. Replacing that line with "startDial()" should work that's how I tested my patch.

[1] https://github.com/mozilla-b2g/gaia/blob/master/apps/communications/dialer/js/telephony_helper.js#L28

Comment 33

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

(In reply to Edmund Wong (:ewong) from comment #27)
> (In reply to Hsin-Yi Tsai (afk till March 28) [:hsinyi] from comment #25)
> > https://hg.mozilla.org/integration/mozilla-inbound/rev/44b99b1c798c
> > https://hg.mozilla.org/integration/mozilla-inbound/rev/e34554f9ec37
> 
> I backed these out due to marionette oranges.

Bug 853024 is mentioning instability of some B2G emulator tests and the above inboud pushes are hidden because of that. I'll push another try.

Comment 34

[Anshul]

Hsin-Yi, could you please point me to the bug number for the corresponding Gaia change. I reopened bug 839363 as that seemed like the gaia bug for this issue. I have a patch for the change and will upload it shortly.

Comment 35

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

Try looks good https://tbpl.mozilla.org/?tree=Try&rev=bca2c1462071

Comment 36

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

https://hg.mozilla.org/integration/mozilla-inbound/rev/c59a33280850
https://hg.mozilla.org/integration/mozilla-inbound/rev/2c10a759015f

Comment 37

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/c59a33280850
https://hg.mozilla.org/mozilla-central/rev/2c10a759015f

Comment 38

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-b2g18/rev/db30caa6d1c5
https://hg.mozilla.org/releases/mozilla-b2g18/rev/243cfff1150e

Comment 39

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

Attachment: Followup for b2g18 - make naming consistant

Comment 40

[Anshul]

Hsin-Yi, you are changing the json string to notify of call error, anytime you make such changes it affects us. Please let us know when you make such changes so we can update the commercial RIL too.

Comment 41

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

(In reply to Anshul from comment #40)
> Hsin-Yi, you are changing the json string to notify of call error, anytime
> you make such changes it affects us. Please let us know when you make such
> changes so we can update the commercial RIL too.

Anshul, yes, there's a followup patch that needs to be checked in b2g18. I have checked the procedure with PM and I was told that since this patch doesn't change interface, so I can directly merge it. That's why I didn't notify you. 

I'll add [NO_UPLIFT] whiteboard and won't check the patch in until you remove that.

Comment 42

[Anshul]

(In reply to Hsin-Yi Tsai [:hsinyi] from comment #41)
> (In reply to Anshul from comment #40)
> > Hsin-Yi, you are changing the json string to notify of call error, anytime
> > you make such changes it affects us. Please let us know when you make such
> > changes so we can update the commercial RIL too.
> 
> Anshul, yes, there's a followup patch that needs to be checked in b2g18. I
> have checked the procedure with PM and I was told that since this patch
> doesn't change interface, so I can directly merge it. That's why I didn't
> notify you. 
> 

Could you please point me to the bug for the followup patch you are talking about here?

> I'll add [NO_UPLIFT] whiteboard and won't check the patch in until you
> remove that.

Thanks Hsin-Yi. The problem is that the Json messages like these are technically not part of an IDL files but is still sort of a contract between what RilContentHelper.js expects from gecko. I am not sure how to catch these type of changes automatically so for now please keep in mind when you guys make such a change.

This is a pretty simple change and so we would be able to adapt very quickly, you don't even need a NO_UPLIFT here as this won't really crash anything.

Comment 43

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

(In reply to Anshul from comment #42)
> (In reply to Hsin-Yi Tsai [:hsinyi] from comment #41)
> > (In reply to Anshul from comment #40)
> > > Hsin-Yi, you are changing the json string to notify of call error, anytime
> > > you make such changes it affects us. Please let us know when you make such
> > > changes so we can update the commercial RIL too.
> > 
> > Anshul, yes, there's a followup patch that needs to be checked in b2g18. I
> > have checked the procedure with PM and I was told that since this patch
> > doesn't change interface, so I can directly merge it. That's why I didn't
> > notify you. 
> > 
> 
> Could you please point me to the bug for the followup patch you are talking
> about here?

Bug 854935 is the original report.
Attachment 736725 [details] [diff] is going to be checked into b2g18.

> 
> > I'll add [NO_UPLIFT] whiteboard and won't check the patch in until you
> > remove that.
> 
> Thanks Hsin-Yi. The problem is that the Json messages like these are
> technically not part of an IDL files but is still sort of a contract between
> what RilContentHelper.js expects from gecko. I am not sure how to catch
> these type of changes automatically so for now please keep in mind when you
> guys make such a change.

Sure, thanks for reminding!

> 
> This is a pretty simple change and so we would be able to adapt very
> quickly, you don't even need a NO_UPLIFT here as this won't really crash
> anything.

Comment 44

[Anshul]

On careful examination I realized that there are other places in the code besides the one you are fixing that sends "callError" message that uses "error" instead of "errorMsg". Instead of fixing the RILContentHelper can we not fix the only place that uses "errorMsg" as every other place uses "error" already.

Comment 45

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

(In reply to Anshul from comment #44)
> On careful examination I realized that there are other places in the code
> besides the one you are fixing that sends "callError" message that uses
> "error" instead of "errorMsg". Instead of fixing the RILContentHelper can we
> not fix the only place that uses "errorMsg" as every other place uses
> "error" already.

The reason I decided to rename the several places to 'errorMsg' is making the naming consistant with current m-c, in order to help decrease inconsistency for any possible future patches.

Comment 46

[Etienne Segonzac (:etienne)]

I'm confused, did the followup reached b2g18?

Comment 47

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

(In reply to Anshul from comment #42)
> 
> This is a pretty simple change and so we would be able to adapt very
> quickly, you don't even need a NO_UPLIFT here as this won't really crash
> anything.

Clearing NO_UPLIFT as Anshul's comment here.

Comment 48

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

Comment on attachment 736725 [details] [diff] [review]
Followup for b2g18 - make naming consistant

Asking for check-in to b2g18 only.

Comment 49

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

Attachment: Followup for b2g18 - make naming consistant. r=allstars.chh

Comment 50

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

Comment on attachment 737824 [details] [diff] [review]
Followup for b2g18 - make naming consistant. r=allstars.chh

Asking for check-in to b2g18 only.

Comment 51

[Ryan VanderMeulen [:RyanVM]]

Comment on attachment 737824 [details] [diff] [review]
Followup for b2g18 - make naming consistant. r=allstars.chh

https://hg.mozilla.org/releases/mozilla-b2g18/rev/84f0cf39e784