Closed Bug 1100818 Opened 6 years ago Closed 6 years ago

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

Categories

(Firefox OS Graveyard :: Bluetooth, defect)

ARM
Gonk (Firefox OS)
defect
Not set
normal

Tracking

(Not tracked)

RESOLVED FIXED

People

(Reporter: jaliu, Assigned: jaliu)

References

Details

(Whiteboard: [webbt-api])

Attachments

(1 file, 9 obsolete files)

20.75 KB, patch
Details | Diff | Splinter Review
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.
Assignee: nobody → jaliu
Status: NEW → ASSIGNED
See Also: → 1033898
Whiteboard: [webbt-api]
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.
Attachment #8525866 - Flags: feedback?(iliu)
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.
Attachment #8525866 - Flags: feedback?(iliu)
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.
Attachment #8525866 - Flags: review?(btian)
(In reply to Ian Liu [:ianliu] from comment #3)
Thank you for the feedback.
I'll investigate it.
Attachment #8528724 - Flags: review?(btian)
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 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".
Attachment #8528724 - Flags: review?(btian)
- Revised patch v1 based on #comment 9.
Attachment #8528724 - Attachment is obsolete: true
Attachment #8535400 - Flags: review?(btian)
(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 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.
Attachment #8535408 - Flags: review?(btian)
Attachment #8535527 - Flags: review?(btian)
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.
Attachment #8535527 - Flags: review?(btian)
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)
Attachment #8537055 - Flags: review?(btian) → review+
Keywords: checkin-needed
- 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.
Attachment #8538918 - Attachment is obsolete: true
Attachment #8538939 - Attachment description: (final) Launch bluetooth certified app by sending system message if it's not ready for receiving BluetoothPairingEvent. (v7) → (final) Launch bluetooth certified app by sending system message if it's not ready for receiving BluetoothPairingEvent. (v7), r=btian
https://hg.mozilla.org/mozilla-central/rev/9cc8561ec1a0
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
See Also: → 1192695
See Also: → 1196125
See Also: → 1229431
You need to log in before you can comment on or make changes to this bug.