Closed Bug 892933 Opened 6 years ago Closed 6 years ago

[Bluetooth] Replace AddReservedServicesInternal by non-blocking implementation

Categories

(Firefox OS Graveyard :: Bluetooth, defect)

ARM
Gonk (Firefox OS)
defect
Not set

Tracking

(Not tracked)

RESOLVED FIXED

People

(Reporter: tzimmermann, Assigned: tzimmermann)

References

Details

Attachments

(1 file)

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 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?
Attachment #782493 - Flags: review?(echou) → review+
(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 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.
Attachment #782493 - Flags: review?(gyeh) → review+
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.
(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. :)
(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. :)
https://hg.mozilla.org/mozilla-central/rev/73dcf35e07cf
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.