1128383 - [Bluetooth] There is no way to cancel incoming pairing request since the API v2 no 'bluetooth-cancel' system message.

Status: RESOLVED FIXED

(Firefox OS Graveyard :: Bluetooth, defect)

Description

[[e-mail], [e-mail]]

Since I'm working on implementation of pairing flow, I find out problem to cancel incoming pairing request.

STD:
1. Turn Bluetooth on.
2. Set visible on.
3. Use a remote device to pair with FFOS phone.
4. The pairing dialog shows to wait a user confirmation.
5. Cancel the pairing request from remote device.

Expected:
The pairing dialog should be close.

Actual:
The pairing dialog still waits a user confirmation.

Comment 1

[[e-mail], [e-mail]]

Since we cannot receive 'bluetooth-cancel' system message, we have to find out a way(Gaia/Gecko) to handle this use case here. I file the bug for a note.

Comment 2

[[e-mail], [e-mail]]

For reference in Gaia code base currently.
https://github.com/mozilla-b2g/gaia/blob/master/apps/bluetooth/js/modules/pair_manager.js#L53-L56

Comment 3

[[e-mail], [e-mail]]

Hi Jamin, 

Per offline discussion for this use case before, is Gaia able to reach the cancel message in API version 2? Thanks.

Comment 4

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

Hi Ian,
Bluetooth team will provide a new event handler called "BluetoothAdapter.onaborted" to let Gaia layer knows the connected was cancelled. (or aborted)

Thank you.

Comment 5

[[e-mail], [e-mail]]

Good idea with the new event. I file another bug 1128399 for tracking Gaia task.

Comment 6

[Ben Tian (inactive)]

Attachment: WIP patch (v1)

Comment 7

[Ben Tian (inactive)]

Attachment: Patch 1 (v2): Add adapter.onpairingaborted event handler

Comment 8

[Ben Tian (inactive)]

Comment on attachment 8570343 [details] [diff] [review]
Patch 1 (v2): Add adapter.onpairingaborted event handler

Request for review and feedback first. I'll rebase on bug 1127665 once it's landed.

Comment 9

[Shawn Huang [:shawnjohnjr][:shuang] (as a happy gecko contributor)]

Comment on attachment 8570343 [details] [diff] [review]
Patch 1 (v2): Add adapter.onpairingaborted event handler

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

For dom/base/nsGkAtomList.h, you need to ask dom peers to review.

Comment 10

[Shawn Huang [:shawnjohnjr][:shuang] (as a happy gecko contributor)]

Comment on attachment 8570343 [details] [diff] [review]
Patch 1 (v2): Add adapter.onpairingaborted event handler

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

Overall, it looks good to me, one comment for error handling. What do you think?

::: dom/base/nsGkAtomList.h
@@ +825,5 @@
>  GK_ATOM(onpagehide, "onpagehide")
>  GK_ATOM(onpageshow, "onpageshow")
>  GK_ATOM(onpaint, "onpaint")
>  GK_ATOM(onpairedstatuschanged, "onpairedstatuschanged")
> +GK_ATOM(onpairingaborted, "onpairingaborted")

Leave this to Dom peer to review.

::: dom/bluetooth2/BluetoothAdapter.cpp
@@ +365,5 @@
>        BluetoothStatusChangedEvent::Constructor(this, aData.name(), init);
>      DispatchTrustedEvent(event);
> +  } else if (aData.name().EqualsLiteral(PAIRING_ABORTED_ID) ||
> +             aData.name().EqualsLiteral(REQUEST_MEDIA_PLAYSTATUS_ID)) {
> +    DispatchEmptyEvent(aData.name());

This seems to me not necessary to extract 6 lines of code to an another new function (and it has only been used here). But if you think the original code is not clear enough, I'm fine with this change.

::: dom/bluetooth2/bluedroid/BluetoothServiceBluedroid.cpp
@@ +1634,4 @@
>      }
> +    case STATUS_BUSY:
> +    case STATUS_AUTH_FAILURE:
> +    case STATUS_RMT_DEV_DOWN:

Followed bug 1128386 (though the case is not the same), can we add case STATUS_FAIL to indicate authentication failure? Although default condition could capture it. stack can send this if there is an internal error.

::: dom/webidl/BluetoothAdapter2.webidl
@@ +80,5 @@
>    [NewObject, AvailableIn=CertifiedApps]
>    Promise<void> disable();
>  
>    [NewObject, AvailableIn=CertifiedApps]
> +  Promise<void> setName(DOMString name);

Good catch!

Comment 11

[Ben Tian (inactive)]

(In reply to Shawn Huang [:shawnjohnjr] from comment #10)
> ::: dom/bluetooth2/bluedroid/BluetoothServiceBluedroid.cpp
> @@ +1634,4 @@
> >      }
> > +    case STATUS_BUSY:
> > +    case STATUS_AUTH_FAILURE:
> > +    case STATUS_RMT_DEV_DOWN:
> 
> Followed bug 1128386 (though the case is not the same), can we add case
> STATUS_FAIL to indicate authentication failure? Although default condition
> could capture it. stack can send this if there is an internal error.
 
I follow API1 that sends 'cancel' for these cases. Might STATUS_FAIL be authentication error?
https://dxr.mozilla.org/mozilla-central/source/dom/bluetooth/bluedroid/BluetoothServiceBluedroid.cpp#1577

Comment 12

[Shawn Huang [:shawnjohnjr][:shuang] (as a happy gecko contributor)]

(In reply to Ben Tian [:btian] from comment #11)
> (In reply to Shawn Huang [:shawnjohnjr] from comment #10)
> > ::: dom/bluetooth2/bluedroid/BluetoothServiceBluedroid.cpp
> > @@ +1634,4 @@
> > >      }
> > > +    case STATUS_BUSY:
> > > +    case STATUS_AUTH_FAILURE:
> > > +    case STATUS_RMT_DEV_DOWN:
> > 
> > Followed bug 1128386 (though the case is not the same), can we add case
> > STATUS_FAIL to indicate authentication failure? Although default condition
> > could capture it. stack can send this if there is an internal error.
>  
> I follow API1 that sends 'cancel' for these cases. Might STATUS_FAIL be
> authentication error?
> https://dxr.mozilla.org/mozilla-central/source/dom/bluetooth/bluedroid/
> BluetoothServiceBluedroid.cpp#1577

This is something we discussed in Bug 1120889 Comment 7 (caused by cgroup change).
During the pairing procedure, we got STATUS_FAIL from stack. It seems to me that anything causes pairing failure and it does not involve actual authentication procedure, the stack returned STATUS_FAIL.

Comment 13

[Ben Tian (inactive)]

Attachment: Patch 1 (v3): Add adapter.onpairingaborted event handler, f=jocelyn, f=jaliu, r=shuang

Comment 14

[Jocelyn Liu [:jocelyn] [:joliu]]

Comment on attachment 8573744 [details] [diff] [review]
Patch 1 (v3): Add adapter.onpairingaborted event handler, f=jocelyn, f=jaliu, r=shuang

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

LGTM.

Comment 15

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

Comment on attachment 8573744 [details] [diff] [review]
Patch 1 (v3): Add adapter.onpairingaborted event handler, f=jocelyn, f=jaliu, r=shuang

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

LGTM

Comment 16

[Shawn Huang [:shawnjohnjr][:shuang] (as a happy gecko contributor)]

Comment on attachment 8573744 [details] [diff] [review]
Patch 1 (v3): Add adapter.onpairingaborted event handler, f=jocelyn, f=jaliu, r=shuang

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

Except for  nsGkAtomList.h,  BluetoothAdapter2.webidl, r=me. Please find DOM peer to check.

Comment 17

[Ben Tian (inactive)]

Comment on attachment 8573744 [details] [diff] [review]
Patch 1 (v3): Add adapter.onpairingaborted event handler, f=jocelyn, f=jaliu, r=shuang

Hi Boris,

Can you help review BluetoothAdapter2.webidl and nsGkAtomList.h change of the patch? This patch adds an event handler to notify application of bluetooth pairing error/cancellation.

Comment 18

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8573744 [details] [diff] [review]
Patch 1 (v3): Add adapter.onpairingaborted event handler, f=jocelyn, f=jaliu, r=shuang

r=me on the webidl/gkatomlist changes.

Comment 19

[Ben Tian (inactive)]

No try since the folder isn't built by default.

https://hg.mozilla.org/integration/b2g-inbound/rev/96351c40afc8

Comment 20

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/96351c40afc8