1169409 - GCF test case 31.4.4.1.2.4 failure : Unwanted call retrieve after retrieve reject

Status: RESOLVED FIXED

(Firefox OS Graveyard :: Gaia::Dialer, defect)

Description

[pgravel]

Steps to reproduce:
1. UE A has a MultiParty call to two destinations (A-B and A-C) both in state U10 "Active" with auxiliary state "Call in MPTY", it has an addition a single call (A-D) in state U10 "Active" with auxiliary state "Call held".
2. Multiparty call is cleared by dialing end call, calls A-B and A-C are now released.
3. UE tries to retrieve the held A-D call, N/W rejects it.
4. UE sends Retrieve again which is unexpected 


This is a bit of a tricky bug that may have some significant repercussion.
Here's a quick overview of the issue
* Gaia currently handles resuming held calls after hanging up the foreground call.
* With bug 978639 fixed, the phone will try calling RIL_REQUEST_HANGUP_FOREGROUND_RESUME_BACKGROUND to hangup a foreground conference call.
* As the name implies that ril request automatically resumes the background call. This means that both gaia and telephony layers try to resume the background call, which causes the GCF test to fail.

The suggested approach to resolve this would be to simplify the Gaia logic and let telephony handle resuming the call as needed. This would make it much easier to satisfy all the various GCF requirements and allows telephony to better control the flow of the calls. This would also prevent issues like bug 1166635 from popping up.

Comment 1

[Johan Lorenzo [:jlorenzo]]

Comms triage: Certification blocker

Comment 2

[Josh Cheng [:josh]]

Hi Dylan,
Can you help to check with Qualcomm about this bug? Why this haven't blocked GCF for previous release? Is this a regression?

Comment 3

[Johan Lorenzo [:jlorenzo]]

This is not a regression. Before 2.2, we couldn't put call on hold via the UI. The UI part was implemented in bug 977588.

Comment 4

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

Hi Aknow and Gabriele,
Phil's  comment 0 sounds reasonable to me but that impacts both gecko and gaia. Could you review his suggestion and see if it's also okay to you? Thank you.

Comment 5

[Gabriele Svelto [:gsvelto]]

Phil is right, in gaia when a call is disconnected we always check if there's another call and if it's held we try to resume it, see here:

https://github.com/mozilla-b2g/gaia/blob/246704817733b93b6f59c5d442809e21a9319dbe/apps/callscreen/js/calls_handler.js#L231

I had no idea that when hanging up a conference call this would be done automatically. As usual with these things either we do something explicitly in gaia or we delegate it to gecko as doing it both ways always puts us in trouble :) We've established a precedent for this in bug 889737 whereas previously we relied on gaia to put a call on hold before dialing a new one and then we decided this would be delegated to gecko to avoid errors. I suppose that the same reasoning applies here (i.e. gecko is the best place to decided whether we should resume or not the remaining call) in which case I can remove the offending code from gaia.

Comment 6

[Josh Cheng [:josh]]

Hi Aknow,
Do you have other opinion about comment from Phil?
Thanks!

Comment 7

[Szu-Yu Chen [:aknow] (inactive)]

(In reply to Josh Cheng [:josh] from comment #6)
> Hi Aknow,
> Do you have other opinion about comment from Phil?
> Thanks!

Sounds good, but I can't see any impacts to gecko. As Gabriele said, all we need to do is removing the code for resuming the call from gaia.

Comment 8

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

(In reply to Szu-Yu Chen [:aknow] from comment #7)
> (In reply to Josh Cheng [:josh] from comment #6)
> > Hi Aknow,
> > Do you have other opinion about comment from Phil?
> > Thanks!
> 
> Sounds good, but I can't see any impacts to gecko. As Gabriele said, all we
> need to do is removing the code for resuming the call from gaia.

Removing the code from gaia could fix this GCF test failure. But I think we will need to handle "automatically resume a remaining call" on gecko, otherwise after the removing, the UI behaviour changes that might be seen as a regression. Am I understanding the thing correctly, Aknow?

Comment 9

[Gabriele Svelto [:gsvelto]]

(In reply to Hsin-Yi Tsai [:hsinyi] from comment #8)
> Removing the code from gaia could fix this GCF test failure. But I think we
> will need to handle "automatically resume a remaining call" on gecko,
> otherwise after the removing, the UI behaviour changes that might be seen as
> a regression. Am I understanding the thing correctly, Aknow?

Yes, that's my feeling too. Naturally I could special-case this in gaia but you know how much I loathe adding unnecessary complexity :)

Comment 10

[Szu-Yu Chen [:aknow] (inactive)]

(In reply to Hsin-Yi Tsai [:hsinyi] from comment #8)
> (In reply to Szu-Yu Chen [:aknow] from comment #7)
> > (In reply to Josh Cheng [:josh] from comment #6)
> > > Hi Aknow,
> > > Do you have other opinion about comment from Phil?
> > > Thanks!
> > 
> > Sounds good, but I can't see any impacts to gecko. As Gabriele said, all we
> > need to do is removing the code for resuming the call from gaia.
> 
> Removing the code from gaia could fix this GCF test failure. But I think we
> will need to handle "automatically resume a remaining call" on gecko,
> otherwise after the removing, the UI behaviour changes that might be seen as
> a regression. Am I understanding the thing correctly, Aknow?

From comment 0, in gecko, we use RIL_REQUEST_HANGUP_FOREGROUND_RESUME_BACKGROUND to hangup a foreground conference call. Doesn't that mean we have already handle the resuming work?

Comment 11

[Szu-Yu Chen [:aknow] (inactive)]

OK. There is one thing we haven't done in gecko. Assume there are two single calls, active and held. Hangup the active one should automatically resume the held one. <= we don't have this part now.

Comment 12

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

(In reply to Szu-Yu Chen [:aknow] from comment #11)
> OK. There is one thing we haven't done in gecko. Assume there are two single
> calls, active and held. Hangup the active one should automatically resume
> the held one. <= we don't have this part now.

Yes, this is the part I was talking about.

Comment 13

[Josh Cheng [:josh]]

Hi HsinYi,
Do you have any update here? Do we still consider fixing this for 2.2?
Thanks

Comment 14

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

(In reply to Josh Cheng [:josh] from comment #13)
> Hi HsinYi,
> Do you have any update here? Do we still consider fixing this for 2.2?
> Thanks

Yes, I think so. In addition to this gaia part, we should have gecko bug 1174673 fixed.

Comment 15

[Gabriele Svelto [:gsvelto]]

(In reply to Hsin-Yi Tsai [:hsinyi] from comment #14)
> Yes, I think so. In addition to this gaia part, we should have gecko bug
> 1174673 fixed.

I'll post the gaia part shortly then, we'll land it when the gecko bits are in place to avoid regressions.

Comment 16

[Dylan Oliver [:doliver]]

ni? to Gabriele as a reminder for comment #15.

Comment 17

[Gabriele Svelto [:gsvelto]]

Taking this.

Comment 18

[Gabriele Svelto [:gsvelto]]

Attachment: [PULL REQUEST] Do not explicitly resume the remaining call when the first one is hung up

Super-simple patch that removes the related bit of code and the accompanying unit-tests.

Comment 19

[Josh Cheng [:josh]]

Hi Tamara,
Could you help to review the patch from Gabriele?
Thanks!

Comment 20

[Tamara Hills [:thills]]

Comment on attachment 8624136 [details] [review]
[PULL REQUEST] Do not explicitly resume the remaining call when the first one is hung up

Hi Gabriele,

It looks good to me.

Thanks,

-tamara

Comment 21

[Gabriele Svelto [:gsvelto]]

Merged to gaia/master b901c8b7be2119f4df42781aac1401ed12765460

https://github.com/mozilla-b2g/gaia/commit/b901c8b7be2119f4df42781aac1401ed12765460

Comment 22

[Hermes Cheng (inactive after July 27, 2015)]

Gabriele, could you also uplift to v2.2 since it is a 2.2 blocker?

Comment 23

[Gabriele Svelto [:gsvelto]]

Bug 1174673 hasn't been uplifted yet, I'd wait for that since it's a dependency before uplifting this one. That being said, aren't we already past the FC date for v2.2? I'd just like to know if it's still OK to land on that branch or not.

Comment 24

[Hermes Cheng (inactive after July 27, 2015)]

I think it is fine if it really breaks important function and is marked as a blocker. NI Josh for more input.

Comment 25

[Josh Cheng [:josh]]

Hi Gabriele, 
Bug 1174673 is uplifted and merged so it's technically okay to land this patch now. 

The issue was originally reported by Qualcomm as GCF blocker thus I like to NI Dylan for whether we still need this for 2.2.
Thanks!

Comment 26

[Gabriele Svelto [:gsvelto]]

I'm trying to uplift to v2.2 but it's proving a non-trivial effort since bug 1166635 is getting in the way and that one is not a 2.2+ blocker. Josh, shall I ask for approval on that one too? It's a rather large revert (with some modifications thrown in) and it would probably need some extra checking to ensure we're not introducing regressions.

Comment 27

[Josh Cheng [:josh]]

Thanks Gabriele!

Hi Dylan,
I do not prefer to land this on 2.2 as this is a large patch with moderate risk per comment 26 from Gabriele. 
Is it possible to communicate with Qualcomm that we already fix this on 2.5 but not in 2.2?
Thanks!

Comment 28

[Carol Yang [:cyang]]

(In reply to Josh Cheng [:josh] from comment #27)
> Thanks Gabriele!
> 
> Hi Dylan,
> I do not prefer to land this on 2.2 as this is a large patch with moderate
> risk per comment 26 from Gabriele. 
> Is it possible to communicate with Qualcomm that we already fix this on 2.5
> but not in 2.2?

This breaks a GCF test case. Ideally, we'd want this change along with bug 1174673. At the moment, it would be ok that this is on 2.5 and higher. However, OEMs/carriers may have different requirements. If this becomes an issue down the road, at least we know exactly what changes needs to be landed on 2.2.

> Thanks!

Comment 29

[Dylan Oliver [:doliver]]

Removing 2.2+ based on comments 27 & 28. Looks like all are in agreement that 2.5 is the safer path here.