If you think a bug might affect users in the 57 release, please set the correct tracking and status flags for Release Management.

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

RESOLVED FIXED in Firefox OS v2.0M

Status

Firefox OS
RIL
RESOLVED FIXED
3 years ago
3 years ago

People

(Reporter: hsinyi, Assigned: hsinyi)

Tracking

(Blocks: 1 bug)

unspecified
2.0 S1 (9may)
ARM
Gonk (Firefox OS)
Dependency tree / graph

Firefox Tracking Flags

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

Details

Attachments

(2 attachments)

(Assignee)

Description

3 years ago
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)

Updated

3 years ago
Assignee: nobody → htsai
(Assignee)

Comment 1

3 years ago
(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
(Assignee)

Comment 2

3 years ago
Created attachment 8511863 [details] [diff] [review]
WIP
(Assignee)

Comment 3

3 years ago
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)
Blocks: 1083402
Duplicate of this bug: 1083402
(Assignee)

Comment 7

3 years ago
(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)
(Assignee)

Comment 8

3 years ago
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+
Blocks: 977056
(Assignee)

Comment 10

3 years ago
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
(Assignee)

Comment 11

3 years ago
Comment on attachment 8511863 [details] [diff] [review]
WIP

Thanks for the comment!
Attachment #8511863 - Flags: feedback?(gtorodelvalle)
(Assignee)

Comment 12

3 years ago
https://hg.mozilla.org/integration/b2g-inbound/rev/bd1f60da7096
https://hg.mozilla.org/mozilla-central/rev/bd1f60da7096
Status: NEW → RESOLVED
Last Resolved: 3 years ago
Resolution: --- → FIXED
Created attachment 8540518 [details] [diff] [review]
(2.0m) [B2G][Telephony] should GetCallFromEverywhere in Telephony::NotifyError. r=aknow
Attachment #8540518 - Flags: review+
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)

Comment 16

3 years ago
Hi Aknow,
Thanks for the heads up.

Hi Kai-Zhen,
Can you help to land this to 2.0M? Thanks!
Blocks: 1054172
blocking-b2g: 2.0M? → 2.0M+
status-b2g-v2.0: --- → affected
status-b2g-v2.0M: --- → affected
status-b2g-v2.1: --- → affected
status-b2g-v2.2: --- → fixed
Flags: needinfo?(jocheng) → needinfo?(kli)
OS: Linux → Gonk (Firefox OS)
Hardware: x86_64 → ARM
Duplicate of this bug: 1108966
https://hg.mozilla.org/releases/mozilla-b2g32_v2_0m/rev/372efde96b1e
status-b2g-v2.0M: affected → fixed
Flags: needinfo?(kli)
Does this need to land on v2.1? If so, please request b2g34 approval on the patch.
Flags: needinfo?(htsai)
(Assignee)

Comment 20

3 years ago
(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)

Comment 21

3 years ago
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)

Updated

3 years ago
Blocks: 1080481
: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.