Closed Bug 788949 Opened 8 years ago Closed 8 years ago

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

Categories

(Core :: DOM: Device Interfaces, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla18
blocking-basecamp +

People

(Reporter: bent.mozilla, Assigned: gyeh)

References

Details

(Whiteboard: [LOE:S])

Attachments

(1 file, 3 obsolete files)

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.
Summary: [b2g-bluetooth] → [b2g-bluetooth] RegisterLocalAgent fails, device scanning fails
This seems bad enough to block.
blocking-basecamp: ? → +
Whiteboard: [LOE:S]
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.
Assignee: echou → gyeh
Attachment #662025 - Flags: review?(kyle)
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.
Attachment #662025 - Flags: review?(kyle) → review-
I'm going to mark bug 791436 as duplicated with this.

Eric, please also help to check readd reserved service part. Thanks.
Attachment #662025 - Attachment is obsolete: true
Attachment #662492 - Flags: review?(kyle)
Attachment #662492 - Flags: review?(echou)
Summary: [b2g-bluetooth] RegisterLocalAgent fails, device scanning fails → [b2g-bluetooth] Add PrepareAdapter function for registering agent and adding reserved services
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.)
Attachment #662492 - Flags: review?(kyle)
Also hook up with OOP in this patch.
Attachment #662492 - Attachment is obsolete: true
Attachment #662492 - Flags: review?(echou)
Attachment #662789 - Flags: review?(kyle)
Attachment #662789 - Flags: review?(echou)
Attachment #662789 - Flags: review?(kyle) → review+
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
Attachment #662789 - Flags: review?(echou) → review+
https://hg.mozilla.org/mozilla-central/rev/095c446daab7
Status: NEW → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla18
You need to log in before you can comment on or make changes to this bug.