Implement pairing in BluetoothAdapter (methods)

RESOLVED FIXED in 2.1 S2 (15aug)

Status

defect
RESOLVED FIXED
5 years ago
3 years ago

People

(Reporter: ben.tian, Assigned: jaliu)

Tracking

({dev-doc-complete})

unspecified
2.1 S2 (15aug)
ARM
Gonk (Firefox OS)

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [webbt-api], [p=2])

Attachments

(2 attachments, 15 obsolete attachments)

805 bytes, patch
Details | Diff | Splinter Review
6.97 KB, patch
Details | Diff | Splinter Review
(Reporter)

Description

5 years ago
* 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();
(Reporter)

Comment 1

5 years ago
Jamin, please help on this bug first. Let me know for any question. Thanks.
Assignee: nobody → jaliu
(Assignee)

Updated

5 years ago
Whiteboard: [webbt-api] → [webbt-api], [p=2]
Target Milestone: --- → 2.0 S6 (18july)
(Assignee)

Updated

5 years ago
Status: NEW → ASSIGNED
(Assignee)

Updated

5 years ago
See Also: → 1032755
(Assignee)

Updated

5 years ago
Target Milestone: 2.0 S6 (18july) → 2.1 S2 (15aug)
(Assignee)

Comment 7

5 years ago
Attachment #8464647 - Attachment is obsolete: true
Attachment #8464647 - Flags: review?(btian)
Attachment #8465158 - Flags: review?(btian)
(Assignee)

Comment 8

5 years ago
Attachment #8464646 - Attachment is obsolete: true
Attachment #8464646 - Flags: review?(btian)
Attachment #8465182 - Flags: review?(btian)
(Reporter)

Comment 9

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

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

LGTM.
Attachment #8464643 - Flags: review?(btian) → review+
(Reporter)

Comment 10

5 years ago
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?
(Reporter)

Comment 11

5 years ago
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?
(Reporter)

Comment 12

5 years ago
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.
Attachment #8464648 - Flags: review?(btian)
(Assignee)

Comment 13

5 years ago
Attachment #8465158 - Attachment is obsolete: true
Attachment #8465158 - Flags: review?(btian)
Attachment #8465908 - Flags: review?(btian)
(Assignee)

Comment 14

5 years ago
(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. :)
(Assignee)

Comment 15

5 years ago
Attachment #8464648 - Attachment is obsolete: true
Attachment #8465909 - Flags: review?(btian)
(Assignee)

Comment 16

5 years ago
(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.
(Assignee)

Comment 17

5 years ago
(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.
(Assignee)

Updated

5 years ago
Attachment #8465932 - Attachment description: Hold the references of BluetoothReplyRunnable during Bluetooth SSP & Pin replying to avoid crash. (v1) → [Part 1] Hold the references of BluetoothReplyRunnable during Bluetooth SSP & Pin replying to avoid crash. (v1)
(Assignee)

Comment 19

5 years ago
Attachment #8465909 - Attachment is obsolete: true
Attachment #8465909 - Flags: review?(btian)
Attachment #8466956 - Flags: review?(btian)
(Assignee)

Updated

5 years ago
Depends on: 1016560
(Assignee)

Comment 20

5 years ago
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.
Attachment #8464649 - Attachment is obsolete: true
(Assignee)

Updated

5 years ago
Depends on: 1032755
No longer depends on: 1016560
(Assignee)

Comment 21

5 years ago
This bug depends on Bug 1032755. I typed the incorrect bug number by accident.
Sorry for any inconvenience caused.
(Reporter)

Comment 22

5 years ago
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.
Attachment #8465932 - Flags: review?(btian)
(Reporter)

Comment 23

5 years ago
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.
Attachment #8465908 - Flags: review?(btian)
(Reporter)

Comment 24

5 years ago
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.
Attachment #8466956 - Flags: review?(btian) → review+
(Reporter)

Comment 25

5 years ago
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.
(Assignee)

Comment 26

5 years ago
(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.
(Assignee)

Comment 27

5 years ago
This bug no longer depends on Bug 1032755 since #attachment 8465908 [details] [diff] [review] has been moved to Bug 104952.
No longer depends on: 1032755
(Assignee)

Comment 28

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

Moved to Bug 1049452.
Attachment #8465908 - Attachment is obsolete: true
(Assignee)

Comment 29

5 years ago
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.
Attachment #8465932 - Attachment is obsolete: true
(Assignee)

Comment 30

5 years ago
Rename this patch since [Pair 1] and [Pair 3] has been moved to Bug 1050068 and Bug 1049452.
Attachment #8466956 - Attachment is obsolete: true
Attachment #8469057 - Flags: review?(btian)
(Assignee)

Comment 31

5 years ago
(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.
(Assignee)

Comment 32

5 years ago
Attachment #8469057 - Attachment is obsolete: true
Attachment #8469057 - Flags: review?(btian)
Attachment #8469145 - Flags: review?(btian)
(Reporter)

Comment 33

5 years ago
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.
Attachment #8469145 - Flags: review?(btian)
(Assignee)

Comment 34

5 years ago
Attachment #8469145 - Attachment is obsolete: true
Attachment #8469783 - Flags: review?(btian)
(Assignee)

Updated

5 years ago
See Also: → 1050637
(Assignee)

Comment 35

5 years ago
(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.
(Reporter)

Comment 36

5 years ago
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.
Attachment #8469783 - Flags: review?(btian) → review+
(Assignee)

Comment 37

5 years ago
Thank Ben for the review.
* rebased.
Attachment #8469783 - Attachment is obsolete: true
(Assignee)

Comment 38

5 years ago
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. :)
Attachment #8469963 - Flags: review?(mrbkap)
(Assignee)

Comment 39

5 years ago
(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
Attachment #8469963 - Flags: review?(mrbkap) → review+
(Assignee)

Comment 40

5 years ago
* Add reviewer's name to commit message.
Attachment #8464643 - Attachment is obsolete: true
(Assignee)

Comment 41

5 years ago
* Add reviewer's names to commit message.
Attachment #8469963 - Attachment is obsolete: true
(Assignee)

Comment 42

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

Updated

5 years ago
Keywords: checkin-needed
(Assignee)

Comment 45

5 years ago
(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].
Keywords: checkin-needed
(Assignee)

Comment 46

5 years ago
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
https://hg.mozilla.org/mozilla-central/rev/3e159fb268c0
https://hg.mozilla.org/mozilla-central/rev/a31cf9931d59
Status: ASSIGNED → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
(Reporter)

Comment 49

5 years ago
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
You need to log in before you can comment on or make changes to this bug.