884840 - [Bluetooth] Replace GetServiceChannel by non-blocking implementation

Status: RESOLVED FIXED

(Firefox OS Graveyard :: Bluetooth, defect)

Description

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

GetServiceChannel should be implemented without blocking on the Bluetooth command thread.

Comment 1

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

Attachment: [01] Bug 884840: Added DBusReplyHandler

A base class for implementing DBus reply handlers.

Comment 2

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

Attachment: [02] Bug 884840: Reimplement GetServiceChannel without Bluetooth command thread

Comment 3

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

I think there is a bug in patch #2. OnGetServiceChannel needs mRunnable. This is set (for example by BluetoothHfpManager) after GetServiceChannel returns. On the desktop, we call OnGetServiceChannel directly from GetServiceChannel. And this will probably fail because mRunnable hasn't been set yet. Please review the patches anyway and I'll fix this problem in the next version.

Comment 4

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

Comment on attachment 766625 [details] [diff] [review]
[02] Bug 884840: Reimplement GetServiceChannel without Bluetooth command thread

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

Hi Thomas,

Sorry for the late reply. I was out of office last week.

Several items have to be revised/answered. Please let me review again after revision. Thanks.

::: dom/bluetooth/linux/BluetoothDBusService.cpp
@@ +2599,5 @@
> +
> +    int channel = -1;
> +
> +    if (aReply && (dbus_message_get_type(aReply) != DBUS_MESSAGE_TYPE_ERROR)) {
> +      dbus_message_ref(aReply); // work around unref in dbus_returns_int32

Question: I'm not sure if we need this. I know aReply would be unref'ed in dbus_returns_int32, but wouldn't it be correct to release aReply in that funciton?

@@ +2627,5 @@
>  
>    nsString objectPath(GetObjectPathFromAddress(sAdapterPath, aDeviceAddress));
>  
> +#ifdef MOZ_WIDGET_GONK
> +  static const int sChannel = 0x0004;

The fixed value '0x0004' represents a service attribute called ProtocolDescriptorList, which is defined in section 5.1.5 in Service Discovery Protocol (SDP). This is how BlueZ knows which attribute value of an SDP service record should return. Therefore it may be better renaming to something like "sProtocolDescriptorListAttribute" or just "sProtocolDescriptorList".

@@ +2637,5 @@
> +
> +  nsRefPtr<OnGetServiceChannelReplyHandler> handler =
> +    new OnGetServiceChannelReplyHandler(objectPath, aServiceUUID, aManager);
> +
> +  bool success = dbus_func_args_async(gThreadConnection->GetConnection(), -1,

Since this is running on main thread, we can use mConnection instead of gThreadConnection->GetConnection().

Comment 5

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

Comment on attachment 766625 [details] [diff] [review]
[02] Bug 884840: Reimplement GetServiceChannel without Bluetooth command thread

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

Sorry for the late reply. I have no problem with this patch and I'd like to hand the review request over to echou. Please update the patch based on his comments, thanks.

Comment 6

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

Hi Eric,

The patch is underway.

> ::: dom/bluetooth/linux/BluetoothDBusService.cpp
> @@ +2599,5 @@
> > +
> > +    int channel = -1;
> > +
> > +    if (aReply && (dbus_message_get_type(aReply) != DBUS_MESSAGE_TYPE_ERROR)) {
> > +      dbus_message_ref(aReply); // work around unref in dbus_returns_int32
> 
> Question: I'm not sure if we need this. I know aReply would be unref'ed in
> dbus_returns_int32, but wouldn't it be correct to release aReply in that
> funciton?

Refcounting is a pure nightmare to me. ;) Correct me if I get it wrong somewhere. Here is how the code is supposed to work:

A reply message from DBus has a refcount of 1. In the current code, we take ownership of that message (line 417 in DBusUtils.cpp), and return it to the caller. The unref for the reply is done in dbus_returns_int32. Afterwards, the reply is gone.

In the new code, we also take the reply from the pending DBus call, but give it to the reply handler function and unref it afterwards (line 261+ in DBusUtils.cpp). This is actually more correct and save, because the callback handler doesn't have to care about refcounting any more. The only drawback is that is collides with the behavior of dbus_returns_int32.

I just grep'ed the code for dbus_returns_int32 and realized that this is the only user of the function. I'll fix the problem directly in dbus_returns_int32.

> @@ +2627,5 @@
> >  
> >    nsString objectPath(GetObjectPathFromAddress(sAdapterPath, aDeviceAddress));
> >  
> > +#ifdef MOZ_WIDGET_GONK
> > +  static const int sChannel = 0x0004;
> 
> The fixed value '0x0004' represents a service attribute called
> ProtocolDescriptorList, which is defined in section 5.1.5 in Service
> Discovery Protocol (SDP). This is how BlueZ knows which attribute value of
> an SDP service record should return. Therefore it may be better renaming to
> something like "sProtocolDescriptorListAttribute" or just
> "sProtocolDescriptorList".

Fixed. 

> @@ +2637,5 @@
> > +
> > +  nsRefPtr<OnGetServiceChannelReplyHandler> handler =
> > +    new OnGetServiceChannelReplyHandler(objectPath, aServiceUUID, aManager);
> > +
> > +  bool success = dbus_func_args_async(gThreadConnection->GetConnection(), -1,
> 
> Since this is running on main thread, we can use mConnection instead of
> gThreadConnection->GetConnection().

Fixed.

Comment 7

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

Attachment: [02] Bug 884840: Reimplement GetServiceChannel without Bluetooth command thread (v2)

Comment 8

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

(In reply to Thomas Zimmermann [:tzimmermann] [:tdz] from comment #6)
> Hi Eric,
> 
> The patch is underway.
> 
> > ::: dom/bluetooth/linux/BluetoothDBusService.cpp
> > @@ +2599,5 @@
> > > +
> > > +    int channel = -1;
> > > +
> > > +    if (aReply && (dbus_message_get_type(aReply) != DBUS_MESSAGE_TYPE_ERROR)) {
> > > +      dbus_message_ref(aReply); // work around unref in dbus_returns_int32
> > 
> > Question: I'm not sure if we need this. I know aReply would be unref'ed in
> > dbus_returns_int32, but wouldn't it be correct to release aReply in that
> > funciton?
> 
> Refcounting is a pure nightmare to me. ;) Correct me if I get it wrong
> somewhere. Here is how the code is supposed to work:
> 
> A reply message from DBus has a refcount of 1. In the current code, we take
> ownership of that message (line 417 in DBusUtils.cpp), and return it to the
> caller. The unref for the reply is done in dbus_returns_int32. Afterwards,
> the reply is gone.
> 
> In the new code, we also take the reply from the pending DBus call, but give
> it to the reply handler function and unref it afterwards (line 261+ in
> DBusUtils.cpp). 

You're right. I missed that the reply would be unref'ed in DBusConnectionSendWithReplyRunnable::Notify() right after callback function.

> This is actually more correct and save, because the callback handler doesn't 
> have to care about refcounting any more. 
>
> The only drawback is that is collides with the behavior of dbus_returns_int32.
> 
> I just grep'ed the code for dbus_returns_int32 and realized that this is the
> only user of the function. I'll fix the problem directly in
> dbus_returns_int32.
> 

I'm ok with the change, but it would make dbus_returns_int32 and dbus_returns_uint32 become inconsistent. dbus_returns_uint32 is used only once in BluetoothDBusService.cpp, too. So please file a bug to replace BluetoothDBusService::AddServiceRecords() by non-blocking implementation as well. Thanks.

Comment 9

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

Comment on attachment 772066 [details] [diff] [review]
[02] Bug 884840: Reimplement GetServiceChannel without Bluetooth command thread (v2)

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

Looks good to me. Please check the comment I left above and file another bug to fix inconsistency between dbus_returns_int32 and dbus_returns_uint32. Thanks.

Comment 10

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

Thanks!

https://hg.mozilla.org/projects/birch/rev/75ff62ef62a8
https://hg.mozilla.org/projects/birch/rev/08607d097c8b

Comment 11

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/75ff62ef62a8
https://hg.mozilla.org/mozilla-central/rev/08607d097c8b