784429 - [WebAPI] WebTelephony: Hang-up while connecting, call is not terminated

Status: RESOLVED INVALID

(Firefox OS Graveyard :: RIL, defect)

Description

[Robert Wood [:rwood]]

Attachment: Logcat

Using the B2G WebTelephony WebAPI.

Receive an incoming call and answer it - but immediately after receiving the 'connecting' call event, and BEFORE receiving the 'connected' event, hangUp the call (via the API). Notice that a 'disconnecting' call event is then received as expected, but a 'disconnected' call event never follows - and actually a 'connected' call event is then received (after the 'disconnecting' event).

At this point, the emulator shows that the call is active (issue the 'gsm list' command to the emulator and the call status is active).

Had expected the call to be terminated - according to the B2G Telephony Call States diagram, if a hang-up occurs during the 'connecting' state (but before the actual 'connected' state), both the 'disconnecting' and 'disconnected' events should be received: https://wiki.mozilla.org/File:B2GTelephonyCallStates.png

To reproduce: Run the attached B2G WebAPI Marionette test (test_incoming_connecting_hangup.js). Expect the test to pass, instead it times out waiting for the 'disconnected' call event after the hang-up. See logcat attached.

Comment 1

[Robert Wood [:rwood]]

Attachment: Marionette B2G test

Comment 2

[Philipp von Weitershausen [:philikon]]

Thanks for the investigation. This is quite the edge case, unlikely to be encountered on the device. Definitely not a blocker. If we can find an extremely easy fix, we'll take it, but we've got bigger fish to fry.

Comment 3

[Ben Turner (not reading bugmail, use the needinfo flag!)]

This is a race, I'd expect that we could hit it in the real world.

Comment 4

[Philipp von Weitershausen [:philikon]]

(In reply to ben turner [:bent] from comment #3)
> This is a race, I'd expect that we could hit it in the real world.

I didn't say we couldn't, just said that it was unlikely given that the 'connecting' and 'connected' events are something like O(100ms) apart (and that's probably pretty generous). Definition for a blocker is something we would hold the release for. Personally I would totally ship the phone with this bug not fixed.

Comment 5

[Ken Chang[:kenkai]]

Put this bug into backlog.

Comment 6

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

Attachment: Part 1: Handle disconnecting state properly

Comment 7

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

Attachment: Part 2: Fix test case

Comment 8

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

Comment on attachment 8408036 [details] [diff] [review]
Part 1: Handle disconnecting state properly

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

::: dom/telephony/TelephonyCall.cpp
@@ +70,5 @@
>  
> +  // Rule for state transition.
> +  // Only accept 'disconnected' after 'disconnecting'.
> +  if (mCallState == nsITelephonyProvider::CALL_STATE_DISCONNECTING &&
> +      aCallState != nsITelephonyProvider::CALL_STATE_DISCONNECTED) {

This is not right. When the remote party releases the call, we will meet a case that aCallState is DISCONNECTED but mCallState isn't DISCONNECTING.

Another example is, if App_1 is the caller to trigger hang up, then App_2 is never received DISCONNECTED call event even that call is indeed released.

Comment 9

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

(In reply to Hsin-Yi Tsai  [:hsinyi] from comment #8)
> Comment on attachment 8408036 [details] [diff] [review]
> Part 1: Handle disconnecting state properly
> 
> Review of attachment 8408036 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: dom/telephony/TelephonyCall.cpp
> @@ +70,5 @@
> >  
> > +  // Rule for state transition.
> > +  // Only accept 'disconnected' after 'disconnecting'.
> > +  if (mCallState == nsITelephonyProvider::CALL_STATE_DISCONNECTING &&
> > +      aCallState != nsITelephonyProvider::CALL_STATE_DISCONNECTED) {
> 
> This is not right. When the remote party releases the call, we will meet a
> case that aCallState is DISCONNECTED but mCallState isn't DISCONNECTING.
> 
> Another example is, if App_1 is the caller to trigger hang up, then App_2 is
> never received DISCONNECTED call event even that call is indeed released.

Oops, sorry... I was dazzled... Please ignore this :(

Comment 10

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

Comment on attachment 8408036 [details] [diff] [review]
Part 1: Handle disconnecting state properly

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

::: dom/telephony/TelephonyCall.cpp
@@ +70,5 @@
>  
> +  // Rule for state transition.
> +  // Only accept 'disconnected' after 'disconnecting'.
> +  if (mCallState == nsITelephonyProvider::CALL_STATE_DISCONNECTING &&
> +      aCallState != nsITelephonyProvider::CALL_STATE_DISCONNECTED) {

Please ignore my previous comment :(

But I am still concerned about the patch:
App_1 is the caller to trigger call.hangUp(). App_2 is simply a monitor. What if 'connected' event really comes, and the call state is indeed connected? Then with this patch, App_1 doesn't get any chance to get updated but App_2 does... We now have some problems with call state synchronization between objects. And this solution seems make things worse...

Comment 11

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

(In reply to Hsin-Yi Tsai  [:hsinyi] from comment #10)
> Comment on attachment 8408036 [details] [diff] [review]
> Part 1: Handle disconnecting state properly
> 
> Review of attachment 8408036 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: dom/telephony/TelephonyCall.cpp
> @@ +70,5 @@
> >  
> > +  // Rule for state transition.
> > +  // Only accept 'disconnected' after 'disconnecting'.
> > +  if (mCallState == nsITelephonyProvider::CALL_STATE_DISCONNECTING &&
> > +      aCallState != nsITelephonyProvider::CALL_STATE_DISCONNECTED) {
> 
> Please ignore my previous comment :(
> 
> But I am still concerned about the patch:
> App_1 is the caller to trigger call.hangUp(). App_2 is simply a monitor.
> What if 'connected' event really comes, and the call state is indeed
> connected? Then with this patch, App_1 doesn't get any chance to get updated
> but App_2 does... We now have some problems with call state synchronization
> between objects. And this solution seems make things worse...

Call state is not synchronized because we introduce +ing state in our system. IMO, if we also broadcast and sync the +ing state between telephony objects, the problem could be solved.

Comment 12

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

After discussing with Hsinyi, we decided to postpone this bug. I will work on synchronizing the +ing state first. That is, all the other telephony objects will also receive 'connecting', 'holding', 'resuming', 'disconnecting' ... state change.

Comment 13

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

"connecting" state is going to be removed in Bug 1095366.