824458 - [Bluetooth] Unable to search devices again after calling StopDiscovery()

Status: RESOLVED FIXED

(Core :: DOM: Device Interfaces, defect)

Description

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

Currently Gaia calls adapter.StopDiscovery() to stop searching Bluetooth devices whenever leaving Settings page or timed out, but Evelyn found that onsuccess/onerror of the DOM Request returned by StopDiscovery() are usually not fired. That would result in a wrong state of Bluetooth UI (still shows "Searching for devices" and "Search" button wouldn't appear even if discovery has been stopped). Please see the attached photo for more detail.

This is a follow-up to bug 820166. You can also get more information from bug 820166 comment 9.

Comment 1

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

Attachment: screenshot after low-level discovery has been already stopped

Comment 2

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

Attachment: patch 1: v1: the callback function set to dubs is usually not called

The root cause is we used the asynchronous dbus function call and hit a known issue of DBus, please see https://bugs.freedesktop.org/show_bug.cgi?id=19796 for details of the bug. It is a timing issue. If the response of the target function returned faster than callback function is set, then the callback would never be invoked.

To solve this, I replaced original async function call with blocking calls on non-main thread.

Comment 3

[Gina Yeh [:gyeh] [:ginayeh]]

Comment on attachment 695450 [details] [diff] [review]
patch 1: v1: the callback function set to dubs is usually not called

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

Looks good to me.

::: dom/bluetooth/linux/BluetoothDBusService.cpp
@@ +161,5 @@
> +                   BluetoothReplyRunnable* aRunnable)
> +    : mAdapterPath(aAdapterPath)
> +    , mDeviceObjectPath(aDeviceObjectPath)
> +    , mRunnable(aRunnable)
> +  {

We'd like to make sure that either aDeviceObjectPath or aRunnable is not null.
MOZ_ASSERT(aDeviceObjectPath && aRunnable);

@@ +192,5 @@
> +
> +private:
> +  nsString mAdapterPath;
> +  const char* mDeviceObjectPath;
> +  BluetoothReplyRunnable* mRunnable;

How about using reference counting pointers nsRefPtr here?
nsRefPtr<BluetoothReplyRunnable> mRunnable;

@@ +203,5 @@
> +                    BluetoothReplyRunnable* aRunnable)
> +    : mAdapterPath(aAdapterPath)
> +    , mMessageName(aMessageName)
> +    , mRunnable(aRunnable)
> +  {

Add assertion to make sure both aMessageName and aRunnable aren't null.
MOZ_ASSERT(aMessageName && aRunnable);

@@ +230,5 @@
> +
> +private:
> +  nsString mAdapterPath;
> +  const char* mMessageName;
> +  BluetoothReplyRunnable* mRunnable;

Using nsRefPtr<BluetoothReplyRunnable>

Comment 4

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

Attachment: patch 1: final: the callback function set to dubs is usually not called, r=gyeh

Problems solved

Comment 5

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

try:

https://tbpl.mozilla.org/?tree=Try&rev=61667e313fe8
https://tbpl.mozilla.org/?tree=Try&rev=17f067eeee45

Comment 6

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

Attachment: patch 1: final: the callback function set to dubs is usually not called, r=gyeh

Updated because of build error

Comment 7

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

https://hg.mozilla.org/integration/mozilla-inbound/rev/914889120beb

Comment 8

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/914889120beb

Comment 9

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/fa89be38bac7
https://hg.mozilla.org/releases/mozilla-b2g18/rev/b357c8c75904