Lockscreen passcode does not survive reboot

VERIFIED FIXED in Firefox OS master

Status

VERIFIED FIXED
3 years ago
3 years ago

People

(Reporter: freddyb, Assigned: freddyb)

Tracking

unspecified
FxOS-S3 (24Jul)
ARM
Gonk (Firefox OS)
Dependency tree / graph

Firefox Tracking Flags

(b2g-master verified)

Details

Attachments

(2 attachments, 1 obsolete attachment)

(Assignee)

Description

3 years ago
** This was introduced by bug 1123325 **

My patch to the settings migrator did not take into account that the default settings are a passcode of "0000", even if it is not enabled.

That means that when you set a different passcode (using the new settings attribute) the migrator will not notice that *both* exist and overwrite the new-style with the old-style since it thinks a migration is necessary.

STR:
1) Create a passcode (e.g., 4444). Locking and Unlocking works fine
2) Reboot
3) Migrator will overwrite new-style value with "0000", because old-style value with "0000" exists.
4) Unlock only possible with "4444".


I am fixing this by modifying the existing check to not only see if an old-style passcode is existant but also to bail out if a new one has been already set.
Created attachment 8631017 [details] [review]
[gaia] mozfreddyb:1181565-settings-migrator-bug > mozilla-b2g:master
(Assignee)

Comment 2

3 years ago
Created attachment 8631018 [details] [review]
github pull request

Fred, can you review?
Assignee: nobody → fbraun
Status: NEW → ASSIGNED
Attachment #8631018 - Flags: review?(gasolin)

Comment 3

3 years ago
Comment on attachment 8631018 [details] [review]
github pull request

please add unit test since the migrator is a high risk area
Attachment #8631018 - Flags: review?(gasolin)
(Assignee)

Updated

3 years ago
Attachment #8631018 - Attachment is obsolete: true
(Assignee)

Comment 4

3 years ago
Comment on attachment 8631017 [details] [review]
[gaia] mozfreddyb:1181565-settings-migrator-bug > mozilla-b2g:master

It's hard to test something that shoots something off asynchronously but does not return the promise. So I had to rewrite a few bits, so that it's returns the promises for things that it kicks off. :-)

We could also rewrite the other unit tests to not use a fake clock but look into the promises that are now returned. I'd prefer if this happens in a follow up.
For now, I left them untouched as they are passing fine.

Fred, can you take another look please?
Attachment #8631017 - Flags: review?(gasolin)

Comment 5

3 years ago
Comment on attachment 8631017 [details] [review]
[gaia] mozfreddyb:1181565-settings-migrator-bug > mozilla-b2g:master

please try mockLazyLoader which turns lazy loader to synchronized and make inner code testable
Attachment #8631017 - Flags: review?(gasolin)
(Assignee)

Comment 6

3 years ago
I don't think this would make a big difference, since neither start() nor keyMigration() return anything. We still need to rewrite those functions. Can we please do this in a follow-up?

I do not want passcodes broken on master for so long.
Flags: needinfo?(gasolin)
Blocks: 1181662

Comment 7

3 years ago
I understand that its hard to test inside of lazyloader, so you don't have to.

The expect tests are:

1. when certain settings value already exists, make sure it not trigger the migration
2. when certain settings value does not exist, the migration (lazyloader part) is called
Flags: needinfo?(gasolin)
(Assignee)

Comment 8

3 years ago
Comment on attachment 8631017 [details] [review]
[gaia] mozfreddyb:1181565-settings-migrator-bug > mozilla-b2g:master

Can you review again, please?
Attachment #8631017 - Flags: review?(gasolin)

Comment 9

3 years ago
Comment on attachment 8631017 [details] [review]
[gaia] mozfreddyb:1181565-settings-migrator-bug > mozilla-b2g:master

Thanks for hard work!

r+ if comment addressed in github. 

Please add suite('when old passcode is not present') test case to make sure its not run to LazyLoader.
Attachment #8631017 - Flags: review?(gasolin) → review+
(Assignee)

Updated

3 years ago
Keywords: checkin-needed
Master: https://github.com/mozilla-b2g/gaia/commit/506a55fcb5b5992c64f3a776f29294b8b15af6b3
Status: ASSIGNED → RESOLVED
Last Resolved: 3 years ago
status-b2g-master: --- → fixed
Keywords: checkin-needed
Resolution: --- → FIXED
Target Milestone: --- → FxOS-S3 (24Jul)
Duplicate of this bug: 1186456
This bug has been verified as "pass" on the latest build of Flame KK 2.5 and Aires KK 2.5 by the STR in comment 0.

Actual results: Lockscreen passcode is correct and not reset to "0000" after reboot.
See attachment: verified_FlameKK_v2.5.3gp
Reproduce rate: 0/10


Device: Flame KK 2.5 (Pass)
Build ID               20150818150207
Gaia Revision          507ba38fb64b27f87d11f4104dfcc58448e12b1a
Gaia Date              2015-08-18 10:50:12
Gecko Revision         https://hg.mozilla.org/mozilla-central/rev/2c272af993c23e803f6ea7798a812b0c8abfad4d
Gecko Version          43.0a1
Device Name            flame
Firmware(Release)      4.4.2
Firmware(Incremental)  eng.cltbld.20150818.184156
Firmware Date          Tue Aug 18 18:42:07 EDT 2015
Firmware Version       v18D v4
Bootloader             L1TC000118D0

Device: Aries KK 2.5(Pass)
Build ID               20150818233027
Gaia Revision          1e1197e0e8e64307aa382ffba4711d1c661de7ca
Gaia Date              2015-08-18 16:54:35
Gecko Revision         https://hg.mozilla.org/mozilla-central/rev/88e1d293ffa9faf065b2a944c94a2705aabf1fb9
Gecko Version          43.0a1
Device Name            aries
Firmware(Release)      4.4.2
Firmware(Incremental)  eng.worker.20150818.225517
Firmware Date          Tue Aug 18 22:55:24 UTC 2015
Bootloader             s1
Status: RESOLVED → VERIFIED
QA Whiteboard: [MGSEI-Triage+]
status-b2g-master: fixed → verified
Created attachment 8649699 [details]
verified_FlameKK_v2.5.3gp
You need to log in before you can comment on or make changes to this bug.