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

Status: RESOLVED FIXED

(Firefox OS Graveyard :: RIL, defect)

Description

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

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);|

Comment 1

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

(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

Comment 2

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

Attachment: WIP

Comment 3

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

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

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

Hi Germán ,

Mind giving a try ?

Comment 4

[Germán Toro del Valle (: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 :)

Comment 5

[Germán Toro del Valle (:gtorodelvalle)]

BTW, I didn't want to set the feedback to + or - until we clarify this last event notification issue :)

Comment 7

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

(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 :)

Comment 8

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

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

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

Hi Aknow,
Mind taking a look? Thank you!

Comment 9

[Szu-Yu Chen [:aknow] (inactive)]

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

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

LGTM. Thank you.

Comment 10

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

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 11

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

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

Thanks for the comment!

Comment 12

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

https://hg.mozilla.org/integration/b2g-inbound/rev/bd1f60da7096

Comment 13

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/bd1f60da7096

Comment 14

[Szu-Yu Chen [:aknow] (inactive)]

Attachment: (2.0m) [B2G][Telephony] should GetCallFromEverywhere in Telephony::NotifyError. r=aknow

Comment 15

[Szu-Yu Chen [:aknow] (inactive)]

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.

Comment 16

[Josh Cheng [:josh]]

Hi Aknow,
Thanks for the heads up.

Hi Kai-Zhen,
Can you help to land this to 2.0M? Thanks!

Comment 18

[Kai-Zhen Li [:seinlin][:kli]]

https://hg.mozilla.org/releases/mozilla-b2g32_v2_0m/rev/372efde96b1e

Comment 19

[Ryan VanderMeulen [:RyanVM]]

Does this need to land on v2.1? If so, please request b2g34 approval on the patch.

Comment 20

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

(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?

Comment 21

[Josh Cheng [:josh]]

Hi Bhavana,
Per my comment at https://bugzilla.mozilla.org/show_bug.cgi?id=1102671#c13 
Please help to make the decision. Thanks!

Comment 22

[bhavana bajaj [:bajaj]]

:hsinyi, if you think this is a critical path for partners on 2.1, its fine to uplift.