Closed Bug 1128386 Opened 5 years ago Closed 5 years ago

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

Categories

(Firefox OS Graveyard :: Bluetooth, defect)

ARM
Gonk (Firefox OS)
defect
Not set

Tracking

(blocking-b2g:2.2+, firefox37 wontfix, firefox38 wontfix, firefox39 fixed, b2g-v2.2 fixed, b2g-master fixed)

RESOLVED FIXED
2.2 S8 (20mar)
blocking-b2g 2.2+
Tracking Status
firefox37 --- wontfix
firefox38 --- wontfix
firefox39 --- fixed
b2g-v2.2 --- fixed
b2g-master --- fixed

People

(Reporter: brsun, Assigned: brsun)

References

Details

Attachments

(1 file, 5 obsolete files)

A follow-up bug from [1].

[1] https://bugzilla.mozilla.org/show_bug.cgi?id=1120889#c7
See Also: → 1132330
Attachment #8573088 - Flags: review?(shuang)
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.
Attachment #8573088 - Flags: review?(shuang)
Attachment #8573088 - Attachment is obsolete: true
Attachment #8573161 - Flags: review?(shuang)
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.
Attachment #8573161 - Flags: feedback-
Attachment #8573161 - Flags: feedback?(tzimmermann) → feedback-
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.
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.
Attachment #8573161 - Attachment is obsolete: true
Attachment #8573740 - Flags: feedback?(tzimmermann)
(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
Add more comments in codes.
Attachment #8573740 - Attachment is obsolete: true
Attachment #8573740 - Flags: feedback?(tzimmermann)
Attachment #8573751 - Flags: feedback?(tzimmermann)
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".
Attachment #8573751 - Flags: feedback?(tzimmermann) → feedback+
Use "Internal failure" as the reply message.
Attachment #8573751 - Attachment is obsolete: true
Attachment #8574537 - Flags: review?(tzimmermann)
Comment on attachment 8574537 [details] [diff] [review]
bug1128386_bond_state_changed_fail.v4.patch

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

Thanks!
Attachment #8574537 - Flags: review?(tzimmermann) → review+
See Also: → 1122177
blocking-b2g: --- → 2.2?
This bug can solve the UI issue mentioned on bug 1122177 comment 18 and bug 1122177 comment 40.
Keywords: checkin-needed
blocking-b2g: 2.2? → 2.2+
https://hg.mozilla.org/mozilla-central/rev/2ff302e89019
Status: NEW → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → 2.2 S8 (20mar)
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
Attachment #8575009 - Flags: approval-mozilla-b2g37?
Attachment #8575009 - Flags: approval-mozilla-b2g37? → approval-mozilla-b2g37+
You need to log in before you can comment on or make changes to this bug.