828160 - B2G RIL: should not send duplicate incoming-call messages in some cases of 'waiting call coming'

Status: VERIFIED FIXED

(Firefox OS Graveyard :: General, defect)

Description

[Shawn Huang [:shawnjohnjr][:shuang] (as a happy gecko contributor)]

AT command CCWA and CIEV:4,1 shall be stopped sending after received CHLD=1. This might breaks PTS TWC_BV_02_I.

Comment 1

[Shawn Huang [:shawnjohnjr][:shuang] (as a happy gecko contributor)]

Attachment: HCI trace of unagi with PTS test cases three way calling

Comment 2

[Shawn Huang [:shawnjohnjr][:shuang] (as a happy gecko contributor)]

	144	Slave	46	AT+CCWA=1.	Enable call waiting notification			22	 00:00:00.000000	2013/1/9 下午 03:22:04.034000	
	145	Master	46	..OK..			Success	18	 00:00:00.016000	2013/1/9 下午 03:22:04.050000	
	147	Slave	46	AT+CMEE=1.	Enable Extended Audio Gateway Error Result Code			22	 00:00:00.016000	2013/1/9 下午 03:22:04.066000	
	149	Master	46	..OK..			Success	18	 00:00:00.015000	2013/1/9 下午 03:22:04.081000	
	152	Master	46	..+CIEV: 4,1..		Call Setup indicator's status report		26	 00:00:43.680000	2013/1/9 下午 03:22:47.761000	
	153	Master	46	..RING..		Incoming Call/Call progress indication		20	 00:00:02.995000	2013/1/9 下午 03:22:50.756000	
	154	Master	46	..+CLIP: "0988095164",129..	+CLIP: "0988095164",129			39	 00:00:00.000000	2013/1/9 下午 03:22:50.756000	
	156	Slave	46	ATA.		Answer mode		16	 00:00:00.032000	2013/1/9 下午 03:22:50.788000	
	159	Master	46	..OK..			Success	18	 00:00:00.093000	2013/1/9 下午 03:22:50.881000	
	164	Master	46	..+CIEV: 2,1..		Call Status indicator's status report		26	 00:00:00.406000	2013/1/9 下午 03:22:51.287000	
	165	Master	46	..+CIEV: 4,0..		Call Setup indicator's status report		26	 00:00:00.000000	2013/1/9 下午 03:22:51.287000	
	968	Master	46	..+CCWA: "0939835820",129..		Type: 129		39	 00:00:35.973000	2013/1/9 下午 03:23:27.260000	
	969	Master	46	..+CIEV: 4,1..		Call Setup indicator's status report		26	 00:00:00.000000	2013/1/9 下午 03:23:27.260000	
	1,160	Slave	46	AT+CHLD=1.	Execute "Call hold and multiparty" Command			22	 00:00:08.487000	2013/1/9 下午 03:23:35.747000	
	1,164	Master	46	..OK..			Success	18	 00:00:00.062000	2013/1/9 下午 03:23:35.809000	
	1,173	Master	46	..+CCWA: "0939835820",129..		Type: 129		39	 00:00:00.421000	2013/1/9 下午 03:23:36.230000	
	1,174	Master	46	..+CIEV: 4,1..		Call Setup indicator's status report		26	 00:00:00.000000	2013/1/9 下午 03:23:36.230000	
	1,181	Master	46	..+CIEV: 2,0..		Call Status indicator's status report		26	 00:00:00.047000	2013/1/9 下午 03:23:36.277000	
	1,194	Master	46	..+CIEV: 2,1..		Call Status indicator's status report		26	 00:00:00.484000	2013/1/9 下午 03:23:36.761000	
	1,195	Master	46	..+CIEV: 4,0..		Call Setup indicator's status report		26	 00:00:00.000000	2013/1/9 下午 03:23:36.761000	
	1,199	Slave	46	AT+CHUP.	Terminate the call			20	 00:00:00.093000	2013/1/9 下午 03:23:36.854000	
	1,203	Master	46	..OK..			Success	18	 00:00:00.047000	2013/1/9 下午 03:23:36.901000	
	1,232	Master	46	..+CIEV: 2,0..		Call Status indicator's status report		26	 00:00:01.311000	2013/1/9 下午 03:23:38.212000

Comment 3

[Gina Yeh [:gyeh] [:ginayeh]]

I traced this issue and found we did receive call state change of one incoming call twice from RIL. Talked with Hsin-yi, and she will help on this issue.

Comment 4

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

Gina, thanks for reporting this. I'll take this and change the bug summary to fit the issue explicitly!

Comment 5

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

STR:
1. make the first call connected, then the 3rd party calls the unagi device
2. hangup the first call
3. accept the 2nd call, i.e. the call the 3rd party made

Expected:
Only one incoming message is sent from RadioInterfaceLayer

Actual:
2 incoming messages are sent

Comment 6

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

Attachment: Patch

Since 'incoming' and 'waiting' states are viewed as the same one in DOM, we don't notify DOM the call state change among these two.

Comment 7

[Shawn Huang [:shawnjohnjr][:shuang] (as a happy gecko contributor)]

This could impact Bluetooth three way-calling feature seriously.
It impacts Bluetooth certification and violate Bluetooth Handsfree specification.

User story impacts:
(1) User might encounter three-way calling failure with car kit system, which cause driving safety.
(2) AT command +CIEV was sent duplicated might create call status inside car kit mismatch, this could lead many unexpected bugs.

If the patch is safe, I really hope RIL can phase in this patch.

Comment 8

[Shawn Huang [:shawnjohnjr][:shuang] (as a happy gecko contributor)]

Additional information.
This blocked Bluetooth Handsfree certification "AG_TWC" conference call whole series test cases.
Suggest to move to BB+.

Comment 9

[Doug Turner (:dougt)]

not blocking.  please request uplift on the patch if it is safe.

Comment 10

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

Comment on attachment 700183 [details] [diff] [review]
Patch

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

::: dom/system/gonk/ril_worker.js
@@ +3025,5 @@
>            // State has changed.
> +          if (newCall.state == CALL_STATE_INCOMING &&
> +              currentCall.state == CALL_STATE_WAITING) {
> +            // Update the call internally but we don't notify DOM since these
> +            // two states are viewed as the same one there.

Seems your check should be done in line 3037 ?

Comment 11

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

(In reply to Yoshi Huang[:yoshi][:allstars.chh] from comment #10)
> Comment on attachment 700183 [details] [diff] [review]
> Patch
> 
> Review of attachment 700183 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: dom/system/gonk/ril_worker.js
> @@ +3025,5 @@
> >            // State has changed.
> > +          if (newCall.state == CALL_STATE_INCOMING &&
> > +              currentCall.state == CALL_STATE_WAITING) {
> > +            // Update the call internally but we don't notify DOM since these
> > +            // two states are viewed as the same one there.
> 
> Seems your check should be done in line 3037 ?

In line#3037, we call _handleChangedCallState() to notify RadioInterfaceLayer/DOM the call state been changed. However, in this special case, we just need to update the call state in ril_worker internally without notifying DOM. So we break before #3037.

Comment 12

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

Comment on attachment 700183 [details] [diff] [review]
Patch

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

::: dom/system/gonk/ril_worker.js
@@ +3033,5 @@
>            if (!currentCall.started && newCall.state == CALL_STATE_ACTIVE) {
>              currentCall.started = new Date().getTime();
>            }
>            currentCall.state = newCall.state;
>            this._handleChangedCallState(currentCall);

I mean 
you do check here,

if (newCall.state != CALL_STATE_INCOMING || 
    currentCall.state != CALL_STATE_WAITING) {
  this._handleChangedCallState(currentCall);
}

That makes code easier to read. Unless you have better reason to use break in line 3031

Comment 13

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

Comment on attachment 700183 [details] [diff] [review]
Patch

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

The code here is totally hard to read, 
Do you think if you can also refine those codes?

at least try no to use 3 levels of if
Very haaaaaaaaaaaaaaaaaaaaaaard to understand.

And use early return as possible.

Comment 14

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

Comment on attachment 700183 [details] [diff] [review]
Patch

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

::: dom/system/gonk/ril_worker.js
@@ +3018,5 @@
>          newCall = newCalls[currentCall.callIndex];
>          delete newCalls[currentCall.callIndex];
>        }
>  
>        if (newCall) {

why another if (newCall),

Can we 
if (!newCall) {
  //do the else part;
  continue;
}

so 1 level if is removed,

@@ +3020,5 @@
>        }
>  
>        if (newCall) {
>          // Call is still valid.
>          if (newCall.state != currentCall.state) {

if (newCall.state != currentCall.state) {
  continue;
}

two levels if remvoed.

Comment 15

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

Attachment: Patch (v2)

Since 'incoming' and 'waiting' states are viewed as the same one in DOM, we don't notify DOM the call state change among these two.

The revision flattens the code structure to improve readability as Yoshi's comment.

Comment 16

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

Comment on attachment 700811 [details] [diff] [review]
Patch (v2)

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

Great!

This time I just spent less in 1 minute and can understand your code very clearly.

::: dom/system/gonk/ril_worker.js
@@ +3036,5 @@
> +      if (newCall.state == CALL_STATE_INCOMING &&
> +          currentCall.state == CALL_STATE_WAITING) {
> +        // Update the call internally but we don't notify DOM since these two
> +        // states are viewed as the same one there.
> +        currentCall.state = newCall.state;

Can we move this line to line 3034?

@@ +3043,5 @@
> +
> +      if (!currentCall.started && newCall.state == CALL_STATE_ACTIVE) {
> +        currentCall.started = new Date().getTime();
> +      }
> +      currentCall.state = newCall.state;

Can we move this line to line 3034?

Comment 17

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

Comment on attachment 700811 [details] [diff] [review]
Patch (v2)

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

::: dom/system/gonk/ril_worker.js
@@ +3036,5 @@
> +      if (newCall.state == CALL_STATE_INCOMING &&
> +          currentCall.state == CALL_STATE_WAITING) {
> +        // Update the call internally but we don't notify DOM since these two
> +        // states are viewed as the same one there.
> +        currentCall.state = newCall.state;

No, we can't because we need to check first (line#3036) before updating the currentCall.

@@ +3043,5 @@
> +
> +      if (!currentCall.started && newCall.state == CALL_STATE_ACTIVE) {
> +        currentCall.started = new Date().getTime();
> +      }
> +      currentCall.state = newCall.state;

We can't. Reason same as above!

Comment 18

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

(In reply to Hsin-Yi Tsai [:hsinyi] from comment #17)
> Comment on attachment 700811 [details] [diff] [review]
> Patch (v2)
> 
> Review of attachment 700811 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: dom/system/gonk/ril_worker.js
> @@ +3036,5 @@
> > +      if (newCall.state == CALL_STATE_INCOMING &&
> > +          currentCall.state == CALL_STATE_WAITING) {
> > +        // Update the call internally but we don't notify DOM since these two
> > +        // states are viewed as the same one there.
> > +        currentCall.state = newCall.state;
> 
> No, we can't because we need to check first (line#3036) before updating the
> currentCall.
> 
This is the reply to Comment 16.
> @@ +3043,5 @@
> > +
> > +      if (!currentCall.started && newCall.state == CALL_STATE_ACTIVE) {
> > +        currentCall.started = new Date().getTime();
> > +      }
> > +      currentCall.state = newCall.state;
> 
> We can't. Reason same as above!

Comment 19

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

Comment on attachment 700811 [details] [diff] [review]
Patch (v2)

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

::: dom/system/gonk/ril_worker.js
@@ +3036,5 @@
> +      if (newCall.state == CALL_STATE_INCOMING &&
> +          currentCall.state == CALL_STATE_WAITING) {
> +        // Update the call internally but we don't notify DOM since these two
> +        // states are viewed as the same one there.
> +        currentCall.state = newCall.state;

But why? if the check is false the currentCall.state will still be set in line 3047

Comment 20

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

Comment on attachment 700811 [details] [diff] [review]
Patch (v2)

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

::: dom/system/gonk/ril_worker.js
@@ +3036,5 @@
> +      if (newCall.state == CALL_STATE_INCOMING &&
> +          currentCall.state == CALL_STATE_WAITING) {
> +        // Update the call internally but we don't notify DOM since these two
> +        // states are viewed as the same one there.
> +        currentCall.state = newCall.state;

Please ignore this comment, as I found out I misunderstand.

Comment 21

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

(In reply to Yoshi Huang[:yoshi][:allstars.chh] from comment #20)
> Comment on attachment 700811 [details] [diff] [review]
> Patch (v2)
> 
> Review of attachment 700811 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: dom/system/gonk/ril_worker.js
> @@ +3036,5 @@
> > +      if (newCall.state == CALL_STATE_INCOMING &&
> > +          currentCall.state == CALL_STATE_WAITING) {
> > +        // Update the call internally but we don't notify DOM since these two
> > +        // states are viewed as the same one there.
> > +        currentCall.state = newCall.state;
> 
> Please ignore this comment, as I found out I misunderstand.
No problem! Thanks for the review. :)

Comment 22

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

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

Comment 23

[Ed Morley [:emorley]]

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

Comment 24

[Gina Yeh [:gyeh] [:ginayeh]]

I'd like to nominate this bug as tracking-b2g+ since it has impact on Bluetooth hands-free profile, and we failed to pass HFP certification without this patch.

Comment 25

[Gina Yeh [:gyeh] [:ginayeh]]

Comment on attachment 700811 [details] [diff] [review]
Patch (v2)

NOTE: Please see https://wiki.mozilla.org/Release_Management/B2G_Landing to better understand the B2G approval process and landings.

[Approval Request Comment]
Bug caused by (feature/regressing bug #): none
User impact if declined: incorrect call states are sent from our phone to bluetooth headset, and the bluetooth headset may not function correctly
Testing completed: mozilla-central
Risk to taking this patch (and alternatives if risky): Fairly low. The only thing done in this patch is to avoid send duplicate incoming-call messages from RIL
String or UUID changes made by this patch: no

Comment 26

[Ryan VanderMeulen [:RyanVM]]

Probably would have helped if I'd included a changeset URL.
https://hg.mozilla.org/releases/mozilla-b2g18/rev/6060dbaff770

Comment 27

[Kai-Chih Hu [:khu]]

This bug(and other 9 bugs) block Bluetooth certification. So, these bugs need to be marked as tef+ and landed to v1.0.1 in order to pass Bluetooth certification: 

bug 827255
bug 827212
bug 827266
bug 828175
bug 823346
bug 827230
bug 828798
bug 835740
bug 846647
bug 828160

So, mark these bugs as tef?. These fixes have some dependency and it would be better to have them landed in a specific order. Gina has a good view on this.

Comment 28

[Gina Yeh [:gyeh] [:ginayeh]]

I've rebased all these patches on v1_0_1, and some bugs which are also mandatory for Bluetooth Hfp certification were missed in the list above:

bug 827204
bug 825861
bug 825851

(I've nominated them as tef?)

Comment 29

[Gina Yeh [:gyeh] [:ginayeh]]

I recommend to land these patches in the following order:

01. bug827204
02. bug827255
03. bug823346
04. bug827230
05. bug828798
06. bug827212, patch 1
06. bug827212, patch 2
07. bug827266
08. bug828175
09. bug846647
10. bug825861
11. bug825851
12. bug835740




I'm going to attach patches for b2g18_v1_0_1 for each bug. Please land them in the above order. There should be no conflict and feel free to let me know if I can be any help.

Comment 30

[Gina Yeh [:gyeh] [:ginayeh]]

Hsinyi, can you help to generate a b2g18_v1_0_1 patch for this bug? Thanks.

Comment 31

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

(In reply to Gina Yeh [:gyeh] [:ginayeh] from comment #30)
> Hsinyi, can you help to generate a b2g18_v1_0_1 patch for this bug? Thanks.

Hi Gina, the attachment #700811 [details] [diff] [review] resolves no conflict for b2g18_v1_0_1. You can simply push that. :)

Comment 32

[Gina Yeh [:gyeh] [:ginayeh]]

Cool! Thanks, Hsinyi~

Comment 33

[Joe Cheng [:jcheng] (please needinfo)]

tef- for now until tef release partner confirms this is blocking BT cert.

Comment 34

[Joe Cheng [:jcheng] (please needinfo)]

this will block tef bt cert. tef?

Comment 35

[Kai-Chih Hu [:khu]]

Can we push the codes to v1.0.1 as soon as possible? Thanks!

Comment 36

[Kai-Chih Hu [:khu]]

Hi, Ryan, are you the person who could push the codes to v1.0.1? Can you help? 
Thank you.

Comment 37

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-b2g18_v1_0_1/rev/23e1d9f575ca

Comment 38

[Kai-Chih Hu [:khu]]

Thanks, Ryan.

Comment 39

[Shawn Huang [:shawnjohnjr][:shuang] (as a happy gecko contributor)]

Hi Michael,
Is this on your radar? Will QC RIL have the same problem? I hear partner also reports this problem. But I guess they used QC RIL.

Comment 40

[Michael Vines [:m1] [:evilmachines]]

Thanks for the heads up Shawn!  We'll take a look.

Comment 41

[Shawn Huang [:shawnjohnjr][:shuang] (as a happy gecko contributor)]

Thanks, Michael. Since this blocks BT certification you might put this as higher priority.

Comment 42

[Shawn Huang [:shawnjohnjr][:shuang] (as a happy gecko contributor)]

Hi anshulj,
Any update?

Comment 43

[Michael Vines [:m1] [:evilmachines]]

Clearing ni as we're following up directly with the customer, so there's no action here.

Comment 44

[Anshul]

Shawn, fix for this in commercial RIL is available in AU_LINUX_GECKO_ICS_STRAWBERRY_V1.01.00.01.019.108

Comment 45

[Sarah Parsons]

Verified fixed on Inari 1.0.1 Build ID: Build ID: 20130528231041

Environmental  Variables:
Kernel Date: May 28
Gecko: /rev/
Gaia: 6d1008a5f7b4509dd84eca47d036bc65c462714a

also verified on the Unagi 1.1 Build ID: 20130531070205

Environmental  Variables:
Kernel Date: Dec 5
Gecko: http://hg.mozilla.org/releases/mozilla-b2g18/rev/4f318822e72c
Gaia: e1c59baed29e4665d1da9392f86239272073f07a