764559 - [b2g-bluetooth] Don't fire BluetoothManager enabled domrequest until AdapterAdded message is fired

Status: RESOLVED FIXED

(Core :: DOM: Device Interfaces, defect)

Description

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

On the B2G phone, when we enable the bluetooth radio, the adapter does not immediately come up. This means DOM API users have to wait an arbitrary about of time before fetching the DefaultAdapter. We need some way to either make sure we don't return enabled before we have an adapter, or else provide a new DOM event to show that an adapter is available.

Comment 1

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

Attachment: v1: add eventlistener for adapteradded in BluetoothManager

In the current code base, the dom request is returned after updating the attribute enable in BluetoothManager, while the default adapter isn't ready yet. I suggested that BluetoothManager return an event called adapteradded to users when its default adapter is ready and they can call getDefaultAdapter immediately. I also move RegisterBluetoothSignalHandler from the constructor of BluetoothManager into function SetEnabled, so that the adapter will be registered every time we enable bluetooth, and an adapteradded event will be returned to Gaia. So as UnregisterbluetoothSignalHandler.

Comment 2

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

Comment on attachment 654940 [details] [diff] [review]
v1: add eventlistener for adapteradded in BluetoothManager

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

r-'ing because the signal registration didn't need to change and the new way breaks things.

More important, this changes made me realize there's a new fun bug in child object node name changes that I'll file after this.

::: dom/bluetooth/BluetoothManager.cpp
@@ -149,4 @@
>  
>  BluetoothManager::~BluetoothManager()
>  {
> -  BluetoothService* bs = BluetoothService::Get();

Leave this here. There's no guarentee we'll disable before we destruct.

@@ +187,4 @@
>  
>    nsRefPtr<BluetoothReplyRunnable> results = new ToggleBtResultTask(this, request, aEnabled);
>    if (aEnabled) {
> +    if (NS_FAILED(bs->RegisterBluetoothSignalHandler(NS_LITERAL_STRING("/"), this))) {

Remove register/unregister blocks here. We can stay registered to the signal for the lifetime of the manager object.

@@ -257,5 @@
> -    NS_WARNING("BluetoothService not available!");
> -    return nullptr;
> -  }
> -  
> -  if (NS_FAILED(bs->RegisterBluetoothSignalHandler(NS_LITERAL_STRING("/"), manager))) {

Leave this here. We can pre-register for signal handling on the manager node because the node address never changes.

@@ +301,4 @@
>  void
>  BluetoothManager::Notify(const BluetoothSignal& aData)
>  {
> +  InfallibleTArray<BluetoothNamedValue> arr;

This isn't used.

@@ +304,5 @@
> +  InfallibleTArray<BluetoothNamedValue> arr;
> +  if (aData.name().EqualsLiteral("AdapterAdded")) {
> +    nsRefPtr<nsDOMEvent> event = new nsDOMEvent(nullptr, nullptr);
> +    nsresult rv = event->InitEvent(NS_LITERAL_STRING("adapteradded"), false, false);
> + 

Nit: extra whitespace

::: dom/bluetooth/linux/BluetoothDBusService.cpp
@@ +899,5 @@
> +                               DBUS_TYPE_INVALID)) {
> +      LOG_AND_FREE_DBUS_ERROR_WITH_MSG(&err, aMsg);
> +      errorStr.AssignLiteral("Cannot parse manager path!");
> +    }
> +    v = NS_ConvertUTF8toUTF16(str);

Notice the if statement checks for an error, but still tries to set v even if there is an error. Set v in an else block (I fixed this for the other places in happens in another bug that's waiting to land).

Comment 3

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

I should say, general idea with the new signal is fine. It's just the registration changes that worry me. :)

Comment 4

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

I made the registration changes because I found an interesting thing when we enable/disable bluetooth. In the current code base, the objectpath of BluetoothManager, '/', is registered and added into mBluetoothSignalObserverTable when BluetoothManager is created. However, it is removed from mBluetoothSignalObserverTable when we disable bluetooth. More specifically, the table is cleared when we call StopInternal in linux/BluetoothDBusService.cpp. As a result, BluetoothManager is notified when there's an adapter added at first, and then if we close the bluetooth and re-open again, BluetoothManager isn't notified since the entry in mBluetoothSignalObserverTable is removed. One way to fix this problem is to register the object path each time whenever the bluetooth is enabled and unregister the object path when it is disabled. Or we should not remove the entry of BluetoothManager when we disable bluetooth. Any idea?

Comment 5

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

> More important, this changes made me realize there's a new fun bug in child
> object node name changes that I'll file after this.

Kyle, is what you observed the same with I mentioned in Comment 4? If it is, then I'll leave it to be fixed in the new bug. Otherwise, should I file another one for it? Or we can fix it here?

Comment 6

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

(In reply to Gina Yeh from comment #4)
>  One way to fix this problem is to
> register the object path each time whenever the bluetooth is enabled and
> unregister the object path when it is disabled.

Oops! Totally forgot that I clear the event table when we disable bluetooth, heh. So it should actually be in /both/ ctor/dtor (for when we bring up a bluetoothmanager with bluetooth already enabled) and enable/disable (for when we explicitly enable/disable) I guess. However, we'd also need a way to check the observer list in BluetoothService to make sure we aren't double-adding the manager listener to the service.

Comment 7

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

(In reply to Gina Yeh from comment #5)
> > More important, this changes made me realize there's a new fun bug in child
> > object node name changes that I'll file after this.
> 
> Kyle, is what you observed the same with I mentioned in Comment 4? If it is,
> then I'll leave it to be fixed in the new bug. Otherwise, should I file
> another one for it? Or we can fix it here?

Nope, it's different, emailed you and eric about it. However, it doesn't take into account that we clear the table on disabling... :)

Comment 8

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

Attachment: v2: add eventlistener for adapteradded in BluetoothManager

RegisterBluetoothSignalHandler when bluetooth is enabled; UnregisterBluetoothSignalHandler when bluetooth is disabled or when BluetoothManager is destroyed (since there's no guarentee bluetooth is disabled before it is destroyed). 

I prefer not to register when we construct BluetoothMamager because, in this way, we don't have to worry about the double-adding issue for manager listener. Plus, I can't find an appropriated way to check the duplicated observers in ObserverList, we can only add/remove observers, get length of list and broadcast to all observers. Am I missing something here?

Comment 9

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

Comment on attachment 655516 [details] [diff] [review]
v2: add eventlistener for adapteradded in BluetoothManager

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

r-'ing because we're still stuck in the chicken and egg issue, but...

This brings up a much larger issue. We don't initialize our Enabled status correctly in BluetoothManager. It's just set to "false" even if it's true at construction time. So we need to expose IsBluetoothEnabled (see BluetoothGonkService) up through BluetoothService so we can check with the system on Manager creation, and set up the signal receiever if it's true. We then check against this value whenever we call SetEnabled, and only run the enable/disable if we're setting on->off or off->on. That'll cover the addition/deletion case too so we don't have to worry about double additions/deletions at all. Make sense?

I'm 99% sure IsBluetoothEnabled is non-blocking, but that's something to check in the bluedroid implementation, been a while since I looked at it.

Comment 10

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

Attachment: v3: add eventlistener for adapteradded in BluetoothManager

IsBluetoothEnabled() is exposed from BluetoothGonkService and it is used when creating a BluetoothManager. If bluetooth daemon is enabled, then register the object path of BluetoothManager and also set mEnabled to true. On the contrary, unregistering is being done when BluetoothManager is destroyed.

On the other hands, when we call BluetoothManager::SetEnabled(), we check the value of mEnabled and do registering/unregistering if necessary

Comment 11

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

Comment on attachment 655950 [details] [diff] [review]
v3: add eventlistener for adapteradded in BluetoothManager

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

Ok, r-'ing on a few reasons. Most importantly, the enabled/disabled stuff will be invalidated by bug 777665, which moves enabled/disabled over to the settings api (I didn't realize this until just now because I just reviewed his bug before yours, sorry about that). I'm saying we land this one after that bug, so coordinate with echou to build on top of his changes. Instead of checking the bluetooth firmware, we'll just check the settings api for enabled/disabled. That should reduce the code... that I just recommended you put in during the last review. :/

Even though it'll be removed, watch how you use the services, too. We can't expose platform specifics to the DOM objects, otherwise we break platform compatibility. We do actually have users that work on both desktop on the phone right now (most importantly, me :) ), not to mention we consider the platform specific processes to be quarentinable to the parent process when we OOP things, so we can never access them directly. That's why everything else accesses platform specifics through BluetoothService.

::: dom/bluetooth/BluetoothManager.cpp
@@ +9,4 @@
>  #include "BluetoothCommon.h"
>  #include "BluetoothAdapter.h"
>  #include "BluetoothService.h"
> +#include "BluetoothGonkService.h"

We can't expose services directly to the DOM objects. That breaks platform agnostics for the DOM objects.

Why are you even bringing this in? You already do the expose via BluetoothService.

@@ +181,4 @@
>      return NS_ERROR_FAILURE;
>    }
>  
> +  if (!mEnabled && aEnabled) {

I just realized this is going to be invalidated the second bug 777665 lands. We need to coordinate with echou.

@@ +267,4 @@
>  BluetoothManager::Create(nsPIDOMWindow* aWindow) {
>  
>    nsRefPtr<BluetoothManager> manager = new BluetoothManager(aWindow);
> +  BluetoothService* bs = BluetoothGonkService::Get();

You can't do this. Or, well, you can, but you shouldn't. You exposed the function up through BluetoothService, why are you calling directly to Gonk?

@@ +274,5 @@
>    }
> +
> +  bool isEnabled = (bs->IsEnabledInternal() > 0) ? true : false;
> +  if (isEnabled) {
> +    manager->SetEnabledInternal(isEnabled);

If getting enabled is a non-block call, why cache?

::: dom/bluetooth/linux/BluetoothDBusService.cpp
@@ +897,5 @@
> +                               DBUS_TYPE_OBJECT_PATH, &str,
> +                               DBUS_TYPE_INVALID)) {
> +      LOG_AND_FREE_DBUS_ERROR_WITH_MSG(&err, aMsg);
> +      errorStr.AssignLiteral("Cannot parse manager path!");
> +    }

Nit: else on same line

::: dom/bluetooth/linux/BluetoothDBusService.h
@@ +82,4 @@
>    virtual bool 
>    SetAuthorizationInternal(const nsAString& aDeviceAddress, bool aAllow);
>  
> +  virtual int IsEnabledInternal() = 0;

You never implement this in BluetoothDBusService, so it'll break desktop B2G. We assume bluetooth is always enabled on desktop, just have it return true all the time (and make a comment that says why).

::: dom/bluetooth/nsIDOMBluetoothManager.idl
@@ +16,5 @@
>  
>    nsIDOMDOMRequest getDefaultAdapter();
>    nsIDOMDOMRequest setEnabled(in boolean enabled);
> +
> +  attribute nsIDOMEventListener onadapteradded;

Roll the UUID, as new events mean new vtable entries (I think :) )

Comment 12

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

Attachment: v4: add eventlistener for adapteradded in BluetoothManager

Sync with echou who's working on Bug 777665 and revise patch based on his final patch. Implemented IsEnableBluetooth() in BluetoothDBusService, too. And also remove the platform specifics part. 

As a result of Bug 777665 and Bug 764559, two events are fired in the process of enabling bluetooth. onenabled means that bluetooth daemon is ready and successfully enabled; onadapteradded means that default adapter is ready in bluetooth daemon.

Comment 13

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

Comment on attachment 656770 [details] [diff] [review]
v4: add eventlistener for adapteradded in BluetoothManager

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

Looks good to me. Still needs an sr now that it's a DOM change.

::: dom/bluetooth/BluetoothManager.h
@@ +50,5 @@
>    nsresult HandleMozsettingChanged(const PRUnichar* aData);
>  
>    bool mEnabled;
>  
> +  NS_DECL_EVENT_HANDLER(adapteradded)

Just a heads up on this, run on try against the tip of m-i before landing. If khuey's DOMEventTargetHelper patch gets in before you, this'll need to be changed.

::: dom/bluetooth/nsIDOMBluetoothManager.idl
@@ +15,5 @@
>    readonly attribute bool enabled;
>  
>    nsIDOMDOMRequest getDefaultAdapter();
>  
> +  attribute nsIDOMEventListener onadapteradded;

This'll have to be changed too if khuey's stuff lands. :)

Comment 14

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

https://tbpl.mozilla.org/?tree=Try&rev=6df960383c31

Comment 15

[Johnny Stenback (:jst)]

Comment on attachment 656770 [details] [diff] [review]
v4: add eventlistener for adapteradded in BluetoothManager

- In BluetoothManager::HandleMozsettingChange():

+  bool IsEnabled = (bs->IsEnabledInternal() > 0) ? true : false;

No need for the "? true : false" part there (or the extra parenthesis), the value of the > operation is already true or false. Also, common style guides says to have variable names start with a lower case character, so isEnabled instead of IsEnabled.

Same comment applies to BluetoothManager::Create().

sr=jst with that.

Comment 16

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

Attachment: Final version: add eventlistener for adapteradded in BluetoothManager, r=qdot, sr=jst

rename variable IsEnabled to isEnabled, and also remove "? true : false" part.

Comment 17

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

try: https://tbpl.mozilla.org/?tree=Try&rev=d8f76e12518c

Comment 18

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

https://hg.mozilla.org/integration/mozilla-inbound/rev/58947f81d2a7

Comment 19

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/58947f81d2a7