Last Comment Bug 731361 - Change bluetooth firmware loading to use a DOMRequest based API
: Change bluetooth firmware loading to use a DOMRequest based API
Status: RESOLVED FIXED
:
Product: Core
Classification: Components
Component: DOM: Device Interfaces (show other bugs)
: unspecified
: x86_64 Gonk (Firefox OS)
: -- normal (vote)
: mozilla14
Assigned To: Eric Chou [:ericchou] [:echou]
:
Mentors:
Depends on: 729470 729991
Blocks: b2g-bluetooth
  Show dependency treegraph
 
Reported: 2012-02-28 12:04 PST by Kyle Machulis [:kmachulis] [:qdot]
Modified: 2012-03-28 14:02 PDT (History)
5 users (show)
ryanvm: in‑testsuite?
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
WIP (7.76 KB, patch)
2012-03-13 01:21 PDT, Eric Chou [:ericchou] [:echou]
no flags Details | Diff | Splinter Review
Patch 1: Use DOMRequest for async event firing (8.88 KB, patch)
2012-03-14 00:38 PDT, Eric Chou [:ericchou] [:echou]
no flags Details | Diff | Splinter Review
Patch 1: Use DOMRequest for async event firing (9.62 KB, patch)
2012-03-14 23:39 PDT, Eric Chou [:ericchou] [:echou]
kyle: feedback+
bent.mozilla: feedback-
Details | Diff | Splinter Review
Patch 1: Use DOMRequest for async event firing (10.67 KB, patch)
2012-03-16 01:12 PDT, Eric Chou [:ericchou] [:echou]
kyle: feedback+
Details | Diff | Splinter Review
Patch 1: Use DOMRequest for async event firing (11.21 KB, patch)
2012-03-19 21:06 PDT, Eric Chou [:ericchou] [:echou]
no flags Details | Diff | Splinter Review
Patch 1: Use DOMRequest for async event firing (11.24 KB, patch)
2012-03-22 00:19 PDT, Eric Chou [:ericchou] [:echou]
no flags Details | Diff | Splinter Review
Patch 1: Use DOMRequest for async event firing (11.22 KB, patch)
2012-03-25 21:03 PDT, Eric Chou [:ericchou] [:echou]
no flags Details | Diff | Splinter Review
Patch 1: Use DOMRequest for async event firing (11.66 KB, patch)
2012-03-26 19:21 PDT, Eric Chou [:ericchou] [:echou]
bent.mozilla: review+
Details | Diff | Splinter Review
Patch 1: Use DOMRequest for async event firing (11.67 KB, patch)
2012-03-26 22:03 PDT, Eric Chou [:ericchou] [:echou]
no flags Details | Diff | Splinter Review

Description Kyle Machulis [:kmachulis] [:qdot] 2012-02-28 12:04:04 PST
Right now we are (or will be) just printing to stderr when bluetooth firmware loading fails. We should change this to a DOMRequest API in order to deal with asynchronous errors correctly.
Comment 1 Eric Chou [:ericchou] [:echou] 2012-03-13 01:21:53 PDT
Created attachment 605312 [details] [diff] [review]
WIP

This patch works, however it's not a good implementation in BluetoothAdapter.Enable() and BluetoothAdapter.Disable(). 
Function ToggleBluetoothAsync() should be called only if mPower 
has been changed, but it would let DOMRequest object be returned 
before event onsuccess & onerror are interpreted. Still try to 
deal with it.
Comment 2 Eric Chou [:ericchou] [:echou] 2012-03-14 00:38:11 PDT
Created attachment 605671 [details] [diff] [review]
Patch 1: Use DOMRequest for async event firing
Comment 3 Kyle Machulis [:kmachulis] [:qdot] 2012-03-14 16:15:59 PDT
Actually, I'm gonna deflect the feedback to bent, as while both him and sicking just explained the solution to this to me, and it totally makes sense (and now parts of javascript make WAY more sense too!), I have no idea how to state it succinctly or in an intelligible manner. :)
Comment 4 Kyle Machulis [:kmachulis] [:qdot] 2012-03-14 20:35:51 PDT
Ok, well, there's a good chance Ben won't look at this 'til tomorrow, so in order to give you some idea of which way to go here without losing a day of work...

Javascript has a "run-to-completion" guarentee, meaning that, unlike threads, a function will finish before returning to the event loop.

http://stackoverflow.com/questions/1573118/how-do-javascripts-multiple-execution-contexts-work

So, as long as we fire the onSuccess event asynchronously and then set up the handlers, we'll be ok. This means that, when power is enabled when it's already enabled, we should send an onSuccess event using DispatchToCurrentThread, since we won't have spawned off the firmware handling thread yet. That way, the event will fire after we leave the function, but we'll have already set up the handlers by then so we should be ok.

Hope that makes sense. :)
Comment 5 Eric Chou [:ericchou] [:echou] 2012-03-14 23:39:43 PDT
Created attachment 606126 [details] [diff] [review]
Patch 1: Use DOMRequest for async event firing

Updated.

Use DispatchToCurrentThread() to create another task for event firing. That will ensure event firing will be executed after leaving the caller function.

On the other hand, we can implement a function isEnabled() in BluetoothAdapter to get mPower, then call mAdapter->isEnabled() in ToggleBtTask::Run to compare with mOnOff, and just do ToggleBtResultTask without calling bt_enable() or bt_disalbe() if mOnOff == mAdapter->isEnabled(). I think that also works.
Comment 6 Kyle Machulis [:kmachulis] [:qdot] 2012-03-15 12:50:20 PDT
Comment on attachment 606126 [details] [diff] [review]
Patch 1: Use DOMRequest for async event firing

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

This looks good. In terms of the isEnabled() call, that idea sounds correct, but do we want that to use mPower or bt_is_enabled() from bluedroid to compare against?
Comment 7 Ben Turner (not reading bugmail, use the needinfo flag!) 2012-03-15 13:13:25 PDT
Comment on attachment 606126 [details] [diff] [review]
Patch 1: Use DOMRequest for async event firing

A couple things need fixing here, but the general approach seems ok.

1. The DOMRequestService is a *service* (i.e. there should only ever be one instance). You're making new instances every time you fire an event which is bound to cause problems. Use the do_GetService helper to get the single instance. (I'll yell at sicking in a sec about not making his constructors private!)

2. You're setting mEnabled to whatever the user passes in regardless of whether or not you actually have to go do something on the other thread. What happens if you do this:

  // Assume bluetooth is not yet enabled here.
  var req1 = navigator.mozBluetooth.setEnabled(true);
  req1.onsuccess = function(event) {
    alert("first request succeeded");
  };
  var req2 = navigator.mozBluetooth.setEnabled(true);
  req2.onsuccess = function(event) {
    alert("second request succeeded");
  };

Let's say the other thread takes a long time to load firmware, etc. In that case you're going to get "second request succeeded" *before* you get "first request succeeded". You'll need to queue the second request here.

3. And what happens if the first request failed? You'll need to try again, right? You can't just fire a success event.
Comment 8 Eric Chou [:ericchou] [:echou] 2012-03-16 01:12:22 PDT
Created attachment 606494 [details] [diff] [review]
Patch 1: Use DOMRequest for async event firing

1. Fix problems pointed by Ben.
2. I change the original mechanism. Now it compares the value of BluedroidFunctions.isEnabled() with mEnabled to decide if bt_enabled() & bt_disabled() are nessacery. The main reason I made that change is because of the 2nd point mentioned by Ben. In order to handle the request in order, dispatch them to the same thread is easier to implement.
3. Add isEnabled() function to .idl. This function returns mPowered directly, it's a synchronous function call. In this case, I postpone the timing of assignment of mPowered to ensure its value represents real-time bluetooth status correctly.
Comment 9 Kyle Machulis [:kmachulis] [:qdot] 2012-03-19 19:35:15 PDT
Comment on attachment 606494 [details] [diff] [review]
Patch 1: Use DOMRequest for async event firing

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

This all looks decent

::: dom/bluetooth/BluetoothAdapter.cpp
@@ +216,2 @@
>  {
> +  *aEnabled = mPowered;

Do we want to change mPowered to be mEnabled too? The reason I originally named it mPowered is because there's actually a dbus property named "powered" on the adapter object, but I'm honestly not sure that'll do us much good here.
Comment 10 Eric Chou [:ericchou] [:echou] 2012-03-19 20:08:37 PDT
Absolutely. I'm going to modify it now and then send a review request to Ben.
Comment 11 Eric Chou [:ericchou] [:echou] 2012-03-19 21:06:19 PDT
Created attachment 607439 [details] [diff] [review]
Patch 1: Use DOMRequest for async event firing
Comment 12 Ben Turner (not reading bugmail, use the needinfo flag!) 2012-03-21 09:47:34 PDT
Comment on attachment 607439 [details] [diff] [review]
Patch 1: Use DOMRequest for async event firing

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

::: dom/base/Navigator.cpp
@@ +1155,3 @@
>  
>      mBluetooth = new bluetooth::BluetoothAdapter();
> +    mBluetooth->Init(window);

Since the Init method isn't doing anything here you might as well pass 'window' to the constructor. Having an Init method is really only useful when it could potentially fail.

::: dom/base/Navigator.h
@@ +93,5 @@
>  class PowerManager;
>  } // namespace power
>  
> +namespace bluetooth {
> +  class BluetoothAdapter;

I think you can revert this once you remove the Init method.

@@ +162,5 @@
>    nsCOMPtr<nsIDOMTelephony> mTelephony;
>  #endif
>    nsRefPtr<network::Connection> mConnection;
>  #ifdef MOZ_B2G_BT
> +  nsRefPtr<bluetooth::BluetoothAdapter> mBluetooth;

I think you can revert this once you remove the Init method.

::: dom/bluetooth/BluetoothAdapter.cpp
@@ +3,5 @@
>  /* This Source Code Form is subject to the terms of the Mozilla Public
>   * License, v. 2.0. If a copy of the MPL was not distributed with this file,
>   * You can obtain one at http://mozilla.org/MPL/2.0/. */
>  
> +#include "DOMRequest.h"

You shouldn't need this, right?

@@ +11,5 @@
>  #include "nsThreadUtils.h"
>  #include "nsXPCOMCIDInternal.h"
>  #include "mozilla/LazyIdleThread.h"
>  #include <dlfcn.h>
>  #include "BluetoothAdapter.h"

Hm, please always make sure that your header file (BluetoothAdapter.h) is the first one included so that you know your header can be included elsewhere.

@@ +129,5 @@
> +      }
> +
> +      // Update bt power status to BluetoothAdapter so that JS can get the right value.
> +      if (result) {
> +        mAdapterPtr->SetEnabled(mEnabled);

This won't work, you're racing with the main thread here. The BluetoothAdapter::mEnabled should only be set and read on the main thread.

@@ +204,5 @@
> +
> +  if (aResult) {
> +    rs->FireSuccess(mDomRequest, JSVAL_VOID);
> +  } else {
> +    rs->FireError(mDomRequest, NS_LITERAL_STRING("Bluetooth firmware loading fails"));

Nit: s/fails/failed/

@@ +220,5 @@
> +
> +NS_IMETHODIMP
> +BluetoothAdapter::SetEnabled(bool aEnabled, nsIDOMDOMRequest** aDomRequest)
> +{
> +  mDomRequest = new DOMRequest(GetOwner());

This won't work.

Remember my previous example, calling 'setEnabled' twice in a row? In that case mDomRequest will get overwritten and you're going to call success/error on the wrong object.

You'll need to pass the DomRequest to the ToggleBtTask runnable and make sure that when it finishes it calls success/error on the right request.

::: dom/bluetooth/nsIDOMBluetoothAdapter.idl
@@ +13,4 @@
>  interface nsIDOMBluetoothAdapter : nsIDOMEventTarget
>  {
> +  nsIDOMDOMRequest setEnabled(in boolean enabled);
> +  bool isEnabled();

Nit: make this 'readonly attribute boolean enabled', rather than a function.
Comment 13 Eric Chou [:ericchou] [:echou] 2012-03-22 00:02:49 PDT
> ::: dom/bluetooth/BluetoothAdapter.cpp
> @@ +3,5 @@
> >  /* This Source Code Form is subject to the terms of the Mozilla Public
> >   * License, v. 2.0. If a copy of the MPL was not distributed with this file,
> >   * You can obtain one at http://mozilla.org/MPL/2.0/. */
> >  
> > +#include "DOMRequest.h"
> 
> You shouldn't need this, right?

I need it is only because I need DOMREQUEST_SERVICE_CONTRACTID to 
call do_GetService(DOMREQUEST_SERVICE_CONTRACTID). 

Should I use "@mozilla.org/dom/dom-request-service;1" directly?

> @@ +129,5 @@
> > +      }
> > +
> > +      // Update bt power status to BluetoothAdapter so that JS can get the right value.
> > +      if (result) {
> > +        mAdapterPtr->SetEnabled(mEnabled);
> 
> This won't work, you're racing with the main thread here. The
> BluetoothAdapter::mEnabled should only be set and read on the main thread
> 
> @@ +220,5 @@
> > +
> > +NS_IMETHODIMP
> > +BluetoothAdapter::SetEnabled(bool aEnabled, nsIDOMDOMRequest** aDomRequest)
> > +{
> > +  mDomRequest = new DOMRequest(GetOwner());
> 
> This won't work.
> 
> Remember my previous example, calling 'setEnabled' twice in a row? In that
> case mDomRequest will get overwritten and you're going to call success/error
> on the wrong object.
> 
> You'll need to pass the DomRequest to the ToggleBtTask runnable and make
> sure that when it finishes it calls success/error on the right request.
> 

Yes, mDomRequest will get overwritten in this case, I'll fix it.

However, I don't know if it'll run into a race condition. 
The code related to creating thread is listed as below:

nsresult
BluetoothAdapter::ToggleBluetoothAsync(bool enabled, nsIDOMDOMRequest* req)
{
  if (!mToggleBtThread) {
    mToggleBtThread = new LazyIdleThread(15000);
  }

  nsCOMPtr<nsIRunnable> r = new ToggleBtTask(enabled, req, this);

  return mToggleBtThread->Dispatch(r, NS_DISPATCH_NORMAL);
}

A new thread would be created only if mToggleBtThread is null, 
so if ToggleBluetoothAsync has been called twice, the tasks 
would be dispatched to the same thread and would be executed 
in order. Does that make sense?
Comment 14 Eric Chou [:ericchou] [:echou] 2012-03-22 00:19:01 PDT
Created attachment 608246 [details] [diff] [review]
Patch 1: Use DOMRequest for async event firing

Updated. Fixed the mDomRequest overridden problem by sending it as a parameter to the runnable object. Also fixed nits mentioned by Ben.
Comment 15 Ben Turner (not reading bugmail, use the needinfo flag!) 2012-03-22 21:41:02 PDT
Comment on attachment 608246 [details] [diff] [review]
Patch 1: Use DOMRequest for async event firing

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

Almost done! Looking great.

::: dom/bluetooth/BluetoothAdapter.cpp
@@ +129,5 @@
>        }
>  
>        bool result;
>  
> +      // return 1 if it's enabled, 0 if it's disabled, 

Nit: You have a 'isEnabled < 0' check below, mention that here too?

@@ +187,4 @@
>  }
>  
> +nsresult
> +BluetoothAdapter::ToggleBluetoothAsync(bool enabled, nsIDOMDOMRequest* req)

This method doesn't really need to exist, right? There's only one caller. Just move this code into SetEnabled.

@@ +198,5 @@
> +  return mToggleBtThread->Dispatch(r, NS_DISPATCH_NORMAL);
> +}
> +
> +nsresult
> +BluetoothAdapter::FireEnabled(bool aResult, nsCOMPtr<nsIDOMDOMRequest> aDomRequest)

Hm, you don't want to copy-construct an nsCOMPtr for the arg here. Either you want to pass a nsCOMPtr reference (&) or just a raw nsIDOMDOMRequest*. I would vote for the latter since you just want a request and you don't need to care what it's stored in.

Also, this doesn't use any BluetoothAdapter members any more so I'd make it a file-level static function.

@@ +226,5 @@
> +    NS_ERROR("No DOMRequest Service!");
> +    return NS_ERROR_FAILURE;
> +  }
> +
> +  rs->CreateRequest(GetOwner(), aDomRequest);

This can fail, you need to check and return whatever nsresult if it does.

Also, please use a nsCOMPtr for this, then call .forget() right before returning:

  nsCOMPtr<nsIDOMDOMRequest> request;
  nsresult rv = rs->CreateRequest(GetOwner(), getter_AddRefs(request));
  NS_ENSURE_SUCCESS(rv, rv);
  ...
  request.forget(aDomRequest);
  return NS_OK;

@@ +232,5 @@
>  #ifdef MOZ_WIDGET_GONK
>    // Platform specific check for gonk until object is divided in
>    // different implementations per platform. Linux doesn't require
>    // bluetooth firmware loading, but code should work otherwise.
>    if(!EnsureBluetoothInit()) {

Wait... Do you need this here on the main thread? I think you should just let the ToggleBtTask do this.

@@ +239,5 @@
>    }
> +
> +  return ToggleBluetoothAsync(aEnabled, *aDomRequest);
> +#else
> +  return NS_OK;

This is a little weird... You're returning a request, but you'll never fire any events at it. Seems like we should do something better in the future, maybe just add a NS_NOTYETIMPLEMENTED there?

@@ +252,4 @@
>  }
>  
> +void
> +BluetoothAdapter::SetEnabled(bool aEnabled)

This method can be inlined in the header.

Also, even though the compiler knows which method you're calling, I'd rename it to 'SetEnabledInternal' so that someone reading the code knows that it's got nothing to do with the other SetEnabled.
Comment 16 Eric Chou [:ericchou] [:echou] 2012-03-25 21:03:00 PDT
Created attachment 609200 [details] [diff] [review]
Patch 1: Use DOMRequest for async event firing

Updated.
Comment 17 Eric Chou [:ericchou] [:echou] 2012-03-25 21:08:14 PDT
(In reply to ben turner [:bent] from comment #15)

> @@ +239,5 @@
> >    }
> > +
> > +  return ToggleBluetoothAsync(aEnabled, *aDomRequest);
> > +#else
> > +  return NS_OK;
> 
> This is a little weird... You're returning a request, but you'll never fire
> any events at it. Seems like we should do something better in the future,
> maybe just add a NS_NOTYETIMPLEMENTED there?

I moved the '#ifdef MOZ_WIDGET_GONK' check to ToggleBtTask because currently
it's the only place requiring bluetooth firmware loading.
Comment 18 Ben Turner (not reading bugmail, use the needinfo flag!) 2012-03-26 15:35:44 PDT
Comment on attachment 609200 [details] [diff] [review]
Patch 1: Use DOMRequest for async event firing

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

With all this fixed I think you should be done! Would still like to see the next patch though.

::: dom/bluetooth/BluetoothAdapter.cpp
@@ +13,5 @@
>  #include "mozilla/LazyIdleThread.h"
>  #include <dlfcn.h>
> +
> +static nsresult
> +FireEnabled(bool aResult, nsIDOMDOMRequest* aDomRequest)

You're not actually using this result here, so let's make this a void func.

@@ +18,5 @@
> +{
> +  nsCOMPtr<nsIDOMRequestService> rs = do_GetService("@mozilla.org/dom/dom-request-service;1");
> +
> +  if (!rs) {
> +    NS_ERROR("No DOMRequest Service!");

NS_WARNING instead.

@@ +25,5 @@
> +
> +  if (aResult) {
> +    rs->FireSuccess(aDomRequest, JSVAL_VOID);
> +  } else {
> +    rs->FireError(aDomRequest, NS_LITERAL_STRING("Bluetooth firmware loading failed"));

Both of these calls can fail, you should warn if they do:

  DebugOnly<nsresult> rv =
    aResult ?
    rs->FireSuccess(...) :
    rs->FireError(...);

  NS_WARN_IF_FALSE(NS_SUCCEEDED(rv), "...");

@@ +167,3 @@
>        }
> +#else
> +      result = true;

This is fine, but please add NS_WARNING here so that devs know this code isn't actually doing anything.

@@ +217,2 @@
>  {
> +  nsCOMPtr<nsIDOMDOMRequest> request;

Nit: Move this below to right before you use it.

@@ +236,2 @@
>  
>    return mToggleBtThread->Dispatch(r, NS_DISPATCH_NORMAL);

This isn't quite right, you'll leak if Dispatch fails. You want:

  r = new ToggleBtTask(...);

  rv = mToggleBtThread->Dispatch(...);
  NS_ENSURE_SUCCESS(rv, rv);

  request.forget(aDomRequest);
  return NS_OK;
Comment 19 Eric Chou [:ericchou] [:echou] 2012-03-26 19:21:49 PDT
Created attachment 609582 [details] [diff] [review]
Patch 1: Use DOMRequest for async event firing

Updated. Fixed bugs listed above by Ben.
Comment 20 Ben Turner (not reading bugmail, use the needinfo flag!) 2012-03-26 21:03:11 PDT
Comment on attachment 609582 [details] [diff] [review]
Patch 1: Use DOMRequest for async event firing

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

Looks great, thanks for sticking with this! One last little thing:

::: dom/bluetooth/BluetoothAdapter.cpp
@@ +167,4 @@
>        }
> +#else
> +      result = true;
> +      NS_WARNING("Not really toggle Bluetooth, just return true for event firing.");

Nit: Let's make this "No bluetooth support in this build configuration, faking a success event instead"
Comment 21 Eric Chou [:ericchou] [:echou] 2012-03-26 22:03:25 PDT
Created attachment 609611 [details] [diff] [review]
Patch 1: Use DOMRequest for async event firing
Comment 22 Ryan VanderMeulen [:RyanVM] 2012-03-27 16:19:52 PDT
https://hg.mozilla.org/integration/mozilla-inbound/rev/722d3bd080c2

To make life easier for those checking in patches for you, please follow the guidelines below for any future patches you submit. Thanks!
https://developer.mozilla.org/en/Mercurial_FAQ#How_can_I_generate_a_patch_for_somebody_else_to_check-in_for_me.3F
Comment 23 Ed Morley [:emorley] 2012-03-28 14:02:15 PDT
https://hg.mozilla.org/mozilla-central/rev/722d3bd080c2

Note You need to log in before you can comment on or make changes to this bug.