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

RESOLVED FIXED in Firefox 39, Firefox OS v2.2

Status

Firefox OS
Bluetooth
RESOLVED FIXED
4 years ago
3 years ago

People

(Reporter: brsun, Assigned: brsun)

Tracking

unspecified
2.2 S8 (20mar)
ARM
Gonk (Firefox OS)

Firefox Tracking Flags

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

Details

Attachments

(1 attachment, 5 obsolete attachments)

(Assignee)

Description

4 years ago
A follow-up bug from [1].

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

Updated

3 years ago
See Also: → bug 1132330
(Assignee)

Comment 1

3 years ago
Created attachment 8573088 [details] [diff] [review]
bug1128386_bond_state_changed_fail.patch
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)
(Assignee)

Comment 3

3 years ago
Created attachment 8573161 [details] [diff] [review]
bug1128386_bond_state_changed_fail.v2.patch
Attachment #8573088 - Attachment is obsolete: true
Attachment #8573161 - Flags: review?(shuang)
Attachment #8573161 - Flags: feedback?(tzimmermann)
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-
Attachment #8573161 - Flags: review?(shuang) → review-
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.
(Assignee)

Comment 6

3 years ago
Created attachment 8573740 [details] [diff] [review]
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.
Attachment #8573161 - Attachment is obsolete: true
Attachment #8573740 - Flags: feedback?(tzimmermann)
(Assignee)

Comment 7

3 years ago
(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
(Assignee)

Comment 8

3 years ago
Created attachment 8573751 [details] [diff] [review]
bug1128386_bond_state_changed_fail.v3.patch

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+
(Assignee)

Comment 10

3 years ago
Created attachment 8574537 [details] [diff] [review]
bug1128386_bond_state_changed_fail.v4.patch

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+
(Assignee)

Updated

3 years ago
See Also: → bug 1122177
(Assignee)

Updated

3 years ago
blocking-b2g: --- → 2.2?
(Assignee)

Comment 12

3 years ago
This bug can solve the UI issue mentioned on bug 1122177 comment 18 and bug 1122177 comment 40.
(Assignee)

Comment 13

3 years ago
Created attachment 8575009 [details] [diff] [review]
bug1128386_bond_state_changed_fail.commit.patch

Try results seem good: https://treeherder.mozilla.org/#/jobs?repo=try&revision=d78a1c3d5a07
Attachment #8574537 - Attachment is obsolete: true
Attachment #8575009 - Flags: review+
(Assignee)

Updated

3 years ago
Keywords: checkin-needed
https://hg.mozilla.org/integration/b2g-inbound/rev/2ff302e89019
Assignee: nobody → brsun
Keywords: checkin-needed

Updated

3 years ago
blocking-b2g: 2.2? → 2.2+
https://hg.mozilla.org/mozilla-central/rev/2ff302e89019
Status: NEW → RESOLVED
Last Resolved: 3 years ago
status-firefox39: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → 2.2 S8 (20mar)
(Assignee)

Comment 16

3 years ago
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?

Updated

3 years ago
Attachment #8575009 - Flags: approval-mozilla-b2g37? → approval-mozilla-b2g37+
https://hg.mozilla.org/releases/mozilla-b2g37_v2_2/rev/535d2726240f
status-b2g-v2.2: --- → fixed
status-b2g-master: --- → fixed
status-firefox37: --- → wontfix
status-firefox38: --- → wontfix
You need to log in before you can comment on or make changes to this bug.