1190079 - [LockScreen][pre-Gleipnir] Remove default values of LockScreen

Status: RESOLVED FIXED

(Firefox OS Graveyard :: Gaia::System::Lockscreen, defect)

Description

[Greg Weng [:snowmantw][:gweng][:λ]]

It's dangerous and people get confused to try to plug holes by "fixing" these values. So my propose is to remove them ASAP. The better way to handle racing is to delay all the requests depend on the settings and other async reading values. And if we still miss some racing, it should throw error and then the screen locker freezes.

One concern is if to allow screen locker crash in such cases plants a new security hole? I don't think so. Since if scripts of LockScreen get stopped because one unexpected exception, it means no unlocking code will be executed, especially it is the one rely on the critical value from mozSettings. And the most important thing is we can debug it to make it more safe, not just to cover the racing with some fallback default value.

Comment 1

[Greg Weng [:snowmantw][:gweng][:λ]]

By the way, I use "pre-Gleipnir" to distinguish bugs before we adopt the new state machine. This indicates the position of bugs in the roadmap. All the pre-Gleipnir bugs are not serious refactoring, but to follow the existing architecture to improve the code quality.

Comment 2

[Greg Weng [:snowmantw][:gweng][:λ]]

Oh, I forgot to mention this features should be landed in all branches affected by Bug 1188716. At least I'll try.

Comment 3

[GitHub Autolander Bot]

Attachment: [gaia] snowmantw:bug1190079-rev2 > mozilla-b2g:master

Comment 4

[Greg Weng [:snowmantw][:gweng][:λ]]

Comment on attachment 8642175 [details] [review]
[gaia] snowmantw:bug1190079-rev2 > mozilla-b2g:master

Unfortunately Bug 1094759 left lots of hidden racing holes in LockScreen component: every LazyLoading now becomes a uncertain weakpoint that need to turn methods and tests, and worse, those methods depends on them, into async units and managed by some "main queue" promise. It's basically what Gleipnir does, but this time, without an overall design and under the time pressure, plus that we're still tangled with the very legacy code, I can't fix them all in one patch without any workaround. However, the good news is with such forcible throwing while there is no such value yet, I've addressed some hidden issues and fixed them all. Moreover, it showed that even with the error by some illegal attempts to read the value before it was read, the screen remained locked. Of course we need QA & sec-team to do double check, but I think basically this patch solved the most of issues.

And since this patch needs to handle LazyLoad racing that don't exist in old branches, I may submit another patch with almost the same design in LockScreen, and we can discuss if we could uplift such patch to those branches. In my opinion we should at least apply such patch on 2.2.

Comment 5

[Greg Weng [:snowmantw][:gweng][:λ]]

Comment on attachment 8642175 [details] [review]
[gaia] snowmantw:bug1190079-rev2 > mozilla-b2g:master

The conclusion of Offline discussion: we should start to solve Bug 1189641 since it's the correct way to handle racing. So this bug is only for 2.2, 2.1 and 2.0.

Comment 6

[GitHub Autolander Bot]

Attachment: [gaia] snowmantw:bug1190079-2.2 > mozilla-b2g:v2.2

Comment 7

[Greg Weng [:snowmantw][:gweng][:λ]]

Comment on attachment 8643459 [details] [review]
[gaia] snowmantw:bug1190079-2.2 > mozilla-b2g:v2.2

Tim: I've found on branches 2.2, 2.1 and 2.0, since we could block the slider before those values of security get read, the patch becomes much simple. All we need to do is to make sure we initialize the slider events emitters and listeners only after those values come. And then there aren't too much work to do.

Comment 8

[Greg Weng [:snowmantw][:gweng][:λ]]

And for master I'll still continue my work to move all listeners from LockScreen, as we discussed and summarized at Comment 5.

Comment 9

[Greg Weng [:snowmantw][:gweng][:λ]]

Oh, and, we do remove the default values. So there are no default values of those security related properties in Lockscreen anymore.

Comment 10

[Greg Weng [:snowmantw][:gweng][:λ]]

Comment on attachment 8643459 [details] [review]
[gaia] snowmantw:bug1190079-2.2 > mozilla-b2g:v2.2

[Approval Request Comment]
[Bug caused by] (feature/regressing bug #): Old design could be tracked to v2.0
[User impact] if declined: Unlocking racing
[Testing completed]: CI result: https://treeherder.mozilla.org/#/jobs?repo=gaia&revision=3c14fccc592d93934a79933a7639ec7f037a9a11
[Risk to taking this patch] (and alternatives if risky): Racing continues
[String changes made]: No

Comment 11

[[:fabrice] Fabrice Desré]

Comment on attachment 8643459 [details] [review]
[gaia] snowmantw:bug1190079-2.2 > mozilla-b2g:v2.2

Greg, I'm not doing the 2.2 approvals.

Comment 12

[Mahendra Potharaju [:mahe]]

Greg, Do you have tests to verify the fix and ensure it doesn't break anything else? Who in QA do you work with closely to test this fix? Please let me know these details, so I can approve upon testing. 

Thanks

Comment 13

[Greg Weng [:snowmantw][:gweng][:λ]]

Since I'm not QA so I only tested what I could (the racing, and other behaviors user could do with LockScreen). I think Naoki is the major QA tested and involved in these racing bugs according to bug comments and mails, but I don't know if he was assigned to do that.

Comment 14

[Mahendra Potharaju [:mahe]]

Thanks Greg. 

Naoki, Can you please help out here?

Thanks.

Comment 15

[Naoki Hirata :nhirata (please use needinfo instead of cc)]

Hrm.  Ok.  So if I understand this, I have to take a look at the race conditions, values and such on the lock screen?  I think CR should also take a look from a security stand point.

Just wondering, did you have a state machine diagram or a UML diagram that you drew up in regards to this?  I would love to take a look.  :D

To make sure I'm on the right track, an example of what I would be testing for race/value conditions : like having settings/camera up while wifi is on, sim card is in, bluetooth is on, and unlocking the device from sleep after OTA. 

To note, I'm going to work up to a complex real-world situation like this ( starting with the easy ).  ie start from having camera up and then unlocking the screen.

Comment 16

[Greg Weng [:snowmantw][:gweng][:λ]]

I think Manhendra just want to make sure this is verified not to break those behaviors we verified for the master patch?

And unfortunately the diagram is just as complicated as code, so I tried but gave up to draw all them down.  As a result some external app related behaviors like FMD now are only guarded by our tests, including manual and automatic parts. So if you encounter any difficulties please just NI me, and we can try to find solutions together.

Comment 17

[Naoki Hirata :nhirata (please use needinfo instead of cc)]

Sorry for the delay.  I had to figure out what I was doing wrong in making a 2.2 OTA build.  Figured it out and unblocked myself.

So far I haven't found anything major.  There are 3 things I have to investigate whether or not they happen without the patch:
* lockscreen noise is late.
- need to check out if that happens prepatch.  minor consequence.
* wifi doesn't turn on and auto connect after reboot
- need to check out if that happens prepatch.
* Sim lock screen does not fill the whole screen.

I also have to checkout what happens on OTA.  The four main conditions in race I was concerned about is reboot/turning on, from sleep/deep sleep.  I turned a lot of things on and tried the two scenarios: alarm, NFC, BT, wifi, SIM, Geolocation, Firefox Accounts, screenlock (of course), lockscreen noise, and USB Storage.  I did the testing manually.  I also tried with sim lock on to test if the sim lock comes up.

side note : lockscreen notifications works even from deep sleep

Comment 18

[Naoki Hirata :nhirata (please use needinfo instead of cc)]

* lockscreen noise is late without the patch.
* wifi does autoconnect; I think this might be a fall out from the patch.
* sim lock isn't full screen without the patch either.

I think the only thing I see is that the wifi isn't autoconnecting.  There might be some race there?
If that's acceptable, then I would review+ this.

Comment 19

[Greg Weng [:snowmantw][:gweng][:λ]]

I don't think my patch is with such power to break Wi-Fi autoconnect... Is it an intermittent error or happened every time?

Comment 20

[Naoki Hirata :nhirata (please use needinfo instead of cc)]

Let me rebuild again and see if there's something else.

Comment 21

[Naoki Hirata :nhirata (please use needinfo instead of cc)]

You're right. It might have been the build or it might have been a state I can't reproduce yet.
review+

I think Mahe just needs to push this forward now then.

Comment 22

[Mahendra Potharaju [:mahe]]

Comment on attachment 8643459 [details] [review]
[gaia] snowmantw:bug1190079-2.2 > mozilla-b2g:v2.2

Thanks Naoki. 

Approved. Please uplift to 2.2 and 2.2R branches.

Comment 23

[Christiane Ruetten [INACTIVE]]

Comment on attachment 8643459 [details] [review]
[gaia] snowmantw:bug1190079-2.2 > mozilla-b2g:v2.2

Removing unsafe defaults is a good thing. So far the promise-based model works well on master, so there's nothing that speaks against a backport.

Comment 24

[Carsten Book [:Tomcat]]

https://github.com/mozilla-b2g/gaia/commit/aeef99e83f39fa3a3d51ae3a48989b5b5980d66a

Comment 25

[Ryan VanderMeulen [:RyanVM]]

This or bug 1181571 is causing Gaia unit test and Linter failures on v2.2.
https://treeherder.mozilla.org/logviewer.html#?job_id=169065&repo=mozilla-b2g37_v2_2
https://treeherder.mozilla.org/logviewer.html#?job_id=169064&repo=mozilla-b2g37_v2_2

Comment 26

[Ryan VanderMeulen [:RyanVM]]

Looks like the Linter failures went away on a later push once Bumper Bot got un-stuck, but the Gaia unit test failures remain.