824895 - b2g crash via dbus_func_send_async

Status: RESOLVED FIXED

(Firefox OS Graveyard :: General, defect, P1)

Description

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

Attachment: minidump decoded

We've seen a reoccurring crash in the BT code during stability test.

Test Steps:
1. Receive MT calls and answer the call.
2. When call is in progress, do BT on\off via settings.
3. mini dumps are generated in the phone.

Please see decoded minidump.

Comment 1

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

Gecko SHA1 for the minidump: 
https://github.com/mozilla/mozilla-central/commit/c12e5b5ac6d0111cfdd7de496dc

Comment 2

[Kai-Chih Hu [:khu]]

I will just bb+ this one. Please minus it if someone thinks it should be bb-. 
Eric, could you help this case?

Comment 3

[Eric Chou [:ericchou] [:echou]]

Hi Michael,

(In reply to Michael Vines [:m1] from comment #0)
> Created attachment 695922 [details]
> minidump decoded
> 
> We've seen a reoccurring crash in the BT code during stability test.
> 
> Test Steps:
> 1. Receive MT calls and answer the call.
> 2. When call is in progress, do BT on\off via settings.
> 3. mini dumps are generated in the phone.
> 
> Please see decoded minidump.

I've checked minidump. The crash was caused by passing a null pointer into dbus function. I'm still figuring out. Answering a call may not be a necessary step to let this occur.

If you don't mind, to make sure how this occurs, could you please provide logcat? I believe there must be some libdbus error messages displayed. Thanks in advance.

Eric

Comment 4

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

(logs sent directly to Eric out-of-band)

Comment 5

[Eric Chou [:ericchou] [:echou]]

Attachment: patch 1: v1: check if everything is ready in SetProperty()

While the turning-off Bluetooth process is running on a non-main thread, if SetProperty() is called on main thread, it may crash. That's because |mConnection| may have been set to nullptr in StopInternal() on that turning-off BT thread but it has still been used as a parameter sent into dbus_func_send_async().

This patch could fix this problem temporarily, we will call IsReady() at the beginning of SetProperty() to ensure that mConnection is not null. However, I think that we should not let non-main thread set mConnection, especially reset it to a nullptr, or it may cause a race like this. Maybe we need a follow-up for this, but sending a simple patch for now should be o.k.

Comment 6

[Gina Yeh [:gyeh] [:ginayeh]]

Comment on attachment 696509 [details] [diff] [review]
patch 1: v1: check if everything is ready in SetProperty()

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

Thanks for finding the root cause. This patch should be worked and let's file a follow-up for this.

Comment 7

[Eric Chou [:ericchou] [:echou]]

try:

https://tbpl.mozilla.org/?tree=Try&rev=ca15a47043b7

Comment 8

[Eric Chou [:ericchou] [:echou]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/d78815cb0440

Comment 9

[Ryan VanderMeulen [:RyanVM]]

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

Comment 11

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/466606aace68
https://hg.mozilla.org/releases/mozilla-b2g18/rev/a283cd02229a