Closed Bug 1061210 Opened 7 years ago Closed 7 years ago

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

Categories

(Firefox OS Graveyard :: RIL, defect)

ARM
Gonk (Firefox OS)
defect
Not set
major

Tracking

(blocking-b2g:-)

RESOLVED FIXED
2.1 S4 (12sep)
blocking-b2g -

People

(Reporter: arvin.zhang, Assigned: aknow)

References

Details

Attachments

(2 files, 2 obsolete files)

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'
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
Flags: needinfo?(etienne)
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.
Flags: needinfo?(sku)
Summary: [tarako]Fail to add new call after communications app was killed while a conference call is background → [tarako/dolphin]Fail to add new call after communications app was killed while a conference call is background
ni? aknow for handling the issue
Flags: needinfo?(szchen)
Flags: needinfo?(sku)
Flags: needinfo?(etienne)
As comment1, gecko doesn't update the conference state correctly. If the value is updated correctly, |telephony.active| should point to a conference group.
Assignee: nobody → szchen
Flags: needinfo?(szchen)
Attachment #8483348 - Flags: review?(htsai)
Depends on: 1048680
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
Attachment #8483348 - Flags: review?(htsai) → review+
Component: Gaia::Dialer → RIL
Attachment #8483348 - Attachment is obsolete: true
Depends on: 1056618
No longer depends on: 1048680
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] <szchen@mozilla.com>
>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;
>+      }
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
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/b0411338116f
Status: NEW → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → 2.1 S4 (12sep)
> 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.
[Blocking Requested - why for this release]: function defects

Dear Szu-Yu,

Could you please help land it into v1.4?
Thanks a lot.
blocking-b2g: --- → 1.4?
Flags: needinfo?(szchen)
Dear Wayne,

This patch is quite urgent for v1.3t so that please help us merge it into v1.3t.
Thank you very much.
Flags: needinfo?(wchang)
No longer taking non-critical patches on 1.3t/1.4.

Let's land on master if applicable.
blocking-b2g: 1.4? → -
Flags: needinfo?(wchang)
(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.
Flags: needinfo?(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.
Flags: needinfo?(wchang)
Flags: needinfo?(szchen)
(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.
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
Flags: needinfo?(szchen)
(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
Flags: needinfo?(szchen)
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
Flags: needinfo?(etienne)
> 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.
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.
(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.
Flags: needinfo?(etienne)
Etienne, thank you very much and I've made the request v1.4 blocking on bug 1015896.
You need to log in before you can comment on or make changes to this bug.