1061210 - [tarako/dolphin]Fail to add new call after communications app was killed while a conference call is background

Status: RESOLVED FIXED

(Firefox OS Graveyard :: RIL, defect)

Description

[helloarvin]

Steps to reproduce:
1> Insert a sim card which supports conference call feature;
2> Boot the device and merge two calls into a conference call;
3> press home key back to homescreen and long-press home key to remove phone;
4> click dialer to make a call;
5> Call ended asap with the error is 'UnspecifiedError'

Comment 1

[helloarvin]

Hi Etienne, could you please help check the issue to identify is this a gaia bug or gecko bug?

It seems that the issue is triggered by the telephony.active(=null)[1] after we restart the dialer app. Under the case, gaia would dial directly rather than hold the current active conference call first so that NAS would cancel the dial request(error type is 'UnspecifiedError').


Thanks a lot.


[1] https://github.com/mozilla-b2g/gaia/blob/v1.3t/apps/communications/dialer/js/telephony_helper.js#L29

Comment 2

[helloarvin]

Hi sku,

The issue can be reproduced with the rate 100% on both v1.3t and v1.4 without any sprd patches.
My analysis result is mentioned in comment1.

Could you please help check it?

Thank you very much.

Comment 3

[shawn ku [:sku]]

ni? aknow for handling the issue

Comment 4

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

As comment1, gecko doesn't update the conference state correctly. If the value is updated correctly, |telephony.active| should point to a conference group.

Comment 5

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

Attachment: Update conference group after enumerateCall

Comment 6

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

Comment on attachment 8483348 [details] [diff] [review]
Update conference group after enumerateCall

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

Thanks for catching this!

::: dom/telephony/Telephony.cpp
@@ +586,5 @@
> +        break;
> +      }
> +    }
> +
> +    printf_stderr("Aknow: Change conference state: %d", callState);

Don't forget to remove this :P

Comment 7

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

Attachment: (1.3T) [final] Update conference group after enumerateCall. r=hsinyi

Comment 8

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

Attachment: [final] Update conference group after enumerateCall. r=hsinyi

Comment 9

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

checkin-needed on master

https://tbpl.mozilla.org/?tree=Try&rev=8e730abae9ff

Comment 10

[helloarvin]

Comment on attachment 8484015 [details] [diff] [review]
(1.3T) [final] Update conference group after enumerateCall. r=hsinyi

># HG changeset patch
># User Szu-Yu Chen [:aknow] <[e-mail]>
>Bug 1061210 - Update conference group after enumerateCall. r=hsinyi
>
>diff --git a/dom/telephony/Telephony.cpp b/dom/telephony/Telephony.cpp
>index 45526f4..9567f55 100644
>--- a/dom/telephony/Telephony.cpp
>+++ b/dom/telephony/Telephony.cpp
>@@ -589,16 +589,31 @@ Telephony::ConferenceCallStateChanged(uint16_t aCallState)
>   return NS_OK;
> }
> 
> NS_IMETHODIMP
> Telephony::EnumerateCallStateComplete()
> {
>   MOZ_ASSERT(!mEnumerated);
> 
>+  // Set conference state.
>+  if (mGroup->CallsArray().Length() >= 2) {
>+    const nsTArray<nsRefPtr<TelephonyCall> > &calls = mGroup->CallsArray();
>+
>+    uint16_t callState = calls[0]->CallState();
>+    for (uint32_t i = 1; i < calls.Length(); i++) {
>+      if (calls[i]->CallState() != callState) {
>+        callState = nsITelephonyService::CALL_STATE_UNKNOWN;
>+        break;
>+      }
>+    }
>+
>+    mGroup->ChangeState(callState);
>+  }
>+
>   mEnumerated = true;
> 
>   if (NS_FAILED(NotifyCallsChanged(nullptr))) {
>     NS_WARNING("Failed to notify calls changed!");
>   }
> 
>   if (NS_FAILED(mProvider->RegisterListener(mListener))) {
>     NS_WARNING("Failed to register listener!");


Dear Szu-Yu,

Thanks a lot for your kindly help.
I verified the patch in v1.3t and the issue never occurred again for that 'activeCall=[object TelephonyCallGroup]' rather than 'null' under the case.

But it seems that we should use 'nsITelephonyProvider' instead of 'nsITelephonyService' in v1.3t, or else there'll be an compiled error: 'nsITelephonyService is undeclared'.

Please help check it:)

>+      if (calls[i]->CallState() != callState) {
>+        callState = nsITelephonyService::CALL_STATE_UNKNOWN;
>+        break;
>+      }

--------->
>+      if (calls[i]->CallState() != callState) {
>+        callState = nsITelephonyProvider::CALL_STATE_UNKNOWN;
>+        break;
>+      }

Comment 11

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/b2g-inbound/rev/b0411338116f

Please avoid running all builds/platforms/tests on your Try pushes unless you really need to. For a B2G-only patch like this, doing so is extremely wasteful of test resources, contributing to backlogs experienced by all other developers.
https://wiki.mozilla.org/Sheriffing/How:To:Recommended_Try_Practices

Comment 12

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

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

Comment 13

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

Attachment: (1.3T) [final#2] Update conference group after enumerateCall. r=hsinyi

Comment 14

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

> Dear Szu-Yu,
> 
> Thanks a lot for your kindly help.
> I verified the patch in v1.3t and the issue never occurred again for that
> 'activeCall=[object TelephonyCallGroup]' rather than 'null' under the case.
> 
> But it seems that we should use 'nsITelephonyProvider' instead of
> 'nsITelephonyService' in v1.3t, or else there'll be an compiled error:
> 'nsITelephonyService is undeclared'.
> 
> Please help check it:)
> 
> >+      if (calls[i]->CallState() != callState) {
> >+        callState = nsITelephonyService::CALL_STATE_UNKNOWN;
> >+        break;
> >+      }
> 
> --------->
> >+      if (calls[i]->CallState() != callState) {
> >+        callState = nsITelephonyProvider::CALL_STATE_UNKNOWN;
> >+        break;
> >+      }

Yes, you are right. We should use nsITelephonyProvider on 1.3T.
Thanks for the catch. I have updated the patch.

Comment 15

[helloarvin]

[Blocking Requested - why for this release]: function defects

Dear Szu-Yu,

Could you please help land it into v1.4?
Thanks a lot.

Comment 16

[helloarvin]

Dear Wayne,

This patch is quite urgent for v1.3t so that please help us merge it into v1.3t.
Thank you very much.

Comment 17

[Wayne Chang [:wchang]]

No longer taking non-critical patches on 1.3t/1.4.

Let's land on master if applicable.

Comment 18

[helloarvin]

(In reply to Wayne Chang [:wchang] from comment #17)
> No longer taking non-critical patches on 1.3t/1.4.
> 
> Let's land on master if applicable.

This issue is indeed critical for those reasons:

1\ Callscreen and dialer(communications) are two independent apps in v1.3t and v1.4, it's a high probability to be killed for dialer app by LMK due to less memory available on those two branches;

2\ Once dialer app had been killed, we'll never succeed to place any new call if an active conference call exists for the initialization defect;

3\ One of our customers regard the issue as the only & most critical block one for release.

Overall, I certainly think the issue is critical and we should land the patch into every branch.
Please help consider our request once more.

Thanks.

Comment 19

[Wayne Chang [:wchang]]

Hello Arvin,
I still have my doubts on this being critical. This is a corner use case where users will be adding more than 2 calls and multitasking with enough load to kill comms app. 
Agree that this should be corrected but we don't think this needs to be picked to a closed release.

Comment 20

[helloarvin]

(In reply to Wayne Chang [:wchang] from comment #19)
> Hello Arvin,
> I still have my doubts on this being critical. This is a corner use case
> where users will be adding more than 2 calls and multitasking with enough
> load to kill comms app. 
> Agree that this should be corrected but we don't think this needs to be
> picked to a closed release.

Hi Wayne,
As you said, indeed, the scene belongs to a corner use case especially for general users. But both our QA and customer had found the issue and set it as BLOCK one. We definitely can merge the patch locally to fix it while I'm still reticent about your opinion here.

Thanks.

Comment 21

[helloarvin]

Dear Szu-Yu,

Sorry to trouble you, have you verified the issue on the master branch with the patch? The issue is still able to reproduce after the patch merged under the rate 100% on the branch v1.4. Could you please tell me your verify result on master branch? (If so, I think this should be a gaia defect[1] for it works well in the v1.3t branch[2])

Thanks a lot.
[1] https://github.com/mozilla-b2g/gaia/blob/v1.4/apps/communications/dialer/js/telephony_helper.js#L27
[2] https://github.com/mozilla-b2g/gaia/blob/v1.3t/apps/communications/dialer/js/telephony_helper.js#L19

Comment 22

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

(In reply to helloarvin from comment #21)
> Dear Szu-Yu,
> 
> Sorry to trouble you, have you verified the issue on the master branch with
> the patch? The issue is still able to reproduce after the patch merged under
> the rate 100% on the branch v1.4. Could you please tell me your verify
> result on master branch? (If so, I think this should be a gaia defect[1] for
> it works well in the v1.3t branch[2])
> 
> Thanks a lot.
> [1]
> https://github.com/mozilla-b2g/gaia/blob/v1.4/apps/communications/dialer/js/
> telephony_helper.js#L27
> [2]
> https://github.com/mozilla-b2g/gaia/blob/v1.3t/apps/communications/dialer/js/
> telephony_helper.js#L19

The reason is that Gaia reads the value of telephony.active too fast. It should wait for the initialization of telephony so that the value (.active) could be valid.

Telephony uses first "callschanged" event to signal the completion of initialization.

In v1.3t [1][2], the code is wrapped by |ensureTelephony|. It guarantees that the value of .active is correct. However, it seems that v1.4 doesn't do that. You could have the similar logic on 1.4 to fix it.

Ex: enclosing line 27 - 49 [3] by the following way.

navigator.mozTelephony.addEventListener('callschanged', function telephonyReady() {
  telephony = navigator.mozTelephony;
  telephony.removeEventListener('callschanged', telephonyReady);

  // Put line 27 - 49 here

});

[1] https://github.com/mozilla-b2g/gaia/blob/v1.3t/apps/communications/dialer/js/telephony_helper.js#L19
[2] https://github.com/mozilla-b2g/gaia/blob/v1.3t/apps/communications/dialer/js/telephony_helper.js#L168
[3] https://github.com/mozilla-b2g/gaia/blob/v1.4/apps/communications/dialer/js/telephony_helper.js#L27

Comment 23

[helloarvin]

Yeah, thanks for your analysis.
We'd better ask Etienne to help land the patch of 1015896 into the v1.4 and master.

Dear etienne,

According to the comment 22, could you please help merge the PR[1] into both the v1.4 and master branches to fix the initialization issue before dialing?

Thanks a lot.

[1] https://github.com/mozilla-b2g/gaia/commit/385967e43ca16f0a38bc7918da0cfc462c9774b3

Comment 24

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

> Sorry to trouble you, have you verified the issue on the master branch with
> the patch? The issue is still able to reproduce after the patch merged under

We also have another patch on the master (Bug 1056618). With this patch, even though Gaia doesn't hold the call before it dials out a new one, gecko could handle everything well.

I do verify the issue on master. As I mentioned in comment 22, my experiment showed that Gaia got the wrong value of telephony.active so the existing call is not held by gaia. It is held in gecko by Bug 1056618. That's the reason why the issue doesn't happen on the master.

Comment 25

[helloarvin]

Perfect!
The fault tolerant mechanism is very necessary!

Do we plan to land the patch of Bug 1056618 into v1.4?

PS: The patch of bug 1015896 should be merged into all branches including v1.4 and master. Let's wait etienne's feedback.

Comment 26

[Etienne Segonzac (:etienne)]

(In reply to helloarvin from comment #23)
> Yeah, thanks for your analysis.
> We'd better ask Etienne to help land the patch of 1015896 into the v1.4 and
> master.
> 
> Dear etienne,
> 
> According to the comment 22, could you please help merge the PR[1] into both
> the v1.4 and master branches to fix the initialization issue before dialing?
> 
> Thanks a lot.
> 
> [1]
> https://github.com/mozilla-b2g/gaia/commit/
> 385967e43ca16f0a38bc7918da0cfc462c9774b3

Looks like it won't be needed on master, for v1.4 we just need to request approval or blocking on bug 1015896. It should be pretty straightforward to uplift, if any issue arise feel free to ping me.

Comment 27

[helloarvin]

Etienne, thank you very much and I've made the request v1.4 blocking on bug 1015896.