789014 - [b2g-bluetooth] Broadcast bonding-related events via system message to settings app

Status: RESOLVED FIXED

(Core :: DOM: Device Interfaces, defect)

Description

[Gina Yeh [:gyeh] [:ginayeh]]

In current implementation, we'll get onrequestconfirmation, onrequestpasskey, and onrequestpincode only when settings app is on, so we need a way to broadcast these events via system message. In this way, if settings app is not running when we broadcast messages, it will be invoked at first and receive these events.

Comment 1

[Gina Yeh [:gyeh] [:ginayeh]]

Attachment: v1: Broadcast bonding-related events via system messages

Note that deviceName is added into the messages. 

When an unknown/non-discoverable device tries to pairing with us, then an dialog or settings app is poped out. At this time, we have to provide some information like deviceName for user to recognize devices.

Comment 2

[Gina Yeh [:gyeh] [:ginayeh]]

Attachment: v2: Broadcast bonding-related events via system messages

Sorry, typo was fixed in v1.

Comment 3

[Kyle Machulis [:qdot] [:kmachulis] (INACTIVE)]

Comment on attachment 658852 [details] [diff] [review]
v2: Broadcast bonding-related events via system messages

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

I'm not qualified to review this, as I've never worked with system messages. We'll have to find someone else.

Comment 4

[Kyle Machulis [:qdot] [:kmachulis] (INACTIVE)]

Comment on attachment 658852 [details] [diff] [review]
v2: Broadcast bonding-related events via system messages

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

Ok something did strike me here that's going to be an issue.

::: dom/bluetooth/BluetoothAdapter.cpp
@@ +363,5 @@
> +    JS_SetProperty(cx, obj, "passkey", &passkeyVal);
> +    JS_SetProperty(cx, obj, "name", &nameVal);
> +
> +    nsString type = NS_ConvertUTF8toUTF16("bluetooth-requestconfirmation");
> +    systemMessenger->BroadcastMessage(type, OBJECT_TO_JSVAL(obj));

We can't do this in the child process, where the DOM object exists. System Messages must be sent from the parent process. We're going to have to back out agent events to BluetoothService and register the service as the listener, so that we can distribute the system messages from there.

Comment 5

[Andrew Overholt [:overholt]]

We discussed this during triage today.  cjones said that he thinks this enables the launch of an app to receive a file transfer and if so this should block.

Comment 6

[Blake Kaplan (:mrbkap) (inactive)]

Comment on attachment 658852 [details] [diff] [review]
v2: Broadcast bonding-related events via system messages

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

Drive by: I don't know too much about system messages (qDot asked me if I could look at this), but I did want to comment a bit on the JSAPI usage. Gina, there are a bunch of good habits to get into when you use the JSAPI. Looking around at the patterns that other JSAPI-using code falls into using is a good idea. It will especially help reviewers concentrate more on the "meat" of your patch.

::: dom/bluetooth/BluetoothAdapter.cpp
@@ +343,5 @@
> +
> +    JSContext *cx = sc->GetNativeContext();
> +
> +    nsString deviceAddress = arr[0].value().get_nsString();
> +    JSString* JsDeviceAddress = JS_NewStringCopyN(cx,

Almost all JS_* functions can fail due to out of memory (OOM), so unless explicitly specified in a comment in jsapi.h, you should check for null or false return values and return early. In this case, since you can't propagate the error, you should also call JS_ReportPendingException(cx);

@@ +360,5 @@
> +
> +    JSObject* obj = JS_NewObject(cx, NULL, NULL, NULL);
> +    JS_SetProperty(cx, obj, "deviceAddress", &deviceAddressVal);
> +    JS_SetProperty(cx, obj, "passkey", &passkeyVal);
> +    JS_SetProperty(cx, obj, "name", &nameVal);

You repeat this pattern of converting to jsvals and creating the object 3 times (though here you add a passkey property as well). You should factor it out into a common function and handle passkey separately.

Comment 7

[Gina Yeh [:gyeh] [:ginayeh]]

Attachment: v3: Broadcast bonding-related events via system messages

This patch is modified as follows:
 - BlutoothService listens agent events
 - send system message in BluetoothService
 - add assertions/error handling for JS_* functions

Comment 8

[Gina Yeh [:gyeh] [:ginayeh]]

Attachment: v4: Broadcast bonding-related events via system messages

The v4 patch moves all agent events to BluetoothService, and assign each of them with an unique type of system message. The implementation of converting an array of nsString into a JSObject is also modified.

Comment 9

[Kyle Machulis [:qdot] [:kmachulis] (INACTIVE)]

Comment on attachment 659657 [details] [diff] [review]
v4: Broadcast bonding-related events via system messages

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

Summation of comments: I think we should stop this short at dealing with objects we already have via discovery. If we have a random device try to pair with us, it seems like we should establish it as an actual BluetoothDevice, meaning bringing back CreateDeviceFromAddress on BluetoothAdapter (we had this back in like, April/May, Eric wrote it then I took it out :| ). We can do that in a followup though.

::: dom/bluetooth/BluetoothService.cpp
@@ +444,5 @@
>    }
>  
>    gBluetoothService.swap(service);
> +
> +  if (NS_FAILED(gBluetoothService->RegisterBluetoothSignalHandler(

There doesn't seem to be a matching unregister anywhere? While we're expecting the service to live for the lifetime of the process, it'd probably still be good to unregister from ourselves when we destruct?

@@ +530,5 @@
> +  }
> +
> +  if (aData.name().EqualsLiteral("RequestConfirmation")) {
> +    NS_ASSERTION(arr.Length() == 3, "RequestConfirmation: Wrong length of parameters");
> +    type = NS_ConvertUTF8toUTF16("bluetooth-requestconfirmation");

type.AssignLiteral("bluetooth-requestconfirmation"), here and below

::: dom/bluetooth/linux/BluetoothDBusService.cpp
@@ +379,5 @@
>        errorStr.AssignLiteral("Invalid arguments for RequestConfirmation() method");
>      } else {
>        nsString deviceAddress = GetAddressFromObjectPath(NS_ConvertUTF8toUTF16(objectPath));
> +      nsString name;
> +      GetDeviceName(objectPath, name);

Blocking calls in our event handler loop seems like asking for problems, especially when we're calling it once per signal from bonding. What if someone walks out of range in between the time you get the signal and query?

We have a BluetoothDevice object already, why are we just floating data in this case when in the case of finding via discovery we establish a full device first? It may be time to reinstate the "create device from address" function. Either way, I think we could do that from a followup. But I'd really rather not have blocking calls in here, so we can remove those and call this done for discovery based bonding (which will unblock gaia for the moment), have gaia match bluetoothdevice objects it already knows about via addresses to get names, and go from there.

@@ +844,5 @@
>    aValue = props;
>  }
>  
> +bool
> +GetDeviceName(const char* aPath,

This isn't just GetDeviceName, it's another call to GetProperties that could've been abstracted and used for both our DeviceProperties array iteration as well as this. But, I really don't think it needs to happen for the moemnt.

Comment 10

[Gina Yeh [:gyeh] [:ginayeh]]

Attachment: v5: Broadcast bonding-related events via system messages

This patch removed GetDeviceName part in BluetoothDBusService since it is a blocking call. We'd like to create an BluetoothDevice when adapter got DeviceCreated message, and then fire a event with an instance of BluetoothDevice. In this way, we don't have to send device properties like device name in system message. Gaia should be able to get these properties from the instances of BluetoothDevice. 

BluetoothAdapter.ondevicecreated is a followup, please see Bug 790133.

Comment 11

[Kyle Machulis [:qdot] [:kmachulis] (INACTIVE)]

Comment on attachment 659962 [details] [diff] [review]
v5: Broadcast bonding-related events via system messages

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

r+ with nits picked

::: dom/bluetooth/BluetoothService.cpp
@@ +144,5 @@
>  
> +BluetoothService::~BluetoothService()
> +{
> +  if (!gBluetoothService) {
> +    return;

Supernit: Can this ever happen? Just making sure I'm not missing something.

@@ +148,5 @@
> +    return;
> +  }
> +
> +  if (NS_FAILED(gBluetoothService->UnregisterBluetoothSignalHandler(
> +        NS_LITERAL_STRING(LOCAL_AGENT_PATH), gBluetoothService))) {

Nit: indent off.

Comment 12

[Gina Yeh [:gyeh] [:ginayeh]]

gBluetoothService will be assigned as a null pointer when it is notified that we're going to shut down. We probably get null for gBluetoothService and can't/shouldn't do unregistering in this situation. 

This question reminds me that we should do unregister every time before assigning gBluetoothService to a null pointer, right? If it is, I'd like to put it in followup as well.

Comment 13

[Blake Kaplan (:mrbkap) (inactive)]

Comment on attachment 659962 [details] [diff] [review]
v5: Broadcast bonding-related events via system messages

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

Nits only. r=me with them picked.

::: dom/bluetooth/BluetoothService.cpp
@@ +492,5 @@
> +SetJsObject(JSContext* aContext,
> +            JSObject* aObj,
> +            const InfallibleTArray<BluetoothNamedValue>& aData)
> +{
> +  bool ok = true;

Nit: this isn't necessary.

@@ +494,5 @@
> +            const InfallibleTArray<BluetoothNamedValue>& aData)
> +{
> +  bool ok = true;
> +  for (int i = 0; i < aData.Length(); i++) {
> +    jsval JsVal;

The normal name for multi-purpose jsvals is simply "v".

@@ +497,5 @@
> +  for (int i = 0; i < aData.Length(); i++) {
> +    jsval JsVal;
> +    if (aData[i].value().type() == BluetoothValue::TnsString) {
> +      nsString data = aData[i].value().get_nsString();
> +      JSString* JsData = ::JS_NewStringCopyN(aContext,

Nit: (and I know we do this in some places) nuke the :: before JS_.

@@ +512,5 @@
> +
> +    ok = JS_SetProperty(aContext, aObj,
> +                        NS_ConvertUTF16toUTF8(aData[i].name()).get(),
> +                        &JsVal);
> +    if (!ok) {

This can totally be:

if (!JS_SetProperty(...)) {
  return false;
}

removing the need for "ok".

@@ +526,5 @@
> +  InfallibleTArray<BluetoothNamedValue> arr = aData.value().get_ArrayOfBluetoothNamedValue();
> +  nsString type;
> +
> +  JSContext* cx = nsContentUtils::GetSafeJSContext();
> +  NS_ASSERTION(!::JS_IsExceptionPending(cx),

Nuke the :: here as well, please.

Comment 14

[Gina Yeh [:gyeh] [:ginayeh]]

Attachment: Final version: Broadcast bonding-related events via system messages, r=qdot, r=mrbkap

Comment 15

[Eric Chou [:ericchou] [:echou]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/7666b019c7ef

Comment 16

[Gina Yeh [:gyeh] [:ginayeh]]

try server
https://tbpl.mozilla.org/?tree=Try&rev=4ccfd395db2a

Comment 17

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/7666b019c7ef