963813 - [B2G][Dialer] Initiating conference call while the active call is to an invalid number, causes the dialer app to freeze

Status: RESOLVED FIXED

(Firefox OS Graveyard :: RIL, defect)

Description

[Salem Elkabule]

Attachment: Dialer freze.txt

Description:
Having placed a call to 1234 then attempting to call a saved contact will result in the state when screen gets frozen without any option to exit

Repro Steps:
1) Updated Buri V1.3 
2) Have at least one contact saved
3) Open Dialer app
4) Call 1234 and quickly tap (+)
5) Select the contact from the contact list

Actual:
The call will hangs and the dialer app freezes in the state without an option to exit or hang up

Expected:
The screen will transition to the active call screen and pause the previous call 

Environmental Variables
Device: buri V1.3 Moz Ril
Build ID: 20140121004137
Gecko: http://hg.mozilla.org/releases/mozilla-aurora/rev/6f7dfe36ab6c
Gaia: 47049555282a9a01fb60d1e1421b57e2810c96f5
Platform Version: 28.0a2 
Firmware Version: v1.2-device.cfg

Notes: the user have to restart their device to be able to have any interaction 

Repro frequency: 100%
See attached: video clip (http://www.youtube.com/watch?v=q4_1kAxNBfI&edit=vd), logcat, 

Extra Notes: Sometimes the device freezes completely

Comment 1

[Salem Elkabule]

This issue does not reproduce on buri V1.2

Environmental Variables
Device: Buri v 1.2.0 MOZ
Build ID: 20140121004053
Gecko: http://hg.mozilla.org/releases/mozilla-b2g26_v1_2/rev/c9f305c1d9a7
Gaia: 539a25e1887b902b8b25038c547048e691bd97f6
Platform Version: 26.0
Firmware Version: v1.2-device.cfg

The user is able to cancel any hanging calls and close the dialer application

Comment 2

[Beatriz Rodríguez [:brg]]

This is a certification blocker, please set flag 1.3+

Comment 3

[Nikolai Khristoforov]

The regression window for this bug is 11/20 to 11/21.

Last Working Build:
Device: Buri 1.3 MOZ
BuildID: 20131120040202
Gaia: c26480b22ce28c812c347290dd4bad090d83db6f
Gecko: 4f993fa378eb
Version: 28.0a1
Firmware Version: V1.2-device.cfg

First Broken Build:
Device: Buri 1.3 MOZ
BuildID: 20131121040202
Gaia: 71063dd91bc8cbb15ba335236ed67a1c5058bd58
Gecko: cf378dddfac8
Version: 28.0a1
Firmware Version: V1.2-device.cfg

Comment 4

[Joe Cheng [:jcheng] (please needinfo)]

rik, mind taking a look? Thanks

Comment 5

[KM Lee [:rexboy] (inactivate after Sep. 22)]

I guess we may need help from RIL, seems there are no significant error messages or events caught by Dialer

Comment 6

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

(In reply to KM Lee [:rexboy] from comment #5)
> I guess we may need help from RIL, seems there are no significant error
> messages or events caught by Dialer

I saw a JS Error as below. No RIL messages in the log |Dialer freze.txt|, but according to the JS Error, it seems gecko did deliver an error event with error name "UnspecifiedError" to gaia. However, gaia doesn't recognize this name that leads to call screen hanging.

==================
01-24 11:28:54.579: E/GeckoConsole(501): [JavaScript Warning: "Error in parsing value for 'font-size'.  Declaration dropped." {file: "app://communications.gaiamobile.org/dialer/index.html#contacts-view" line: 0 column: 0 source: "40"}]
01-24 11:28:54.649: E/GeckoConsole(501): Content JS ERROR at app://communications.gaiamobile.org/dialer/js/telephony_helper.js:126 in errorCB: Unexpected error:  UnspecifiedError
==================

Comment 7

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

Attachment: log-963813 - RIL log

Seems not a gaia issue but a gecko bug.

In line#5937 (as below), we try to hang up a call, whose connection hasn't been established (we can tell that from its callindex), that causes a problem. The REQUEST_HANGUP fails but our code doesn't handle this error properly.

============= line#5937 ==============
01-28 18:36:13.259 I/Gecko   ( 1344): RIL Worker[0]: Received chrome message {"callIndex":4294967295,"rilMessageToken":15,"rilMessageType":"hangUp"}

Comment 8

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

Attachment: 963813.patch

properly hang up a call whose connection hasn't really established

Comment 9

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

Comment on attachment 8366567 [details] [diff] [review]
963813.patch

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

Hi Aknow,
Could you help review this? Thanks!

Comment 10

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

(In reply to Hsin-Yi Tsai (OOO Jan. 29 ~ Feb. 5th) [:hsinyi] from comment #7)
> Created attachment 8366565 [details]
> log-963813 - RIL log
> 
> Seems not a gaia issue but a gecko bug.
> 
> In line#5937 (as below), we try to hang up a call, whose connection hasn't
> been established (we can tell that from its callindex), that causes a
> problem. The REQUEST_HANGUP fails but our code doesn't handle this error
> properly.
> 
Sorry the comment isn't correct. In this case, I don't see REQUEST_HANGUP returns error.

> ============= line#5937 ==============
> 01-28 18:36:13.259 I/Gecko   ( 1344): RIL Worker[0]: Received chrome message
> {"callIndex":4294967295,"rilMessageToken":15,"rilMessageType":"hangUp"}

Comment 11

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

I reviewed the log again, and I wonder my previous patch doesn't really point to the root cause.

From the log Attachment #8366565 [details], the second call(0916259153) is still dialed out even the 1st call(12345) state stays "alerting/connecting". However, [1] doesn't allow the 2nd outgoing call if there's already a dialing call. Needs to figure out what's wrong with the code.

============
line#5415 01-28 18:35:39.689 I/Gecko   ( 1344): TelephonyProvider: handleCallStateChange: {"state":3,"callIndex":1,"toa":129,"isMpty":false,"isMT":false,"als":0,"isVoice":true,"isVoicePriv     acy":false,"number":"12345","numberPresentation":0,"name":"","namePresentation":0,"uusInfo":null,"isOutgoing":true,"isEmergency":false,"isConference":false}

line#5459 01-28 18:35:50.289 I/Gecko   ( 1344): -*- RadioInterface[0]: Received message from worker: {"rilMessageToken":13,"rilMessageType":"enumerateCalls","calls":[{"state":3,"callIndex"     :1,"toa":129,"isMpty":false,"isMT":false,"als":0,"isVoice":true,"isVoicePrivacy":false,"number":"12345","numberPresentation":0,"name":"","namePresentation":0,"uusInfo":null,"isOu     tgoing":true,"isEmergency":false,"isConference":false}]}

line#5469 01-28 18:35:50.359 I/Gecko   ( 1344): TelephonyProvider: Dialing 0916259153
============

[1] http://dxr.mozilla.org/mozilla-central/source/dom/telephony/Telephony.cpp?from=Telephony.cpp#249

Comment 12

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

Attachment: 963813-v2.patch

[1] checks if call->IsOutgoing() is true while IsOutgoing() returns mOutgoing which is set to true if |CallState == STATE_DIALING|.

But dialer app initiates different telephony objects for telephony.dial(). And think about the case:  A new telephony object is created while there's already one outgoing call on-going. The new telephony object calls enumerate() and it gets response from RIL. If the response time is longer, then the first outgoing call might change to 'alerting' state. So when the new telephony objects new a call object for this call after the response from RIL, mOutgoing won't be set to true because the call state isn't dialing... 

Back to the bug, we don't really need IsOutgoing(). What we need is check the call state and see if the call is still in dialing process.
[1] http://dxr.mozilla.org/mozilla-central/source/dom/telephony/Telephony.cpp#221

Comment 13

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

Comment on attachment 8367134 [details] [diff] [review]
963813-v2.patch

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

Good.

Comment 14

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

Attachment: 963813-v3.patch. r=aknow

FINAL version - remove unreferenced members.

Comment 15

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

(In reply to Hsin-Yi Tsai (OOO Jan. 29 ~ Feb. 5th) [:hsinyi] from comment #14)
> Created attachment 8367137 [details] [diff] [review]
> 963813-v3.patch. r=aknow
> 
> FINAL version - remove unreferenced members.

The patch is ready and manual test looks great.
However, try tree is closed. I will push to try and check-in later. (Time to catch my train to home!)

Comment 16

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

Attachment: 963813-final-aurora.patch r=aknow, a=1.3+

patch for mozilla-aurora (1.3)

Comment 17

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

Comment on attachment 8367137 [details] [diff] [review]
963813-v3.patch. r=aknow

FINAL patch for mozilla-central

Comment 18

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

Attachment: 963813-p2.patch -- part2 - test case

Though the try server looks green, I hit error when answer(call) was called in setupConferenceThreeCalls(), in test_conference.js. 
=========
TEST-UNEXPECTED-FAIL | test_conference.js | conference.state - got connected, expected held
=========
With debugging I realized that it's a timing issue.

When there's already a connected conference call, answering a new incoming call triggers conference state change. We should wait for |conference.onstatechange| before checking the state of the conference call.

Hi Aknow/Vicamo/Yoshi,
Hope you are not gone for the Chinese new year. Could any of you help review this? Thanks!

Comment 19

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

(In reply to Hsin-Yi Tsai (OOO Jan. 29 ~ Feb. 5th) [:hsinyi] from comment #18)
> Created attachment 8367415 [details] [diff] [review]
> 963813-p2.patch -- part2 - test case
> 
> Though the try server looks green, I hit error when answer(call) was called
 
I hit the error when I ran marionette locally

> in setupConferenceThreeCalls(), in test_conference.js. 
> =========
> TEST-UNEXPECTED-FAIL | test_conference.js | conference.state - got
> connected, expected held
> =========
> With debugging I realized that it's a timing issue.
> 
> When there's already a connected conference call, answering a new incoming
> call triggers conference state change. We should wait for
> |conference.onstatechange| before checking the state of the conference call.
> 
> Hi Aknow/Vicamo/Yoshi,
> Hope you are not gone for the Chinese new year. Could any of you help review
> this? Thanks!

Comment 20

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

Comment on attachment 8367415 [details] [diff] [review]
963813-p2.patch -- part2 - test case

Okay... bug 943275 has been filed for tracking the test script failure before. And there's indeed a flaw in the script code. I will move the test case to bug 943275 to not block this blocker (because it's chinese new year now... reviewers are from computer...)

Comment 21

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

try on m-c looks green: https://tbpl.mozilla.org/?tree=Try&rev=60cbd8d190d2a
try on m-aurora failed due to bug 943275, rest looking good: https://tbpl.mozilla.org/?tree=Try&rev=60cbd8d190d2a

As bug 943275 has been there for a while, it's not caused by this patch. I say this patch is safe to go. I will provide the solution to bug 943275 after I come back to office.

push: https://tbpl.mozilla.org/?tree=Try&rev=60cbd8d190d2a

Comment 22

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

(In reply to Hsin-Yi Tsai (OOO Jan. 29 ~ Feb. 5th) [:hsinyi] from comment #21)
> try on m-c looks green: https://tbpl.mozilla.org/?tree=Try&rev=60cbd8d190d2a
> try on m-aurora failed due to bug 943275, rest looking good:
> https://tbpl.mozilla.org/?tree=Try&rev=60cbd8d190d2a
> 
> As bug 943275 has been there for a while, it's not caused by this patch. I
> say this patch is safe to go. I will provide the solution to bug 943275
> after I come back to office.
> 
> push: https://tbpl.mozilla.org/?tree=Try&rev=60cbd8d190d2a

push:https://hg.mozilla.org/integration/b2g-inbound/rev/49bc6f0b1d01 this is the right link.

Comment 23

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/49bc6f0b1d01

Comment 24

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/ab540076cb46