1100818 - [Bluetooth 2] Launch bluetooth certified app by sending system message if it's not ready for receiving BluetoothPairingEvent.

Status: RESOLVED FIXED

(Firefox OS Graveyard :: Bluetooth, defect)

Description

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

Since we decided to move pairingReq listener from system app to bluetooth app, it's necessary to provide a mechanism to launch bluetooth app when receiving incoming pairing request.

Comment 1

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

Attachment: Launch bluetooth certified app by sending system message if it's not ready for receiving BluetoothPairingEvent. (v0)

Comment 2

[Ben Tian (inactive)]

Comment on attachment 8525866 [details] [diff] [review]
Launch bluetooth certified app by sending system message if it's not ready for receiving BluetoothPairingEvent. (v0)

Ian,

Per your offline discussion with Jamin, please verify this patch with gaia API v2 change and let us know for any problem. Thanks.

Comment 3

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

Comment on attachment 8525866 [details] [diff] [review]
Launch bluetooth certified app by sending system message if it's not ready for receiving BluetoothPairingEvent. (v0)

Per offline discussion with Jamin, the patch is not working for receiving 'bluetooth-request-pairing' system message @Bluetooth app.

Comment 4

[Ben Tian (inactive)]

Comment on attachment 8525866 [details] [diff] [review]
Launch bluetooth certified app by sending system message if it's not ready for receiving BluetoothPairingEvent. (v0)

Clear r? since the patch requires revision for comment 3.

Comment 5

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

(In reply to Ian Liu [:ianliu] from comment #3)
Thank you for the feedback.
I'll investigate it.

Comment 6

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

Attachment: Launch bluetooth certified app by sending system message if it's not ready for receiving BluetoothPairingEvent. (v0)

Comment 7

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

Attachment: Launch bluetooth certified app by sending system message if it's not ready for receiving BluetoothPairingEvent. (v1)

Comment 8

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

Hi Ben,
I'd like to ask you to review it. :)
Ian and I have verified #attachment 8528724 [details] [diff] [review] by sending and receiving pairing request with the WIP patch of Bug 1102796 Bug 1070823.

Comment 9

[Ben Tian (inactive)]

Comment on attachment 8528724 [details] [diff] [review]
Launch bluetooth certified app by sending system message if it's not ready for receiving BluetoothPairingEvent. (v1)

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

Please see my comment below.

::: dom/bluetooth2/BluetoothAdapter.cpp
@@ +199,5 @@
>    , mDiscoveryHandleInUse(nullptr)
>  {
>    MOZ_ASSERT(aWindow);
>  
> +  // Retrieval the app status and origin for permission checking

'Retrieve'

@@ +208,5 @@
> +    doc->NodePrincipal()->GetAppStatus(&appStatus);
> +    doc->NodePrincipal()->GetOrigin(getter_Copies(appOrigin));
> +  }
> +
> +  // Only allow certificated bluetooth application to receive pairing request

'certified'

@@ +209,5 @@
> +    doc->NodePrincipal()->GetOrigin(getter_Copies(appOrigin));
> +  }
> +
> +  // Only allow certificated bluetooth application to receive pairing request
> +  if (appStatus == nsIPrincipal::APP_STATUS_CERTIFIED

Wrap checking of bluetooth as a function.

  if (IsBluetoothApp()) {
    // create pairing listener
  }

@@ +210,5 @@
> +  }
> +
> +  // Only allow certificated bluetooth application to receive pairing request
> +  if (appStatus == nsIPrincipal::APP_STATUS_CERTIFIED
> +      && appOrigin.EqualsASCII(BLUETOOTH_APP_ORIGIN)) {

nit: put '&&' at the end of previous line.

::: dom/bluetooth2/BluetoothCommon.h
@@ +139,5 @@
>  #define KEY_LOCAL_AGENT       "/B2G/bluetooth/agent"
>  #define KEY_REMOTE_AGENT      "/B2G/bluetooth/remote_device_agent"
>  #define KEY_MANAGER           "/B2G/bluetooth/manager"
>  #define KEY_ADAPTER           "/B2G/bluetooth/adapter"
> +#define KEY_PAIRINGREQS       "/B2G/bluetooth/adapter/pairingreqs"

Should be #define KEY_PAIRING_LISTENER "B2G/bluetooth/pairing_listener"

@@ +168,5 @@
>  #define PAIRING_REQ_TYPE_CONFIRMATION         "pairingconfirmationreq"
>  #define PAIRING_REQ_TYPE_CONSENT              "pairingconsentreq"
>  
>  /**
> + * A system message used for launching bluetooth certified app if there is no

System message to launch bluetooth app if no pairing listener is ready to receive pairing requests.

@@ +174,5 @@
> + */
> +#define SYS_MSG_BT_PAIRING_REQ                "bluetooth-pairing-request"
> +
> +/**
> + * The app origin of the bluetooth app which is responsible for listening

... of bluetooth app, which ...

::: dom/bluetooth2/BluetoothPairingListener.cpp
@@ +25,5 @@
>    MOZ_ASSERT(aWindow);
> +
> +  BluetoothService* bs = BluetoothService::Get();
> +  NS_ENSURE_TRUE_VOID(bs);
> +  bs->RegisterBluetoothSignalHandler( NS_LITERAL_STRING(KEY_PAIRINGREQS), this);

nit: remove extra space

@@ +43,5 @@
>  
>  BluetoothPairingListener::~BluetoothPairingListener()
>  {
> +  BluetoothService* bs = BluetoothService::Get();
> +  // It can be null on shutdown.

nullptr

@@ +107,5 @@
> +
> +    // Notify pairing listener of pairing requests
> +    DispatchPairingEvent(device, passkey, type);
> +  } else {
> +    BT_WARNING("Not handling adapter signal: %s",

'pairing listener' signal

::: dom/bluetooth2/bluedroid/BluetoothServiceBluedroid.cpp
@@ +881,5 @@
>    return NS_OK;
>  }
>  
> +void
> +BluetoothServiceBluedroid::RegisterBluetoothSignalHandler(

Move out the queuing mechanism to BluetoothService.cpp.

::: dom/bluetooth2/ipc/BluetoothServiceChildProcess.cpp
@@ +93,4 @@
>    if (sBluetoothChild && !IsSignalRegistered(aNodeName)) {
>      sBluetoothChild->SendUnregisterSignalHandler(nsString(aNodeName));
>    }
> +  BluetoothService::UnregisterBluetoothSignalHandler(aNodeName, aHandler);

Why move this line?

::: dom/webidl/BluetoothAdapter2.webidl
@@ +41,5 @@
>    readonly attribute boolean                discoverable;
>    readonly attribute boolean                discovering;
>  
> +  /**
> +   * Only allow one certified app to listen pairing requests.

Suggest to revise as folllowing.

  Only bluetooth app is allowed to listen to pairing requests.
  Bluetooth app's origin is "app://bluetooth.gaiamobile.org".

Comment 10

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

Attachment: Launch bluetooth certified app by sending system message if it's not ready for receiving BluetoothPairingEvent. (v2)

- Revised patch v1 based on #comment 9.

Comment 11

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

Attachment: Launch bluetooth certified app by sending system message if it's not ready for receiving BluetoothPairingEvent. (v3)

Comment 12

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

(In reply to Ben Tian [:btian] from comment #9)
> ::: dom/bluetooth2/ipc/BluetoothServiceChildProcess.cpp
> @@ +93,4 @@
> >    if (sBluetoothChild && !IsSignalRegistered(aNodeName)) {
> >      sBluetoothChild->SendUnregisterSignalHandler(nsString(aNodeName));
> >    }
> > +  BluetoothService::UnregisterBluetoothSignalHandler(aNodeName, aHandler);
> 
> Why move this line?
After I read comments on Bug 860075, I reverted this change at Attachment #8535408 [details] [diff].
Thank you for asking.

Comment 13

[Ben Tian (inactive)]

Comment on attachment 8535408 [details] [diff] [review]
Launch bluetooth certified app by sending system message if it's not ready for receiving BluetoothPairingEvent. (v3)

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

Please see my comment below. I prefer to move all queuing mechanism from BluetoothServiceBluedroid into BluetoothService, since the mechanism is backend-independent.

::: dom/bluetooth2/BluetoothAdapter.cpp
@@ +198,5 @@
>    , mDiscoveryHandleInUse(nullptr)
>  {
>    MOZ_ASSERT(aWindow);
>  
> +  // Only allow certified bluetooth application to receive pairing request

nit: request's'

@@ +230,5 @@
>  
>    BluetoothService* bs = BluetoothService::Get();
>    NS_ENSURE_TRUE_VOID(bs);
>    bs->UnregisterBluetoothSignalHandler(NS_LITERAL_STRING(KEY_ADAPTER), this);
> +  bs->UnregisterBluetoothSignalHandler(NS_LITERAL_STRING(KEY_PAIRING_LISTENER),

Move this unregistration into |BluetoothPairingListener| and add function |DisconnectFromOwner| as [1], since |BluetoothPairingListener| also inherits |DOMEventTargetHelper| [2].

[1] http://dxr.mozilla.org/mozilla-central/source/dom/bluetooth2/BluetoothAdapter.cpp#222
[2] http://dxr.mozilla.org/mozilla-central/source/dom/events/DOMEventTargetHelper.h#128

@@ +770,5 @@
> +bool
> +BluetoothAdapter::IsBluetoothCertifiedApp()
> +{
> +  // Retrieve the app status and origin for permission checking
> +  nsCOMPtr<nsIDocument> doc = GetOwner()->GetExtantDoc();

nit: reorder to declare |doc| close to where it's used.

{
  uint16_t appStatus = nsIPrincipal::APP_STATUS_NOT_INSTALLED;
  nsAutoCString appOrigin;

  // Retrieve the app status and origin for permission checking
  nsCOMPtr<nsIDocument> doc = GetOwner()->GetExtantDoc();
  NS_ENSURE_TRUE(doc, false);

  doc->NodePrincipal()->GetAppStatus(&appStatus);
  doc->NodePrincipal()->GetOrigin(getter_Copies(appOrigin));
  ...
}

::: dom/bluetooth2/BluetoothPairingListener.cpp
@@ +109,5 @@
> +
> +    // Notify pairing listener of pairing requests
> +    DispatchPairingEvent(device, passkey, type);
> +  } else {
> +    BT_WARNING("Not handling 'pairing listener' signal: %s",

In prior comment I leave single quote mark of 'pairing listener' only to emphasize. Please remove it:)

  "Not handling pairing listener signal: %s",

::: dom/bluetooth2/BluetoothService.cpp
@@ +291,5 @@
>    }
>  
>    ol->AddObserver(aHandler);
> +
> +  // Distribute pending paring requests when pairing listener has been added

typo: pa'i'ring requests

::: dom/bluetooth2/bluedroid/BluetoothServiceBluedroid.cpp
@@ +1528,5 @@
> +
> +  // If there is no BluetoohPairingListener in observer table, put the signal
> +  // into a pending queue of paring requests and send a system message to launch
> +  // the app.
> +  if (!mBluetoothSignalObserverTable.Get(

Move this queuing mechanism into |BluetoothService::DistributeSignal|.

@@ +1586,5 @@
> +                                   NS_LITERAL_STRING(KEY_PAIRING_LISTENER),
> +                                   BluetoothValue(propertiesArray));
> +
> +
> +  // If there is no BluetoohPairingListener in observer table, put the signal

Ditto.

Comment 14

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

Attachment: Launch bluetooth certified app by sending system message if it's not ready for receiving BluetoothPairingEvent. (v4)

- Revise patch v3 based on comment 13

Comment 15

[Ben Tian (inactive)]

Comment on attachment 8535527 [details] [diff] [review]
Launch bluetooth certified app by sending system message if it's not ready for receiving BluetoothPairingEvent. (v4)

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

Almost there. Please see my comment below.

::: dom/bluetooth2/BluetoothAdapter.cpp
@@ +771,5 @@
> +  // Retrieve the app status and origin for permission checking
> +  uint16_t appStatus = nsIPrincipal::APP_STATUS_NOT_INSTALLED;
> +  nsAutoCString appOrigin;
> +  nsCOMPtr<nsIDocument> doc = GetOwner()->GetExtantDoc();
> +  if (doc) {

I prefer to check |!doc| here and return false directly here, to clarify that false is returned when |!doc|. But I'm fine with either way.

@@ +777,5 @@
> +    doc->NodePrincipal()->GetOrigin(getter_Copies(appOrigin));
> +  }
> +
> +  return appStatus == nsIPrincipal::APP_STATUS_CERTIFIED &&
> +         appOrigin.EqualsASCII(BLUETOOTH_APP_ORIGIN);

Use |EqualsLiteral|.

::: dom/bluetooth2/BluetoothPairingListener.cpp
@@ +128,5 @@
> +
> +  BluetoothService* bs = BluetoothService::Get();
> +  NS_ENSURE_TRUE_VOID(bs);
> +  bs->UnregisterBluetoothSignalHandler(NS_LITERAL_STRING(KEY_PAIRING_LISTENER),
> +                                     this);

nit: align the 2 lines.

::: dom/bluetooth2/BluetoothService.cpp
@@ +294,5 @@
> +
> +  // Distribute pending pairing requests when pairing listener has been added
> +  // to signal observer table.
> +  if (IsMainProcess() &&
> +      aNodeName.Equals(NS_LITERAL_STRING(KEY_PAIRING_LISTENER))) {

Use |EqualsLiteral|. Also ensure |!mPendingPairReqSignals.IsEmpty()|.

@@ +359,5 @@
>  
>    BluetoothSignalObserverList* ol;
>    if (!mBluetoothSignalObserverTable.Get(aSignal.path(), &ol)) {
> +    // If there is no BluetoohPairingListener in observer table, put the signal
> +    // into a pending queue of paring requests and send a system message to launch

nit: queue of pa'i'ring requests

@@ +360,5 @@
>    BluetoothSignalObserverList* ol;
>    if (!mBluetoothSignalObserverTable.Get(aSignal.path(), &ol)) {
> +    // If there is no BluetoohPairingListener in observer table, put the signal
> +    // into a pending queue of paring requests and send a system message to launch
> +    // the app.

nit: 'launch bluetooth certified app' is clearer.

@@ +361,5 @@
>    if (!mBluetoothSignalObserverTable.Get(aSignal.path(), &ol)) {
> +    // If there is no BluetoohPairingListener in observer table, put the signal
> +    // into a pending queue of paring requests and send a system message to launch
> +    // the app.
> +    if (aSignal.path().EqualsASCII(KEY_PAIRING_LISTENER) ) {

Use |EqualsLiteral|. Also remove space between two ')'

@@ +364,5 @@
> +    // the app.
> +    if (aSignal.path().EqualsASCII(KEY_PAIRING_LISTENER) ) {
> +      mPendingPairReqSignals.AppendElement(aSignal);
> +
> +      nsString type = NS_LITERAL_STRING(SYS_MSG_BT_PAIRING_REQ);

Simplify to NS_NAMED_LITERAL_STRING(type, SYS_MSG_BT_PAIRING_REQ)

http://dxr.mozilla.org/mozilla-central/source/xpcom/string/nsLiteralString.h#31

@@ +365,5 @@
> +    if (aSignal.path().EqualsASCII(KEY_PAIRING_LISTENER) ) {
> +      mPendingPairReqSignals.AppendElement(aSignal);
> +
> +      nsString type = NS_LITERAL_STRING(SYS_MSG_BT_PAIRING_REQ);
> +      InfallibleTArray<BluetoothNamedValue> parameters;

nit: rename to |emptyArr| to clarify it's an empty array.

@@ +366,5 @@
> +      mPendingPairReqSignals.AppendElement(aSignal);
> +
> +      nsString type = NS_LITERAL_STRING(SYS_MSG_BT_PAIRING_REQ);
> +      InfallibleTArray<BluetoothNamedValue> parameters;
> +      BT_ENSURE_TRUE_VOID_BROADCAST_SYSMSG(type, parameters);

Can we simplify all 3 lines to the following?

  BT_ENSURE_TRUE_VOID_BROADCAST_SYSMSG(NS_LITERAL_STRING(SYS_MSG_BT_PAIRING_REQ),
                                       BluetoothValue(EmptyString()));

::: dom/bluetooth2/bluedroid/BluetoothServiceBluedroid.cpp
@@ +1521,5 @@
>    BT_APPEND_NAMED_VALUE(propertiesArray, "passkey", EmptyString());
>    BT_APPEND_NAMED_VALUE(propertiesArray, "type",
>                          NS_LITERAL_STRING(PAIRING_REQ_TYPE_ENTERPINCODE));
>  
> +

nit: remove this extra newline.

Comment 16

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

Attachment: Launch bluetooth certified app by sending system message if it's not ready for receiving BluetoothPairingEvent. (v5)

Comment 17

[Ben Tian (inactive)]

Comment on attachment 8537055 [details] [diff] [review]
Launch bluetooth certified app by sending system message if it's not ready for receiving BluetoothPairingEvent. (v5)

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

r=me with comment addressed. Thanks for the effort! Please request DOM peer review to land the webidl change.

::: dom/bluetooth2/BluetoothAdapter.cpp
@@ +767,5 @@
>  
> +bool
> +BluetoothAdapter::IsBluetoothCertifiedApp()
> +{
> +  // Retrieve the app status and origin for permission checking

nit: reorder as following to make declaration close to where it's used.

{
  // Retrieve the app status and origin for permission checking
  nsCOMPtr<nsIDocument> doc = GetOwner()->GetExtantDoc();
  NS_ENSURE_TRUE(doc, false);

  uint16_t appStatus = nsIPrincipal::APP_STATUS_NOT_INSTALLED;
  nsAutoCString appOrigin;

  doc->NodePrincipal()->GetAppStatus(&appStatus);
  doc->NodePrincipal()->GetOrigin(getter_Copies(appOrigin));

  return appStatus == nsIPrincipal::APP_STATUS_CERTIFIED &&
         appOrigin.EqualsLiteral(BLUETOOTH_APP_ORIGIN);
}

@@ +771,5 @@
> +  // Retrieve the app status and origin for permission checking
> +  uint16_t appStatus = nsIPrincipal::APP_STATUS_NOT_INSTALLED;
> +  nsAutoCString appOrigin;
> +  nsCOMPtr<nsIDocument> doc = GetOwner()->GetExtantDoc();
> +  if (!doc) {

Simplify to NS_ENSURE_TRUE(doc, false)

Comment 18

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

Attachment: (final) Launch bluetooth certified app by sending system message if it's not ready for receiving BluetoothPairingEvent. (v6)

Thank Ben for reviewing the patch. :)

Comment 19

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

It looks fine on treeherder.
https://treeherder.mozilla.org/#/jobs?repo=try&revision=4c358dc3d61f

Comment 20

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

Attachment: (final) Launch bluetooth certified app by sending system message if it's not ready for receiving BluetoothPairingEvent. (v6)

- convert to hg format

Comment 21

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

Attachment: (final) Launch bluetooth certified app by sending system message if it's not ready for receiving BluetoothPairingEvent. (v7), r=btian

- Revert the change on BluetoothAdatper2.webidl.

I put an comment on BluetoothAdatper2.webidl to explain the usage and restriction of adapter.pairingReqs on patch v6.
However, since every webidl change requires for a DOM peer review, it seems annoying to ask DOM peer to review a patch which only added one simple comment.
Per offline discussion with Ben, I decide to move this comment to Bug 1063444 which is going to change BluetoothAdatper2.webidl and ask for DOM peer review.

Comment 22

[Ben Tian (inactive)]

https://hg.mozilla.org/integration/b2g-inbound/rev/9cc8561ec1a0

Comment 23

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/9cc8561ec1a0