843913 - DBusThread can be used after is is deleted

Status: RESOLVED FIXED

(Core :: DOM: Device Interfaces, defect)

Description

[Ben Turner (not reading bugmail, use the needinfo flag!)]

==229== Invalid read of size 4
==229==    at 0x5CB19DE: mozilla::ipc::DBusWakeup(void*) (DBusThread.cpp:284)
==229==    by 0x6C2A855: ??? (in /system/lib/libdbus.so)
==229==    by 0x6C2FB5D: _dbus_connection_queue_received_message_link (in /system/lib/libdbus.so)
==229==    by 0x6C4FC23: _dbus_transport_queue_messages (in /system/lib/libdbus.so)
==229==    by 0x6C50BE3: ??? (in /system/lib/libdbus.so)
==229==    by 0x6C5082D: ??? (in /system/lib/libdbus.so)
==229==    by 0x6C50F8B: ??? (in /system/lib/libdbus.so)
==229==  Address 0x86731e4 is 20 bytes inside a block of size 24 free'd
==229==    at 0x48062B0: free (vg_replace_malloc.c:446)
==229==    by 0x61A5F4B: moz_free (mozalloc.cpp:48)
==229==    by 0x5CB1DA9: nsAutoPtr<mozilla::ipc::DBusThread>::~nsAutoPtr() (mozalloc.h:225)
==229==    by 0x5CB2133: mozilla::ipc::StopDBus() (DBusThread.cpp:498)
==229==    by 0x585A01B: mozilla::dom::bluetooth::BluetoothDBusService::StopInternal() (BluetoothDBusService.cpp:1714)
==229==    by 0x585A353: mozilla::dom::bluetooth::BluetoothGonkService::StopInternal() (BluetoothGonkService.cpp:143)
==229==    by 0x584C0F3: mozilla::dom::bluetooth::BluetoothService::ToggleBtTask::Run() (BluetoothService.cpp:196)
==229==    by 0x5CD9DBF: nsThread::ProcessNextEvent(bool, bool*) (nsThread.cpp:627)
==229==    by 0x5CB7517: NS_ProcessNextEvent_P(nsIThread*, bool) (nsThreadUtils.cpp:238)
==229==    by 0x5CDA253: nsThread::ThreadFunc(void*) (nsThread.cpp:265)
==229==    by 0x4F86A47: ??? (in /mnt/sdcard/b2g/libnspr4.so)

We need to figure this out pretty quickly.

Comment 1

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

Ben, which gecko did you use? b2g18, 1.0.1, or m-c? Could you provide STR? Thanks.

Comment 2

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

Oh, or it's the result of running Valgrind?

Comment 3

[Ben Turner (not reading bugmail, use the needinfo flag!)]

Yep, this is me running valgrind on an unagi with yesterday's m-c. I can't always reproduce it (it's obviously a race) but I was using the notification panel to turn bluetooth on and off, also turning airplane mode on and off.

Comment 4

[Ben Turner (not reading bugmail, use the needinfo flag!)]

I'm hoping this is the cause of bug 832385. No real evidence though.

Comment 5

[Ben Turner (not reading bugmail, use the needinfo flag!)]

(In reply to ben turner [:bent] from comment #4)
> I'm hoping this is the cause of bug 832385. No real evidence though.

Oops, ignore that. Wrong bug. I'll hunt for the right one.

Comment 6

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

Kyle, Could you take time looking into this issue? Please feel free to tell me if you are not available, thanks!

Comment 7

[Andrew Overholt [:overholt]]

It's not clear that this should be a blocker.  Please re-nom if we discover issues caused by this race.

Comment 8

[Kyle Machulis [:qdot] [:kmachulis] (INACTIVE)]

Well, uh, damn. I thought the only reason Wakeup would ever get called is due to 830290, but that hasn't even gotten r+'d much less landed yet.

I'll play around with it and see what I can do.

Comment 9

[Kyle Machulis [:qdot] [:kmachulis] (INACTIVE)]

Attachment: Patch 1 (v1) - Set watch function to NULL during exit

We aren't setting the watch function to null when we exit our poll loop. That said, this may also be changing drastically with bug 830290. Having tzimmermann review, but I have no way to test, so putting bent on for feedback via magic phone valgrind.

Comment 10

[Ben Turner (not reading bugmail, use the needinfo flag!)]

I don't know if this will work or not... I looked over some of the dbus code for this [1] and it doesn't look like calling the wakeup function happens with consistent locking (search for |_dbus_connection_wakeup_mainloop|, it's called under the lock sometimes and not others...). So I don't think that simply setting the wakeup function to null will guarantee safety here without somehow knowing that the dbus thread has shut down already.

Could we instead make DBusThread refcounted, add a ref to the DBusThread when calling dbus_connection_set_wakeup_main_function(), and then pass a free function that calls DBusThread->Release() so that it manages the lifetime for us? That seems safer to me.

[1]: http://dbus.freedesktop.org/doc/api/html/dbus-connection_8c_source.html

Comment 11

[Kyle Machulis [:qdot] [:kmachulis] (INACTIVE)]

Going to go ahead and land this as is to at least fix the first problem. Follow up for possible refcounting or other solution is bug 845001

Comment 12

[Kyle Machulis [:qdot] [:kmachulis] (INACTIVE)]

Er make that bug 845116.

Comment 13

[Ben Turner (not reading bugmail, use the needinfo flag!)]

Comment on attachment 717316 [details] [diff] [review]
Patch 1 (v1) - Set watch function to NULL during exit

NOTE: 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 #): 
User impact if declined: Random crash possible when disabling bluetooth
Testing completed: Valgrind is happy now
Risk to taking this patch (and alternatives if risky): None, it's a very simple patch that makes us strictly smarter
String or UUID changes made by this patch: None

Comment 14

[Lukas Blakk [:lsblakk] use ?needinfo]

If this is related to the issue in bug 845148 then we need to block on this and land to both v1-train as well as v1.0.1

Comment 15

[Lukas Blakk [:lsblakk] use ?needinfo]

Comment on attachment 717316 [details] [diff] [review]
Patch 1 (v1) - Set watch function to NULL during exit

Blocker, approval not needed, go ahead with uplift to branches.

Comment 16

[Kyle Machulis [:qdot] [:kmachulis] (INACTIVE)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/85e419239adb

Comment 17

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/85e419239adb

Comment 18

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-b2g18/rev/d05fc45f6319
https://hg.mozilla.org/releases/mozilla-b2g18_v1_0_1/rev/04457dea9355