985949 - [Bluetooth] BlueZ backend crashes during pointer handling

Status: VERIFIED FIXED

(Firefox OS Graveyard :: Bluetooth, defect)

Description

[Thomas Zimmermann [:tzimmermann] [:tdz]]

This is fallout from bug 967364. When handing over pointers to DBus operations, BlueZ crashes during pointer cleanups. The stack is

(gdb) c
Continuing.
[New Thread 781.832]
[New Thread 781.796]

Program received signal SIGSEGV, Segmentation fault.
[Switching to Thread 781.796]
0x415bbf04 in ~already_AddRefed (this=0x44cc2e00) at ../../dist/include/nsCOMPtr.h:177
177	      MOZ_ASSERT(!mRawPtr);
(gdb) bt
#0  0x415bbf04 in ~already_AddRefed (this=0x44cc2e00) at ../../dist/include/nsCOMPtr.h:177
#1  AddReservedServiceRecordsTask::Run (this=0x44cc2e00) at /home/mozilla/Projects/mozilla/src/mozilla-central/dom/bluetooth/bluez/BluetoothDBusService.cpp:1532
#2  0x40e326f4 in MessageLoop::RunTask (this=0x43d78dc4, task=0x44cc2e00) at /home/mozilla/Projects/mozilla/src/mozilla-central/ipc/chromium/src/base/message_loop.cc:344
#3  0x40e32dfe in MessageLoop::DeferOrRunPendingTask (this=0x4f, pending_task=<value optimized out>)
    at /home/mozilla/Projects/mozilla/src/mozilla-central/ipc/chromium/src/base/message_loop.cc:352
#4  0x40e33a44 in MessageLoop::DoWork (this=0x43d78dc4) at /home/mozilla/Projects/mozilla/src/mozilla-central/ipc/chromium/src/base/message_loop.cc:430
#5  0x40e28b16 in base::MessagePumpLibevent::Run (this=0x40301e20, delegate=0x43d78dc4)
    at /home/mozilla/Projects/mozilla/src/mozilla-central/ipc/chromium/src/base/message_pump_libevent.cc:311
#6  0x40e32c06 in MessageLoop::RunInternal (this=0x43d78dc4) at /home/mozilla/Projects/mozilla/src/mozilla-central/ipc/chromium/src/base/message_loop.cc:226
#7  0x40e32c1e in MessageLoop::RunHandler (this=0x43d78dc4) at /home/mozilla/Projects/mozilla/src/mozilla-central/ipc/chromium/src/base/message_loop.cc:219
#8  MessageLoop::Run (this=0x43d78dc4) at /home/mozilla/Projects/mozilla/src/mozilla-central/ipc/chromium/src/base/message_loop.cc:193
#9  0x40e35a3a in base::Thread::ThreadMain (this=0x4031c380) at /home/mozilla/Projects/mozilla/src/mozilla-central/ipc/chromium/src/base/thread.cc:162
#10 0x40e2912c in ThreadFunc (closure=0x1) at /home/mozilla/Projects/mozilla/src/mozilla-central/ipc/chromium/src/base/platform_thread_posix.cc:39
#11 0x40120e18 in __thread_entry (func=0x40e29125 <ThreadFunc>, arg=0x4031c380, tls=<value optimized out>) at bionic/libc/bionic/pthread.c:217
#12 0x4012096c in pthread_create (thread_out=<value optimized out>, attr=0xbe9677f8, start_routine=0x40e29125 <ThreadFunc>, arg=0x4031c380)
    at bionic/libc/bionic/pthread.c:357
#13 0x00000000 in ?? ()


STR:

  - build BlueZ with bug 967364 applied
  - turn on Bluetooth

Expected:

  - it works

Actual result:

  - BT crashes immediately

Comment 1

[Thomas Zimmermann [:tzimmermann] [:tdz]]

Attachment: [01] Bug 985949: Fix pointer handling in BlueZ backend

Kyle, we need to clear nsRefPtrs here without decrementing the reference counter. The patch swaps them with a cleared pointer, which work. Is that OK, or is there a better pattern?

Comment 2

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

Comment on attachment 8394151 [details] [diff] [review]
[01] Bug 985949: Fix pointer handling in BlueZ backend

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

You can change

refPtr.forget();

to

unused << refPtr.forget();

And add comments about where the Release() happens.

(Might need to include mozilla/unused.h and grab unused from the mozilla namespace)

Comment 3

[Thomas Zimmermann [:tzimmermann] [:tdz]]

Comment on attachment 8394151 [details] [diff] [review]
[01] Bug 985949: Fix pointer handling in BlueZ backend

OK, thanks. Removing r? for now.

Comment 4

[Thomas Zimmermann [:tzimmermann] [:tdz]]

Attachment: [01] Bug 985949: Fix pointer handling in BlueZ backend (v2)

Changes since v1:

  - shift |unused| by value, as outlined in the feedback

Comment 5

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

Comment on attachment 8394194 [details] [diff] [review]
[01] Bug 985949: Fix pointer handling in BlueZ backend (v2)

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

Thanks!

Comment 6

[Thomas Zimmermann [:tzimmermann] [:tdz]]

Attachment: [02] Bug 985949: Correctly forget pointer references in Bluedroid backend

The patch from bug 983590.

Comment 7

[Thomas Zimmermann [:tzimmermann] [:tdz]]

Great, thanks a lot! :)

https://hg.mozilla.org/integration/b2g-inbound/rev/3b1e6a7980d5
https://hg.mozilla.org/integration/b2g-inbound/rev/7a9c4ebb1ec6

https://tbpl.mozilla.org/?tree=B2g-Inbound&rev=7a9c4ebb1ec6

Comment 8

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/3b1e6a7980d5
https://hg.mozilla.org/mozilla-central/rev/7a9c4ebb1ec6

Comment 10

[Jason Smith [:jsmith]]

Migrating flags over.

Comment 11

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/67a1b14ec375
https://hg.mozilla.org/releases/mozilla-aurora/rev/8aad11bd4201

Comment 13

[Natalya Kot [:nkot]]

Verified fixed.

I've tested on Open_C device (JB device, uses bluez as back-end according to https://bugzilla.mozilla.org/show_bug.cgi?id=984733#c3)

STR:
1. on Ubuntu --> enable BT
2. on Open_C --> Settings --> Enable BT --> send pairing request to Ubuntu BT
==> pairs successfully without crashes

Open_C 1.4
BuildID: 20140506000202
Gaia: b1242f33981024de59b8b4c26bacff8b876211b1
Gecko: fe4080728c60
Version: 30.0

Open_C master m-c
BuildID: 20140506040204
Gaia: 4af716f09747edfbea637f5b60f7fd7d0183f19b
Gecko: 81651ad5e43c
Version: 32.0a1

Base: P821A10V1.0.0B06_LOG_DL