892933 - [Bluetooth] Replace AddReservedServicesInternal by non-blocking implementation

Status: RESOLVED FIXED

(Firefox OS Graveyard :: Bluetooth, defect)

Description

[Thomas Zimmermann [:tzimmermann] [:tdz]]

AddReservedServicesInternal blocks the Bluetooth command thread. We should replace it by a non-blocking implementation.

This bug depends on bug 891866, about reimplementing RegisterAgent, because the call to RegisterAgent will need to be reimplemented in the DBus reply handler of AddReservedServiceRecords.

Comment 1

[Thomas Zimmermann [:tzimmermann] [:tdz]]

Attachment: Bug 892933: Replace AddServiceRecordsInternal by non-blocking implementation

Comment 2

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

Comment on attachment 782493 [details] [diff] [review]
Bug 892933: Replace AddServiceRecordsInternal by non-blocking implementation

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

r=me with the question answered.

::: dom/bluetooth/linux/BluetoothDBusService.cpp
@@ +1227,5 @@
> +    MOZ_ASSERT(handler.get());
> +
> +    const dbus_uint32_t* services = sServices;
> +
> +    bool success = dbus_func_args_async(gThreadConnection->GetConnection(), -1,

Do we need to use another nsRefPtr to hold gThreadConnection here like line 1171?

Comment 3

[Thomas Zimmermann [:tzimmermann] [:tdz]]

(In reply to Eric Chou [:ericchou] [:echou] from comment #2)
> Comment on attachment 782493 [details] [diff] [review]
> Bug 892933: Replace AddServiceRecordsInternal by non-blocking implementation
> 
> Review of attachment 782493 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> r=me with the question answered.
> 
> ::: dom/bluetooth/linux/BluetoothDBusService.cpp
> @@ +1227,5 @@
> > +    MOZ_ASSERT(handler.get());
> > +
> > +    const dbus_uint32_t* services = sServices;
> > +
> > +    bool success = dbus_func_args_async(gThreadConnection->GetConnection(), -1,
> 
> Do we need to use another nsRefPtr to hold gThreadConnection here like line
> 1171?

Let's just reuse threadConnection from above. I don't know why I didn't see this problem in the first place. If you don't object, I'll fix this and commit the patch with your r+.

Comment 4

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

Comment on attachment 782493 [details] [diff] [review]
Bug 892933: Replace AddServiceRecordsInternal by non-blocking implementation

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

Looks great. r=me. Please take care of my comments below. Thanks.

::: dom/bluetooth/linux/BluetoothDBusService.cpp
@@ +1129,5 @@
> +    ExtractHandles(aReply, handles);
> +
> +    if(!RegisterAgent(&sAgentVTable)) {
> +      NS_WARNING("Failed to register agent");
> +      return;

please remove the "return" here.

@@ +1146,5 @@
> +    dbus_bool_t success = dbus_message_get_args(aMessage, &error,
> +                                                DBUS_TYPE_ARRAY,
> +                                                DBUS_TYPE_UINT32,
> +                                                &handles, &length,
> +                                                DBUS_TYPE_INVALID);

It would be fine to use dbus type here, but I would suggest to use C++ primitive type for consistency.

@@ +1225,5 @@
> +
> +    nsRefPtr<DBusReplyHandler> handler = new AddReservedServiceRecordsReplyHandler();
> +    MOZ_ASSERT(handler.get());
> +
> +    const dbus_uint32_t* services = sServices;

It would be fine to use dbus type here, but I would suggest to use C++ primitive type for consistency.

Comment 5

[Thomas Zimmermann [:tzimmermann] [:tdz]]

Thanks for the review.

> @@ +1225,5 @@
> > +
> > +    nsRefPtr<DBusReplyHandler> handler = new AddReservedServiceRecordsReplyHandler();
> > +    MOZ_ASSERT(handler.get());
> > +
> > +    const dbus_uint32_t* services = sServices;
> 
> It would be fine to use dbus type here, but I would suggest to use C++
> primitive type for consistency.

We supply this array directly to DBus, which expects dbus_uint32_t, and there is no strict type checking here. While dbus_uint32_t and C++' uint32_t should be compatible, there can be subtle differences, such as memory alignment. It's therefore much safer to use dbus_uint32_t, although it's inconsistent with the rest of the code.

Comment 6

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

(In reply to Thomas Zimmermann [:tzimmermann] [:tdz] from comment #5)
> Thanks for the review.
> 
> > @@ +1225,5 @@
> > > +
> > > +    nsRefPtr<DBusReplyHandler> handler = new AddReservedServiceRecordsReplyHandler();
> > > +    MOZ_ASSERT(handler.get());
> > > +
> > > +    const dbus_uint32_t* services = sServices;
> > 
> > It would be fine to use dbus type here, but I would suggest to use C++
> > primitive type for consistency.
> 
> We supply this array directly to DBus, which expects dbus_uint32_t, and
> there is no strict type checking here. While dbus_uint32_t and C++' uint32_t
> should be compatible, there can be subtle differences, such as memory
> alignment. It's therefore much safer to use dbus_uint32_t, although it's
> inconsistent with the rest of the code.

Thanks for your clarification. :)

Comment 7

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

(In reply to Thomas Zimmermann [:tzimmermann] [:tdz] from comment #3)
> (In reply to Eric Chou [:ericchou] [:echou] from comment #2)
> > Comment on attachment 782493 [details] [diff] [review]
> > Bug 892933: Replace AddServiceRecordsInternal by non-blocking implementation
> > 
> > Review of attachment 782493 [details] [diff] [review]:
> > -----------------------------------------------------------------
> > 
> > r=me with the question answered.
> > 
> > ::: dom/bluetooth/linux/BluetoothDBusService.cpp
> > @@ +1227,5 @@
> > > +    MOZ_ASSERT(handler.get());
> > > +
> > > +    const dbus_uint32_t* services = sServices;
> > > +
> > > +    bool success = dbus_func_args_async(gThreadConnection->GetConnection(), -1,
> > 
> > Do we need to use another nsRefPtr to hold gThreadConnection here like line
> > 1171?
> 
> Let's just reuse threadConnection from above. I don't know why I didn't see
> this problem in the first place. If you don't object, I'll fix this and
> commit the patch with your r+.

Sure, please go ahead. :)

Comment 8

[Thomas Zimmermann [:tzimmermann] [:tdz]]

Thanks! :)

https://hg.mozilla.org/integration/b2g-inbound/rev/73dcf35e07cf

Comment 9

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/73dcf35e07cf