827888 - Race condition in Bluetooth DBUS connection

Status: RESOLVED FIXED

(Firefox OS Graveyard :: General, defect)

Description

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

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.

Comment 1

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

You can trigger this bug when quickly trying to connect to the same peer multiple times; as described in bug 826122.

Comment 2

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

Attachment: Explicitly call notifier if DBUS call completes early

This is a workaround for the problem that increases success rate from ~10% to ~75%.

Comment 3

[Eric Chou [:ericchou] [: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. :)

Comment 4

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

Attachment: 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.

Comment 5

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

Thanks everyone for your reviews.

Comment 6

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

Attachment: 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: -

Comment 7

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

Attachment: 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: -

Comment 8

[Daniel Holbert [:dholbert]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/3b6af33dae4f
https://hg.mozilla.org/integration/mozilla-inbound/rev/222efefe167d

Comment 9

[Daniel Holbert [:dholbert]]

(once this gets approval, please add checkin-needed again for the b2g18 landing)

Comment 10

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

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.

Comment 11

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/3b6af33dae4f
https://hg.mozilla.org/mozilla-central/rev/222efefe167d

Comment 12

[Lukas Blakk [:lsblakk] use ?needinfo]

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.

Comment 13

[Lukas Blakk [:lsblakk] use ?needinfo]

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

Comment 14

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-b2g18/rev/d59488b53753
https://hg.mozilla.org/releases/mozilla-b2g18/rev/6a30466ee238