[Flame][Bluetooth]"Visible to all" button will keep on after you turn off /on bluetooth sometimes.

RESOLVED FIXED

Status

Firefox OS
Bluetooth
RESOLVED FIXED
3 years ago
3 years ago

People

(Reporter: Shine(Leave from Mozilla), Assigned: jaliu)

Tracking

(Blocks: 1 bug)

unspecified
ARM
Gonk (Firefox OS)

Firefox Tracking Flags

(blocking-b2g:-, firefox41 fixed, b2g-v2.1 affected, b2g-v2.2 affected, b2g-master verified)

Details

(Whiteboard: [caf priority: p2][CR 830995])

Attachments

(5 attachments, 2 obsolete attachments)

(Reporter)

Description

3 years ago
[1.Description]:
[Flame][v2.1&2.2][Bluetooth] "Visible to all" button will keep on after you turn off /on bluetooth sometimes
Found time:22:25
See attachment: VIDEO_2225.MP4 and llogcat_2225.txt

[2.Testing Steps]: 
1.Turn on bluetooth  via Setting->Bluetooth
2.Turn on Visible to all
3.Turn off blutooth
4.Turn on blutooth
5.Repeat step 2-4

[3.Expected Result]: 
4.DUT will turn off  Visible to all

[4.Actual Result]: 
4.DUT will turn off  then turn on visible to all automatic

[5.Reproduction build]: 
FLame2.1 build:
Gaia-Rev        4c159e75a1568afbbf0c83c1235ec56facfbe87d
Gecko-Rev       https://hg.mozilla.org/releases/mozilla-b2g34_v2_1/rev/b9849b3c6aaa
Build-ID        20141112001201
Version         34.0

FLame 2.2 buiild:
Gaia-Rev        5ae28ff11b982e2bd7d1aa097cda131536952bdc
Gecko-Rev       https://hg.mozilla.org/mozilla-central/rev/688f821edcd4
Build-ID        20141112040208
Version         36.0a1

[6.Reproduction Frequency]: 
occasionally Recurrence,2/20
(Reporter)

Comment 1

3 years ago
Created attachment 8522136 [details]
logcat
(Reporter)

Comment 2

3 years ago
Created attachment 8522139 [details]
VIDEO
Bug is simliar with bug 1069868
Assignee: nobody → jaliu
(Assignee)

Comment 4

3 years ago
I reproduced this bug on flame-kk 2.2 and I noticed the device wasn't actually 'discoverable' when the UI said it's "Visible to all".

I added a log print to https://github.com/mozilla-b2g/gaia/blob/master/apps/settings/js/bluetooth.js#L178
The property |defaultAdapter.discoverable| is false even if DUT hits this bug.

>    function initial() {
>      console.log("initial(), defaultAdapter.discoverable = " + defaultAdapter.discoverable);
>      visibleCheckBox.checked = defaultAdapter.discoverable;

I believe there is some kind of timing issue in settings/js/bluetooth.js which causes incorrect |visibleCheckBox| update status incorrectly.

Both updateDeviceInfo() and initial() would change |visibleCheckBox.checked| by calling setDiscoverable().
In general, initial() is called *after* updateDeviceInfo() and it would reset |discoverable| to false.
However, for some reason updateDeviceInfo() might be called after initial() by following STR in #Description. In such a case, |visibleCheckBox.checked| would be set to |true| by updateDeviceInfo().

Hi Ian,
I seems like it's a gaia::settings issue.
Would you mind to take a look?
Thank you.
Assignee: jaliu → nobody
Component: Bluetooth → Gaia::Settings
(Assignee)

Comment 5

3 years ago
I forgot to ni? Ian on comment 4.
Sorry for inconvenience caused.
Flags: needinfo?(iliu)
This is an old design for remaining user's pref for the visible settings. Settings::Bluetooth will reload the user's pref from 'bluetooth.visible' key in updateDeviceInfo(). And initial() will use `defaultAdapter.discoverable` to update the visible check box. Sometimes, the user's pref is not sync with the hardware side(defaultAdapter.discoverable). So that we see the check box is turning on/off while enabling Bluetooth.

Since the new spec. of Settings::Bluetooth won't remain user's visible pref, I would like to fix the problem in the next version.
Flags: needinfo?(iliu)
(Assignee)

Updated

3 years ago
See Also: → bug 1134244
Blocks: 1063044
blocking-b2g: --- → 2.2?

Updated

3 years ago
Whiteboard: [CR 833576]

Updated

3 years ago
Whiteboard: [CR 833576] → [caf priority: p2][CR 833576]
This is more easily reproducible when restricting memory via |fastboot oem mem 320m|
Whiteboard: [caf priority: p2][CR 833576] → [caf priority: p2][CR 830995]

Comment 8

3 years ago
Not blocking, user can just switch off visible to all when they found it's on after turning BT on, and the visible to all indicator is right below the BT on/off switch, no difficulties for user to notice the status.
And this is not a regression.
blocking-b2g: 2.2? → -

Comment 9

3 years ago
(In reply to howie [:howie] from comment #8)
> Not blocking, user can just switch off visible to all when they found it's
> on after turning BT on, and the visible to all indicator is right below the
> BT on/off switch, no difficulties for user to notice the status.
> And this is not a regression.

How would users know that the UI is not correctly representing the current state?  I don't think users would know that visibility is generally turned off when BT is turned off then on again.
Flags: needinfo?(hochang)
I'm not sure if this was made clear, but when this bug happens and "visible to all" is stuck in the on position, the device is actually NOT visible to all.

This is an inconsistency between what the device actually does, and what the setting displays to the user.

The device is doing the correct thing by default not being visible to all when BT is turned on, but the Settings app is displaying the wrong state for the "visible to all" setting.
Created attachment 8603181 [details] [review]
[gaia] ian-liu:bluetooth/bug1098228_discoverable_is_still_on_after_turned_bt_off/on_sometimes > mozilla-b2g:master
Comment on attachment 8603181 [details] [review]
[gaia] ian-liu:bluetooth/bug1098228_discoverable_is_still_on_after_turned_bt_off/on_sometimes > mozilla-b2g:master

This is a debugging patch for dumping following log.

I/GeckoBluetooth( 1753): AdapterStateChangedNotification: BT_STATE: 1
I/Settings( 7889): Content JS LOG: initial(): visibleCheckBox.checked = defaultAdapter.discoverable = true, then setDiscoverable 
I/Settings( 7889):     at initial (app://settings.gaiamobile.org/js/bluetooth.js:181:1)
I/Settings( 7889): Content JS LOG: --> setDiscoverable(): set visibleCheckBox.checked = true 
I/Settings( 7889):     at setDiscoverable (app://settings.gaiamobile.org/js/bluetooth.js:214:1)
I/qcom-bt-wlan-coex( 8261): /system/etc/init.qcom.coex.sh: btwlancoex/abtfilt not available 
W/bt-btif ( 8223): bta_dm_check_av:0
Comment on attachment 8603181 [details] [review]
[gaia] ian-liu:bluetooth/bug1098228_discoverable_is_still_on_after_turned_bt_off/on_sometimes > mozilla-b2g:master

This is a debugging patch for dumping following log.

I/GeckoBluetooth( 1753): AdapterStateChangedNotification: BT_STATE: 1
I/Settings( 7889): Content JS LOG: initial(): visibleCheckBox.checked = defaultAdapter.discoverable = true, then setDiscoverable 
I/Settings( 7889):     at initial (app://settings.gaiamobile.org/js/bluetooth.js:181:1)
I/Settings( 7889): Content JS LOG: --> setDiscoverable(): set visibleCheckBox.checked = true 
I/Settings( 7889):     at setDiscoverable (app://settings.gaiamobile.org/js/bluetooth.js:214:1)
I/qcom-bt-wlan-coex( 8261): /system/etc/init.qcom.coex.sh: btwlancoex/abtfilt not available 
W/bt-btif ( 8223): bta_dm_check_av:0
Jamin, 

Per the log in comment 13, I find out Gaia got |defaultAdapter.discoverable = true| while Gaia::Settings::Bluetooth panel init with adapter for Bluetooth off/on. The reproduced rate is 1/10 for me. It's a platform issue, I think. And it's probably relative with bug 1069868. Could you please help to investigate the problem? Feel free to let me know if you need any Gaia help. Thanks.
Flags: needinfo?(jaliu)

Comment 15

3 years ago
Let's clarify the issue first.

If the current issue is the same as original description: "Visible to all button will keep on after you turn off /on bluetooth sometimes", then this should not block just like comment 8 said.

If the issue is UI not correctly representing the current state like Comment 9 and 10, then this should be fixed.

Ian and Jamin will help with this.
Flags: needinfo?(hochang)
Since I dump the log for debug, the device is actually visible to all. It's sync with platform value. And a user is still able to toggle visible state if he/she wants.
(Assignee)

Comment 17

3 years ago
(In reply to Ian Liu [:ianliu] from comment #14)
> Jamin, 
> 
> Per the log in comment 13, I find out Gaia got |defaultAdapter.discoverable
> = true| while Gaia::Settings::Bluetooth panel init with adapter for
> Bluetooth off/on. The reproduced rate is 1/10 for me. It's a platform issue,
> I think. And it's probably relative with bug 1069868. Could you please help
> to investigate the problem? Feel free to let me know if you need any Gaia
> help. Thanks.

Thank you for the feedback.
I'm investigating this issue from the Gecko side.
Flags: needinfo?(jaliu)
(Assignee)

Updated

3 years ago
Assignee: nobody → jaliu
(Assignee)

Updated

3 years ago
Status: NEW → ASSIGNED

Comment 18

3 years ago
Should we create a new bugzilla to track the issue where device is not visible to all, even though UI has the "Visible" switch in the On position?
Flags: needinfo?(iliu)
Flags: needinfo?(hochang)

Comment 19

3 years ago
Since Jamin is working on this case, I don't think we should create a new ticket for this issue.

--
Keven

Comment 20

3 years ago
I think Jamin is working on "why platform set visible = true after turn bt on (visible should suppose to be off)"

But do we have the problem as mentioned in Comment 18 now? Jamin, Ian?
Flags: needinfo?(jaliu)
Flags: needinfo?(iliu)
Flags: needinfo?(hochang)
(Assignee)

Comment 21

3 years ago
(In reply to howie [:howie] from comment #20)
> I think Jamin is working on "why platform set visible = true after turn bt
> on (visible should suppose to be off)"
> 
> But do we have the problem as mentioned in Comment 18 now? Jamin, Ian?

I'm working on the the bug which is described in #Description.

By reading the information provided in #comment18, I would treat these two issues you and Greg described here as the same one. However, I need the complete bug description and STR to know whether the two issues share the same root cause.

If Greg or anyone else finds a bug which is not filed yet, please feel free to do so. :)
Thanks.
Flags: needinfo?(jaliu)
(Assignee)

Comment 22

3 years ago
Created attachment 8605082 [details] [diff] [review]
Set Bluetooth discoverable to default value when BT state is BT_STATE_OFF. (v1)
Attachment #8605082 - Flags: review?(shuang)
Per my investigation in comment 16, I can make sure that the UI and platform value is sync while the issue happened. And I recheck the UI/platform value |visible=true| is really visible or not. The result is not. i.e. UI/platform value is sync, but they are not sync with Bluetooth chipset.

Since we're still investigating and fixing now, I would like to suggest to verify the issue is really fixed or not after patch landed. And need QA to check the device is visible=on/off really from other searching devices.

My suggestion is no needed to create the other bug.
Flags: needinfo?(iliu)
(Assignee)

Updated

3 years ago
Component: Gaia::Settings → Gaia::Bluetooth

Updated

3 years ago
Component: Gaia::Bluetooth → Bluetooth
Comment on attachment 8605082 [details] [diff] [review]
Set Bluetooth discoverable to default value when BT state is BT_STATE_OFF. (v1)

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

::: dom/bluetooth/bluedroid/BluetoothServiceBluedroid.cpp
@@ +2406,5 @@
>  #else
>    MOZ_ASSERT(NS_IsMainThread());
>  
> +  // If BT is enabled, distributes "PropertyChanged" signals to adapters.
> +  if (IsEnabled()) {

Can you double confirm these static variable will be correctly initialized if put guard to check Bluetooth enabled? I'm afraid this can cause side effect. AFAIK, adapter properties changed update before bluetooth adapter state changed, this is behavior of bluedroid stack. And if you don't update static variable here, you may face fail to load up bonded device address list. You have to load it up after new device get paired.
Attachment #8605082 - Flags: review?(shuang) → review-
(Assignee)

Comment 25

3 years ago
(In reply to Shawn Huang [:shawnjohnjr] from comment #24)
Thank you for your comments.
You're absolutely correct. I should fix that.
(Assignee)

Comment 26

3 years ago
Created attachment 8605763 [details] [diff] [review]
Set Bluetooth discoverable to default value when BT state is BT_STATE_OFF. (v2)

- revise previous patch based on #comment 24
Attachment #8605082 - Attachment is obsolete: true
Attachment #8605763 - Flags: review?(shuang)
Comment on attachment 8605763 [details] [diff] [review]
Set Bluetooth discoverable to default value when BT state is BT_STATE_OFF. (v2)

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

::: dom/bluetooth/bluedroid/BluetoothServiceBluedroid.cpp
@@ +2371,5 @@
> +      // by defalut. 'AdapterStateChangedNotification' would set the default
> +      // properties to BT stack once Bluetooth is enabled.
> +      if (IsEnabled()) {
> +        sAdapterDiscoverable =
> +          (p.mScanMode == SCAN_MODE_CONNECTABLE_DISCOVERABLE);

Can we set |sAdapterDiscoverable| even Bluetooth is disabled?
Attachment #8605763 - Flags: review?(shuang)
(Assignee)

Comment 28

3 years ago
(In reply to Shawn Huang [:shawnjohnjr] from comment #27)
> Comment on attachment 8605763 [details] [diff] [review]
> Set Bluetooth discoverable to default value when BT state is BT_STATE_OFF.
> (v2)
> 
> Review of attachment 8605763 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: dom/bluetooth/bluedroid/BluetoothServiceBluedroid.cpp
> @@ +2371,5 @@
> > +      // by defalut. 'AdapterStateChangedNotification' would set the default
> > +      // properties to BT stack once Bluetooth is enabled.
> > +      if (IsEnabled()) {
> > +        sAdapterDiscoverable =
> > +          (p.mScanMode == SCAN_MODE_CONNECTABLE_DISCOVERABLE);
> 
> Can we set |sAdapterDiscoverable| even Bluetooth is disabled?

If we set |sAdapterDiscoverable| = true when Bluetooth is disabled, |GetDefaultAdapterPathInternal| will reply discoverable as true to the constructor of adapter.

Through |adapter.discoverable| will be updated to false right after it's been constructed, it still leave a short period that |adapter.discoverable| is true.

Therefore, I don't think we should set |sAdapterDiscoverable| when Bluetooth is disabled.
(In reply to Jamin Liu [:jaliu] from comment #28)
> (In reply to Shawn Huang [:shawnjohnjr] from comment #27)
> > Comment on attachment 8605763 [details] [diff] [review]
> > Set Bluetooth discoverable to default value when BT state is BT_STATE_OFF.
> > (v2)
> > 
> > Review of attachment 8605763 [details] [diff] [review]:
> > -----------------------------------------------------------------
> > 
> > ::: dom/bluetooth/bluedroid/BluetoothServiceBluedroid.cpp
> > @@ +2371,5 @@
> > > +      // by defalut. 'AdapterStateChangedNotification' would set the default
> > > +      // properties to BT stack once Bluetooth is enabled.
> > > +      if (IsEnabled()) {
> > > +        sAdapterDiscoverable =
> > > +          (p.mScanMode == SCAN_MODE_CONNECTABLE_DISCOVERABLE);
> > 
> > Can we set |sAdapterDiscoverable| even Bluetooth is disabled?
> 
> If we set |sAdapterDiscoverable| = true when Bluetooth is disabled,
> |GetDefaultAdapterPathInternal| will reply discoverable as true to the
> constructor of adapter.
> 
> Through |adapter.discoverable| will be updated to false right after it's
> been constructed, it still leave a short period that |adapter.discoverable|
> is true.
> 
> Therefore, I don't think we should set |sAdapterDiscoverable| when Bluetooth
> is disabled.

Ok, I see.
Comment on attachment 8605763 [details] [diff] [review]
Set Bluetooth discoverable to default value when BT state is BT_STATE_OFF. (v2)

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

::: dom/bluetooth/bluedroid/BluetoothServiceBluedroid.cpp
@@ +2369,4 @@
>  
> +      // If BT is not enabled, Bluetooth scan mode should be non-discoverable
> +      // by defalut. 'AdapterStateChangedNotification' would set the default
> +      // properties to BT stack once Bluetooth is enabled.

nit:s/BT stack/bluetooth backend

@@ +2432,5 @@
>        BluetoothScanMode newMode = p.mScanMode;
>  
> +      // If BT is not enabled, Bluetooth scan mode should be non-discoverable
> +      // by defalut. 'AdapterStateChangedNotification' would set the default
> +      // properties to BT stack once Bluetooth is enabled.

nit:s/BT stack/bluetooth backend
Attachment #8605763 - Flags: review+
(Assignee)

Comment 31

3 years ago
Created attachment 8606143 [details] [diff] [review]
(final) Set Bluetooth discoverable to default value when BT state is BT_STATE_OFF. (v3), r=shuang

- revise previous patch based on #comment 30

Thank Shawn for reviewing the patch.
Attachment #8605763 - Attachment is obsolete: true
(Assignee)

Comment 32

3 years ago
Treeherder:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=1c7abc1d7dc3

Comment 33

3 years ago
https://hg.mozilla.org/integration/b2g-inbound/rev/d9cdfc062d1b
https://hg.mozilla.org/mozilla-central/rev/d9cdfc062d1b
Status: ASSIGNED → RESOLVED
Last Resolved: 3 years ago
status-firefox41: --- → fixed
Resolution: --- → FIXED

Comment 35

3 years ago
Created attachment 8613941 [details]
0433.mp4

According to the STR of Comment 0, this bug has been verified as pass on latest Nightly Flame v3.0 and Nexus5 v3.0
Actual results: After you toggle on bluetooth again, DUT will toggle off "Visible to all" item automatically.
See attachment: 0433.mp4
Reproduce rate: 0/20

Device: Flame 3.0 build(Pass)
Build ID               20150601160204
Gaia Revision          6d477a7884273886605049b20f60af5c1583a150
Gaia Date              2015-06-01 16:41:42
Gecko Revision         https://hg.mozilla.org/mozilla-central/rev/56241c1f8a3b
Gecko Version          41.0a1
Device Name            flame
Firmware(Release)      4.4.2
Firmware(Incremental)  eng.cltbld.20150601.193935
Firmware Date          Mon Jun  1 19:39:44 EDT 2015
Bootloader             L1TC000118D0             

Device: Nexus5 3.0 build(Pass)
Build ID               20150601075320
Gaia Revision          85e6fcef45c0cb2c017739df42b68b96cf5bb9c3
Gaia Date              2015-06-01 06:40:19
Gecko Revision         https://hg.mozilla.org/mozilla-central/rev/666b584fb521
Gecko Version          41.0a1
Device Name            hammerhead
Firmware(Release)      5.1
Firmware(Incremental)  eng.cltbld.20150601.112653
Firmware Date          Mon Jun  1 11:27:09 EDT 2015
Bootloader             HHZ12f

Updated

3 years ago
QA Whiteboard: [MGSEI-Triage+]
status-firefox41: fixed → verified
status-b2g-master: --- → verified
status-firefox41: verified → fixed
You need to log in before you can comment on or make changes to this bug.