1180021 - [Utility Tray] The WiFi icon in the notification center will say it is enabled and be unresponsive after the phone is restarted with WiFi disabled

Status: RESOLVED DUPLICATE of bug 1183128

(Firefox OS Graveyard :: Gaia::System::Status bar, Utility tray, Notification, defect)

Description

[Adam Alldredge [:AdamA][Inactive]]

Attachment: logcat

Description:
If the phone is restarted while wi-fi is disabled then the wi-fi icon in the notification tray will say that wifi is turned on while it is off. the icon will also not be responsive until users go into setting and enable wifi there.

Repro Steps:
1) Update a Aries to 20150702210513
2) Make sure WiFi is disabled
3) Restart the phone
4) Pull down utility tray
5) Observe WiFi Icon

Actual:
Wifi icon will say that wifi is enabled when it is disabled

Expected:
it is expected that the notification tray reflects the actual setting of the phone.

Environmental Variables:
Device: Aries 2.5 (Full Flash)
Build ID: 20150702210513
Gaia: 722028715a56a03f327e2e70f2c32dcb6d819d4c
Gecko: 2f25351c5b05
Gonk: 2916e2368074b5383c80bf5a0fba3fc83ba310bd
Version: 42.0a1 (2.5)
Firmware Version: D5803_23.1.A.1.28_NCB.ftf
User Agent: Mozilla/5.0 (Mobile; rv:42.0) Gecko/42.0 Firefox/42.0

Repro frequency: 10/10
See attached: video clip(https://youtu.be/sxuOGUrFi10), logcat

Comment 1

[Adam Alldredge [:AdamA][Inactive]]

This issue DOES occur on Flame 2.5.

Environmental Variables:
Device: Flame 2.5 (Full Flash)
BuildID: 20150702010204
Gaia: b901c8b7be2119f4df42781aac1401ed12765460
Gecko: f5e3bacfb60e
Gonk: a4f6f31d1fe213ac935ca8ede7d05e47324101a4
Version: 42.0a1 (2.5) 
Firmware Version: v18D-1
User Agent: Mozilla/5.0 (Mobile; rv:42.0) Gecko/42.0 Firefox/42.0

Result:
Wifi icon will say that wifi is enabled when it is disabled
---------------------------------------------
This issue DOES NOT occur on Flame 2.2.

Environmental Variables:
Device: Flame 2.2 (Full Flash)
BuildID: 20150702002503
Gaia: bd386f346eb1591fddbc84bf034b22700e7e2a58
Gecko: f16c1125b9d6
Gonk: bd9cb3af2a0354577a6903917bc826489050b40d
Version: 37.0 (2.2) 
Firmware Version: v18D-1
User Agent: Mozilla/5.0 (Mobile; rv:37.0) Gecko/37.0 Firefox/37.0

Result:
The notification tray reflects the current wifi status

Comment 2

[Peter Bylenga [:PBylenga]]

[Blocking Requested - why for this release]:
Noticeable regression.

Requesting a window.

Comment 3

[Jayme Mercado [:JMercado] (Inactive 3/4/2016)]

Bug 1162040 seems to have caused this issue.

B2g-inbound Regression Window

Last Working 
Environmental Variables:
Device: Flame 2.5
BuildID: 20150605130240
Gaia: 1d62b32408567f9f7cf1c71c1e5a0c6593be757b
Gecko: 163b199605d9
Version: 41.0a1 (2.5) 
Firmware Version: v18D-1
User Agent: Mozilla/5.0 (Mobile; rv:41.0) Gecko/41.0 Firefox/41.0

First Broken 
Environmental Variables:
Device: Flame 2.5
BuildID: 20150605130544
Gaia: 55858863f320efc73c0bfd9b3eef905e49998e39
Gecko: 7a85fbd2a0f8
Version: 41.0a1 (2.5) 
Firmware Version: v18D-1
User Agent: Mozilla/5.0 (Mobile; rv:41.0) Gecko/41.0 Firefox/41.0

Last Working gaia / First Broken gecko - Issue does NOT occur
Gaia: 1d62b32408567f9f7cf1c71c1e5a0c6593be757b
Gecko: 7a85fbd2a0f8

First Broken gaia / Last Working gecko - Issue DOES occur
Gaia: 55858863f320efc73c0bfd9b3eef905e49998e39
Gecko: 163b199605d9

Gaia Pushlog: https://github.com/mozilla-b2g/gaia/compare/1d62b32408567f9f7cf1c71c1e5a0c6593be757b...55858863f320efc73c0bfd9b3eef905e49998e39

Comment 4

[KTucker [:KTucker][Inactive 3/4/2016]]

Yura, can you take a look at this please? Looks like the landing for bug 1162040 might have caused this.

Comment 5

[GitHub Autolander Bot]

Attachment: [gaia] yzen:bug-1180021 > mozilla-b2g:master

Comment 6

[Tim Guan-tin Chien [:timdream] (please needinfo)]

Comment on attachment 8630464 [details] [review]
[gaia] yzen:bug-1180021 > mozilla-b2g:master

I am a bit confused with the relationship of this "regression" with bug 1162040. So property in |dataset| is always a string, but we previously use |delete el.dataset.prop| to set the prop to |false|. I would say it's a safer pattern than remember to always evaluate the prop as "true"/"false" strings.

Comment 7

[Yura Zenevich [:yzen] (please use "needinfo")]

(In reply to Tim Guan-tin Chien [:timdream] (slow response; please ni? to queue) from comment #6)
> Comment on attachment 8630464 [details] [review]
> [gaia] yzen:bug-1180021 > mozilla-b2g:master
> 
> I am a bit confused with the relationship of this "regression" with bug
> 1162040. So property in |dataset| is always a string, but we previously use
> |delete el.dataset.prop| to set the prop to |false|. I would say it's a
> safer pattern than remember to always evaluate the prop as "true"/"false"
> strings.

I agree, I was pretty unconvinced that the bug 1162040 is the culprit. However the logic of checking !!"false" will always return true, same as !!"true" and it would not work correctly with the original logic (especially when we check for airport mode enabled for data toggle). 

Given that data-enabled is not a truly boolean attribute, I think it will be less confusing to consistently check for the "true" value of the attribute. 

I can, optionally, take the logic out into _isEnabled method for example and then we could call this._isEnabled('wifi') instead of this.wifi.dataset.enabled === "true". This is at least, arguably, a little more safe.

Comment 8

[Tim Guan-tin Chien [:timdream] (please needinfo)]

(In reply to Yura Zenevich [:yzen] from comment #7)
> I agree, I was pretty unconvinced that the bug 1162040 is the culprit.
> However the logic of checking !!"false" will always return true, same as
> !!"true" and it would not work correctly with the original logic (especially
> when we check for airport mode enabled for data toggle). 
> 
> Given that data-enabled is not a truly boolean attribute, I think it will be
> less confusing to consistently check for the "true" value of the attribute. 
> 
> I can, optionally, take the logic out into _isEnabled method for example and
> then we could call this._isEnabled('wifi') instead of
> this.wifi.dataset.enabled === "true". This is at least, arguably, a little
> more safe.

I would recommend identifying the place where dataset.enabled is being set to any string other than "true" and fix it. I see |delete| still in place in that file most of the places (if not all).

The proper fix would not only wrap the comparison inside a _isEnabled() function but to keep these states in JavaScript objects -- dataset is not the right place to keep JS states; DOM should be considered only the rendering layer of the app, but it's fine if we don't want to do that for a bug fixing regression.

Comment 9

[Tim Guan-tin Chien [:timdream] (please needinfo)]

According to comment 7.

Comment 10

[Tim Guan-tin Chien [:timdream] (please needinfo)]

Oh, I shouldn't remove the regression flag here...

Comment 11

[Tim Guan-tin Chien [:timdream] (please needinfo)]

Wait, should the patch to fix the regression simply revert the two lines here?

https://github.com/mozilla-b2g/gaia/compare/1d62b32408567f9f7cf1c71c1e5a0c6593be757b...55858863f320efc73c0bfd9b3eef905e49998e39#diff-c6f6a93ef2f3dacce1ce5b6dcf60b793L197

Comment 12

[Yura Zenevich [:yzen] (please use "needinfo")]

(In reply to Tim Guan-tin Chien [:timdream] (slow response; please ni? to queue) from comment #8)
> (In reply to Yura Zenevich [:yzen] from comment #7)
> > I agree, I was pretty unconvinced that the bug 1162040 is the culprit.
> > However the logic of checking !!"false" will always return true, same as
> > !!"true" and it would not work correctly with the original logic (especially
> > when we check for airport mode enabled for data toggle). 
> > 
> > Given that data-enabled is not a truly boolean attribute, I think it will be
> > less confusing to consistently check for the "true" value of the attribute. 
> > 
> > I can, optionally, take the logic out into _isEnabled method for example and
> > then we could call this._isEnabled('wifi') instead of
> > this.wifi.dataset.enabled === "true". This is at least, arguably, a little
> > more safe.
> 
> I would recommend identifying the place where dataset.enabled is being set
> to any string other than "true" and fix it. I see |delete| still in place in
> that file most of the places (if not all).
> 
> The proper fix would not only wrap the comparison inside a _isEnabled()
> function but to keep these states in JavaScript objects -- dataset is not
> the right place to keep JS states; DOM should be considered only the
> rendering layer of the app, but it's fine if we don't want to do that for a
> bug fixing regression.

Let me know what you think. I opted for tracking state in JS instead of with buttons' data-enabled attributes. I'm also fine with just making changes to get rid of cases where we have values other than 'true'.

Comment 13

[Tim Guan-tin Chien [:timdream] (please needinfo)]

(In reply to Yura Zenevich [:yzen] from comment #12)
> Let me know what you think. I opted for tracking state in JS instead of with
> buttons' data-enabled attributes. I'm also fine with just making changes to
> get rid of cases where we have values other than 'true'.

In the interest of preventing further regressions, this bug should be used to revert the source of regression as pointed out. Moving the state to JS can be done in another bug -- assuming that itself is a patch bigger enough to maybe cause regression.

I have no problem if you are confident to work on both things here though.

Comment 14

[Yura Zenevich [:yzen] (please use "needinfo")]

(In reply to Tim Guan-tin Chien [:timdream] (slow response; please ni? to queue) from comment #13)
> (In reply to Yura Zenevich [:yzen] from comment #12)
> > Let me know what you think. I opted for tracking state in JS instead of with
> > buttons' data-enabled attributes. I'm also fine with just making changes to
> > get rid of cases where we have values other than 'true'.
> 
> In the interest of preventing further regressions, this bug should be used
> to revert the source of regression as pointed out. Moving the state to JS
> can be done in another bug -- assuming that itself is a patch bigger enough
> to maybe cause regression.
> 
> I have no problem if you are confident to work on both things here though.

Ok, cleaned up the PR to have only regression fixes. Will open the state bug now.

Comment 16

[Tim Guan-tin Chien [:timdream] (please needinfo)]

Comment on attachment 8630464 [details] [review]
[gaia] yzen:bug-1180021 > mozilla-b2g:master

Should this bug be dup to bug 1183128 instead?

Comment 17

[Yura Zenevich [:yzen] (please use "needinfo")]

(In reply to Tim Guan-tin Chien [:timdream] (slow response; please ni? to queue) from comment #16)
> Comment on attachment 8630464 [details] [review]
> [gaia] yzen:bug-1180021 > mozilla-b2g:master
> 
> Should this bug be dup to bug 1183128 instead?

Sounds good.