1106773 - [dolphin] [RIL] RIL_Worker should not send success response directly to DIAL request, or it might become invalid that the detecting on dialing the same number within a short time

Status: RESOLVED DUPLICATE of bug 1013745

(Firefox OS Graveyard :: RIL, defect)

Description

[helloarvin]

Steps to reproduce:
1\ Enter dialer and make a MOC with any valid number;
2\ Hang up the call once the callscreen appeared;
3\ Click the dial button multiple quickly to make the MOC again;
4\ Repeat the step 2 and 3 quickly;
5\ There'll be no response when you click to dial unless restart the dialer app.

Another way to reproduce the issue with nearly 100% rate is to call the function 'startDial' two times via editing codes[1]:

Original:
    if (!activeCall) {
      startDial(cardIndex, conn, sanitizedNumber, oncall, onconnected,
                ondisconnected, onerror);
      return;
    }

Modified:
    if (!activeCall) {
      startDial(cardIndex, conn, sanitizedNumber, oncall, onconnected,
                ondisconnected, onerror);
      + startDial(cardIndex, conn, sanitizedNumber, oncall, onconnected,
      +          ondisconnected, onerror);
      return;
    }

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

Comment 1

[helloarvin]

Root-cause analysed:

When the issue happened, the 'telephony.active'[1] is not null but in fact there's no active call existed currently. As a result, the new dialing request triggered by clicking dial button would never have chance to be sent to gecko unless the dialer app been killed.

The cause of the issue is that the second dial request on the same number has not been stopped as expected.

1) On the first dialing, the success response in RIL_WORKER will be sent directly so that the isDialing in TelephonyProvider will be reset and meanwhile call notifyDialSuccess to create new voice call in Telephony.

2) Then, the second dialing request would be sent to RIL_WORKER due to the flag isDialing has been set as false. So the success response also send directly and the additional new voice call(the same number with the first call) will be created.

3) After we hang up the 1st call, there'll be a spooky call left in Telephony so that we cannot make new MOC unless the mozTelephony re-init.

It is because the response of RIL_WORKER on dial request will always be sent directly, and it will trigger to reset the isDialing as false so that the fast second call will be sent to RIL_WORKER successfully.

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

Comment 2

[helloarvin]

Dear Shawn,

Could you please help check the issue based on the comment1 and the email to give me some suggestions?

Thanks a lot.

Comment 3

[shawn ku [:sku]]

Hi Aknow:
 Could you please give some suggestion based on v1.4?
Is there any quick solution for this case?

Thanks!!
Shawn

Comment 4

[helloarvin]

Thanks for Shawn's support.

Hi Aknow,

By contrast, the issue cannot reproduce on the v2.1 version. But it seems that it's impossible to extract a patch for v1.4 due to mechanism refactoring. Could you please help find a quick solution on gecko part?

Thank you very much.
Arvin

Comment 5

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

(In reply to helloarvin from comment #4)
> Thanks for Shawn's support.
> 
> Hi Aknow,
> 
> By contrast, the issue cannot reproduce on the v2.1 version. But it seems
> that it's impossible to extract a patch for v1.4 due to mechanism
> refactoring. Could you please help find a quick solution on gecko part?
> 
> Thank you very much.
> Arvin

Hi helloarvin, 

I dont't quite understand the issue. I know that why we might create two calls with the same number. But, will it cause any problem? Could you tell me how many calls will appear in gaia (call screen). I haven't test it but I think it should be two. If it is, we could simply hangup these two calls.

So, the problem is that does the second call shown on the screen.

Comment 6

[helloarvin]

(In reply to Szu-Yu Chen [:aknow] from comment #5)
> (In reply to helloarvin from comment #4)
> > Thanks for Shawn's support.
> > 
> > Hi Aknow,
> > 
> > By contrast, the issue cannot reproduce on the v2.1 version. But it seems
> > that it's impossible to extract a patch for v1.4 due to mechanism
> > refactoring. Could you please help find a quick solution on gecko part?
> > 
> > Thank you very much.
> > Arvin
> 
> Hi helloarvin, 
> 
> I dont't quite understand the issue. I know that why we might create two
> calls with the same number. But, will it cause any problem? Could you tell
> me how many calls will appear in gaia (call screen). I haven't test it but I
> think it should be two. If it is, we could simply hangup these two calls.
> 
> So, the problem is that does the second call shown on the screen.

Hi Aknow,

There's only one call rather than two appeared in callscreen.

Please try to reproduce the issue via editing code[1] on both v1.4 and v2.1 version, then you'll see the difference between them.

v2.1 : there'll be an error message 'Unable to make a phone call now' appeared and one call shown on callscreen;

v1.4 : one call shown on callscreen but no error message appeared for the second 'telephony.dial'

Then, we cannot dial any number for the telephony.activeCall is not null on v1.4 whereas it works well on v2.1 version.

Based on my preliminary analysis, the second 'telephony.dial' had been dropped for the flag 'isDialing' is true on v2.1 version. But in v1.4 version, the flag 'isDialing' had been reset as false due to the 'always success response' in ril_worker, and the result is that the second 'telephony.dial'(should be dropped directly) implements successfully as well. Accordingly, another 'always success response' had been sent to TelephonyProvider from ril_worker and it's just the root-cause for creation of the additional call in mozTelephony. After we hang up the call on callscreen, there'll still be an activecall in mozTelephony so that there's no chance to make dial unless re-start the dialer app.  


So, the real problem is that the second dial should be dropped as expected to make sure 'one outgoing call at a time'.

Please ni me if there's any doubts or questions.

Thanks.

Arvin

[1] please insert the test code to reproduce the issue : gaia/apps/communications/dialer/js/telephony_helper.js

Original:
    if (!activeCall) {
      startDial(cardIndex, conn, sanitizedNumber, oncall, onconnected,
                ondisconnected, onerror);
      return;
    }

Modified:
    if (!activeCall) {
      startDial(cardIndex, conn, sanitizedNumber, oncall, onconnected,
                ondisconnected, onerror);
      + startDial(cardIndex, conn, sanitizedNumber, oncall, onconnected,
      +          ondisconnected, onerror);
      return;
    }

Comment 7

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

I can't come out a easy fix or work around. However, one of the proper fix could be found here, Bug 1013745. The patches could not simply apply on 1.4, but I think it's not difficult to resolve the conflict.

Comment 8

[helloarvin]

Attachment: rebase_v1.4_ril_worker.patch

Rebase the patch on ril_worker.js for v1.4

Comment 9

[helloarvin]

Attachment: rebase_v1.4_telephony.patch

Rebase the patch on telephony for v1.4

Comment 10

[helloarvin]

(In reply to Szu-Yu Chen [:aknow] from comment #7)
> I can't come out a easy fix or work around. However, one of the proper fix
> could be found here, Bug 1013745. The patches could not simply apply on 1.4,
> but I think it's not difficult to resolve the conflict.

Hi Aknow,

Thank you for your kindly support.

The solution on bug1013745 indeed can fix the issue reported here for https://bugzilla.mozilla.org/show_bug.cgi?id=1013745#c0. As you said, there's several conflicts should be solved before it can be applied on v1.4. Due to the large changes, could you please help review the patch rebased for v1.4 and give some feedback so as to ensure no error or risk?

Thanks a lot.

Arvin

Comment 11

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

(In reply to helloarvin from comment #10)
> (In reply to Szu-Yu Chen [:aknow] from comment #7)
> > I can't come out a easy fix or work around. However, one of the proper fix
> > could be found here, Bug 1013745. The patches could not simply apply on 1.4,
> > but I think it's not difficult to resolve the conflict.
> 
> Hi Aknow,
> 
> Thank you for your kindly support.
> 
> The solution on bug1013745 indeed can fix the issue reported here for
> https://bugzilla.mozilla.org/show_bug.cgi?id=1013745#c0. As you said,
> there's several conflicts should be solved before it can be applied on v1.4.
> Due to the large changes, could you please help review the patch rebased for
> v1.4 and give some feedback so as to ensure no error or risk?
> 
> Thanks a lot.
> 
> Arvin

Both patches look good to me. Thank you.

Comment 12

[helloarvin]

Hi Aknow,

Thank you very much for great supports.
We'll do a specific testing on the patch after landed and might ask for your help if necessary.

Arvin

Comment 13

[helloarvin]

Attachment: fix_build_error.zip

Hi Aknow,

The error occurred during building process:
../../../gecko/dom/telephony/Telephony.cpp:720: error: undefined reference to 'mozilla::dom::Telephony::GetOutgoingCall()'
collect2: error: ld returned 1 exit status
make[6]: *** [libxul.so] Error 1

This should be caused by the following code:

Telephony::NotifyError(uint32_t aServiceId,
                       int32_t aCallIndex,
                       const nsAString& aError)
{
  // Special handler for dial failure.
  if (aCallIndex == -1) {
    nsRefPtr<TelephonyCall> call = GetOutgoingCall();

    // The function could be called on all telephony objects. However the
    // OutgoingCall only exists in one telephony object which dial it out. Thus,
    // for other telpehony objects, it is not an error to get a null result.
    if (call) {
      call->NotifyError(aError);
    }
    return NS_OK;
  }


We had removed the function GetOutgoingCall in the previous patch hence it should not be called anyhow.

Please help check my new patch which deleted the declaration in Telephony.h and the reference in Telephony.cpp.

Thanks.

Comment 14

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

(In reply to helloarvin from comment #13)
> Created attachment 8534197 [details]
> fix_build_error.zip
> 
> Hi Aknow,
> 
> The error occurred during building process:
> ../../../gecko/dom/telephony/Telephony.cpp:720: error: undefined reference
> to 'mozilla::dom::Telephony::GetOutgoingCall()'
> collect2: error: ld returned 1 exit status
> make[6]: *** [libxul.so] Error 1
> 
> This should be caused by the following code:
> 
> Telephony::NotifyError(uint32_t aServiceId,
>                        int32_t aCallIndex,
>                        const nsAString& aError)
> {
>   // Special handler for dial failure.
>   if (aCallIndex == -1) {
>     nsRefPtr<TelephonyCall> call = GetOutgoingCall();
> 
>     // The function could be called on all telephony objects. However the
>     // OutgoingCall only exists in one telephony object which dial it out.
> Thus,
>     // for other telpehony objects, it is not an error to get a null result.
>     if (call) {
>       call->NotifyError(aError);
>     }
>     return NS_OK;
>   }
> 
> 
> We had removed the function GetOutgoingCall in the previous patch hence it
> should not be called anyhow.
> 
> Please help check my new patch which deleted the declaration in Telephony.h
> and the reference in Telephony.cpp.
> 
> Thanks.

Sorry that I didn't find this in the previous round of review.

I think your fix is correct.
The case of callIndex == -1 is removed here (attachment 8532995 [details] [diff] [review] , ril_worker.js line: 5377). So in Telephony.cpp, we don't need the if block, and removing it should not cause any problem.

Thank you for catching the problem and fix it.

Comment 15

[helloarvin]

Thanks for your quick feedback:-)

Comment 16

[helloarvin]

fixed.