Closed Bug 821559 Opened 12 years ago Closed 12 years ago

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

Categories

(Core :: DOM: Device Interfaces, defect)

x86
macOS
defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla21
blocking-basecamp +
Tracking Status
firefox19 --- wontfix
firefox20 --- wontfix
firefox21 --- fixed
b2g18 --- fixed

People

(Reporter: gwagner, Assigned: hsinyi)

Details

Attachments

(1 file, 1 obsolete file)

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
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.
blocking-basecamp: --- → ?
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
Could this be some sort of gecko <-> gaia mismatch?

QA:  can you reproduce?

Ben, any thoughts here?  Doug suggested you might have some insight.
Flags: needinfo?(bent.mozilla)
Keywords: qawanted
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."
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
Removing QA wanted
Keywords: qawanted
Comment 5 sounds right to me.
Flags: needinfo?(bent.mozilla)
Assignee: nobody → htsai
Attached patch WIP (obsolete) — Splinter Review
Address assertion more gracefully
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!
Attachment #697844 - Flags: feedback?(bent.mozilla)
blocking-basecamp: ? → +
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.
Attachment #697844 - Flags: feedback?(bent.mozilla) → review+
Attached patch Patch. r=bentSplinter Review
Comment 10 addressed.
Attachment #697844 - Attachment is obsolete: true
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 :)
Keywords: qawanted
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.
Keywords: qawanted
Sorry, I meant to land as soon as possible so we get maximum dogfood and QA testing. Let's get this in.
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 on attachment 698542 [details] [diff] [review]
Patch. r=bent

># HG changeset patch
># User Hsin-Yi Tsai <htsai@mozilla.com>
># 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
(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 <htsai@mozilla.com>
> ># 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 :)
(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!
https://hg.mozilla.org/mozilla-central/rev/9cfb277387dc
Status: NEW → RESOLVED
Closed: 12 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla21
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: