Closed Bug 1089534 Opened 10 years ago Closed 10 years ago

[B2G][Telephony] should GetCallFromEverywhere in Telephony::NotifyError

Categories

(Firefox OS Graveyard :: RIL, defect)

ARM
Gonk (Firefox OS)
defect
Not set
normal

Tracking

(blocking-b2g:2.0M+, b2g-v2.0 affected, b2g-v2.0M fixed, b2g-v2.1 affected, b2g-v2.2 fixed)

RESOLVED FIXED
2.0 S1 (9may)
blocking-b2g 2.0M+
Tracking Status
b2g-v2.0 --- affected
b2g-v2.0M --- fixed
b2g-v2.1 --- affected
b2g-v2.2 --- fixed

People

(Reporter: hsinyi, Assigned: hsinyi)

References

Details

Attachments

(2 files)

Consider a scenario: a call in a conference is released with error. Then, [1] is reached and an exception is thrown because that call isn't in mCalls but in mGroup. We need to fix it by calling |nsRefPtr<TelephonyCall> callToNotify = GetCallFromEverywhere(aServiceId, aCallIndex);|
Assignee: nobody → htsai
(In reply to Hsin-Yi Tsai [:hsinyi] from comment #0) > Consider a scenario: a call in a conference is released with error. Then, > [1] is reached and an exception is thrown because that call isn't in mCalls > but in mGroup. We need to fix it by calling |nsRefPtr<TelephonyCall> > callToNotify = GetCallFromEverywhere(aServiceId, aCallIndex);| [1] http://dxr.mozilla.org/mozilla-central/source/dom/telephony/Telephony.cpp?from=Telephony.cpp#637
Attached patch WIP (deleted) — Splinter Review
Comment on attachment 8511863 [details] [diff] [review] WIP Review of attachment 8511863 [details] [diff] [review]: ----------------------------------------------------------------- Hi Germán , Mind giving a try ?
Attachment #8511863 - Flags: feedback?(gtorodelvalle)
I looove it!!! :) At least I am starting to see the end of the tunnel :p OK, good news is that I am getting the same results as Johan did last Friday as mentioned in https://bugzilla.mozilla.org/show_bug.cgi?id=977056#c76 You can see the logs here: https://pastebin.mozilla.org/6943662 , since Johan's pastebin expired :( As mentioned in that comment, we are getting an event notification which was not expected according to our specs: https://bug977056.bugzilla.mozilla.org/attachment.cgi?id=8510217 It is the last one, from line 47 on in https://pastebin.mozilla.org/6943662 Is it right? Just to add it to our specs or to file a bug in case it is not right :)
BTW, I didn't want to set the feedback to + or - until we clarify this last event notification issue :)
Flags: needinfo?(htsai)
(In reply to Germán Toro del Valle from comment #4) > I looove it!!! :) At least I am starting to see the end of the tunnel :p > > OK, good news is that I am getting the same results as Johan did last Friday > as mentioned in https://bugzilla.mozilla.org/show_bug.cgi?id=977056#c76 You > can see the logs here: https://pastebin.mozilla.org/6943662 , since Johan's > pastebin expired :( > > As mentioned in that comment, we are getting an event notification which was > not expected according to our specs: > https://bug977056.bugzilla.mozilla.org/attachment.cgi?id=8510217 It is the > last one, from line 47 on in https://pastebin.mozilla.org/6943662 Is it > right? Just to add it to our specs or to file a bug in case it is not right > :) The last additional event TelephonyCallGroup.onstatechange is expected because in this case, telephonyCallGroup state is changing from 'connected' to 'unknown' (no calls in the group). Everything looks perfect from the API point of view :)
Flags: needinfo?(htsai)
Comment on attachment 8511863 [details] [diff] [review] WIP Review of attachment 8511863 [details] [diff] [review]: ----------------------------------------------------------------- Hi Aknow, Mind taking a look? Thank you!
Attachment #8511863 - Flags: review?(szchen)
Comment on attachment 8511863 [details] [diff] [review] WIP Review of attachment 8511863 [details] [diff] [review]: ----------------------------------------------------------------- LGTM. Thank you.
Attachment #8511863 - Flags: review?(szchen) → review+
https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=171722783fd1 looks good in addition to nice local marionette-webapi tests. Going to land this soon
Comment on attachment 8511863 [details] [diff] [review] WIP Thanks for the comment!
Attachment #8511863 - Flags: feedback?(gtorodelvalle)
Status: NEW → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
I suggest to land this patch on 2.0m and the risk is pretty low. From our analysis, we suspect that it's the root cause that lead to Bug 1108966. Moreover, we also observed the exactly same syndrome in the log and that really convinced me. So, please consider the uplifting.
blocking-b2g: --- → 2.0M?
Flags: needinfo?(jocheng)
Hi Aknow, Thanks for the heads up. Hi Kai-Zhen, Can you help to land this to 2.0M? Thanks!
Blocks: Woodduck
blocking-b2g: 2.0M? → 2.0M+
Flags: needinfo?(jocheng) → needinfo?(kli)
OS: Linux → Gonk (Firefox OS)
Hardware: x86_64 → ARM
Does this need to land on v2.1? If so, please request b2g34 approval on the patch.
Flags: needinfo?(htsai)
(In reply to [Away 24-Dec to 4-Jan] Ryan VanderMeulen [:RyanVM UTC-5] from comment #19) > Does this need to land on v2.1? If so, please request b2g34 approval on the > patch. Thanks Ryan! Hi Josh, Do we need this for v2.1 as well?
Flags: needinfo?(htsai) → needinfo?(jocheng)
Target Milestone: --- → 2.0 S1 (9may)
Hi Bhavana, Per my comment at https://bugzilla.mozilla.org/show_bug.cgi?id=1102671#c13 Please help to make the decision. Thanks!
Flags: needinfo?(jocheng) → needinfo?(bbajaj)
:hsinyi, if you think this is a critical path for partners on 2.1, its fine to uplift.
Flags: needinfo?(bbajaj)
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: