821559 - Telephony: ASSERTION: Serious logic problem here!: 'aCallState == nsIRadioInterfaceLayer::CALL_STATE_INCOMING

Status: RESOLVED FIXED

(Core :: DOM: Device Interfaces, defect)

Description

[Gregor Wagner [:gwagner]]

On otoro with B2G git tip debug build.
After cancelling an outgoing call. 

E/GeckoConsole(  439): [JavaScript Warning: "HTTP "Content-Type" of "text/html" is not supported. Load of media resource app://communications.gaiamobile.org/dialer/oncall.html failed." {file: "app://communications.gaiamobile.org/dialer/oncall.html#unlocked" line: 0}]
I/Gecko   (  439): [Child 439] ###!!! ASSERTION: Serious logic problem here!: 'aCallState == nsIRadioInterfaceLayer::CALL_STATE_INCOMING || aCallState == nsIRadioInterfaceLayer::CALL_STATE_DIALING', file /Volumes/2mac/otoro/2/B2G/gecko/dom/telephony/Telephony.cpp, line 410
I/Gecko   (  439): [Child 439] ###!!! ASSERTION: Something is really wrong here!: 'mCalls[index]->CallIndex() != aCallIndex', file /Volumes/2mac/otoro/2/B2G/gecko/dom/telephony/Telephony.cpp, line 440
E/GeckoConsole(  439): Content JS WARN at app://communications.gaiamobile.org/shared/js/l10n.js:52 in consoleWarn: [l10n] [l10n] #connecting is undefined.
E/AudioHardwareMSM76XXA(  111): setMode:mode = 2, status = -17
E/AudioPolicyManager(  111): setPhoneState() state 2
W/AudioPolicyManager(  111): setPhoneState() setting same state 2

Comment 1

[Gregor Wagner [:gwagner]]

I noticed that making a few phone calls that aren't picked up and receiving phone calls that end leave our phone in a bad state. After some time no more calls can be received.

I also notice once that tabbing on a missed call entry on our Call log made my phone call itself.

Comment 2

[Gregor Wagner [:gwagner]]

Another one is 
[Child 513] ###!!! ASSERTION: Serious logic problem here!: 'aCallState == nsIRadioInterfaceLayer::CALL_STATE_INCOMING || aCallState == nsIRadioInterfaceLayer::CALL_STATE_DIALING', file /Volumes/2mac/gaia/3src/dom/telephony/Telephony.cpp, line 410
[Child 513] ###!!! ASSERTION: Should be live!: 'mLive', file /Volumes/2mac/gaia/3src/dom/telephony/TelephonyCall.cpp, line 92
[Child 513] ###!!! ASSERTION: Didn't know about this one!: 'mCalls.Contains(aCall)', file /Volumes/2mac/gaia/3src/dom/telephony/Telephony.h, line 73
[Child 513] ###!!! ASSERTION: Should have auto-added new call!: 'mCalls.Contains(call)', file /Volumes/2mac/gaia/3src/dom/telephony/Telephony.cpp, line 416

Comment 3

[Andrew Overholt [:overholt]]

Could this be some sort of gecko <-> gaia mismatch?

QA:  can you reproduce?

Ben, any thoughts here?  Doug suggested you might have some insight.

Comment 4

[Andrew Overholt [:overholt]]

Gregor told me on IRC that he "filed it on the 13th and yesterday saw the same problem. So it doesn't look like a tip issue."

Comment 5

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

I investigated this issue a little bit. This issue seems resulting from the asynchronous 'enumerateCalls' in Telephony constructor. 

In Telephony constructor, we call |aRIL->EnumerateCalls| to make sync with the RIL status and obtain calls which have been there before this telephony object created. The code works well if the enumerate results are sent back early than any possible following callstate change events. Unfortunately, that's not always the case. Taking an incoming call as an example:

dialer.js is launched by 'telephony-new-call' system message --> 
oncall.js is opened and instantiates to mozTelephony -->
in Telephony constructor, aRIL->EnumerateCalls --> 
callstate change event comes before enumerate result & an incoming call object is created-->
enumerate result comes back, then *assertion* failure http://mxr.mozilla.org/mozilla-central/source/dom/telephony/Telephony.cpp#439

Comment 6

[[e-mail] [:onecyrenus]]

Removing QA wanted

Comment 7

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

Comment 5 sounds right to me.

Comment 8

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

Attachment: WIP

Address assertion more gracefully

Comment 9

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

As comment 5, the reported assertion results from asynchronous enumerateCalls(). I was first thinking that we don't send any call state change events from RIL until the call enumeration result is sent back to the callback. However, this method might miss several call state transition information as my second thought. Since we don't guarantee the enumeration results arrive before any following call state events, this wip addresses the assertion and condition more gracefully.

Ben, how do think about this? Any comments? Thanks!

Comment 10

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

Comment on attachment 697844 [details] [diff] [review]
WIP

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

I think this looks ok. It will need some good testing though, so let's get it in quick.

::: dom/telephony/Telephony.cpp
@@ +436,5 @@
>    // Make sure we don't somehow add duplicates.
>    for (uint32_t index = 0; index < mCalls.Length(); index++) {
> +    nsRefPtr<TelephonyCall>& tempCall = mCalls[index];
> +    if (tempCall->CallIndex() == aCallIndex) {
> +      NS_WARNING("We have the call already. Skip it.");

I guess this isn't worth warning about now.

Comment 11

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

Attachment: Patch. r=bent

Comment 10 addressed.

Comment 12

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

Telephony marionette tests are passed.
Try result looks good as well 
https://tbpl.mozilla.org/?tree=Try&rev=d5a92c0e6861

However as bent said, we still need some good testing before pushing the patch. So asking for QA's help to better test the patch. Thank you :)

Comment 13

[Geo Mealer [:geo] -- This account is inactive after 2015-07-07]

Hsin-Yi, we don't typically test patches before checkin. If you want to arrange something specifically with a QA engineer, you'll need to provide them with a flashable provisional build, etc.

Go ahead and reply if that's how you want to go, otherwise we'll verify after landing. Removing qawanted in the meantime.

Comment 14

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

Sorry, I meant to land as soon as possible so we get maximum dogfood and QA testing. Let's get this in.

Comment 15

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

Sorry that I misunderstood the procedure, and thanks for clarifying that for me :)

The patch works with automatic tests and the manually test by myself. I will land it right away!

Comment 16

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

https://hg.mozilla.org/integration/mozilla-inbound/rev/9cfb277387dc

Comment 17

[Daniel Holbert [:dholbert]]

Comment on attachment 698542 [details] [diff] [review]
Patch. r=bent

># HG changeset patch
># User Hsin-Yi Tsai <[e-mail]>
># Date 1357289538 -28800
># Node ID 7b9948d5a2dd0683d9edf54aa07268e66eff8a74
># Parent  38407b98003bd57a1cf03fe71a50f66761932174
>Bug 821559 - Telephony: ASSERTION: Serious logic problem here. r=bent

For future reference: commit messages should include a brief explanation of what you changed, rather than describing the problem.  (This means that usually, the bug summary does not make a good commit message.)

See: https://developer.mozilla.org/en-US/docs/Developer_Guide/Committing_Rules_and_Responsibilities#Checkin_comment

Comment 18

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

(In reply to Daniel Holbert [:dholbert] from comment #17)
> Comment on attachment 698542 [details] [diff] [review]
> Patch. r=bent
> 
> ># HG changeset patch
> ># User Hsin-Yi Tsai <[e-mail]>
> ># Date 1357289538 -28800
> ># Node ID 7b9948d5a2dd0683d9edf54aa07268e66eff8a74
> ># Parent  38407b98003bd57a1cf03fe71a50f66761932174
> >Bug 821559 - Telephony: ASSERTION: Serious logic problem here. r=bent
> 
> For future reference: commit messages should include a brief explanation of
> what you changed, rather than describing the problem.  (This means that
> usually, the bug summary does not make a good commit message.)
> 
> See:
> https://developer.mozilla.org/en-US/docs/Developer_Guide/
> Committing_Rules_and_Responsibilities#Checkin_comment

Daniel, thanks for reminding :)

Comment 19

[Geo Mealer [:geo] -- This account is inactive after 2015-07-07]

(In reply to Hsin-Yi Tsai [:hsinyi] from comment #15)
> Sorry that I misunderstood the procedure, and thanks for clarifying that for
> me :)
> 
> The patch works with automatic tests and the manually test by myself. I will
> land it right away!

No worries. It'd be awesome if we did have the bandwidth to pretest every patch.

Thanks for the desk-testing prior to checkin--wish everyone was that thorough!

Comment 20

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/9cfb277387dc

Comment 21

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-b2g18/rev/9b82e914c2c4