1119734 - [Bluetooth][API v2] Cannot receive any pairing event while BT is launching via system message.

Status: RESOLVED FIXED

(Firefox OS Graveyard :: Bluetooth, defect)

Description

[[e-mail], [e-mail]]

Per comment https://bugzilla.mozilla.org/show_bug.cgi?id=1102798#c2, Gaia cannot receive any pairing event while BT is launching via system message.

For the steps of flashing WIP in detail:

1. pull https://github.com/mozilla-b2g/gaia/pull/26139
2. make reset-gaia
3. pull https://github.com/mozilla-b2g/gaia/pull/27186
4. make install-gaia APP=bluetooth

Comment 1

[[e-mail], [e-mail]]

Jamin, could you please help to investigate the problem? Thanks.

Comment 2

[Shawn Huang [:shawnjohnjr][:shuang] (as a happy gecko contributor)]

(In reply to Ian Liu [:ianliu] from comment #1)
> Jamin, could you please help to investigate the problem? Thanks.

Thanks for reporting this bug. We will take it as the first priority.

Comment 3

[Jamin Liu [:jaliu][:Jamin]]

(In reply to Ian Liu [:ianliu] from comment #1)
> Jamin, could you please help to investigate the problem? Thanks.

Hi Ian,
Thank you for your feedback.
I've reproduced the issue with your patches.
The event was missing because pairing event was fired before Gaia added event handler.

> 01-12 13:54:22.848 I/Jamin   ( 2220): BluetoothManager::ReselectDefaultAdapter: --> DispatchAttributeEvent
> 01-12 13:54:22.848 I/Jamin   ( 1431): BluetoothService::RegisterBluetoothSignalHandler: KEY_PAIRING_LISTENER
> 01-12 13:54:22.938 I/Bluetooth Manager( 2220): Content JS LOG: --> _onDefaultAdapterChanged(): newAdapter = [object BluetoothAdapter] 
> 01-12 13:54:22.948 I/Bluetooth Manager( 2220): Content JS LOG: --->>> 222222 _watchPairingRequst() 
> 01-12 13:54:22.958 I/Bluetooth Manager( 2220): Content JS LOG: ---> start watching

I'll upload a patch to fix this issue tomorrow. :)

Comment 4

[Jamin Liu [:jaliu][:Jamin]]

Attachment: Delay dispatching Bluetooth pairing event if event handlers haven't been attached. (v1)

Comment 5

[Ben Tian (inactive)]

Comment on attachment 8547968 [details] [diff] [review]
Delay dispatching Bluetooth pairing event if event handlers haven't been attached. (v1)

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

Please see my comment below. Race condition of firing order exists among incoming pairing requests.

::: dom/bluetooth2/BluetoothPairingListener.cpp
@@ +14,5 @@
>  USING_BLUETOOTH_NAMESPACE
>  
> +// The polling interval which is used to check whether all pairing event
> +// handlers are attached.
> +const unsigned int CHECK_PAIRING_HANDLER_INTERVAL_MS = 50;

- |PAIRING_HANDLER_POLLING_INTERVAL_MS| is clearer.
- Remove 'which is used to' to simplify.

@@ +62,5 @@
> +                           BluetoothDevice* aDevice,
> +                           const nsAString& aPasskey,
> +                           const nsAString& aType)
> +    : mPairingListener(aPairingListener), mDevice(aDevice), mPasskey(aPasskey),
> +      mType(aType)

nit: Break into 4 lines.

@@ +72,5 @@
> +  void Run() MOZ_OVERRIDE
> +  {
> +    MOZ_ASSERT(NS_IsMainThread());
> +
> +    if (mPairingListener->HasListenersFor(nsGkAtoms::ondisplaypasskeyreq) &&

Suggest to revise with guardian clause:

  {
    MOZ_ASSERT(NS_IsMainThread());

    // Dispatch pairing event once all pairing event handlers are attached.
    if (...) {
      mPairingListener->DispatchPairingEvent(mDevice, mPasskey, mType);
      return;
    }

    // Re-check later until all pairing event handlers are attached.
    MessageLoop::current()->PostDelayedTask(FROM_HERE,
      new DispatchPairingEventTask(mPairingListener, mDevice, mPasskey, mType),
                                   CHECK_PAIRING_HANDLER_INTERVAL_MS);
  }

@@ +148,5 @@
>      nsRefPtr<BluetoothDevice> device =
>        BluetoothDevice::Create(GetOwner(), props);
>  
>      // Notify pairing listener of pairing requests
> +    // Delay dispatching task if any pairing event handler hasn't been attached.

- Move this line into |DispatchPairingEventTask| since the delay doesn't occur here.
- The task doesn't ensure order of incoming pairing requests. A later request may be fired earlier if its task checks immediately after all event handlers are attached.

Comment 6

[Jamin Liu [:jaliu][:Jamin]]

Attachment: Delay registering Bluetooth signal handler if pairing event handlers haven't been attached. (v2)

Comment 7

[Ben Tian (inactive)]

Comment on attachment 8550044 [details] [diff] [review]
Delay registering Bluetooth signal handler if pairing event handlers haven't been attached. (v2)

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

Please see my comment below. I prefer to wrap signal handler registration into a function and call it 1) during initialization and 2) when an event handler is added.

::: dom/bluetooth2/BluetoothPairingListener.cpp
@@ +19,5 @@
>  NS_IMPL_ADDREF_INHERITED(BluetoothPairingListener, DOMEventTargetHelper)
>  NS_IMPL_RELEASE_INHERITED(BluetoothPairingListener, DOMEventTargetHelper)
>  
>  BluetoothPairingListener::BluetoothPairingListener(nsPIDOMWindow* aWindow)
> +  : DOMEventTargetHelper(aWindow), isBtSignalHandlerRegistered(false)

nit: newline here.

  : DOMEventTargetHelper(aWindow)
  , isBtSignalHandlerRegistered(false)

@@ +24,4 @@
>  {
>    MOZ_ASSERT(aWindow);
>  
> +  // Register Bluetooth signal handler only if every pairing event handler has

Wrap here as a function. Feel free to rename the function/variables for better understandability.

void
BluetoothPairingListener::TryListeningToBluetoothSignal()
{
  if (mHasListenedToSignal) {
    // We've handled prior pending pairing requests
    return;
  }

  // Listen to bluetooth signal only if all pairing event handlers have been
  // attached. All pending pairing requests queued in BluetoothService would
  // be fired when pairing listener starts listening to bluetooth signal.

  if (!HasListenersFor(nsGkAtoms::ondisplaypasskeyreq) ||
      !HasListenersFor(nsGkAtoms::onenterpincodereq) ||
      !HasListenersFor(nsGkAtoms::onpairingconfirmationreq) ||
      !HasListenersFor(nsGkAtoms::onpairingconsentreq)) {
    BT_LOGR("Pairing listener is not ready to handle pairing requests!");
    return;
  }

  // Start listening to bluetooth signal to handle pairing requests
  BluetoothService* bs = BluetoothService::Get();
  NS_ENSURE_TRUE_VOID(bs);
  bs->RegisterBluetoothSignalHandler(NS_LITERAL_STRING(KEY_PAIRING_LISTENER),
	                                   this);

  mHasListenedToSignal = true;
}

@@ +146,5 @@
> +BluetoothPairingListener::EventListenerAdded(nsIAtom* aType)
> +{
> +  DOMEventTargetHelper::EventListenerAdded(aType);
> +
> +  // If every pairing event handler is ready, register Bluetooth signal handler.

Call |TryListeningtoBluetoothSignal| here.

::: dom/bluetooth2/BluetoothPairingListener.h
@@ +48,5 @@
>  private:
>    BluetoothPairingListener(nsPIDOMWindow* aWindow);
>    ~BluetoothPairingListener();
> +
> +  bool isBtSignalHandlerRegistered;

|mHasListenedToSignal|

Comment 8

[Jamin Liu [:jaliu][:Jamin]]

Attachment: Delay registering Bluetooth signal handler if pairing event handlers haven't been attached. (v3)

- Revise patch v2 based on #comment 7
- Rebase

Comment 9

[Ben Tian (inactive)]

Comment on attachment 8551132 [details] [diff] [review]
Delay registering Bluetooth signal handler if pairing event handlers haven't been attached. (v3)

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

r=me with comment addressed. Thanks.

::: dom/bluetooth2/BluetoothPairingListener.h
@@ +49,5 @@
>    BluetoothPairingListener(nsPIDOMWindow* aWindow);
>    ~BluetoothPairingListener();
> +
> +/**
> + * Register Bluetooth signal handler if all pairing event handlers is ready.

'are' ready.

Also replace 'Register bluetooth signal handler' with 'Listen to bluetooth signal' for consistency. 'BluetoothService registers signal handler of pairing listener' is equivalent to 'BluetoothPairingListener starts listening to bluetooth signal.'

@@ +52,5 @@
> +/**
> + * Register Bluetooth signal handler if all pairing event handlers is ready.
> + *
> + * Listen to bluetooth signal only if all pairing event handlers have been
> + * attached. All pending pairing requests queued in BluetoothService would

would be fired ...

@@ +54,5 @@
> + *
> + * Listen to bluetooth signal only if all pairing event handlers have been
> + * attached. All pending pairing requests queued in BluetoothService would
> + * when pairing listener starts listening to bluetooth signal.
> + */

nit: indent this paragraph of comment.

@@ +57,5 @@
> + * when pairing listener starts listening to bluetooth signal.
> + */
> +  void TryListeningToBluetoothSignal();
> +
> +  bool mHasListenedToSignal;

Leave comment to explain meaning of this member variable.

Comment 10

[Jamin Liu [:jaliu][:Jamin]]

Attachment: Delay registering Bluetooth signal handler if pairing event handlers haven't been attached. (v4), r=btian

- Revise patch v3 based on #comment 9
- Add reviewer's name to commit message
- Convert to hg format

Comment 11

[Jamin Liu [:jaliu][:Jamin]]

https://hg.mozilla.org/integration/b2g-inbound/rev/3f106e7f5b13

Comment 12

[Jamin Liu [:jaliu][:Jamin]]

(In reply to Ben Tian [:btian] from comment #9)
> Comment on attachment 8551132 [details] [diff] [review]
> Delay registering Bluetooth signal handler if pairing event handlers haven't
> been attached. (v3)
> 
> Review of attachment 8551132 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> r=me with comment addressed. Thanks.

Thank Ben for reviewing the patch.

Comment 13

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/3f106e7f5b13