936711 - [B2g][Buri] crash in mozilla::ipc::RawDBusConnection::SendWithReply(void (*)(DBusMessage*, void*), void*, int, DBusMessage*)

Status: RESOLVED FIXED

(Core :: IPC, defect)

Description

[Marcia Knous [:marcia]]

This bug was filed from the Socorro interface and is 
report bp-dd27a28b-dda7-4478-b045-e517b2131108.
=============================================================

Gaia   01800b04cc497db9904ed8e3837187a22ebe47ee
SourceStamp f003c386c77a
BuildID 20131108123740
Version 28.0a1
Base Image: 11-4-13

Seen during testing today.

STR:
1. Connect a Plantronics headset
2. Toggled bluetooth button on and off quickly
3. Eventually the phone froze and I got this crash

Will see if I can reproduce the crash.

Comment 1

[Marcia Knous [:marcia]]

Attachment: Logcat captured during testing

This is reproducible for me using the following steps:

1. Pair a Plantronics PLT_M165 headset.
2. Turn off Bluetooth.
3. Being a sequence of turning BT on and off, fairly rapidly.
4. Device eventually freezes and I have to reboot by pulling the battery out.

After rebooting the phone I don't get a crash banner, but I do see the crashes in adb.

Comment 2

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

I can reproduce by pushing the old audio.conf into my device.

First of all, Marcia, the Bluetooth configuration file on your device seems to be out-of-date. Would you mind flashing the full image to your device? Once the flash is done, please enter command

  "adb shell cat /etc/bluetooth/audio.conf | grep General -A 3"

to check if A2DP and AVRCP are supported. If A2DP and AVRCP are supported, the result should be like

  [General]
  Enable=Sink,Control
  Disable=Headset,Gateway,Source

If you have used older config file, you may see

  [General]
  Disable=Headset,Gateway,Source,Sink,Control

and the music stream wouldn't be redirected to Bluetooth headset in this case.

*************************************

Back to the original problem, this happened because BluetoothDBusService::SendSinkMessage was called with an invalid BluetoothDBusService::mConnection, which had been already null out in BluetoothDBusService::StopInternal() after turning off BT.

The patch is coming.

Comment 3

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

Attachment: patch 1: v1: fix

* Fix crash which happened at disabling Bluetooth during reconnection.
** Check mConnection before using it.
** Check if sControllerArray is empty in NextBluetoothProfileController() since the array would be cleared in StopInternal().

Comment 4

[Naoki Hirata :nhirata (please use needinfo instead of cc)]

Eric, we do not flash the images onto our phones, we flash the gecko/gaia as that would be closer to the OEM build when released.

There are private repos for the gonk layers that we might not have which causes certain things to break if we do a flash of the images.  For example wifi breaks on the buri device when full flashed; configuration files have to be modified in order for the device to work correctly, and even then we cannot guarantee that it will work 100%.

Comment 5

[Marcia Knous [:marcia]]

(In reply to Eric Chou [:ericchou] [:echou] from comment #2)
> I can reproduce by pushing the old audio.conf into my device.
> 
> First of all, Marcia, the Bluetooth configuration file on your device seems
> to be out-of-date. Would you mind flashing the full image to your device?
> Once the flash is done, please enter command
> 
>   "adb shell cat /etc/bluetooth/audio.conf | grep General -A 3"
> 
> to check if A2DP and AVRCP are supported. If A2DP and AVRCP are supported,
> the result should be like
>
[snip]

Eric: I tried doing what you describe, but in my case even running as root I could not push the file back the device - I was getting a read only file system error. I am using the builds flashed from the pvt directory.

Comment 6

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

(In reply to Naoki Hirata :nhirata (please use needinfo instead of cc) from comment #4)
> Eric, we do not flash the images onto our phones, we flash the gecko/gaia as
> that would be closer to the OEM build when released.
> 
> There are private repos for the gonk layers that we might not have which
> causes certain things to break if we do a flash of the images.  For example
> wifi breaks on the buri device when full flashed; configuration files have
> to be modified in order for the device to work correctly, and even then we
> cannot guarantee that it will work 100%.

ok, I can understand. Then may I ask what would you do if the configuration file has to be updated? We have uploaded a newer version of audio.conf for over 2 months (https://github.com/mozilla-b2g/android-device-hamachi/commit/01aff5da10598e13404d86240e014f428b67568a) so I would expect A2DP and AVRCP was enabled on your devices.

Comment 7

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

(In reply to Marcia Knous [:marcia - use needinfo] from comment #5)
> (In reply to Eric Chou [:ericchou] [:echou] from comment #2)
> > I can reproduce by pushing the old audio.conf into my device.
> > 
> > First of all, Marcia, the Bluetooth configuration file on your device seems
> > to be out-of-date. Would you mind flashing the full image to your device?
> > Once the flash is done, please enter command
> > 
> >   "adb shell cat /etc/bluetooth/audio.conf | grep General -A 3"
> > 
> > to check if A2DP and AVRCP are supported. If A2DP and AVRCP are supported,
> > the result should be like
> >
> [snip]
> 
> Eric: I tried doing what you describe, but in my case even running as root I
> could not push the file back the device - I was getting a read only file
> system error. I am using the builds flashed from the pvt directory.

So I think it only flashed Gecko and Gaia, not system.img. Then I'll wait for Naoki's response to figure out how you guys update configuration files. Thanks, Marcia.

Comment 8

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

Comment on attachment 830115 [details] [diff] [review]
patch 1: v1: fix

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

Nothing serious I can pick up. Just some suggestions for you. You can make your own choice. ;)

::: dom/bluetooth/linux/BluetoothDBusService.cpp
@@ +2004,5 @@
>    MOZ_ASSERT(aInterface);
>  
> +  if (!mConnection) {
> +    return NS_ERROR_FAILURE;
> +  }

Recommend to use NS_ENSURE_TRUE. When we failed to get |mConnection|, a warning is shown.

NS_ENSURE_TRUE(mConnection, NS_ERROR_FAILURE);

@@ -2586,5 @@
>  
>  static void
>  NextBluetoothProfileController()
>  {
> -  sControllerArray[0] = nullptr;

Good catch.

@@ +2602,1 @@
>    }

At first, I was confused that why we should move |RemoveElement| into the if-statement. After checking the comment, I agree to your proposal.

I'm thinking that it could increase the readability if we check the array length with |NS_ENSURE_TRUE_VOID|. One more advantage is that we don't have to put an if-statement into an if-statement.

  ...
  MOZ_ASSERT(NS_IsMainThread());

  NS_ENSURE_TRUE_VOID(!sControllerArray.IsEmpty());
  sControllerArray.RemoveElementAt(0);

  if (!sControllerArray.IsEmpty()) {
    sControllerArray[0]->Start();
  }

Comment 9

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

From the point of view of code logic, it should be reproducible on v1.2

Comment 10

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

Attachment: patch 1: final: fix, r=gyeh

* nits picked. Thanks.

Comment 11

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

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

Comment 12

[Jason Smith [:jsmith]]

QA Wanted to check 1.2 reproducibility.

Comment 13

[Ed Morley [:emorley]]

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

Comment 14

[Naoki Hirata :nhirata (please use needinfo instead of cc)]

We would have to update that configuration file manually until the OEM vendor picks up the change.

Comment 15

[Sarah Parsons]

This issue reproduces on the Buri 1.2 Build ID: 20131112004004

Gaia   35b1c6b669b7bf08126df451221113e72642abca
SourceStamp 566e850868c7
BuildID 20131112004004
Version 26.0

Comment 16

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

(In reply to Naoki Hirata :nhirata (please use needinfo instead of cc) from comment #14)
> We would have to update that configuration file manually until the OEM
> vendor picks up the change.

Got it. Thanks.

Comment 17

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

Nominate as koi+ per comment 9 and comment 15.

Comment 18

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-b2g26_v1_2/rev/cdfe03e77e66

Comment 19

[Naoki Hirata :nhirata (please use needinfo instead of cc)]

FYI, the audio.conf file change is in the OEM 1.2 build.  We should be able to test this when using the OEM 1.2 Base build

Sparson, please be sure to use the 1.2 OEM build as the base build.  Dwatson should be able to help you flash that as the base build.