788949 - [b2g-bluetooth] Add PrepareAdapter function for registering agent and adding reserved services

Status: RESOLVED FIXED

(Core :: DOM: Device Interfaces, defect)

Description

[Ben Turner (not reading bugmail, use the needinfo flag!)]

Recently I've seen RegisterLocalAgent fail after turning bluetooth off and on a few times (specifically the "Can't register object path" line). It doesn't always show up and it seems to happen randomly so I suspect a race. Whenever I see the message, though, device scanning fails to find my other bluetooth devices.

Comment 1

[Andrew Overholt [:overholt]]

This seems bad enough to block.

Comment 2

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

Attachment: v1: Register agents when adapter is added

In the original design, we'll register agents (including local agent and remote agent) when we get default adapter. Since this function is called in both system app and settings app, register process will be down twice, and we'll see the log message like following:

Agent already registered, still returning true
RegisterLocalAgent: Can't register object path ... for agent!

One way to fix this is to register these agents when the adapter is added after bluetooth is enabled, and unregister them when bluetooth is turned off.

Comment 3

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

Comment on attachment 662025 [details] [diff] [review]
v1: Register agents when adapter is added

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

::: dom/bluetooth/linux/BluetoothDBusService.cpp
@@ +998,5 @@
>      } else {
>        sDefaultAdapterPath = NS_ConvertUTF8toUTF16(str);
>        AddReservedServices(sDefaultAdapterPath);
>        v = sDefaultAdapterPath;
> +      RegisterAgent(sDefaultAdapterPath);

We can't do this here. RegisterAgent accesses the gThreadConnection DBusConnection. gThreadConnection is supposed to only be accessed by the BluetoothService mCommandThread. Unfortunately, we also have no interface to BluetoothService to do agent registration at the moment. We need to bounce this to BluetoothService somehow, which most likely means a new interface function.

So, this is also wrong for the AddReservedServices right above it. So, if you wanted to make some sort of PrepareAdapter function that called both of those via BluetoothService, that would work too and kill two potentially nasty thread bugs with one stone.

I'm gonna start really strictly enforcing the "No functions called directly from Eventfilter" rule. Remember, it's on its own thread due to the poll(). Always bounce out to a runnable. It's gross, but until we can think of a nice way to queue events to mCommandThread in BluetoothService from a non-main-thread context, we're stuck with it.

Comment 4

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

Attachment: v2: Add PrepareAdapter function for registering agent and add reserved services

I'm going to mark bug 791436 as duplicated with this.

Eric, please also help to check readd reserved service part. Thanks.

Comment 5

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

Comment on attachment 662492 [details] [diff] [review]
v2: Add PrepareAdapter function for registering agent and add reserved services

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

Doesn't look like you included the header additions to BluetoothService/BluetoothDBusService/IPDL BluetoothService stuff?

Also, we should go ahead and start adding IPDL stubs for these. Since this should never ever be called from the child process, it's actually just a matter of copying/pasting some stuff. Check out how I deal with GetDevicePropertiesInternal in patch 2 of Bug 791147.

Other than that, looks good. Would like to see another patch version with the above changes before I r+.

::: dom/bluetooth/linux/BluetoothDBusService.cpp
@@ +650,5 @@
> +public:
> +  PrepareAdapterRunnable(const nsAString& aPath) :
> +    mPath(aPath)
> +  {
> +    MOZ_ASSERT(NS_IsMainThread());

We don't need this assertion. (Still needs to be removed from other runnables, can't remember why I put it there in the first place.)

Comment 6

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

Attachment: v3: Add PrepareAdapter function for registering agent and add reserved services

Also hook up with OOP in this patch.

Comment 7

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

Comment on attachment 662789 [details] [diff] [review]
v3: Add PrepareAdapter function for registering agent and add reserved services

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

Looks good to me.

::: dom/bluetooth/linux/BluetoothDBusService.cpp
@@ +1306,5 @@
>  
>    dbus_connection_remove_filter(mConnection, EventFilter, nullptr);
> +
> +  if (!dbus_connection_unregister_object_path(gThreadConnection->GetConnection(),
> +                                              LOCAL_AGENT_PATH) ){

Nit: misplaced blank

@@ +1312,5 @@
> +        __FUNCTION__, LOCAL_AGENT_PATH);
> +  }
> +
> +  if (!dbus_connection_unregister_object_path(gThreadConnection->GetConnection(),
> +                                              REMOTE_AGENT_PATH) ){

Nit: misplaced blank

Comment 8

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

Attachment: Final version: Add PrepareAdapter function for registering agent and adding reserved services, r=qdot, r=echou

try server: https://tbpl.mozilla.org/?tree=Try&rev=88d240dd9ac0

Comment 9

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

https://hg.mozilla.org/integration/mozilla-inbound/rev/095c446daab7

Comment 10

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/095c446daab7