1036233 - Implement pairing in BluetoothAdapter (methods)

Status: RESOLVED FIXED

(Firefox OS Graveyard :: Bluetooth, defect)

Description

[Ben Tian (inactive)]

* Pairing: https://wiki.mozilla.org/B2G/Bluetooth/WebBluetooth-v2/BluetoothAdapter#pair.28DOMString_aDeviceAddress.29

Methods:
  Promise<void> pair(DOMString aAddress);
  Promise<void> unpair(DOMString aAddress);
  // BluetoothDevice[]
  Promise<any> getPairedDevices();

Comment 1

[Ben Tian (inactive)]

Jamin, please help on this bug first. Let me know for any question. Thanks.

Comment 2

[Jamin Liu [:jaliu][:Jamin]]

Attachment: [Part 2] Initialize the data member of BluetoothDevice. (v0)

Comment 3

[Jamin Liu [:jaliu][:Jamin]]

Attachment: [Part 1] Handle Bluetooth SSP reply by maintaining a static runnable array. (v0)

Comment 4

[Jamin Liu [:jaliu][:Jamin]]

Attachment: [Part 3] Check the status of Bluedroid command after bond state changed. (v0)

Comment 5

[Jamin Liu [:jaliu][:Jamin]]

Attachment: [Pair 4] Implement pairing methods of Bluetooth API v2. (v0)

Comment 6

[Jamin Liu [:jaliu][:Jamin]]

Attachment: (draft - see also Bug 1032755) Return an error string when BluetoothReplyRunnable rejects the promise. (v0)

Comment 7

[Jamin Liu [:jaliu][:Jamin]]

Attachment: [Part 3] Check the status of Bluedroid command after bond state changed. (v1)

Comment 8

[Jamin Liu [:jaliu][:Jamin]]

Attachment: [Part 1] Handle Bluetooth SSP reply by maintaining a static runnable array. (v1)

Comment 9

[Ben Tian (inactive)]

Comment on attachment 8464643 [details] [diff] [review]
[Part 2] Initialize the data member of BluetoothDevice. (v0)

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

LGTM.

Comment 10

[Ben Tian (inactive)]

Comment on attachment 8465158 [details] [diff] [review]
[Part 3] Check the status of Bluedroid command after bond state changed. (v1)

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

Why not use |ReplyStatusError| directly?

Comment 11

[Ben Tian (inactive)]

Comment on attachment 8465182 [details] [diff] [review]
[Part 1] Handle Bluetooth SSP reply by maintaining a static runnable array. (v1)

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

Why is |sSspRunnableArray| required?

Comment 12

[Ben Tian (inactive)]

Comment on attachment 8464648 [details] [diff] [review]
[Pair 4] Implement pairing methods of Bluetooth API v2. (v0)

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

Please see my comment below.

::: dom/bluetooth2/BluetoothAdapter.cpp
@@ +509,5 @@
>  
>    nsRefPtr<DOMRequest> request = new DOMRequest(win);
> +// Using conditional compilation to avoid Build Error temporarily for Bluetooth API v2
> +// TODO: Rewrite this function when Bluetooth connection V2 is implemented
> +#if 0

I suggest to keep |GetDevicesTask| and here until we really modify |GetConnectedDevices|.

@@ +536,5 @@
>  {
> +  nsTArray<nsRefPtr<BluetoothDevice> > pairedDevices;
> +  for (uint32_t i = 0; i < mDevices.Length(); ++i) {
> +    if (mDevices[i]->Paired()) {
> +      pairedDevices.AppendElement(mDevices[i]);

Why not append to |aDevices| directly?

@@ +553,4 @@
>      aRv.Throw(NS_ERROR_FAILURE);
>      return nullptr;
>    }
> +  nsRefPtr<Promise> promise = Promise::Create(global, aRv);

Add |NS_ENSURE_TRUE(!aRv.Failed(), nullptr)| check.

@@ +560,5 @@
> +   * - device address is not empty,
> +   * - adapter is already enabled, and
> +   * - BluetoothService is available.
> +   */
> +  BT_ENSURE_TRUE_RESOLVE(!aDeviceAddress.IsEmpty(), JS::UndefinedHandleValue);

We should reject if device address is empty.

@@ +567,1 @@
>  

nit: remove this newline.

::: dom/bluetooth2/BluetoothAdapter.h
@@ +105,4 @@
>      Unpair(const nsAString& aDeviceAddress, ErrorResult& aRv);
> +
> +  /**
> +   * Return paired devices array. The function is called when applications call

I think the later sentence is redundant...

::: dom/webidl/BluetoothAdapter2.webidl
@@ +115,4 @@
>    [NewObject, Throws]
> +  Promise unpair(DOMString deviceAddress);
> +
> +  sequence<BluetoothDevice> getPairedDevices();

Please update wiki page as well. Thanks.

Comment 13

[Jamin Liu [:jaliu][:Jamin]]

Attachment: [Part 3] Check the status of Bluedroid command after bond state changed. (v2)

Comment 14

[Jamin Liu [:jaliu][:Jamin]]

(In reply to Ben Tian [:btian] from comment #10)
> Comment on attachment 8465158 [details] [diff] [review]
> [Part 3] Check the status of Bluedroid command after bond state changed. (v1)
> 
> Review of attachment 8465158 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Why not use |ReplyStatusError| directly?

Good advice.
Thank you. :)

Comment 15

[Jamin Liu [:jaliu][:Jamin]]

Attachment: [Pair 4] Implement pairing methods of Bluetooth API v2. (v1)

Comment 16

[Jamin Liu [:jaliu][:Jamin]]

(In reply to Ben Tian [:btian] from comment #12)
> Comment on attachment 8464648 [details] [diff] [review]
Thank you for your comments.
I've uploaded a new patch based on your comments.

Comment 17

[Jamin Liu [:jaliu][:Jamin]]

(In reply to Ben Tian [:btian] from comment #11)
> Comment on attachment 8465182 [details] [diff] [review]
> 
> Why is |sSspRunnableArray| required?

I added this array to keep the reference count to avoid a segmentation fault.

Program received signal SIGSEGV, Segmentation fault.
> #0  mozilla::dom::bluetooth::BluetoothReply::MaybeDestroy (this=0x5a5a5a5a, aNewType=mozilla::dom::bluetooth::BluetoothReply::T__None) at BluetoothTypes.cpp:524
> #1  0xb448f1a2 in mozilla::dom::bluetooth::BluetoothReply::~BluetoothReply (this=0x5a5a5a5a, __in_chrg=<optimized out>) at BluetoothTypes.cpp:591
> #2  0xb4d3e378 in assign (aNewPtr=0xab8ce360, this=0xab8cd1cc) at ../../dist/include/nsAutoPtr.h:40
> #3  operator= (aRhs=0xab8ce360, this=0xab8cd1cc) at ../../dist/include/nsAutoPtr.h:110
> #4  mozilla::dom::bluetooth::BluetoothReplyRunnable::SetReply (this=0xab8cd1c0, aReply=0xab8ce360) at ../../../gecko/dom/bluetooth2/BluetoothReplyRunnable.cpp:34
> #5  0xb4d50016 in mozilla::dom::bluetooth::DispatchBluetoothReply (aRunnable=0xab8cd1c0, aValue=..., aErrorStr=<optimized out>) at ../../../gecko/dom/bluetooth2/bluedroid/BluetoothUtils.cpp:182
> #6  0xb4d4a73e in SspReplyResultHandler::SspReply (this=<optimized out>) at ../../../gecko/dom/bluetooth2/bluedroid/BluetoothServiceBluedroid.cpp:1640

I think I can find a lighter way to achieve the same thing.
I will test my experimental patch and make an update today.

Comment 18

[Jamin Liu [:jaliu][:Jamin]]

Attachment: [Part 1] Hold the references of BluetoothReplyRunnable during Bluetooth SSP & Pin replying to avoid crash. (v1)

Comment 19

[Jamin Liu [:jaliu][:Jamin]]

Attachment: [Part 4] Implement pairing methods of Bluetooth API v2. (v2)

Comment 20

[Jamin Liu [:jaliu][:Jamin]]

Comment on attachment 8464649 [details] [diff] [review]
(draft - see also Bug 1032755) Return an error string when BluetoothReplyRunnable rejects the promise. (v0)

Move #Attachment 8464649 [details] [diff] to Bug 1016560.

Comment 21

[Jamin Liu [:jaliu][:Jamin]]

This bug depends on Bug 1032755. I typed the incorrect bug number by accident.
Sorry for any inconvenience caused.

Comment 22

[Ben Tian (inactive)]

Comment on attachment 8465932 [details] [diff] [review]
[Part 1] Hold the references of BluetoothReplyRunnable during Bluetooth SSP & Pin replying to avoid crash. (v1)

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

Please move this patch to another bug to modify all |mRunnable| inside result handlers, explain your analysis and possible cause, and request for Thomas' review. Thanks.

Comment 23

[Ben Tian (inactive)]

Comment on attachment 8465908 [details] [diff] [review]
[Part 3] Check the status of Bluedroid command after bond state changed. (v2)

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

::: dom/bluetooth2/bluedroid/BluetoothServiceBluedroid.cpp
@@ +104,5 @@
> +
> +static void
> +ConnectDisconnect(bool aConnect, const nsAString& aDeviceAddress,
> +                  BluetoothReplyRunnable* aRunnable,
> +                  uint16_t aServiceUuid, uint32_t aCod);

nit: I suggest to group these declaration as static variables.
  
  // Static function declarations
  static bool EnsureBluetoothHalLoad();
  static nsresult StartGonkBluetooth();
  static nsresult StopGonkBluetooth();
  static void ReplyStatusError(BluetoothReplyRunnable* aBluetoothReplyRunnable,
                               int aStatusCode, const nsAString& aCustomMsg);
  static void NextBluetoothProfileController();
  static void ConnectDisconnect(bool aConnect, const nsAString& aDeviceAddress,
                                BluetoothReplyRunnable* aRunnable,
                              uint16_t aServiceUuid, uint32_t aCod);

@@ +738,5 @@
>    {
>      MOZ_ASSERT(NS_IsMainThread());
>  
>      if (mBonded && !sBondingRunnableArray.IsEmpty()) {
> +      if (mStatus == BT_STATUS_SUCCESS || mStatus == BT_STATUS_DONE) {

Leave comment to explain that we regard BT_STATUS_DONE as success.

@@ +742,5 @@
> +      if (mStatus == BT_STATUS_SUCCESS || mStatus == BT_STATUS_DONE) {
> +        DispatchBluetoothReply(sBondingRunnableArray[0],
> +                               BluetoothValue(true), EmptyString());
> +      } else {
> +        ReplyStatusError(sBondingRunnableArray[0], mStatus, EmptyString());

Also reply error in |AdapterPropertiesCallbackTask| and |RemoteDevicePropertiesCallbackTask|.

@@ +1052,5 @@
> +      replyError.AppendLiteral(":BT_STATUS_RMT_DEV_DOWN");
> +      break;
> +    case BT_STATUS_AUTH_REJECTED:
> +      replyError.AppendLiteral(":BT_STATUS_AUTH_REJECTED");
> +      break;

Add default case.

Comment 24

[Ben Tian (inactive)]

Comment on attachment 8466956 [details] [diff] [review]
[Part 4] Implement pairing methods of Bluetooth API v2. (v2)

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

r=me with comment addressed. Thanks.

::: dom/bluetooth2/BluetoothAdapter.cpp
@@ +619,5 @@
> +   * - device address is not empty,
> +   * - adapter is already enabled, and
> +   * - BluetoothService is available.
> +   */
> +  if (aDeviceAddress.IsEmpty()) {

I suggest to reject with nsError first and revise all promise rejection at once once bug 1032755 is landed.

::: dom/bluetooth2/BluetoothAdapter.h
@@ +105,4 @@
>      Unpair(const nsAString& aDeviceAddress, ErrorResult& aRv);
> +
> +  /**
> +   * Get a list of Bluetooth devices which have been paired with.

nit: I think 'Get a list of paired bluetooth devices' is simpler.

Comment 25

[Ben Tian (inactive)]

Comment on attachment 8466956 [details] [diff] [review]
[Part 4] Implement pairing methods of Bluetooth API v2. (v2)

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

::: dom/bluetooth2/BluetoothAdapter.cpp
@@ +617,5 @@
> +  /**
> +   * Ensure
> +   * - device address is not empty,
> +   * - adapter is already enabled, and
> +   * - BluetoothService is available.

One more thing: we should also ensure BluetoothDevice.paired is false/true before pair/unpair. Please make this a followup bug.

Comment 26

[Jamin Liu [:jaliu][:Jamin]]

(In reply to Ben Tian [:btian] from comment #23)
> Comment on attachment 8465908 [details] [diff] [review]
Thank you for your comments.
I moved #attachment 8465908 [details] [diff] [review] to Bug 1049452.

Comment 27

[Jamin Liu [:jaliu][:Jamin]]

This bug no longer depends on Bug 1032755 since #attachment 8465908 [details] [diff] [review] has been moved to Bug 104952.

Comment 28

[Jamin Liu [:jaliu][:Jamin]]

Comment on attachment 8465908 [details] [diff] [review]
[Part 3] Check the status of Bluedroid command after bond state changed. (v2)

Moved to Bug 1049452.

Comment 29

[Jamin Liu [:jaliu][:Jamin]]

Comment on attachment 8465932 [details] [diff] [review]
[Part 1] Hold the references of BluetoothReplyRunnable during Bluetooth SSP & Pin replying to avoid crash. (v1)

Move this patch to Bug 1050068.
Please refer to #Comment 22.

Comment 30

[Jamin Liu [:jaliu][:Jamin]]

Attachment: [Part 1] Implement pairing methods of Bluetooth API v2. (v3), r=btian

Rename this patch since [Pair 1] and [Pair 3] has been moved to Bug 1050068 and Bug 1049452.

Comment 31

[Jamin Liu [:jaliu][:Jamin]]

(In reply to Ben Tian [:btian] from comment #25)
> Comment on attachment 8466956 [details] [diff] [review]
> [Part 4] Implement pairing methods of Bluetooth API v2. (v2)
> 
> Review of attachment 8466956 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: dom/bluetooth2/BluetoothAdapter.cpp
> @@ +617,5 @@
> > +  /**
> > +   * Ensure
> > +   * - device address is not empty,
> > +   * - adapter is already enabled, and
> > +   * - BluetoothService is available.
> 
> One more thing: we should also ensure BluetoothDevice.paired is false/true
> before pair/unpair. Please make this a followup bug.

Thank you for reminding me.
I modified previous patch and ensure device is paired before unpair it. 

However, I think we don't have to ensure BluetoothDevice.paired is true when user call BluetoothAdapter.pair() since the link key in the remote device might be deleted unilaterally without notifying us. In that case, we still have to pair again even the 'paired' attribute is true.

Comment 32

[Jamin Liu [:jaliu][:Jamin]]

Attachment: [Part 1] Implement pairing methods of Bluetooth API v2. (v3), r=btian

Comment 33

[Ben Tian (inactive)]

Comment on attachment 8469145 [details] [diff] [review]
[Part 1] Implement pairing methods of Bluetooth API v2. (v3), r=btian

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

Please remove BluetoothDevice.paired check from this patch and open a followup to discuss whether we need the check.

::: dom/bluetooth2/BluetoothAdapter.cpp
@@ +611,3 @@
>      aRv.Throw(NS_ERROR_FAILURE);
>      return nullptr;
>    }

nit: newline after this line.

@@ +611,5 @@
>      aRv.Throw(NS_ERROR_FAILURE);
>      return nullptr;
>    }
> +  NS_ENSURE_TRUE(!aRv.Failed(), nullptr);
> +  nsRefPtr<Promise> promise = Promise::Create(global, aRv);

Incorrect code.

  nsRefPtr<Promise> promise = Promise::Create(global, aRv);
  NS_ENSURE_TRUE(!aRv.Failed(), nullptr);

@@ +619,5 @@
> +   * - device address is not empty,
> +   * - adapter is already enabled, and
> +   * - BluetoothService is available.
> +   */
> +  BT_ENSURE_TRUE_REJECT(aDeviceAddress.IsEmpty(),

!aDeviceAddress.IsEmpty()

@@ +627,4 @@
>    BluetoothService* bs = BluetoothService::Get();
> +  BT_ENSURE_TRUE_REJECT(bs, NS_ERROR_NOT_AVAILABLE);
> +
> +  // Ensure device is paired before unpair it.

The logic should be revised since BluetoothDevice.paired should be false before it's paired. I suggest to remove this check section from the patch and open a followup bug (as I mentioned before) to discuss whether we need this check.

Comment 34

[Jamin Liu [:jaliu][:Jamin]]

Attachment: [Part 1] Implement pairing methods of Bluetooth API v2. (v4), r=btian

Comment 35

[Jamin Liu [:jaliu][:Jamin]]

(In reply to Ben Tian [:btian] from comment #33)
> Comment on attachment 8469145 [details] [diff] [review]
> [Part 1] Implement pairing methods of Bluetooth API v2. (v3), r=btian
> 
> The logic should be revised since BluetoothDevice.paired should be false
> before it's paired. I suggest to remove this check section from the patch
> and open a followup bug (as I mentioned before) to discuss whether we need
> this check.

Thank you for your comments.
The follow-up Bug 1050637 has been filed.

Comment 36

[Ben Tian (inactive)]

Comment on attachment 8469783 [details] [diff] [review]
[Part 1] Implement pairing methods of Bluetooth API v2. (v4), r=btian

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

r=me with comment addressed.

::: dom/webidl/BluetoothAdapter2.webidl
@@ +112,2 @@
>    [NewObject, Throws]
> +  Promise pair(DOMString deviceAddress);

Please rebase on bug 1045743.

Promise<void> pair(DOMString deviceAddress);

@@ +115,2 @@
>    [NewObject, Throws]
> +  Promise unpair(DOMString deviceAddress);

Ditto.

Comment 37

[Jamin Liu [:jaliu][:Jamin]]

Attachment: [Part 1] Implement pairing methods of Bluetooth API v2. (v5), r=btian

Thank Ben for the review.
* rebased.

Comment 38

[Jamin Liu [:jaliu][:Jamin]]

Comment on attachment 8469963 [details] [diff] [review]
[Part 1] Implement pairing methods of Bluetooth API v2. (v5), r=btian

Hi Blake,

This is Jamin from Bluetooth team.
Bluetooth team is working on API refinement and we plan to expose Bluetooth APIs v2 at B2G 2.2.
Bz has helped us a lot by reviewing the patches which modified webidl, including BT on/off, discovery and pairing event.

Since bz is taking a long vacation, I'm looking for a reviewer from DOM perspective.

In this bug, I revised BluetoothAdapter2.webidl[1] and implemented three pairing related methods.
The implementation patch #Attachment 8469963 [details] [diff] has already been reviewed by Ben, one of our Bluetooth module peers.
Would you mind to take a look and give me some feedback as a reviewer, or could you recommend someone else ?

Thank you. :)

Comment 39

[Jamin Liu [:jaliu][:Jamin]]

(In reply to Jamin Liu [:jaliu] from comment #38)
> In this bug, I revised BluetoothAdapter2.webidl[1] and implemented three
> pairing related methods.

- Promise<void> pair(DOMString aAddress);
- Promise<void> unpair(DOMString aAddress);
- sequence<BluetoothDevice> getPairedDevices();

[1] https://wiki.mozilla.org/B2G/Bluetooth/WebBluetooth-v2/BluetoothAdapter#Interfaces

Comment 40

[Jamin Liu [:jaliu][:Jamin]]

Attachment: [Part 2] Initialize the data member of BluetoothDevice. (v0), r=btian

* Add reviewer's name to commit message.

Comment 41

[Jamin Liu [:jaliu][:Jamin]]

Attachment: [Part 1] Implement pairing methods of Bluetooth API v2. (v5), r=btian, r=mrbkap

* Add reviewer's names to commit message.

Comment 42

[Jamin Liu [:jaliu][:Jamin]]

(In reply to Jamin Liu [:jaliu] from comment #41)
> Created attachment 8471281 [details] [diff] [review]
Thank Blake and Ben for reviewing the patch.

Comment 43

[Ben Tian (inactive)]

https://hg.mozilla.org/integration/b2g-inbound/rev/2d0ebc1bc7d6
https://hg.mozilla.org/integration/b2g-inbound/rev/c5d50f1bc995

Comment 44

[Nigel Babu [:nigelb]]

Backed out for build bustage in

https://hg.mozilla.org/integration/b2g-inbound/rev/24d3ede65752
https://hg.mozilla.org/integration/b2g-inbound/rev/9db7ec28bcd2

Bustage: https://tbpl.mozilla.org/php/getParsedLog.php?id=45716489&tree=B2g-Inbound

Comment 45

[Jamin Liu [:jaliu][:Jamin]]

(In reply to Nigel Babu [:nigelb] from comment #44)

Thank you Nigel.

The build bustage was caused by #Attachment 8469965 [details] [diff] from Bug 1036233.
The patch has been fixed at #Attachment 8471322 [details] [diff].

Comment 46

[Jamin Liu [:jaliu][:Jamin]]

TBPL:
https://tbpl.mozilla.org/?tree=Try&rev=4bfdde5c95b4

The patches wouldn't affect bluetooth1 and pass all Bluetooth tests on TBPL.
- TEST-PASS | test_dom_BluetoothManager_enabled.js | took 17954ms
- TEST-PASS | test_dom_BluetoothManager_adapteradded.js | took 8786ms
- TEST-PASS | test_dom_BluetoothAdapter_setters.js | took 8462ms
- TEST-PASS | test_dom_BluetoothAdapter_getters.js | took 7965ms
- TEST-PASS | test_dom_BluetoothAdapter_discovery.js | took 8642ms
- TEST-PASS | test_dom_BluetoothAdapter_pair.js | took 11693ms

Comment 47

[Ben Tian (inactive)]

https://hg.mozilla.org/integration/b2g-inbound/rev/3e159fb268c0
https://hg.mozilla.org/integration/b2g-inbound/rev/a31cf9931d59

Comment 48

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/3e159fb268c0
https://hg.mozilla.org/mozilla-central/rev/a31cf9931d59

Comment 49

[Ben Tian (inactive)]

This bug is part of WebBluetooth API refinement (bug 1005848). Preliminary documentation is on wiki [1] and this bug implements comment 0.

[1] https://wiki.mozilla.org/B2G/Bluetooth/WebBluetooth-v2

Comment 50

[Florian Scholz (Open Web Docs)]

https://developer.mozilla.org/en-US/docs/Mozilla/B2G_OS/API/BluetoothAdapter