997626 - [Tarako][bluez] Handle RequestPairingConsent event and reply pairing confirmation directly to resolve pairing failure case

Status: RESOLVED FIXED

(Firefox OS Graveyard :: Bluetooth, defect)

Description

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

A follow-up for bug 938003.
Pairing confirmation shall be replied.

Comment 1

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

Attachment: Bug 994411 - [bluez] Ignore pending MO call index, while making MO call with bluetooth headset

Comment 2

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

Attachment: Bug 997626 - [bluez] Handle RequestPairingConsent event and reply pairing confirmation directly to resolve pairing failure

Comment 3

[Xinhe Yan]

The patch can fix compatibility of Nokia BH105. We need merge patch to 1.3t branch.

Hi Shawn,
you need give a patch for 1.3t.

Comment 4

[Xinhe Yan]

We need 1.3t+ to fix compatibility of Nokia BH105.

Comment 5

[Ben Tian (inactive)]

Comment on attachment 8408934 [details] [diff] [review]
Bug 997626 - [bluez] Handle RequestPairingConsent event and  reply pairing confirmation directly to resolve pairing failure

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

f=me with following comment addressed.

::: dom/bluetooth/bluez/BluetoothDBusService.cpp
@@ +1186,5 @@
> +
> +    if (!sPairingReqTable->Get(mDeviceAddress, &msg) && mRunnable) {
> +      BT_WARNING("%s: Couldn't get original request message.", __FUNCTION__);
> +      errorStr.AssignLiteral("Couldn't get original request message.");
> +      if (mRunnable) {

This |if| is redundant since outer |if| ensures mRunnable is non-empty.

@@ +1219,5 @@
> +
> +    dbus_message_unref(msg);
> +    dbus_message_unref(reply);
> +    sPairingReqTable->Remove(mDeviceAddress);
> +    if (!mRunnable) {

nit: rewrite as following to be consistent with prior condition.

if (mRunnable) {
  DispatchBluetoothReply(mRunnable, v, errorStr);
}

@@ +1404,2 @@
>  
>      // Do not send an notification to upper layer, too annoying.

nit: 'a' notification

@@ +1408,5 @@
> +    // Directly SetPairingconfirmation for RequestPairingConsent here
> +    if (!dbus_message_get_args(msg, nullptr,
> +                               DBUS_TYPE_OBJECT_PATH, &objectPath,
> +                               DBUS_TYPE_INVALID)) {
> +      errorStr.AssignLiteral("Invalid arguments: RequestConfirmation()");

Should the error string still be "RequestConfirmation()"?

@@ +1416,5 @@
> +    nsString address = GetAddressFromObjectPath(NS_ConvertUTF8toUTF16(objectPath));
> +    sPairingReqTable->Put(address, msg);
> +    Task* task = new SetPairingConfirmationTask(address, true, nullptr);
> +    DispatchToDBusThread(task);
> +    // Increases dbus message reference count, it will be decreased in

nit: 'Increase' dbug message

@@ +1419,5 @@
> +    DispatchToDBusThread(task);
> +    // Increases dbus message reference count, it will be decreased in
> +    // SetPairingConfirmationTask
> +    dbus_message_ref(msg);
> +    // Do not send an notification to upper layer

nit: 'a' notification

Comment 6

[James Zhang (Spreadtrum)]

When can you land this patch?

Comment 7

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

(In reply to James Zhang from comment #6)
> When can you land this patch?

The patch can't land unless it passes review process. I'll review it today.

Comment 8

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

(In reply to James Zhang from comment #6)
> When can you land this patch?

https://bugzilla.mozilla.org/show_bug.cgi?id=938003#c39

Comment 9

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

Attachment: Bug 997626 - [bluez] Handle RequestPairingConsent event and reply pairing confirmation directly to resolve pairing failure (master)

Comment 10

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

Comment on attachment 8409528 [details] [diff] [review]
Bug 997626 - [bluez] Handle RequestPairingConsent event and  reply pairing confirmation directly to resolve pairing failure (master)

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

Actually you can just reply to BlueZ once you get "RequestPairingConsent" instead of dispatching the job to SetPairingConfirmationTask, but yeah, the patch should work as well.

Comment 11

[James Zhang (Spreadtrum)]

1.3t+ and land it, please.

Comment 12

[Joe Cheng [:jcheng] (please needinfo)]

triage: partner blocker, 1.3T+

Comment 13

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

Attachment: Bug 997626 - [bluez] Handle RequestPairingConsent event and reply pairing confirmation directly (v1.3t)

Comment 14

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

Attachment: Bug 997626 - [bluez] Handle RequestPairingConsent event and reply pairing confirmation directly, r=echou, f=btian, a=1.3t+ (master)

Comment 15

[Ben Tian (inactive)]

Comment on attachment 8409638 [details] [diff] [review]
Bug 997626 - [bluez] Handle RequestPairingConsent event and reply pairing confirmation directly (v1.3t)

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

f=me with following comment. Thanks.

::: dom/bluetooth/bluez/linux/BluetoothDBusService.cpp
@@ +1159,5 @@
>  
> +    // Do not send a notification to upper layer, too annoying.
> +    return DBUS_HANDLER_RESULT_HANDLED;
> +  } else if (dbus_message_is_method_call(msg, DBUS_AGENT_IFACE, "RequestPairingConsent")) {
> +    // Directly SetPairingconfirmation for RequestPairingConsent here

The comment should be removed or revised since SetPairingConfirmation is not called.

Comment 16

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

Attachment: Bug 997626 - [bluez] Handle RequestPairingConsent event and reply pairing confirmation directly (v1.3t), f=btian, a=1.3t+

Comment 17

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

Attachment: Bug 997626 - [bluez] Handle RequestPairingConsent event and reply pairing confirmation directly, r=echou, f=btian, a=1.3t+ (v1.3t)

Comment 18

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/b2g-inbound/rev/8da49c28d370

Comment 19

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/8da49c28d370

Comment 20

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

Comment on attachment 8410056 [details] [diff] [review]
Bug 997626 - [bluez] Handle RequestPairingConsent event and reply pairing confirmation directly, r=echou, f=btian, a=1.3t+  (v1.3t)

NOTE: This flag is now for security issues only. Please see https://wiki.mozilla.org/Release_Management/B2G_Landing to better understand the B2G approval process and landings.

[Approval Request Comment]
Bug caused by (feature/regressing bug #): 
  None
User impact if declined:
  The Bluetooth headset got paired when:
1. Local IO capabilities are DisplayYesNo and remote IO capabiltiies are DisplayOnly or NoInputNoOutput.
2. Call PairingConsent callback for "incoming" request.
This patch is try to directly reply pairing confirmation for consent cases.
Testing completed: 
  1. Initialize pairing request with IO capabilities-NoInputNoOutput as
headset role to b2g phone
2. Check pairing status of bluetooth headset
Risk to taking this patch (and alternatives if risky): 
  None
String or UUID changes made by this patch:
  None

Comment 21

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

Hi Fabrice, could you help this uplift for 1.3t?

Comment 22

[[:fabrice] Fabrice Desré]

 https://hg.mozilla.org/releases/mozilla-b2g28_v1_3t/rev/884697a7baa1

Comment 23

[Jan Jongboom [:janjongboom] (inactive)]

Comment on attachment 8410056 [details] [diff] [review]
Bug 997626 - [bluez] Handle RequestPairingConsent event and reply pairing confirmation directly, r=echou, f=btian, a=1.3t+  (v1.3t)

Please see https://bugzilla.mozilla.org/show_bug.cgi?id=997626#c20 for the approval request form.

We'll need this uplifted if possible for our Dolphin 1.4 release.

Comment 24

[Preeti Raghunath(:Preeti)]

Comment on attachment 8410056 [details] [diff] [review]
Bug 997626 - [bluez] Handle RequestPairingConsent event and reply pairing confirmation directly, r=echou, f=btian, a=1.3t+  (v1.3t)

will need to land in Dolphin branch

Comment 25

[Wayne Chang [:wchang]]

Hi Shawn,

Can you request for approval to land this to 1.4 again if the patch is ready?
We're now going to land directly to 1.4 instead of a dolphin branch.

Thanks!

Comment 26

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

Wayne,
Dolphin actually change bluetooth backend from bluez to bluedroid. The original patch fixed for bluez. Even if we re-base the original patch for v1.4, still need to make another patch for bluedroid. I kept ni flag and will update later after patch for bluedroid completed.

Comment 27

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

My bad, after testing it looks like the consent pairing can work on bluedroid backend based.

Comment 28

[Ivan Tsay (:ITsay)]

(In reply to Shawn Huang [:shuang] [:shawnjohnjr] from comment #27)
> My bad, after testing it looks like the consent pairing can work on
> bluedroid backend based.

Hi Shawn,

So does this mean the bug does not affect v1.4? If so, we should change status-b2g-v1.4 to be unaffected.

Comment 29

[Ivan Tsay (:ITsay)]

Thanks Shawn for the update. Dolphin is using Bluedroid so that the bug does not affect Dolphin v1.4. Given that 1.4 is dolphin branch now, there is no need to uplift this to v1.4.