Closed Bug 1180021 Opened 9 years ago Closed 9 years ago

[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

Categories

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

Product:
Firefox OS Graveyard
Component:
Gaia::System::Status bar, Utility tray, Notification
Platform:
ARM
Gonk (Firefox OS)
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

(blocking-b2g:2.5?, b2g-v2.2 unaffected, b2g-master affected)

Status:
RESOLVED DUPLICATE of bug 1183128
Project Flags:
blocking-b2g 2.5?
Tracking Flags:
Tracking Status
b2g-v2.2 --- unaffected
b2g-master --- affected

People

(Reporter: AdamA, Assigned: yzen)

References

(
URL
)

Details

(Keywords: regression, Whiteboard: [2.5-Daily-Testing][Spark])

Attachments

(2 files)

logcat
(deleted), text/plain
Details
[gaia] yzen:bug-1180021 > mozilla-b2g:master
(deleted), text/x-github-pull-request
Details
Attached file logcat (deleted) — 
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
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
Blocks: Foxfood-papercuts
URL: https://youtu.be/sxuOGUrFi10
QA Whiteboard: [QAnalyst-Triage?]
status-b2g-v2.2: --- → unaffected
status-b2g-master: --- → affected
Flags: needinfo?(pbylenga)
Keywords: regression
Whiteboard: [2.5-Daily-Testing][Spark]
[Blocking Requested - why for this release]:
Noticeable regression.

Requesting a window.
blocking-b2g: --- → 2.5?
QA Whiteboard: [QAnalyst-Triage?] → [QAnalyst-Triage+]
Flags: needinfo?(pbylenga)
Keywords: regressionwindow-wanted
QA Contact: jmercado
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
QA Whiteboard: [QAnalyst-Triage+] → [QAnalyst-Triage?]
Flags: needinfo?(ktucker)
Keywords: regressionwindow-wanted
Yura, can you take a look at this please? Looks like the landing for bug 1162040 might have caused this.
Blocks: 1162040
QA Whiteboard: [QAnalyst-Triage?] → [QAnalyst-Triage+]
Flags: needinfo?(ktucker) → needinfo?(yzenevich)
Assignee: nobody → yzenevich
Status: NEW → ASSIGNED
Flags: needinfo?(yzenevich)
Attachment #8630464 - Flags: review?(timdream)
Comment on attachment 8630464 [details]
[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.
Flags: needinfo?(yzenevich)
(In reply to Tim Guan-tin Chien [:timdream] (slow response; please ni? to queue) from comment #6)
> Comment on attachment 8630464 [details]
> [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.
Flags: needinfo?(yzenevich) → needinfo?(timdream)
(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.
Flags: needinfo?(timdream)
According to comment 7.
No longer blocks: 1162040
Keywords: regression
Oh, I shouldn't remove the regression flag here...
Keywords: regression
(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'.
Flags: needinfo?(yzenevich)
(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.
(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 on attachment 8630464 [details]
[gaia] yzen:bug-1180021 > mozilla-b2g:master

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

Sounds good.
Status: ASSIGNED → RESOLVED
Closed: 9 years ago
Resolution: --- → DUPLICATE
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: