Closed Bug 843913 Opened 9 years ago Closed 9 years ago
Thread can be used after is is deleted
==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.
Ben, which gecko did you use? b2g18, 1.0.1, or m-c? Could you provide STR? Thanks.
Oh, or it's the result of running Valgrind?
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.
I'm hoping this is the cause of bug 832385. No real evidence though.
(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.
Kyle, Could you take time looking into this issue? Please feel free to tell me if you are not available, thanks!
It's not clear that this should be a blocker. Please re-nom if we discover issues caused by this race.
blocking-b2g: tef? → ---
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.
Assignee: nobody → kyle
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.
Attachment #717316 - Flags: review?(tzimmermann) → review+
I don't know if this will work or not... I looked over some of the dbus code for this  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. : http://dbus.freedesktop.org/doc/api/html/dbus-connection_8c_source.html
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
Er make that bug 845116.
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
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 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.
Status: NEW → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla22
You need to log in before you can comment on or make changes to this bug.