979626 - Crash in Telephony while running stability scripts

Status: RESOLVED FIXED

(Firefox OS Graveyard :: RIL, defect)

Description

[Greg Grisco]

Attachment: 39dbcfa3-e261-bf19-65bd8866-6732921e.extra

I don't have STR at this point, just the minidump info and log snippet (in the attached .extra file)

 [@ mozPersonalDictionary::AddRef() | mozilla::dom::Telephony::NotifyCdmaCallWaiting(unsigned int, nsAString_internal const&) | mozilla::dom::telephony::TelephonyIPCProvider::NotifyCdmaCallWaiting(unsigned int, nsAString_internal const&) | mozilla::dom::telephony::TelephonyChild::RecvNotifyCdmaCallWaiting(unsigned int const&, nsString const&) ]

Comment 1

[Greg Grisco]

Attachment: decoded minidump of crash

Comment 2

[Jason Smith [:jsmith]]

Hsin-Yi - Is only having the crash stack enough to make this bug actionable?

Comment 3

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

(In reply to Jason Smith [:jsmith] from comment #2)
> Hsin-Yi - Is only having the crash stack enough to make this bug actionable?

I know STR isn't available right not, but please understand having more context of the scripts is great help. Please keep us posted once you get the steps. I will look at the log first anyway.

And, I guess the gecko/gaia version is mozilla_b2g28_v1_3 and mozillaorg/v1.3. Is that right?

Comment 4

[Greg Grisco]

Yes, gecko and gaia are both v1.3.

Comment 5

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

according to .extra.log, seems timing issue b/w enumerateCalls and notifyCdmaCallWaiting.
==============
03-03 21:54:00.851   304   304 D NS_TELEPHONY_PROVIDER_QC_B2G: [SUB0] TelephonyListener:NotifyCdmaCallWaiting:
03-03 21:54:01.461   304   304 D NS_TELEPHONY_PROVIDER_QC_B2G: [SUB0] TelephonyListener:EnumerateCalls:
03-03 21:54:01.461   304   304 D NS_TELEPHONY_PROVIDER_QC_B2G: [SUB1] TelephonyListener:EnumerateCalls:
==============

We use 'EnumerateCalls' to synchronize TelephonyDOM with RIL. And we expect when cdmaCallWaiting event comes there's already one call on TelephonyDOM. However, there's a chance that a CdmaCallWaiting event comes before DOM completely retrieving the calls maintained in RIL (see log snippet above). So there's no mCalls[0] in [1] that leads the crash...

A possible quick solution: register ril listener only after enumerateCallComplete. 

[1] http://dxr.mozilla.org/mozilla-central/source/dom/telephony/Telephony.cpp?from=Telephony.cpp&case=true#728

Comment 6

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

Attachment: WIP

Please see comment 5.

Hi Greg,

Please help verify if this patch solves the issue. Thanks.

Comment 7

[Preeti Raghunath(:Preeti)]

Michael

Is this still needed for 1.3?

Comment 8

[Michael Vines [:m1] [:evilmachines]]

We'd like to see a fix, yes.  Stability on v1.3 does still not meet the CS goal

Comment 9

[Wesley Huang [:wesley_huang] (EPM) (NI me)]

I saw Hsinyi had the WIP.

Comment 10

[Sandip Kamat]

PM Triage: 1.3 stability needs to reach target MTBFs. Stays 1.3+.

Comment 11

[Greg Grisco]

> Hi Greg,
> 
> Please help verify if this patch solves the issue. Thanks.

We're having problems with our internal mirrors right now and so the patch doesn't apply.  Once we get this worked out I can apply the patch and make an eng build for re-test.

Comment 12

[Greg Grisco]

Patch applied.  I will report back when results are available.

Comment 13

[Greg Grisco]

We haven't seen this crash repeat with this patch applied.

Comment 14

[Greg Grisco]

Hsin-Yi, can you move this forward?

Comment 15

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

Okay, I'll ask for a formal review and push this patch into try server. Thanks for the feedback!

Comment 16

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

Attachment: v1

Please see comment 5 for diagnosis.

I'd say it's really an easy but not perfect solution. With this patch, the API behaviour becomes a little bit weird. For example, gaia could get mozTelephony which is unable to receive RIL messages. However, as this is an urgent bug and it's not the right time to refactor the whole structure. I think it's okay to go.

Hi Vicamo, may I have your review? Thanks.

Comment 17

[Vicamo Yang [:vicamo][:vyang]]

(In reply to Hsin-Yi Tsai  [:hsinyi] from comment #16)
> I'd say it's really an easy but not perfect solution. With this patch, the
> API behaviour becomes a little bit weird. For example, gaia could get
> mozTelephony which is unable to receive RIL messages.

IMO, receiving telephony signals is not a big deal here because of Javascript run-to-complete rule.  In contrast, it brings a problem when we make a call right after getting mozTelephony.  Actually, the original design suffers from the same problem, but this patch just makes that a little more serious.

> However, as this is an
> urgent bug and it's not the right time to refactor the whole structure. I
> think it's okay to go.

Please also file an follow-up for rewrite call enumeration mechanism.  Thank you :)

Comment 18

[Vicamo Yang [:vicamo][:vyang]]

Comment on attachment 8393306 [details] [diff] [review]
v1

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

r=me

Comment 19

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

(In reply to Vicamo Yang [:vicamo][:vyang] from comment #17)
> (In reply to Hsin-Yi Tsai  [:hsinyi] from comment #16)
> > I'd say it's really an easy but not perfect solution. With this patch, the
> > API behaviour becomes a little bit weird. For example, gaia could get
> > mozTelephony which is unable to receive RIL messages.
> 
> IMO, receiving telephony signals is not a big deal here because of
> Javascript run-to-complete rule.  

We've discussed this offline. Let me just write down my explanation for others' reference.

Before this patch, gaia is never able to get mozTelephony when listener registration fails. However, this patch will return telephony first then registering listener later. That means, no matter registration fails or not, gaia still holds a valid mozTelephony, but she will *never* receive any RIL signals.

> In contrast, it brings a problem when we
> make a call right after getting mozTelephony.  Actually, the original design
> suffers from the same problem, but this patch just makes that a little more
> serious.
> 
> > However, as this is an
> > urgent bug and it's not the right time to refactor the whole structure. I
> > think it's okay to go.
> 
> Please also file an follow-up for rewrite call enumeration mechanism.  Thank
> you :)

No problem!

Comment 20

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

Local tests pass. Let's wait for try result https://tbpl.mozilla.org/?tree=Try&rev=7c172c7a99ee before landing the patch.

Comment 21

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

try green. Go for land it.

Comment 22

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

Attachment: patch for b2g-inbound - add r=vicamo

Comment 23

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

https://tbpl.mozilla.org/?tree=Try&rev=7c172c7a99ee try green.

Comment 24

[Ryan VanderMeulen [:RyanVM]]

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

Comment 25

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/479570be21be

Comment 26

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

Comment on attachment 8393954 [details] [diff] [review]
patch for b2g-inbound - add r=vicamo

NOTE: This flag is now for security issues only. Please see https://wiki.mozilla.org/Release_Management/B2G_Landing to better understand the B2G approval process and landings.

[Approval Request Comment]
Bug caused by (feature/regressing bug #): bug 822210
User impact if declined: no obvious user impact, but this bug is caught by a stability test, it has chances that crash appears.
Testing completed: yes, landed in m-c and passed try server
Risk to taking this patch (and alternatives if risky): low
String or UUID changes made by this patch: no

Comment 27

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

> > 
> > > However, as this is an
> > > urgent bug and it's not the right time to refactor the whole structure. I
> > > think it's okay to go.
> > 
> > Please also file an follow-up for rewrite call enumeration mechanism.  Thank
> > you :)
> 
> No problem!

Here, Bug 986333.

Comment 28

[bhavana bajaj [:bajaj]]

Comment on attachment 8393954 [details] [diff] [review]
patch for b2g-inbound - add r=vicamo

Approving this for 1.3 given the verification and low risk here.

This also needs to be landed on mozilla-aurora so this patch is not missed on 1.4. Flagginf :ryanvm to help with this one-off landings here.

Comment 29

[bhavana bajaj [:bajaj]]

ryan, can you please help with aurora landing with a=bajaj, please NI :hsinyi in case the patch does not apply cleanly for a branch specific patch.

Comment 30

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/c6a0507f6ae5
https://hg.mozilla.org/releases/mozilla-b2g28_v1_3/rev/4db5bee75b60