Race condition in Bluetooth DBUS connection

RESOLVED FIXED in Firefox 21

Status

--
major
RESOLVED FIXED
6 years ago
6 years ago

People

(Reporter: tzimmermann, Assigned: tzimmermann)

Tracking

unspecified
B2G C4 (2jan on)
ARM
Gonk (Firefox OS)

Firefox Tracking Flags

(firefox19 wontfix, firefox20 wontfix, firefox21 fixed, b2g1819+ fixed, b2g18-v1.0.0 wontfix)

Details

Attachments

(2 attachments, 2 obsolete attachments)

When pairing with other Bluetooth devices, it can happen that the pair response receives before the message handler is set up. In this case the message is lost and the device thinks it's still pairing.

We need a reliable way of setting up the message handler. DBUS doesn't provide this feature for us.
You can trigger this bug when quickly trying to connect to the same peer multiple times; as described in bug 826122.
Assignee: echou → tzimmermann
Created attachment 699761 [details] [diff] [review]
Explicitly call notifier if DBUS call completes early

This is a workaround for the problem that increases success rate from ~10% to ~75%.
Attachment #699761 - Flags: review?(echou)
Comment on attachment 699761 [details] [diff] [review]
Explicitly call notifier if DBUS call completes early

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

Looks good to me, but I think it would be better if Kyle can take a look at this since he is the original author. :)
Attachment #699761 - Flags: review?(kyle)
Attachment #699761 - Flags: review?(echou)
Attachment #699761 - Flags: review+
Created attachment 699827 [details] [diff] [review]
Fix usage of sIsParing variable

This patch fixes another race condition during the pairing setup and allows to run several pairing requests in parallel. With both patches applied, I wasn't able to reproduce the original problem any longer.
Attachment #699827 - Flags: review?(kyle)
Attachment #699827 - Flags: review?(echou)
Attachment #699827 - Flags: review?(echou) → review+
Thanks everyone for your reviews.
Keywords: checkin-needed
Keywords: checkin-needed
Created attachment 700308 [details] [diff] [review]
Explicitly call notifier if DBUS call completes early

[Approval Request Comment]
Bug caused by (feature/regressing bug #): 
User impact if declined: Bluetooth pairing might fail with no or incorrect error
Testing completed: On my unagi
Risk to taking this patch (and alternatives if risky): Low
String or UUID changes made by this patch: -
Attachment #699761 - Attachment is obsolete: true
Attachment #700308 - Flags: review+
Attachment #700308 - Flags: approval-mozilla-b2g18?
Created attachment 700310 [details] [diff] [review]
Fix usage of sIsParing variable

[Approval Request Comment]
Bug caused by (feature/regressing bug #): 
User impact if declined: Bluetooth pairing might fail with no or incorrect error; multiple concurrent pairing requests will interfere
Testing completed: On my unagi
Risk to taking this patch (and alternatives if risky): Low
String or UUID changes made by this patch: -
Attachment #699827 - Attachment is obsolete: true
Attachment #700310 - Flags: review+
Attachment #700310 - Flags: approval-mozilla-b2g18?
Keywords: checkin-needed
(once this gets approval, please add checkin-needed again for the b2g18 landing)
For really fixing this problem, we'd need to handle all interaction with DBus from within a single thread. I currently work on converting the DBus event loop into an nsRunnable and scheduling DBus requests within the same thread context. Patches will follow soon. I hope to have this ready for v2.
https://hg.mozilla.org/mozilla-central/rev/3b6af33dae4f
https://hg.mozilla.org/mozilla-central/rev/222efefe167d
Status: NEW → RESOLVED
Last Resolved: 6 years ago
Resolution: --- → FIXED
tracking-b2g18: --- → ?
Leaving this nom for approval, given that it's not blocking and is a fairly large change in the code we'll wait until 1.0.1 for this.  See https://wiki.mozilla.org/Release_Management/B2G_Landing for more details but basically the options are to wait until b2g18 opens up for 1.0.1 landings after 1/25 or land now to the date project branch so it gets merged in after 1/25.
status-b2g18: --- → affected
tracking-b2g18: ? → 19+
status-b2g18-v1.0.0: --- → wontfix
Attachment #700308 - Flags: approval-mozilla-b2g18? → approval-mozilla-b2g18+
Comment on attachment 700310 [details] [diff] [review]
Fix usage of sIsParing variable

Please go ahead with uplift to the tip of mozilla-b2g18 branch for v1.0.1
Attachment #700310 - Flags: approval-mozilla-b2g18? → approval-mozilla-b2g18+
https://hg.mozilla.org/releases/mozilla-b2g18/rev/d59488b53753
https://hg.mozilla.org/releases/mozilla-b2g18/rev/6a30466ee238
status-b2g18: affected → fixed
status-firefox19: --- → wontfix
status-firefox20: --- → wontfix
status-firefox21: --- → fixed
Target Milestone: --- → B2G C4 (2jan on)
You need to log in before you can comment on or make changes to this bug.