1161403 - [Telephony] Deprecate internal call states including connecting, disconnecting, holding and resuming.

Status: RESOLVED FIXED

(Firefox OS Graveyard :: RIL, defect)

Description

[Ben Hsu [:HoPang]]

When a telephonyCall is being controlled (answer, hangUp, hold or resume), its internal state will be updated, which is used to blocked consecutive requests. However, when a request fails, we should restore the internal state of the call, otherwise following requests to the call will be failed.

Comment 1

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

nominate as this is the root cause of bug 1160325, a certification blocker.

Comment 2

[Ben Hsu [:HoPang]]

Attachment: Part 1: Add mCallStatePrevious and RestorePreviousInternalState()

Comment 3

[Ben Hsu [:HoPang]]

Attachment: Part 2: Enhance TelephonyCallBack.notifyError()

Comment 4

[Ben Hsu [:HoPang]]

Attachment: Part 3: A new testcase

Comment 5

[Ben Hsu [:HoPang]]

Comment on attachment 8601431 [details] [diff] [review]
Part 3: A new testcase

Hi Aknow,

Since now we can only disable the hold feature of the emulator, I only implement the internal state restoration test for hold failures, but I think this testcase is still capable of covering the scenario of Bug 1160325. However, if other internal state restoration tests is needed, I suggest we file another bug for enhancing both this testcase and the emulator.

Comment 6

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

Comment on attachment 8601431 [details] [diff] [review]
Part 3: A new testcase

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

We should add the test to manifest.

::: dom/telephony/test/marionette/test_internal_state_restoration.js
@@ +39,5 @@
> +    .then(() => incoming())
> +    .then(() => answer())
> +
> +    .then(() => emulator.runCmd("gsm disable hold"))
> +    .then(() => hold())

If we disable hold, looks like the check in hold() will fail.
I mean => gCheckAll(null, [inCall], "", [], [inInfo.held])

Did you handle this?

Comment 7

[Ben Hsu [:HoPang]]

Comment on attachment 8601431 [details] [diff] [review]
Part 3: A new testcase

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

::: dom/telephony/test/marionette/test_internal_state_restoration.js
@@ +39,5 @@
> +    .then(() => incoming())
> +    .then(() => answer())
> +
> +    .then(() => emulator.runCmd("gsm disable hold"))
> +    .then(() => hold())

Yes, the |hold()| will fail, and the following two checks (|gCheckAll()| and |is()|) will be also be skipped. I think I should make this part more clear.

Comment 8

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

I am thinking of a way to help us get rid of the internal state. Then we don't need to figure out how to restore the previous state because there is no state changes at all.

We have three operations that could create a transition state on TelephonyCall or TelephonyCallGroup:
(1) hold
(2) resume
(3) hangup

So, how about having three distinct Promises in TelephonyCall and TelephonyCallGroup. Each indicates whether the operation is ongoing.

In details.

1. When user want to hold/resume/hangup a call/callGroup, see if the promise exists
 i. Yes, return the same promise.
    We don't have to block the consecutive request. They all get the same promise and it will be resolved at the same time.
 ii. No, create a new promise and record it in call/callGroup. Also, return it to user.
     When the underlying request is done, the callback will set the recorded promise back to null.

Some trick between TelephonyCall and TelephonyCallGroup:

When we hold/resume/hangup a call in a callgroup, we could see whether the entire callGroup is being controlled now. It it is, we could return the promise stored in the callgroup.


Any ideas about the proposal?

Comment 9

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

I've been thinking about the same question. My previous rough idea is that we introduce different internal callbacks for each request. Then, checking if a certain type of callback existence. But in this case, a distinct promise will be returned when a consecutive request comes. I think your proposal looks better than mine. Returning the same promise makes things easier.

+1 for the proposal.

Comment 10

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

(In reply to Hsin-Yi Tsai [:hsinyi] from comment #9)
> I've been thinking about the same question. My previous rough idea is that
> we introduce different internal callbacks for each request. Then, checking
> if a certain type of callback existence. But in this case, a distinct
> promise will be returned when a consecutive request comes. I think your
> proposal looks better than mine. Returning the same promise makes things
> easier.
> 
> +1 for the proposal.

However, I'd still like to raise that this is going to be a 2.2 blocker. We should think about twice b/w the solution and the risk. I am a little concerned the proposal might changes too much, will it?

Comment 11

[Ben Hsu [:HoPang]]

Attachment: Part 1: Deprecate internal states

Comment 12

[Ben Hsu [:HoPang]]

Attachment: Part 2: A new testcase

Comment 13

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

Comment on attachment 8602054 [details] [diff] [review]
Part 2: A new testcase

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

::: dom/telephony/test/marionette/test_consecutive_hold.js
@@ +30,5 @@
> +          () => log("This hold request is rejected as expected."))
> +    .then(() => gCheckAll(inCall, [inCall], "", [], [inInfo.active]))
> +    .then(() => is(inCall.disconnectedReason, null))
> +
> +    // Disable the Hold function of the emulator, then hold the active call,

'Enable' the hold  ...

@@ +31,5 @@
> +    .then(() => gCheckAll(inCall, [inCall], "", [], [inInfo.active]))
> +    .then(() => is(inCall.disconnectedReason, null))
> +
> +    // Disable the Hold function of the emulator, then hold the active call,
> +    // where the hold request will fail and the call will remain active.

wrong comment

Comment 14

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

Comment on attachment 8602052 [details] [diff] [review]
Part 1: Deprecate internal states

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

Looks good. Only one concern

Hsinyi,

We need to remove some internal states in this patch.  Is it ok to do it and change the ril interface?

Comment 15

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

Comment on attachment 8602052 [details] [diff] [review]
Part 1: Deprecate internal states

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

It's totally fine for the patch landed on m-c, but I'd suggest we don't touch nsITelephonyService.idl for v2.2 version based on the milestone we are. Thank you.

Comment 16

[Ben Hsu [:HoPang]]

Attachment: Part 2: A new testcase (V2)

Comment 17

[Wesley Huang [:wesley_huang] (EPM) (NI me)]

triage: blocking the GCF test failure (bug 1160325) which is a blocker.

Comment 18

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

Hi Anshul,
We'd like to have your feedback to make sure we solve the issue in a correct way and don't result in side effects.

Currently, a telephonyCall state isn't restored when a hold request fails that blocks the second request. Our attempt in this bug is not only to resolve that Hold-a-single-call GCF tests issue you've reported but also to resolve potential issues, such as Resume-a-single-call or hold-a-conference-call cases.

A solution could be to restore the internal holding/resuming/disconnecting state back to connected if a hold/resume/hangUp request fails. However, the change on 2.2 is huge especially for the conference cases, given in 2.2 there's no callback to know if conference.hold()/.resume() succeeds or not. To achieve that, internal ril interface changes are required. Your RIL will need corresponding revision.

Another solution could be that we just loosen the check of the internal call state before executing a request, e.g. in [1]. In this way, the 2nd request won't be impacted by a previous failure. This solution slightly changes the webapi behaviour because it implies we don't block a consecutive request at DOM level. However, I think it's okay because that current gaia doesn't care request failures but relies on only call state events from network.

Considering the milestone we are and the small changes going to be made, we are going to take the 2nd solution. I couldn't think of side effects on your RIL, but let us know if you have concerns. Thank you!

[1] https://dxr.mozilla.org/mozilla-central/source/dom/telephony/TelephonyCall.cpp?from=telephonycall.cpp#339

Comment 19

[Anshul]

Hsin-Yi, your proposal for 2.2 sounds good. If your patch is ready, Phil can give it a shot to confirm there is no side effect of your change.

Comment 20

[Ben Hsu [:HoPang]]

Attachment: (2.2) Part 1: Loosen the internal state checks

Hi Aknow,

Since the |on***ing| events haven't been removed from |TelephonyCall.webidl| of branch 2.2, I can not deprecate the internal states, and all I can do is loosening the internal state checks to make the request can still be visited after a previous failure. Similarily, the testcase for branch 2.2 is different from the one for m-c because of the internal states. Do you think this solution is acceptable?

Comment 21

[Ben Hsu [:HoPang]]

Attachment: (2.2) Part 2: A new testcase (V2)

Comment 22

[Ben Hsu [:HoPang]]

Dear sheriff,

Can you help checkin the two patches without (2.2) in their name to mozilla-central? While the other two patches are for branch 2.2 and still need to be further verified.

https://treeherder.mozilla.org/#/jobs?repo=try&revision=1f121f005b6a&exclusion_profile=false

Comment 23

[Ben Hsu [:HoPang]]

Hi Phil,

I think the fix is ready to go. Do you mind trying whether |(2.2) Part 1: Loosen the internal state checks| fixes Bug 1160325 without any side effect. Thank you in advance.

Comment 24

[Pulsebot]

https://hg.mozilla.org/integration/b2g-inbound/rev/9aa9bdd73175
https://hg.mozilla.org/integration/b2g-inbound/rev/0003e0021c08

Comment 25

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

Anshul or Phil, any update here?

Comment 27

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/9aa9bdd73175
https://hg.mozilla.org/mozilla-central/rev/0003e0021c08

Comment 28

[Gabriele Svelto [:gsvelto]]

Quick question, does this patch makes the connecting, disconnecting, holding and resuming states invisible from gaia? If it does I'll remove the (few) uses we have of them.

Comment 29

[Anshul]

Comment on attachment 8603183 [details] [diff] [review]
(2.2) Part 1: Loosen the internal state checks

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 #): 
User impact if declined: 
Testing completed: 
Risk to taking this patch (and alternatives if risky): 
String or UUID changes made by this patch:

Comment 30

[Anshul]

Comment on attachment 8603184 [details] [diff] [review]
(2.2) Part 2: A new testcase (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 #): 
User impact if declined: 
Testing completed: 
Risk to taking this patch (and alternatives if risky): 
String or UUID changes made by this patch:

Comment 31

[pgravel]

(In reply to Hsin-Yi Tsai [:hsinyi] from comment #25)
> Anshul or Phil, any update here?

Hi Hsin-Yi,

While the patch does fix the unresponsive hold button, there are still problems with the UI changing state when it shouldn't. Once the hold button is pressed, the mute and speaker buttons immediately become grayed out and disabled even though the call is still active because the hold request failed.

Comment 32

[pgravel]

I'll file a different bug for the UI changing before it should. This patch is still a good fix for the hold button becoming unresponsive.

Comment 33

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

(In reply to Gabriele Svelto [:gsvelto] from comment #28)
> Quick question, does this patch makes the connecting, disconnecting, holding
> and resuming states invisible from gaia? If it does I'll remove the (few)
> uses we have of them.

Hi Gabriele,
Not exactly. The connecting, disconnecting, holding and resuming states have been invisible to gaia since bug 1095366. This bug deprecates those from gecko internal usage.

Bug 1095366 wasn't landed in 2.2. so the WebAPI is different from v2.2 to master. For the 2.2 solution, we don't touch webapi change; we just loosen the internal check to pass a hold request to RIL even after a failure.

Comment 34

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

(In reply to Anshul from comment #29)
> Comment on attachment 8603183 [details] [diff] [review]
> (2.2) Part 1: Loosen the internal state checks
> 
> 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 #): GCF test failure
> User impact if declined: GCF test fails
> Testing completed: yes, confirmed by Partners
> Risk to taking this patch (and alternatives if risky): low, the bug scope is limited
> String or UUID changes made by this patch:no

Comment 35

[Ben Hsu [:HoPang]]

Comment on attachment 8603183 [details] [diff] [review]
(2.2) Part 1: Loosen the internal state checks

Sorry Anshulj,

I have to cancel this uplift request since there is an emulator enhancement should be uplifted first. Please refer to Bug 1164329.

Comment 36

[Ben Hsu [:HoPang]]

Comment on attachment 8603184 [details] [diff] [review]
(2.2) Part 2: A new testcase (V2)

As comment comment #35

Comment 37

[Anshul]

(In reply to pgravel from comment #32)
> I'll file a different bug for the UI changing before it should. This patch
> is still a good fix for the hold button becoming unresponsive.
Phil, seems like there is now a bug 1082588 to track Gaia change.

Comment 38

[Ben Hsu [:HoPang]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=c51b9d3ae431&exclusion_profile=false

Comment 39

[Ben Hsu [:HoPang]]

Comment on attachment 8603183 [details] [diff] [review]
(2.2) Part 1: Loosen the internal state checks

[Approval Request Comment]
Bug caused by (feature/regressing bug #): Bug 1160325
User impact if declined: Call states remains when operations fail and blocks following operation requests.
Testing completed: Manual test on Flame and Try server
Risk to taking this patch (and alternatives if risky): NA 
String or UUID changes made by this patch: NA

Comment 40

[Ben Hsu [:HoPang]]

Comment on attachment 8603184 [details] [diff] [review]
(2.2) Part 2: A new testcase (V2)

[Approval Request Comment]
Bug caused by (feature/regressing bug #): Bug 1160325
User impact if declined: Need this testcase to ensure the previous patch works
Testing completed: Try server
Risk to taking this patch (and alternatives if risky): NA 
String or UUID changes made by this patch: NA

Comment 41

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-b2g37_v2_2/rev/59e6a781882d
https://hg.mozilla.org/releases/mozilla-b2g37_v2_2/rev/5e56e3ac5c5b