Closed Bug 843913 Opened 9 years ago Closed 9 years ago

DBusThread can be used after is is deleted

Categories

(Core :: DOM: Device Interfaces, defect)

ARM
Gonk (Firefox OS)
defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla22
blocking-b2g tef+
Tracking Status
firefox20 --- wontfix
firefox21 --- wontfix
firefox22 --- fixed
b2g18 --- fixed
b2g18-v1.0.0 --- wontfix
b2g18-v1.0.1 --- fixed

People

(Reporter: bent.mozilla, Assigned: qdot)

Details

Attachments

(1 file)

==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!
Flags: needinfo?(kyle)
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.
Flags: needinfo?(kyle)
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)
Attachment #717316 - Flags: feedback?(bent.mozilla)
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 [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
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 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
Attachment #717316 - Flags: feedback?(bent.mozilla)
Attachment #717316 - Flags: feedback+
Attachment #717316 - Flags: approval-mozilla-b2g18?
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
blocking-b2g: --- → tef+
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.
Attachment #717316 - Flags: approval-mozilla-b2g18?
https://hg.mozilla.org/mozilla-central/rev/85e419239adb
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.