Last Comment Bug 717158 - B2G telephony: unknown (outgoing) call index is assumed to be incoming later
: B2G telephony: unknown (outgoing) call index is assumed to be incoming later
Status: RESOLVED FIXED
:
Product: Core
Classification: Components
Component: DOM: Device Interfaces (show other bugs)
: Trunk
: All All
: -- normal (vote)
: mozilla12
Assigned To: Ben Turner (not reading bugmail, use the needinfo flag!)
:
Mentors:
Depends on:
Blocks: webtelephony
  Show dependency treegraph
 
Reported: 2012-01-10 22:50 PST by Philipp von Weitershausen [:philikon]
Modified: 2012-01-13 05:07 PST (History)
4 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Patch, v1 (5.35 KB, patch)
2012-01-11 09:09 PST, Ben Turner (not reading bugmail, use the needinfo flag!)
no flags Details | Diff | Review
Patch, v2 (5.72 KB, patch)
2012-01-11 13:41 PST, Philipp von Weitershausen [:philikon]
cjones.bugs: review+
Details | Diff | Review

Description Philipp von Weitershausen [:philikon] 2012-01-10 22:50:54 PST
Regression introduced in bug 674726: an outgoing call somehow registers as an "incoming" one. Unsurprisingly, the following assertion is logged:

###!!! ASSERTION: Serious logic problem here!: 'aCallState == nsITelephone::CALL_STATE_INCOMING', file /home/philipp/dev/mc.hg/dom/telephony/Telephony.cpp, line 380

Subsequent events don't make it to the DOM either and often eventually lead to a crash. On the device it doesn't crash, but the DOM events seem to be AWOL as well and audio isn't turned on always (especially for incoming calls.)

I wouldn't be surprised if there were several bugs here, but for now I don't have any specific leads. I will start by reading the ril_worker.js and nsTelephoneWorker.js changes in bug 674726 very carefully again. There also seems to be something finish in the C++ side, particularly if you consider the crash in bug 717156.
Comment 1 Ben Turner (not reading bugmail, use the needinfo flag!) 2012-01-11 08:19:04 PST
Hm, I goofed the outgoing index code somehow. Patch in a sec.
Comment 2 Ben Turner (not reading bugmail, use the needinfo flag!) 2012-01-11 09:09:58 PST
Created attachment 587723 [details] [diff] [review]
Patch, v1

I think this should do it. I wasn't handling that weird case where we don't yet know the RIL's callIndex yet properly.
Comment 3 Philipp von Weitershausen [:philikon] 2012-01-11 13:41:50 PST
Created attachment 587806 [details] [diff] [review]
Patch, v2

bent and I found an additional problem that the v1 patch did not address.
Comment 4 Chris Jones [:cjones] inactive; ni?/f?/r? if you need me 2012-01-11 17:03:31 PST
Comment on attachment 587806 [details] [diff] [review]
Patch, v2

>diff --git a/dom/telephony/Telephony.cpp b/dom/telephony/Telephony.cpp

>+        tempCall->CallState() < nsITelephone::CALL_STATE_CONNECTED) {

I prefer using bitfields and bitmasks to express sets of states,
but that can be fixed in a followup.  DISCONNECTED is > CONNECTED
so this is OK.

>+    if (tempCall->CallIndex() == PR_UINT32_MAX) {

Make a symbolic constant for this like
CALL_INDEX_OUTGOING_PLACEHOLDER (or something like that).  Make
sure to update the other places it's used.

>+  // If nothing matched above and the call state isn't incoming but we do have
>+  // an outgoing call then we must be seeing a status update for out outgoing
>+  // call.
>+  if (!modifiedCall &&
>+      aCallState != nsITelephone::CALL_STATE_INCOMING &&
>+      outgoingCall) {

This is pretty subtle but it looks right to me.  The problem is a
race condition between Gecko placing a call and ril receiving
one.

>diff --git a/dom/telephony/TelephonyCall.cpp b/dom/telephony/TelephonyCall.cpp

>+  if (aCallState == nsITelephone::CALL_STATE_DIALING) {
>+    mOutgoing = true;
>+  }
>+

Nit: |mOutgoing = (aCallState == nsITelephone::CALL_STATE_DIALING);|

pinch-r=me with those fixes.  I think this is pretty small but
let's ask Mounir if he wants a follow-on review.
Comment 5 Ben Turner (not reading bugmail, use the needinfo flag!) 2012-01-11 18:23:11 PST
(In reply to Chris Jones [:cjones] [:warhammer] from comment #4)
> Nit: |mOutgoing = (aCallState == nsITelephone::CALL_STATE_DIALING);|

This code gets run several times so I don't want to unset that. Thanks for the review!

https://hg.mozilla.org/integration/mozilla-inbound/rev/a9eb2cfdebd8
Comment 6 Matt Brubeck (:mbrubeck) 2012-01-12 11:05:17 PST
https://hg.mozilla.org/mozilla-central/rev/a9eb2cfdebd8
Comment 7 Mounir Lamouri (:mounir) 2012-01-13 05:07:23 PST
Sorry for not getting to this review faster, I was at a career fair yesterday and didn't find time for this.

Note You need to log in before you can comment on or make changes to this bug.