898348 - (lockscreen-as-app) Isolating the lockscreen as a separate application

Status: RESOLVED WONTFIX

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

Description

[Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please)]

Multiple sources have expressed interest in replacing the lockscreen or adding more custom data on it.

Also in a world where Gaia will target multiple really different kind of devices it will make it harder and harder to implement the correct layout for all those.

Moving the lockscreen as a separate application with allow third party to install their own custom lockscreen. It will also ensure that third party data does not run into the main thread of the main process and have priority over vital tasks such as receiving a phone call.

I see that as a 2 steps process:
 - isolate the lockscreen app inside the system app as an iframe in order to isolate the dependency with the system app.
 - Move it to a separate app.

I assume that such a process will force us to resolve some issues such as displaying notifications on the lockscreen which can help to answer some questions about how third party can provide data for the lockscreen too.

Data Store can be an answer - this is not clear to me yet.

Comment 1

[Sergi Mansilla [:sergi] (Telenor) [PTO]]

Progress on this issue can be seen at https://github.com/comoyo/gaia/compare/lockscreen_decouple

Comment 2

[Sergi Mansilla [:sergi] (Telenor) [PTO]]

Continuing in new branch. Updated and synched to latest master at: https://github.com/comoyo/gaia/tree/lockscreen_decouple2

Comment 3

[Sergi Mansilla [:sergi] (Telenor) [PTO]]

Attachment: Github branch diff where this issue is implemented

I'd like some feedback on whether this is the desired approach for the first step, which is to decouple lockscreen in its own iframe. Right now, the lockscreen communicates actions such as 'locked', 'unlocked' or 'enabled' through `window.postMessage`, and it uses the parent window objects, such as `ICCHelper` and others.

Other approaches could be to do everything via postMessage (I don't think it makes sense for the static objects that we can import) or have lockscreen's HTML page load these scripts (fine for now, but more complicated in case we want to enable third-party lockscreens eventually).

Comment 4

[Tim Guan-tin Chien [:timdream] (please needinfo)]

Comment on attachment 792725 [details]
Github branch diff where this issue is implemented

Not using postMessage() at this stage might be the simplest approach at this stage. Also, we should avoid having a white flash or see-through flash while the iframe is loaded (hint: listen to iframe.onload and handle it async). I will land a patch like that as the first step.

After that, we could explore app (even 3rd-app) approach in the future.

BTW, it doesn't really make sense to use postMessage within the same app. I don't see changing the code to use postMessage API will take us anywhere.

Comment 5

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

Would we have to keep the lockscreen app alive when the phone is unlocked?  Apps (and the accompanying process isolation) are expensive in terms of memory usage.

Comment 6

[Sergi Mansilla [:sergi] (Telenor) [PTO]]

Updated PR to latest lockscreen changes.

Comment 7

[Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please)]

(In reply to Kyle Huey [:khuey] ([e-mail]) from comment #5)
> Would we have to keep the lockscreen app alive when the phone is unlocked? 
> Apps (and the accompanying process isolation) are expensive in terms of
> memory usage.

The goal is to not keep it alive.

Comment 8

[Sergi Mansilla [:sergi] (Telenor) [PTO]]

Attachment: Updated PR to not use Post message and to make lockscreen visible only when loaded.

Comment 9

[Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please)]

Comment on attachment 809826 [details]
Updated PR to not use Post message and to make lockscreen visible only when loaded.

Let's have this goes through the last person that has touch the lockscreen

Comment 10

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

Comment on attachment 809826 [details]
Updated PR to not use Post message and to make lockscreen visible only when loaded.

Since ver1.2's lockscreen is still in the progress of enhancement, include Bug 919854, Bug 919858, Bug 919410 and Bug 919850, I don't know whether 's a good idea to refactor the lockscreen like that at this stage. However, I'm 100% agree that the lockscreen can be a standalone app. 

Maybe after this patch landed I just need to solve the foreseeable conflicts each time I solved the bugs listed above, and I hope it's not too tough.

Comment 11

[Sergi Mansilla [:sergi] (Telenor) [PTO]]

I can keep an eye on these issues and keep updating the new lockscreen, I've been doing it for the last weeks. I'll add myself as a viewer to these bugs.

Comment 12

[Sergi Mansilla [:sergi] (Telenor) [PTO]]

Synced with 0ee3a340830895f8571c0a1f16971d586605d989.

Comment 13

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

Hi Sergi,

The current lockscreen implementation is nearly done. The next major change would be rewritting it with canvas or other methods that can completely eliminate the vertical bands problem. So in my opinion that switching from the current implementation to a standalone lockscreen app would be possible in near future, though I still need to discuss this with others. However, thanks for your efforts again.

Comment 14

[Sergi Mansilla [:sergi] (Telenor) [PTO]]

Hi Greg,

Great! It would be good if you can give me a heads up for when I can sync the standalone with your latest changes again, and get the standalone version integrated.

Thanks,

Sergi

Comment 15

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

According to the progress, I may announce a WIP version of canvas implementation today, and it would be in the Bug 919410.

This version currently still keep the legacy code, so I would need some extra time to refactor it.
After that, I'll needinfo you to start our switching.

Comment 16

[Nikhil Marathe [:nsm] (No longer reading bugmail, please needinfo?)]

(In reply to Greg Weng [:snowmantw] from comment #15)
> According to the progress, I may announce a WIP version of canvas
> implementation today, and it would be in the Bug 919410.
> 
> This version currently still keep the legacy code, so I would need some
> extra time to refactor it.
> After that, I'll needinfo you to start our switching.

Are we implementing lockscreen using Canvas? That will make customization harder for apps that want to show info on the lockscreen.

Comment 17

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

Yes. Due to the glitches problem mentioned in the Bug 919410, DOM elements with CSS animation can't satisfy our request. However, the slide would be the only one component impleted with Canvas, other parts are still plain DOMs.

Comment 18

[[:fabrice] Fabrice Desré]

Do we a bug filed to fix the underlying gfx bug instead of doing a canvas-based bandaid?

Comment 19

[Tim Guan-tin Chien [:timdream] (please needinfo)]

(In reply to Fabrice Desré [:fabrice] from comment #18)
> Do we a bug filed to fix the underlying gfx bug instead of doing a
> canvas-based bandaid?

It would be helpful if you file that bug -- it would be much more descriptive.

Comment 20

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

Hello! I'm glad to announce that the Canvas implemented lockscreen has been landed. So we may start to make lockscreen as an individual app.

Maybe Sergi could take a look with the new version and to see how to make the new lockscreen as app?

Comment 21

[Sergi Mansilla [:sergi] (Telenor) [PTO]]

Good job Greg!

I will take a look to porting the new lockscreen to its new app tomorrow, cheers!

Comment 22

[Dietrich Ayala (:dietrich)]

Hi Sergi! have you had a chance to work on this?

Comment 23

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

I want to refactor the current lockscreen.js before we make it as an app. So I set a dependency here.

Comment 24

[Sergi Mansilla [:sergi] (Telenor) [PTO]]

Hi Dietrich, yeah I started working on it, but looks like it keeps getting postponed. I thought that after the move to canvas it was all clear to work on it again but now Greg mentions a refactor of the code...should I stop working on it then? Part of my move to an app included getting rid of dependencies, like mentioned in 960381.

Comment 25

[Sergi Mansilla [:sergi] (Telenor) [PTO]]

Also, can't we work on parallel with this? We could define responsibilities for each bug and gain time this way.

Comment 26

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

If we can start irrelevantly, it could be parallel. For example: we start from an empty 'lockscreen' app, and solve the communications issues like how to notify unlock, lock, will-unlock and other current events to IAC ways. Meanwhile I keep refactoring the current lockscreen. And after we finish our work individually, the new vanilla lockscreen app, can keep designation as clear and flexible enough as possible, so the refactored components can be plug into the new lockscreen app in a reasonable way.

However if we want to work like this, we must ensure our work really loose-coupling as much as possible. Or if you invent some components I'm refactoring, we would get in trouble due to the conflicts.

Comment 27

[Teiichiro Fukuda]

Hi Greg,

I have a question.

Can a lock screen set the lock screen application that a general developer developed?

for example:
I set the lock screen application that I downloaded from market place to a lock screen.

Thanks,

Teiichiro

Comment 28

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

This is one goal we want to achieve via this bug (so currently you can't do this). But as you can see, the refactoring is the first step. If we bring the current architecture, which has enough its own problems, with the potential bugs in the progress of making it as an app, we would get in big trouble.

Comment 29

[Sergi Mansilla [:sergi] (Telenor) [PTO]]

I won't have time to work on this anytime soon, has been postponed for too long. Unassigning it from me.

Comment 30

[Sergi Mansilla [:sergi] (Telenor) [PTO]]

I won't have time to work on this anytime soon, has been postponed for too long. Unassigning it from me.

Comment 31

[(inactive after 6/18) Alive Kuo [:alive]]

We need lockscreen-window to launch the lockscreen app as what we did in homescreen.

Comment 32

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

Attachment: Patch

I almost completed this bug. Since this bug is relevant to System and LockScreen, I may set 2 reviewers (Alive & Tim).

Comment 33

[Paul Theriault [:pauljt] (no longer reading bugmail)]

(In reply to Greg Weng [:snowmantw][:gweng][:λ] from comment #32)
> Created attachment 8410892 [details] [review]
> WIP Patch
> 
> I almost completed this bug. Since this bug is relevant to System and
> LockScreen, I may set 2 reviewers (Alive & Tim).

We should think about the security model here - if we want third-party lockscreens, we need them to be privileged or lower. And So we should eventually be changing the way we validate the passcode, so that this still happens in the system app (parent), not in the lockscreen (child).

Maybe this is a seperate bug, and we might even want a Gecko API for this purpose instead of IAC (do we don't have a permission model for IAC for privileged apps yet? Not that I am aware of).

Comment 35

[Preeti Raghunath(:Preeti)]

I agree you about the secure module. However, I don't think IAC, or cross-app XHR, can be missing in the new and privileged LockScreens. If we allow privileged LockScreens, we can't restrict developers that they can only do something in our thoughts (for example, partner's LockScreens), so a generic cross-app message passing is still necessary. 

Yes we have no privileged IAC and it would not happen, but an idea of cross-app XHR/WebSocket instead. The discussion is here:

https://groups.google.com/forum/#!topic/mozilla.dev.webapi/Cl-9vXI1Gfc

Comment 36

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

Comment on attachment 8410892 [details] [review]
Patch

I think it's better to start the reviewing process as soon as possible. Since the tests now would only fail at two irrelevant tests, I set the review flag now.

The tests are:

1. System unit test would fail at the bootstrap_test:143, it would even fail at the newest master, so it's irrelevant

2. Marionette test would fail at the Settings app, and it's irrelevant because the panel is totally irrelevant to LockScreen, the last screenshot shows no related UI changes, and the Settings app failed randomly before (different panels).

I would send a PR based on the master to see if it would pass the test, and try to pin down the root cause of these two tests. However, I think reviewing should not be blocked by these two failed tests.
---

And I don't want to change the original LockScreen, including the events it would fire and receive. This is because I want to keep things work fine as before, so these further changes should be done at the following bugs:

* To change the unlock, will-unlock and lock events to lockscreenwindow-close, closing and open.

* To fix performance and visual regressions

Comment 37

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

The Travis with two failed tests:

https://travis-ci.org/mozilla-b2g/gaia/builds/24942066

Comment 38

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

Comment on attachment 8410892 [details] [review]
Patch

Since this patch modify both System and LockScreen app, I set two reviewers.

Comment 39

[(inactive after 6/18) Alive Kuo [:alive]]

Comment on attachment 8410892 [details] [review]
Patch

Commented.

Comment 40

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

Comment on attachment 8410892 [details] [review]
Patch

Fixed addressed issues.

Marionette still random timeout at different 'before each' stages. I'm trying to pin down the root cause. Unfortunately, I currently haven't found the link between those tests and the patch.

The JSHint xfails is actually not increased: they're just moved old file from System app to shared and LockScreen app. I'll fix all of them after the LockScreen 2.0 visual refresh, and we may even no need to fix them because I want to refactor most of these files.

Comment 41

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

I'm now testing the Travis so temporarily reverted the commit.

Comment 42

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

The debug messages shows that in Settings, the panel entry would be tapped, but the animation would not perform:

https://travis-ci.org/mozilla-b2g/gaia/jobs/25050472
https://github.com/snowmantw/gaia/commit/ab371bfab22d1f54a0a68a0ada95f205c628eec5

Next I would check:

a. If other random fails are all caused by the missing animation, although they're definitely in the different apps.

b. Disable some parts of my patch in the System app step-by-step, and re-run the test every time. This may help to pin down the root cause, but needs lots of time to make sure the disabling would not break the system.

Comment 43

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

This is the log with screenshots. It shows that when the test failed, the entry would be tapped, but the animation would not perform.

https://travis-ci.org/mozilla-b2g/gaia/jobs/25062965#L3351

Comment 44

[Tim Guan-tin Chien [:timdream] (please needinfo)]

Comment on attachment 8410892 [details] [review]
Patch

I don't think I can do the review effectively remotely. Let's do this next week and have the scope clear on what is written and what is simply copied.

Comment 45

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

The integration tests would pass if I pull a new Gaia repo from mozilla-b2g, apply my patch on it, and then push it to my own repo:

https://travis-ci.org/snowmantw/gaia/jobs/25656362

However, to test it on the original PR, it would still fail with the random timeout errors:

https://travis-ci.org/mozilla-b2g/gaia/jobs/25657782

This means:

1. To test it with the PR to my own repo, namely, test on my own Travis session would pass
2. To test it with the PR to the mozilla-b2g's repo, would *NOT* pass, but with only timeout errors, not real errors

Comment 46

[Tim Guan-tin Chien [:timdream] (please needinfo)]

Comment on attachment 8410892 [details] [review]
Patch

Discussed offline and some comment have left on Github. We are getting close!

Comment 47

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

Tested with the PR to the Mozilla repo and there is only 1 failure again.

Comment 48

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

I've fixed what Tim addressed, but I found that I can't fix a11y UI tests to make work, since I really don't know how to make them works. So I may fire another bug for this and CC some persons to fix them after the patch has been landed.

And the marionette test failures may shows the tests are unstable and it's irrelevant to the patch (since it passed once today, and has passed 5+ times at 5/16~5/17). So I would fire another bug to see what can we do for that as well.

The System app has one unit test failed that is about the missing 'AttentionScreen' in the bootstrap_test.js. This happens even on newest master, so I think my patch can land even with that error.

Comment 50

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

Comment on attachment 8410892 [details] [review]
Patch

Updated the patch and now is waiting Travis. I set the review flag first and will fix all stable tests if they fail.

Comment 51

[Tim Guan-tin Chien [:timdream] (please needinfo)]

Comment on attachment 8410892 [details] [review]
Patch

Please make sure the CI tests are stable enough and we are not breaking any device automation tests before landing. Thanks! Nice work!

Comment 52

[Tim Guan-tin Chien [:timdream] (please needinfo)]

Additionally, if making the lock screen inproc in this bug can fix these tests first, let's do that in this bug and move lock screen oop to the next bug.

Comment 53

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

Travis now is all green:

https://travis-ci.org/mozilla-b2g/gaia/builds/26103814

TBPL failed at a test should be already disabled:

https://tbpl.mozilla.org/php/getParsedLog.php?id=40438984&tree=Gaia-Try

And a platform issue:

https://tbpl.mozilla.org/php/getParsedLog.php?id=40439098&tree=Gaia-Try

Comment 54

[howie [:howie]]

move to 2.1

Comment 55

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

After Evan's recent patch to MarionetteJS client, we finally could confirm that the timeout now are all caused by the two windows both with 'trasntion-state: opened'. That is, it seems in the '#windows', we would have two opened window, and the LockScreenWindow may overlap on the foreground app. I'll start to check why this happen.

Comment 56

[Evan Tseng [:evanxd]]

After disabling lockscreen app, all marionettes tests except lockscreen related tests could be passed on Travis[1]. If we fix the overlapping issue, then the marionette_js job could be passed. Greg is working on the issue. So let's close Bug 1013883.

[1] https://travis-ci.org/snowmantw/gaia/builds/26356030

Comment 57

[Evan Tseng [:evanxd]]

We could disable lockscreen and ftu in gaia profile by default. We already have a patch for that in Bug 1018081.

Comment 58

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

Since we'd missed the best merging window (before UI refresh and 2.0 branch out) because of the delay caused by testing issues, I've found that I can't merge it without any possible regressions after I tried to solve the serious conflicts unsuccessfully. The new strategy would be breaking down the patch and land them one-by-one. This would introduce some temporary workarounds to make sure the old code can work with new parts.

I would keep this bug as a tracking bug, and to gather those depending bugs on this bugs.

Comment 59

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

After the discussion, we want to reduce the risks as much as possible. So I would take several steps in the following bugs and update the dependencies.

Comment 60

[Kai-Chih Hu [:khu]]

Howie, can you update the target milestone? Thanks.

Comment 61

[howie [:howie]]

Hi Greg, please update the milestone accordingly ,thanks.

Comment 62

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

Don't know how many bugs need to be resolved at this stage, but I would give a most loose one if this is the necessary action to track a bug. Hope this would not make the milestone become meaningless.

Comment 63

[Dietrich Ayala (:dietrich)]

Is this feature still under development, or not active anymore?

Comment 64

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

Working on Bug 1057198. Sorry I forget to update this bug.

Comment 65

[sgiles]

Hey, can you update the status here?  I'd like to help get this moving.

Comment 66

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

The history: I've got test troubles when I tried to make the app worked as the current LockScreen, and that happened in the last month of v2.0, so I had no choices but to give up the WIP patch almost worked well, which could be launched, ran and even restarted as an app on my device. Unfortunately, it broke 90+ integration tests because people used to test their marionette tests without handling the LockScreen properly. In fact, they dealt with that in a very nasty way. And after I finally fixed that, the next trouble was there were some wholly irrelevant unit tests failed because of my patch, and I didn't have time to fix that, since I still needed to handle 2.0 features before FC.

And the second try was in the long progress of v2.2. This time, I almost successfully achieved the goal . Nevertheless, CI server tricked me again. There were some weird CI-only failed tests blocked the patch, and I also encountered some serious graphic and responsible issues on my device. Moreover, I was still an one-man army that need to handle all the release features, architecture changes and blockers while some of them were even from other irrelevant bugs, therefore I had no chance to fix these issues and landed the patch.

Now we're in the end of v2.5, and what I can help on this thing is to tell you currently the possible difficulties and the clash with new features are:

1. Bug 1094759 used lots of LazyLoad and the so-called 'base modules' technologies to solve bootstrapping dependencies and boost the performance. However, it also buried lots of possible racing defects that the code in LockScreen need to manage. I worry that if LockScreen becomes an app, and therefore the communication between System and the app become asynchronous, those racing issues might become worse. 

2. There are some polyfills to imitate how LockScreen should work as an app while it isn't. For example, the LockScreen 'frame', which is in fact a div, is being added with some dummy methods to support WindowManager can handle it as other iframe based windows. If it now becomes an app, some of these polyfills may be the obstacles.

3. The legacy NotificationScreen and other old components may still treat LockScreen as a reachable component, so the new patch need to handle that, as what I've done but finally failed to land.

4. CI server. It's the root of all troubles, although sometimes the new infrastructure improvements may ease the pain, and solve some issues implicitly, but I would not bet on that.

5. The 'new' statusbar would cause lots trouble since it's very hard to get understand how it works now, especially you may find there are some *CSS dependencies* across the LockScreen, screen and statusbar.

6. Since we encountered serious security issue exposed by Bug 1094759, another performance issue of mozSettings and LockScreen has been exposed. To solve that, Paul and me planned to implement a new DeviceLock API (Bug 1199100), which will seriously change the way to control and prob the status of LockScreen. If you want to pull LockScreen as an app now, these two planes need to be coordinated very carefully to prevent we 'trash' each other's patch  (a term that someone indeed threw on me, when I tried to do some things to decouple notifications and LockScreen).

7. Since LockScreen input pad (the passcode keypad) is living in another frame (DIV again), an LockScreen in iframe will need to notify it the related status changes by IAC or other ways. This may cause lots of troubles especially there might be some animations need to be synced.

To sum up, I will not say it's impossible, but please understand I now don't have enough resources to finish the plan (again), especially the API issue is critical for performance. However, I actually have another plan to avoid these issues, which is

A. We could start from another very simple ScreenLock app: only the slider and a input bar for password, so it keeps working on the two major unlocking functions. And since it is an app from the beginning, there are no nasty and legacy code need to be maintained. It also covers the need of raising real keyboard while inputting the password, so we can also improve our security in that way.

While in this stage, no matter whether the new DeviceLock API get implemented, we can at least guarantee we get rid of the legacy event-patching mechanism with new asynchronous messages. Another advantage of this is we will at least have a screen lock as an app very soon, and gain the benefit of security and what B2GDroid without any regression risks.

B. The app should be included as a certified app but not replace the current LockScreen by default. Instead, we can make a new build flag to set |NO_LOCKSCREEN=1| to disable the old one, and |SCREEN_LOCK=1| to launch the new lock at bootstrapping automatically. In this way, we can separate the ordinary tests and the new lock only tests.

To add a build flag like this to switch the lock maybe also needed by B2GDroid: we could switch to the new lock app on B2GDroid, while it's simple enough to be implemented in a short term, and can be switched again on the ordinary FFOS build.

C. Then, we add features we need on the new lock **very carefully**. It's also a proper time to replace the old and messy "widgets" with the new component based system we have discussed and tried in Bug 1115311. I would like to invite UX to review the whole screen lock design at this stage.

E. And after we finish all of these work, we may enable the new lock app and deprecate the old one.

This is the reasonable but of course very long way to make the things work without the risks I encountered and mentioned. However, it could be a more solid way to gain a LockScreen *app* since we can avoid the tough issues listed above, compare to work on the current LockScreen.

And if you want to discuss more about this with me, we may arrange a video meeting for that.

Comment 67

[[:fabrice] Fabrice Desré]

I agree with starting from scratch on a simpler lockscreen app used only on b2gdroid at first. Greg, would you have time to help on that?

Comment 68

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

Okay, I think I can list what we at least need to do:

1. Add a new flag for the new screen lock, and a mozSetting value indicates to the new URL
2. Start to create the new app
3. Weld the new app in the path of bootstrapping, and there should be a switcher knows which one should be launched

And then to modify the build config in B2GDroid to make it only use the new screen lock. Other steps could be postponed until there are no other more critical tasks need to be done.

I may be able to finish 1. and 2. (as a demo), but I don't know if I can finish all the tasks. So if :sgiles want to help, maybe we can discuss about how to make the progress collaboratively.

Comment 69

[BMO Automation]

Firefox OS is not being worked on