Closed Bug 1136156 Opened 7 years ago Closed 7 years ago
Lockscreen shows incorrect time format after it's modified for the first time
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
Last good: 395b88463487 First bad: e75a3c76835a Pushlog: http://hg.mozilla.org/integration/b2g-inbound/pushloghtml?fromchange=395b88463487&tochange=e75a3c76835a
Strongly suspect bug 1115311. Requesting info from author.
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.
OK now I'm checking this bug.
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
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
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.
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.
Comment on attachment 8571271 [details] [review] [gaia] snowmantw:bug1136156 > mozilla-b2g:master The fully "reasonable" version, which brings performance issue.
OK I've patched the bug with my second patch. The video: https://drive.google.com/file/d/0BzMbOZyJEHicNXlBblZuX3pHNTA/view?usp=sharing
Comment on attachment 8571282 [details] [review] [gaia] snowmantw:bug1136156-workaround > mozilla-b2g:master Ahh sorry I forgot to add test. Set the flag later.
Comment on attachment 8571282 [details] [review] [gaia] snowmantw:bug1136156-workaround > mozilla-b2g:master OK I added the test.
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?
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.
And Tim we may have an offline discussion if my explanation isn't clear enough. Thanks.
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.
Attachment #8571282 - Flags: review?(timdream) → review+
TBPL is green: https://treeherder.mozilla.org/#/jobs?repo=gaia-try&revision=bf3f021cf45c master: https://github.com/mozilla-b2g/gaia/commit/71149de168770fa623dc604495e6f9efc3d9aa49
Status: NEW → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.