1128386 - [Bluetooth] Handle |STATUS_FAIL| in |BluetoothServiceBluedroid::BondStateChangedNotification()|.

Status: RESOLVED FIXED

(Firefox OS Graveyard :: Bluetooth, defect)

Description

[Bruce Sun [:brsun]]

A follow-up bug from [1].

[1] https://bugzilla.mozilla.org/show_bug.cgi?id=1120889#c7

Comment 1

[Bruce Sun [:brsun]]

Attachment: bug1128386_bond_state_changed_fail.patch

Comment 2

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

Comment on attachment 8573088 [details] [diff] [review]
bug1128386_bond_state_changed_fail.patch

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

Please add some comments to explain the reason.

Comment 3

[Bruce Sun [:brsun]]

Attachment: bug1128386_bond_state_changed_fail.v2.patch

Comment 4

[Thomas Zimmermann [:tzimmermann] [:tdz]]

Comment on attachment 8573161 [details] [diff] [review]
bug1128386_bond_state_changed_fail.v2.patch

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

::: dom/bluetooth/BluetoothInterfaceHelpers.cpp
@@ +27,5 @@
>    return NS_OK;
>  }
>  
> +nsresult
> +Convert(BluetoothStatus& aIn, nsAString& aOut)

This is for interface helpers only. Can we please put the conversion function into BluetoothServiceBluedroid. Also, the implementation should be done with a simple lookup table, instead of the if-else branching.

Comment 5

[Thomas Zimmermann [:tzimmermann] [:tdz]]

Comment on attachment 8573161 [details] [diff] [review]
bug1128386_bond_state_changed_fail.v2.patch

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

Hi Bruce,

Just another remark. I don't know much about the logic in |BluetoothServiceBluedroid::BondStateChangedNotification| and how it related to the DOM API. The current code returns "Authentication failure", which is (probably) what BlueZ originally returned. I'm all for cleaning up a bit, but if you change the error strings, you're possibly changing the DOM API. Or maybe not, as I said, I don't know the semantics and dependencies. In any case, I suggest to check this twice against the Gaia code and the current spec.

Comment 6

[Bruce Sun [:brsun]]

Attachment: bug1128386_bond_state_changed_fail.v3.patch

Thanks for the inspiration from btian. It would be good to keep things simple because Bluetooth API v1 would be phased out in the near future.

I keep everything unchanged except for the default path.

Comment 7

[Bruce Sun [:brsun]]

(In reply to Thomas Zimmermann [:tzimmermann] [:tdz] from comment #5)
> Comment on attachment 8573161 [details] [diff] [review]
> bug1128386_bond_state_changed_fail.v2.patch
> 
> Review of attachment 8573161 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Hi Bruce,
> 
> Just another remark. I don't know much about the logic in
> |BluetoothServiceBluedroid::BondStateChangedNotification| and how it related
> to the DOM API. The current code returns "Authentication failure", which is
> (probably) what BlueZ originally returned. I'm all for cleaning up a bit,
> but if you change the error strings, you're possibly changing the DOM API.
> Or maybe not, as I said, I don't know the semantics and dependencies. In any
> case, I suggest to check this twice against the Gaia code and the current
> spec.

From MDN[1] and mozilla wiki[2][3], I cannot find the description of "Authentication failure" on existing documents. And it seems "Authentication failure" is not used in existing logics from the grep result of B2G code base, either.

After cross checking with bug 1032755, bug 1056413, and bug 1067206, which are related to this string definition, this string seems for information only, not for string comparison.

But anyway, let's keep the original BluetoothReplyRunnable unchanged.

[1] https://developer.mozilla.org/en-US/docs/Web/API/Web_Bluetooth_API
[2] https://wiki.mozilla.org/WebAPI/WebBluetooth
[3] https://wiki.mozilla.org/B2G/Bluetooth/WebBluetooth-v2

Comment 8

[Bruce Sun [:brsun]]

Attachment: bug1128386_bond_state_changed_fail.v3.patch

Add more comments in codes.

Comment 9

[Thomas Zimmermann [:tzimmermann] [:tdz]]

Comment on attachment 8573751 [details] [diff] [review]
bug1128386_bond_state_changed_fail.v3.patch

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

::: dom/bluetooth/bluedroid/BluetoothServiceBluedroid.cpp
@@ +1596,5 @@
> +      // Dispatch a reply to unblock the waiting status of pairing.
> +      if (!sBondingRunnableArray.IsEmpty()) {
> +        DispatchBluetoothReply(sBondingRunnableArray[0],
> +                               BluetoothValue(true),
> +                               NS_LITERAL_STRING("Fail"));

I guess that will do the job. :D Maybe say "Generic failure" or "Internal failure".

Comment 10

[Bruce Sun [:brsun]]

Attachment: bug1128386_bond_state_changed_fail.v4.patch

Use "Internal failure" as the reply message.

Comment 11

[Thomas Zimmermann [:tzimmermann] [:tdz]]

Comment on attachment 8574537 [details] [diff] [review]
bug1128386_bond_state_changed_fail.v4.patch

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

Thanks!

Comment 12

[Bruce Sun [:brsun]]

This bug can solve the UI issue mentioned on bug 1122177 comment 18 and bug 1122177 comment 40.

Comment 13

[Bruce Sun [:brsun]]

Attachment: bug1128386_bond_state_changed_fail.commit.patch

Try results seem good: https://treeherder.mozilla.org/#/jobs?repo=try&revision=d78a1c3d5a07

Comment 14

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/integration/b2g-inbound/rev/2ff302e89019

Comment 15

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/2ff302e89019

Comment 16

[Bruce Sun [:brsun]]

Comment on attachment 8575009 [details] [diff] [review]
bug1128386_bond_state_changed_fail.commit.patch

NOTE: 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 #): bug 1081871
User impact if declined: The user will experience endless pairing if the Bluetooth backend encounters some specific errors.
Testing completed: https://treeherder.mozilla.org/#/jobs?repo=try&revision=d78a1c3d5a07
Risk to taking this patch (and alternatives if risky): Low. This patch only completes the logic of handling unexpected errors. Original logics of handling known errors stay unchanged.
String or UUID changes made by this patch: N/A

Comment 17

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-b2g37_v2_2/rev/535d2726240f