729991 - Bluetooth followup

Status: RESOLVED FIXED

(Core :: DOM: Device Interfaces, defect)

Description

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

Followup to bug 713116, some little things and one particularly subtle race condition.

Here are the things I see:

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

::: dom/bluetooth/BluetoothAdapter.cpp
@@ +15,5 @@
> +#if defined(MOZ_WIDGET_GONK)
> +#include <bluedroid/bluetooth.h>
> +#endif
> +
> +#define POWERED_EVENT_NAME NS_LITERAL_STRING("powered")

Since you have to use this constant three times (in header, in InitEvent, and NS_IMPL_EVENT_HANDLER) I would suggest changing this as follows:

Header:

  #define POWERED_EVENT_NAME "powered"
 
Cpp:

  event->InitEvent(NS_LITERAL_STRING(POWERED_EVENT_NAME), ...)
  ...
  NS_IMPL_EVENT_HANDLER(BluetoothAdapter, POWERED_EVENT_NAME)

@@ +17,5 @@
> +#endif
> +
> +#define POWERED_EVENT_NAME NS_LITERAL_STRING("powered")
> +
> +BEGIN_BLUETOOTH_NAMESPACE

Nit: This is not necessary. In fact, I'd recommend using the anonymous namespace here.

@@ +27,5 @@
> +      : mResult(result)
> +    {
> +      MOZ_ASSERT(!NS_IsMainThread()); // This should be running on the worker thread
> +
> +      mAdapterPtr.swap(adapterPtr);

Nit: Since this is tricky a comment saying why we do this could be helpful.

@@ +32,5 @@
> +    }
> +
> +    NS_IMETHOD Run() {
> +      MOZ_ASSERT(NS_IsMainThread()); // This method is supposed to run on the main thread!
> +      mAdapterPtr->FirePowered();

Here's the race! Need to null out mAdapterPtr here before returning (while you're guaranteed to be on the main thread).

The background thread maintains a (threadsafe) ref on this runnable while it is dispatching to the main thread, so if this runnable runs on main thread before background thread finishes cleaning up then the background thread could end up with the last reference to this runnable. When it releases the last ref it will destroy this runnable and call release on mAdapterPtr if it is non-null. That will crash in the CC.

Very subtle. The only way I know about this is because I've committed this exact bug about 500 times now, and had to debug it every time :-/

@@ +38,5 @@
> +      return NS_OK;
> +    }
> +
> +  private:
> +    bool mResult;

This is unused and we should figure out what to do here. Simple warning? JS console message? Fire some error event at the page? If we don't care then we should remove this.

@@ +47,5 @@
> +{
> +  public:
> +    ToggleBtTask(bool onOff, BluetoothAdapter* adapterPtr)
> +      : mOnOff(onOff),
> +      mAdapterPtr(adapterPtr)

Nit: Indentation is weird here.

@@ +70,5 @@
> +#endif
> +
> +      // Create a result thread and pass it to Main Thread, 
> +      nsCOMPtr<nsIRunnable> resultRunnable = new ToggleBtResultTask(result, mAdapterPtr);
> +      NS_DispatchToMainThread(resultRunnable);

This can (theoretically) fail. I'd make this:

  if (NS_FAILED(NS_DispatchToMainThread(...))) {
    NS_WARNING("Failed to dispatch to main thread!");
  }

@@ +87,2 @@
>  
>  USING_BLUETOOTH_NAMESPACE

Nit: I'd move this to the top of the file, then you don't have to add the 'mozilla::dom::bluetooth::' in the line above.

@@ +91,5 @@
> +
> +NS_IMPL_CYCLE_COLLECTION_TRAVERSE_BEGIN_INHERITED(BluetoothAdapter,
> +    nsDOMEventTargetHelper)
> +NS_CYCLE_COLLECTION_TRAVERSE_EVENT_HANDLER(powered)
> +  NS_IMPL_CYCLE_COLLECTION_TRAVERSE_END

Nit: indent on these macros got screwed up somewhere along the way.

@@ +114,5 @@
>  NS_IMETHODIMP
>  BluetoothAdapter::GetPower(bool* aPower)
>  {
> +#if defined(MOZ_WIDGET_GONK)  
> +  *aPower = bt_is_enabled();

Eric thinks that this is a blocking call, and this is a synchronous getter that happens on the main thread. Can we just always return the mPower value (i.e. the last thing we set to)? Otherwise we need to reassess here.

@@ +142,5 @@
> +  }
> +
> +  nsCOMPtr<nsIRunnable> r = new ToggleBtTask(mPower, this);
> +
> +  mToggleBtThread->Dispatch(r, 0);

This can fail, so you should change ToggleBluetoothAsync to return nsresult, then make sure you use the result in SetPower so the web page will know if something goes wrong.

Also, don't use '0' here, use NS_DISPATCH_NORMAL (from nsIEventTarget.idl)

@@ +153,5 @@
> +  nsresult rv = event->InitEvent(POWERED_EVENT_NAME, false, false);
> +  NS_ENSURE_SUCCESS(rv, rv);
> +
> +  bool dummy;
> +  rv = DispatchEvent(event, &dummy);

This event will be untrusted by default. Need to call SetIsTrusted on it before dispatching.

::: dom/bluetooth/BluetoothAdapter.h
@@ +13,5 @@
>  
>  BEGIN_BLUETOOTH_NAMESPACE
>  
>  class BluetoothAdapter : public nsIDOMBluetoothAdapter
> +                        ,public nsDOMEventTargetHelper

Nit: space between , and public, then line up. Or put , on previous line.

@@ +22,5 @@
>  
> +  NS_FORWARD_NSIDOMEVENTTARGET(nsDOMEventTargetHelper::)
> +
> +  NS_DECL_CYCLE_COLLECTION_CLASS_INHERITED(BluetoothAdapter,
> +                                           nsDOMEventTargetHelper)

Ok, since you've made this an event target you have to change the flags in nsDOMClasInfo.cpp. Specifically, use nsEventTargetSH and EVENTTARGET_SCRIPTABLE_FLAGS (rather than nsDOMGenericSH and DOM_DEFAULT_SCRIPTABLE_FLAGS).

@@ +34,5 @@
> +
> +  NS_DECL_EVENT_HANDLER(powered)
> +
> +private:
> +  nsCOMPtr<nsIEventTarget> mToggleBtThread;

Since you're using nsIEventTarget here you should forward declare it above (before BEGIN_BLUETOOTH_NAMESPACE)

Comment 1

[Philipp von Weitershausen [:philikon]]

(Follow-up bugs always *block* the original bug. It may sound weird at first, but it makes sense if you think in terms of dependency trees.)

Comment 2

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

> @@ +87,2 @@
> >  
> >  USING_BLUETOOTH_NAMESPACE

> Nit: I'd move this to the top of the file, then you don't have to add the 
> 'mozilla::dom::bluetooth::' in the line above.

The problem with this is that we'd then have 

DOMCI_INFO(BluetoothAdapter, BluetoothAdapter);

One of which would be supposed to be in the global namespace, and one which would be in m::d::bt. This'll get fixed with the change to MozBluetoothAdapter, but for now that won't work, I think?

Comment 3

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

(In reply to Kyle Machulis [:kmachulis] [:qdot] from comment #2)
> One of which would be supposed to be in the global namespace, and one which
> would be in m::d::bt. This'll get fixed with the change to
> MozBluetoothAdapter, but for now that won't work, I think?

No, it's fine. The macro expands that to kDOMClassInfo_BluetoothAdapter_interfaces.

Comment 4

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

Attachment: Patch 1: fix followup problems of bug 713116, r=ben

Comment 5

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

Comment on attachment 600393 [details] [diff] [review]
Patch 1: fix followup problems of bug 713116, r=ben

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

A few little things here, and need qDot's input on error handling (see below). Also, need to remember to change nsDOMClassInfo flags like I mentioned in comment 0.

::: dom/bluetooth/BluetoothAdapter.cpp
@@ +15,5 @@
>  #if defined(MOZ_WIDGET_GONK)
>  #include <bluedroid/bluetooth.h>
>  #endif
>  
> +#define POWERED_EVENT_NAME powered

If you put this in the header then you can use it for the NS_DECL_EVENT_HANDLER macro too.

@@ +17,5 @@
>  #endif
>  
> +#define POWERED_EVENT_NAME powered
> +#define TO_NS_LITERAL_STRING(A) NS_LITERAL_STRING(#A)
> +#define IMPL_EVENT_HANDLER(A, B) NS_IMPL_EVENT_HANDLER(A, B)

This shouldn't be needed, right?

@@ +35,5 @@
>      NS_IMETHOD Run() {
> +      MOZ_ASSERT(NS_IsMainThread());
> + 
> +      // mAdapterPtr must have to be null before this thread ended to 
> +      // solve the race condition problem. 

Nit: I would say "mAdapterPtr must be null before returning to prevent the background thread from racing to release it during the destruction of this runnable."

@@ +67,2 @@
>        } else {
> +        bt_disable();

Hm, so now we're just ignoring the result here... Is that ok? qDot should probably weigh in here.

@@ +124,5 @@
>    if (mPower != aPower) {
>      mPower = aPower;
>  
> +    if (NS_FAILED(ToggleBluetoothAsync())) {
> +      NS_WARNING("Fail to dispatch toggle bt task to worker thread");

I don't think a warning is sufficient here. I would return whatever error ToggleBluetoothAsync returns (the web page should know if this call fails).

Comment 6

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

Yeah, we'd like to get back the error if something's != 0 from bt_enable/disable. Those do return meaningful codes.

Comment 7

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

(In reply to Kyle Machulis [:kmachulis] [:qdot] from comment #6)
> Yeah, we'd like to get back the error if something's != 0 from
> bt_enable/disable. Those do return meaningful codes.

Ok, and what should happen if that fails? Do we just need to put a warning in stderr? A warning in the JS console? Or do we need to fire some sort of error event so that the web page knows? (Since this is async we can't just throw an exception)

Comment 8

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

Hmm. If bt_enable doesn't work, we probably want to throw an error. If bt_disable doesn't work... I guess we should also throw? I mean, we could also check bt_is_enabled, but improper shutdown could possibly mean we can't bring it back up right either.

Comment 9

[Philipp von Weitershausen [:philikon]]

(In reply to Kyle Machulis [:kmachulis] [:qdot] from comment #8)
> Hmm. If bt_enable doesn't work, we probably want to throw an error.

Throw to where? I would hope that bt_enable isn't called synchronously from the DOM. So yeah, I think you want to dispatch an error event.

Comment 10

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

Since we've decided to move to DOMRequest errors in re: 731361, we'll just have errors print to stderr right now to get this patch thru.

Comment 11

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

(In reply to ben turner [:bent] from comment #5)
> ::: dom/bluetooth/BluetoothAdapter.cpp
> @@ +15,5 @@
> >  #if defined(MOZ_WIDGET_GONK)
> >  #include <bluedroid/bluetooth.h>
> >  #endif
> >  
> > +#define POWERED_EVENT_NAME powered
> 
> If you put this in the header then you can use it for the
> NS_DECL_EVENT_HANDLER macro too.
> 
> @@ +17,5 @@
> >  #endif
> >  
> > +#define POWERED_EVENT_NAME powered
> > +#define TO_NS_LITERAL_STRING(A) NS_LITERAL_STRING(#A)
> > +#define IMPL_EVENT_HANDLER(A, B) NS_IMPL_EVENT_HANDLER(A, B)
> 
> This shouldn't be needed, right?
> 

The macro NS_IMPL_EVENT_HANDLER(_class, _event) is using '##' to concatenate the argument to a string, in this case, if we use

    NS_IMPL_EVENT_HANDLER(POWERED_EVENT_NAME)

, compile errors will occur because POWERED_EVENT_NAME would be treated as a string, not a macro. The error message is like:

error: no 'nsresult mozilla::dom::bluetooth::BluetoothAdapter::GetOnPOWERED_EVENT_NAME(nsIDOMEventListener**)' member function declared in class 'mozilla::dom::bluetooth::BluetoothAdapter'

That's why I use an intermediate macro IMPL_EVENT_HANDLER. If there is not a better solution for this, I would prefer not using macro to wrap 'powered'.

Comment 12

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

Attachment: Patch 1: fix followup problems of bug 713116, r=ben

Comment 13

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

Comment on attachment 602245 [details] [diff] [review]
Patch 1: fix followup problems of bug 713116, r=ben

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

Looks great, thanks! Just fix these nits before checking in:

::: dom/bluetooth/BluetoothAdapter.cpp
@@ +15,5 @@
>  #if defined(MOZ_WIDGET_GONK)
>  #include <bluedroid/bluetooth.h>
>  #endif
>  
>  #define POWERED_EVENT_NAME NS_LITERAL_STRING("powered")

Nit: Remove, just use 'NS_LITERAL_STRING("powered")' below.

@@ +30,5 @@
>  
>        mAdapterPtr.swap(adapterPtr);
>      }
>  
>      NS_IMETHOD Run() {

Nit: Please put { on the next line.

@@ +35,5 @@
> +      MOZ_ASSERT(NS_IsMainThread());
> +
> +      if (!mResult) {
> +        //TODO:Bug-731361
> +        fprintf(stderr, "BT firmware loading fails.\n"); 

Nit: NS_WARNING here

@@ +63,3 @@
>      }
>  
>      NS_IMETHOD Run() {

Nit: Please put { on the next line.

@@ +96,1 @@
>  DOMCI_DATA(BluetoothAdapter, mozilla::dom::bluetooth::BluetoothAdapter)

Nit: remove 'mozilla::dom::bluetooth::'

@@ +114,5 @@
>  
>  NS_IMPL_ADDREF_INHERITED(BluetoothAdapter, nsDOMEventTargetHelper)
>  NS_IMPL_RELEASE_INHERITED(BluetoothAdapter, nsDOMEventTargetHelper)
>  
>  BluetoothAdapter::BluetoothAdapter() : mPower(false)

Nit: Please put the : and member initializaton list on the next line

Comment 14

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

Attachment: Patch 1: fix followup problems of bug 713116, r=ben

Comment 15

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

http://hg.mozilla.org/integration/mozilla-inbound/rev/4dd463134071

Comment 16

[Matt Brubeck (:mbrubeck)]

https://hg.mozilla.org/mozilla-central/rev/4dd463134071