Dave Hunt [:davehunt] [he/him] ⌚BST Reporter
	Description • 9 years ago

Steps to reproduce:
1. Flash with latest b2g-inbound tinderbox build.
2. Complete FTU with default values.
3. Launch Settings
4. Open Date & Time and change format from 12-hour to 24-hour
5. Press the power button to sleep device
6. Press the power button to wake device

Expected:
Time will be shown on lockscreen in 24-hour format.

Actual:
Time is shown in 12-hour format.

Unlocking the device and repeating steps 5 & 6 show the expected value on the lockscreen. This can be replicated manually (as above), however it was discovered by this automated test: https://github.com/mozilla-b2g/gaia/blob/2722edf0f20c4abfe101923035143cfd00ade5de/tests/python/gaia-ui-tests/gaiatest/tests/functional/settings/test_settings_change_time_format.py#L24

$ ./check_versions.sh 
Gaia-Rev        9e2e3726ee5c0d96dadb97aee4607375aa274613
Gecko-Rev       https://hg.mozilla.org/integration/b2g-inbound/rev/a56e6bbcf9c3
Build-ID        20150224043321
Version         39.0a1
Device-Name     flame
FW-Release      4.4.2
FW-Incremental  eng.cltbld.20150224.061930
FW-Date         Tue Feb 24 06:19:41 EST 2015
Bootloader      L1TC100118D0

	Dave Hunt [:davehunt] [he/him] ⌚BST Reporter
	Comment 1 • 9 years ago

Last good: 395b88463487
First bad: e75a3c76835a
Pushlog: http://hg.mozilla.org/integration/b2g-inbound/pushloghtml?fromchange=395b88463487&tochange=e75a3c76835a

	Dave Hunt [:davehunt] [he/him] ⌚BST Reporter
	Comment 2 • 9 years ago

Strongly suspect bug 1115311. Requesting info from author.

	Greg Weng [:snowmantw][:gweng][:λ] Assignee
	Comment 3 • 9 years ago

Thanks for the report. Since timezone issue I may handle it tomorrow, so if this is urgent (blocking tests) please feel free to back it out. I've expected this may need further verifications (see Bug 1115311 Comment 14) to make sure it's stable enough because it's really a huge refactoring.

	Greg Weng [:snowmantw][:gweng][:λ] Assignee
	Comment 4 • 9 years ago

OK now I'm checking this bug.

	Greg Weng [:snowmantw][:gweng][:λ] Assignee
	Comment 5 • 9 years ago

Dave: I've flashed my device with latest master Gecko + Gaia from PVT, but I couldn't reproduce it. The video is here:

https://drive.google.com/file/d/0BzMbOZyJEHicSXBLRnFxZWFoZzg/view?usp=sharing

In the video I change the time to PM to show the 12/24 format more clearly. And although in the video there are SIM cards in the device, it's no matter with or without the SIM cards since I've tried several ways to reproduce it according to the STR, but I still couldn't reproduce it.

Also, in Bug 1115311 Comment 12 you can see I landed this patch with the passed automation test result. So could you check it again?

--- 

Flame: 

Gaia-Rev        e4a5a54c0891a1cc964b2d94918408f2625c7aaa
Gecko-Rev       https://hg.mozilla.org/mozilla-central/rev/0a8b3b67715a
Build-ID        20150224160313
Version         39.0a1
Device-Name     flame
FW-Release      4.4.2
FW-Incremental  65
FW-Date         Mon Dec 15 18:51:29 CST 2014
Bootloader      L1TC000118D0

	Dave Hunt [:davehunt] [he/him] ⌚BST Reporter
	Comment 6 • 9 years ago

In your video you first change the time to PM and lock, which could invalidate the STR as this appear to happen for me on the first sleep/wake after a flash.

How did you flash your device? Perhaps this is important. I used https://github.com/Mozilla-TWQA/B2G-flash-tool and the following command line:
$ ./flash_tbpl.py -v b2g-inbound -d flame-kk -f --eng

My device has one inactive SIM, but we've also reproduced this on our device lab in Mountain View - these devices all have active SIM cards. If you're on VPN you should be able to see the failure here: http://jenkins1.qa.scl3.mozilla.com:8080/view/UI/job/flame-kk-319.mozilla-central.nightly.ui.functional.non-smoke.1/52/HTML_Report/

The automated test results you refer to are run on the desktop simulator, however this is failing on device. I've recorded a video myself: https://drive.google.com/open?id=0B9anWbWFbwXaTlQydFBpcjc1Rms&authuser=1

	Greg Weng [:snowmantw][:gweng][:λ] Assignee
	Comment 7 • 9 years ago

OK I finally got it, thanks Dave's video. The issue would only appear when user first locks the screen *after* change the timeformat. In my video I checked it *before* I change the format, this would invalid the STR. Now I'm going to patch it.

	Greg Weng [:snowmantw][:gweng][:λ] Assignee
	Comment 8 • 9 years ago

Well I've found this is not a simple bug: to solve it we would face the ancient debt of FTU vs. LockScreen racing. I try to resolve it but the solution faced another performance issue about LockScreen wallpaper. However, IMO it's very necessary to solve the issue. So I may take more time to debug this bug.

Anyway, I would still try to submit a workaround to solve this case.

	Greg Weng [:snowmantw][:gweng][:λ] Assignee
	Comment 10 • 9 years ago

Comment on attachment 8571271 [details] [review]
[gaia] snowmantw:bug1136156 > mozilla-b2g:master

The fully "reasonable" version, which brings performance issue.

	Greg Weng [:snowmantw][:gweng][:λ] Assignee
	Comment 12 • 9 years ago

OK I've patched the bug with my second patch. The video:

https://drive.google.com/file/d/0BzMbOZyJEHicNXlBblZuX3pHNTA/view?usp=sharing

	Greg Weng [:snowmantw][:gweng][:λ] Assignee
	Comment 13 • 9 years ago

Comment on attachment 8571282 [details] [review]
[gaia] snowmantw:bug1136156-workaround > mozilla-b2g:master

Ahh sorry I forgot to add test. Set the flag later.

	Greg Weng [:snowmantw][:gweng][:λ] Assignee
	Comment 14 • 9 years ago

Comment on attachment 8571282 [details] [review]
[gaia] snowmantw:bug1136156-workaround > mozilla-b2g:master

OK I added the test.

	Tim Guan-tin Chien [:timdream] (please needinfo)
	Comment 15 • 9 years ago

contrary to comment 8, the STR in comment 0 has nothing to do with FTU.

I would argue this is the correct fix, not a workaround -- or what else would you be able to ensure the time format is correct when the tick state starts?


This is where we first update the time format value:

https://github.com/snowmantw/gaia/blob/bug1136156-workaround/apps/system/lockscreen/js/widgets/clock/lockscreen_clock_widget_setup.js#L22

Could we make sure we only reach the tick state after the format value is updated here? What's stopping us from make sure of that?

	Greg Weng [:snowmantw][:gweng][:λ] Assignee
	Comment 16 • 9 years ago

The FTU is the root cause and Comment 0 exactly mentioned FTU at step 2. You can take a look the video at Comment 6: Dave didn't lock the screen before he changed the format, while I did it in the video at Comment 5. That's why I couldn't reproduce it.

The LockScreen window should never be opened if FTU, or other window except secure app, is overlapped on the most top of System app, not to mention when Homescreen appears *after* the FTU, therefore we can guarantee that when LockScreen is on and the timeformat is fetched from API, user has no chance to change that, unless they unlock the screen and set it in Settings. So there is no need to get timeformat again, no matter from listening event (what the old lockscreen.js did), or forcibly fetching it every time we enter the ticking state.

So if we keep the premise true, when the screen had been unlocked, the widget would be stopped, destroyed, and thus no information would be keep including the timeformat. It would only be updated when the next time the user lock it again, which means we would go through the whole Setup process again. However, as I mentioned, we have a performance issue that I still have no clue even with my profiling work of LockScreenWindow & LockScreenWindowManager, I must keep the trick I left in LWM:

https://github.com/mozilla-b2g/gaia/blob/master/apps/system/js/lockscreen_window_manager.js#L239-L243

This trick would create the window immediately when the LockScreen is enabled. If we remove it, user would see a pure black background when the window is first opened about 3 seconds. With the profiling work I can't find any clue about why this happened, and I suspect there is some nasty thing happen in the platform, since it doesn't get solved after I suspend the only obvious performance bottleneck from lockscreen.js.

The result is, with such trick, we couldn't guarantee the LockScreen code, including the widget setup step, only be executed when user has no chance to change the timeformat, which is a very reasonable assumption from my point of view. So the situation now becomes we now would have a 'created' LockScreen window *after* FTU, and user could change the timeformat even when the window is invisible, while the 'Setup' step had been executed and fetched the old timeformat. That's why I say this patch is a workaround.

	Greg Weng [:snowmantw][:gweng][:λ] Assignee
	Comment 17 • 9 years ago

And Tim we may have an offline discussion if my explanation isn't clear enough. Thanks.

	Tim Guan-tin Chien [:timdream] (please needinfo)
	Comment 18 • 9 years ago

Comment on attachment 8571282 [details] [review]
[gaia] snowmantw:bug1136156-workaround > mozilla-b2g:master

Discussed offline for this workaround patch. Please file another bug to land the real fix and file a graphics/performance bug that blocks the bug to file.

	Greg Weng [:snowmantw][:gweng][:λ] Assignee
	Comment 19 • 9 years ago

TBPL is green:

https://treeherder.mozilla.org/#/jobs?repo=gaia-try&revision=bf3f021cf45c

master:

https://github.com/mozilla-b2g/gaia/commit/71149de168770fa623dc604495e6f9efc3d9aa49