1018836 - Lockscreen fails to respond for a few seconds

Status: RESOLVED FIXED

(Firefox OS Graveyard :: Gaia::System::Window Mgmt, defect, P1)

Description

[Lucas Adamski [:ladamski]]

Flame 2.0 20140601 (10G-2 base)

When trying to unlock the phone, the lockscreen is unresponsive for 2-3 seconds.  Sometimes it doesn't even render the lockscreen at all and you see just see the translucent overlay and background during that time.

Comment 1

[Lucas Adamski [:ladamski]]

Attachment: Blank unresponsive lockscreen

Comment 2

[Marcia Knous [:marcia]]

Using the Flame and:

Gaia   82679a5ce84d1b6bf388da6536d5682a3ad56de3
SourceStamp 6a984e21c2ca
BuildID 20140602072051
Version 32.0a1
10-G base

I still see the issue with the lockscreen being sluggish to open, although I haven't seen it not rendering the lockscreen at all. Adding the appropriate keywords to nail down when this regression started.

Comment 3

[Gregor Wagner [:gwagner]]

Thats a profile:
http://people.mozilla.org/~bgirard/cleopatra/#report=4008a136ed93e986d05a173235a06c09ddb1d046

We see 3 touch events where I try to use the slider but nothing happens.
Afterwards the transition to the homescreen is very slow.

Comment 4

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

I've bisect to hunt the similar symptom, which is the LockScreen would show a semi-transparent background of the homescreen. And here is the result:

450feb56670368032aafcca287418137d874b53e is the first bad commit
commit 450feb56670368032aafcca287418137d874b53e
Author: Alive Kuo <[e-mail]>
Date:   Wed Apr 16 15:56:03 2014 +0800

    Bug 950673 - Delay animationend event until active app loaded.


But I don't know whether this is the same bug. I'll do more research on it.

Comment 5

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

Attachment: Similar symptom

Comment 6

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

Attachment: Similar symptom

This is what I've bisected.

Comment 7

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

NI Alive because of the regression. Maybe we need a back out, because this is a regression.

Comment 8

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

Oh, and another reason I did the NI is because it now would make a flashing before the LockScreen got shown, and I don't know whether it's a correct way to show the LockScreen (which also 'hide' the semi-transparent homescreen, if it still exists), or it's just another bug (which means a bug hide another bug's symptom, and this means we'll have two bugs need to be solved).

Comment 9

[Gregor Wagner [:gwagner]]

Thanks Greg for the analysis! Please back out the patch that causes the regression if we can't find an immediate fix here.

Comment 10

[Jason Smith [:jsmith]]

Is this going to be needed on 2.0 after bug 950673 lands on 1.4? If so, please nom this for 1.4?

Comment 11

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

I've bisect that the blinking, and the responding delay was caused by

2aa5680b7013157ee1911cee4cefd7bec4151e91 is the first bad commit
commit 2aa5680b7013157ee1911cee4cefd7bec4151e91
Author: Etienne Segonzac <[e-mail]>
Date:   Wed May 28 12:53:15 2014 +0200

    Bug 1016483 - Stop doing the expensive #screen.edges restyle

Before it it's the old issue that there is a layout, which would show the semi-transparent homescreen before the window got opened, and it got disappeared after the patch landed, with the blinking effect and the responding delay. I'll take a video to show that and NI the author.

Comment 12

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

I've uploaded the video. There are two problems:

1. Flicking caused by the patch, or a semi-transparent layout after reverting it
2. Fail to response user after the window got opened

Comment 13

[Etienne Segonzac (:etienne)]

I'm having a look but this bug was filed before bug 1016483 landed, and I also encountered the issue before that...

So I'd like to understand exactly what is causing this and if/why bug 1016483 is making things worth.

Comment 14

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

I think the semi-transparent layout was not caused by the Bug 1016483, and need to bisect again. However, the "unresponding time" was getting obvious because there is a window opened and it seems already ready to be manipulated, which may be not true when user can see a semi-transparent homescreen before the lockscreen.

Comment 15

[Etienne Segonzac (:etienne)]

If you activate the "Flash repainted area" setting you can see that there is a full screen repaint just before the lockscreen finally becomes responsive.

Comment 16

[Etienne Segonzac (:etienne)]

Attachment: POC - opening the lockscreen when turning the screen off

I might be missing something but why aren't we opening the lockscreen app as soon as the screen is turned off?

This is helping a lot for the unresponsiveness issue, does it have unwanted side effects?

Comment 17

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

> I might be missing something but why aren't we opening the lockscreen app as soon as the screen is turned off?

I remembered I did it once but there were some issues pushed me to give it up. I may retry this idea and to see if it's a good idea to solve this.

Comment 18

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

Well it doesn't help. I've tested to do NO revert and just patch LWM as you suggested, but the unresponding time still occur. There would be two attachments to show that. I think the real problem is something change in the transition control, which now would not open the window after the animation like before (so the user would see an 'untouchable' LockScreen, but these are all my guests).

However I would also need to start to bisect to remove the 'semi-trasparent' homescreen first, which seems relevant with this regression.

Comment 19

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

Patch in LWM only: http://youtu.be/moI4sMJWLUY
Revert the patch: http://youtu.be/S8j59iWslro

Comment 20

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

I've bisected that the 'semi-transparent' layout is a regression by:

Author: Alive Kuo <[e-mail]>
Date:   Wed Apr 16 15:56:03 2014 +0800

    Bug 950673 - Delay animationend event until active app loaded.

As usual, NI the author to discuss how to solve the regression.

Comment 21

[Etienne Segonzac (:etienne)]

(In reply to Greg Weng [:snowmantw][:gweng][:λ] from comment #19)
> Patch in LWM only: http://youtu.be/moI4sMJWLUY
> Revert the patch: http://youtu.be/S8j59iWslro

If you turn on the screen right after you turned it off we don't benefit from loading the lockscreen app early.

But in the real life use case of turning the screen off, putting the phone in your pocket, and turning it on a bit later, then there is no delay.

Comment 22

[Lucas Adamski [:ladamski]]

Comment #16 echos what I was wondering.  FWIW I went back to a 5/29 build and so far haven't run into the issue.

Comment 23

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

(In reply to Lucas Adamski [:ladamski] (plz needinfo) from comment #22)
> Comment #16 echos what I was wondering.  FWIW I went back to a 5/29 build
> and so far haven't run into the issue.

As I mentioned, it's (according to what I've found) actually caused by two patches. So I don't know whether you mean that both two symptoms are all disappeared or not. I'll give it a try.

Comment 24

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

(In reply to Etienne Segonzac (:etienne) from comment #21)
> (In reply to Greg Weng [:snowmantw][:gweng][:λ] from comment #19)
> > Patch in LWM only: http://youtu.be/moI4sMJWLUY
> > Revert the patch: http://youtu.be/S8j59iWslro
> 
> If you turn on the screen right after you turned it off we don't benefit
> from loading the lockscreen app early.
> 
> But in the real life use case of turning the screen off, putting the phone
> in your pocket, and turning it on a bit later, then there is no delay.

I don't think the point is whether this is a real life case, especially the demo I put is also not a edge case. It's a regression, and what I can do is to find out which bugs cause it, just like people would do for other regression bugs. If I bisect the wrong bugs out, it's welcome to correct me and we can find out the real root cause.

Comment 25

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

I am moving this is window management because the regression happened on AppWindow and regressed LockScreenWindow.

Comment 26

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

Well, I don't have any quick idea and I am afraid I don't have much time on this right now, but
1. What if the animationend event is passed to lockscreenWindow always without delay?
2. I don't see what's the so-called semi-transparent homescreen during lockscreenWindow is opening..
but if you think something is above lockscreenWindow to block the user to touch it, try to use AppManager to see what it is. Maybe a z-index issue which was not found before.

And for sure if this is blocking something, bug 950673 could be backout-ed. But since we are all working on system app, try to find out the root cause and fix it will benefit than just blaming and throwing the ball back.

I know everyone is busy at this point, thank you all for all kind of effort.

Comment 27

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

No, I don't blame anyone. As I said, I just find out the regression. To back out or not to back out, and even who should fix, is not my main purpose, and it can be discussed with leaders who own the actual resources management duty.

The fact is, which in my opinion it's a very clear, the patch caused the regression, and no one has found it before the landing (no blaming). So we need to kill the regression as soon as possible, rather than to guess why I do this or "throwing the ball back".

Comment 28

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

And I don't know whether we have clear rules about who should fix the regressions except the author (when the author has no resource), or under what situation we can/can't back them out. In this case, I did avoid to directly back out the patch (as others did on my regressions) because it's a System patch, which may cause other regressions, but that doesn't mean the patch didn't break things (unless I bisect it incorrectly), or the bug doesn't important (it exactly block user experience).

Comment 29

[Mike Lee [:mlee]]

(In reply to Greg Weng [:snowmantw][:gweng][:λ] from comment #27)
> No, I don't blame anyone. As I said, I just find out the regression. To back
> out or not to back out, and even who should fix, is not my main purpose, and
> it can be discussed with leaders who own the actual resources management
> duty.
> 
> The fact is, which in my opinion it's a very clear, the patch caused the
> regression, and no one has found it before the landing (no blaming). So we
> need to kill the regression as soon as possible, rather than to guess why I
> do this or "throwing the ball back".


Tim,

Please may a call on whether to back this out. It's confirmed as causing a regression and needs to be fixed or backed out asap. This also needs an assignee.

Thanks,
FxOS Perf Triage

Comment 30

[Jason Smith [:jsmith]]

Greg - I'm confused about the bisection discussion above. Which bug is the root cause of this regression?

Comment 31

[Etienne Segonzac (:etienne)]

(In reply to Greg Weng [:snowmantw][:gweng][:λ] from comment #27)
> No, I don't blame anyone. As I said, I just find out the regression. To back
> out or not to back out, and even who should fix, is not my main purpose, and
> it can be discussed with leaders who own the actual resources management
> duty.
> 
> The fact is, which in my opinion it's a very clear, the patch caused the
> regression, and no one has found it before the landing (no blaming). So we
> need to kill the regression as soon as possible, rather than to guess why I
> do this or "throwing the ball back".

Finding a regression range is good, but still less useful that understanding the issue.

The fact is, we open the lockscreen app when turning the screen on, so there's always gonna be a delay.
We can work on making this delay shorter, but a setVisible(true) isn't free and there are many things that can cause the phone CPU to be under stress an make this even slower (check for updates, downloads, calendar syncing...).
The WindowManager can't guarantee an instantaneous app opening, even if it's warm, even if the app isn't OOP, but the LockscreenWindowManager is kind of based on this assumption right now.

When we turn the screen off we know we'll want to display the lockscreen next, why wait?

Comment 32

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

I think it's another issue as I said: the unresponding time wasn't exist even with the LWM without the pre-launching. However, it suddenly occur after some patches. From my point of view, your suggestion is to hide the symptom, not to eliminate it. In fact, this only leave the issue unsolved. 

I my opinion this bug can be solved by taking both actions:

1. kill the unresponding time and restore to the previous states, unless there is some other concerns
2. test and make sure the pre-launching really works and break nothing

I would need to take a detailed test to make sure the 2. really works. As I said, I remembered that I did it once, and something got broken, so I removed the code.

Comment 33

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

The bisect shows that

1. The Bug 950673 cause the 'semi-transparent' window before the LockScreen got show

In this case user can't manipulate LockScreen until the window got disappeared.

2. The Bug 1016483 hide it

In this cause user still can't manipulate the LockScreen, but now user can see LockScreen, not the strange window. This make things worse because the user would feel the window now is able to manipulate, even though it actually isn't as the case 1. shows.

Comment 34

[Etienne Segonzac (:etienne)]

(In reply to Etienne Segonzac (:etienne) from comment #31)
> The fact is, we open the lockscreen app when turning the screen on, so
> there's always gonna be a delay.
> We can work on making this delay shorter, but a setVisible(true) isn't free
> and there are many things that can cause the phone CPU to be under stress an
> make this even slower (check for updates, downloads, calendar syncing...).
> The WindowManager can't guarantee an instantaneous app opening, even if it's
> warm, even if the app isn't OOP, but the LockscreenWindowManager is kind of
> based on this assumption right now.

I'd really like this point to be addressed before we start ripping things apart. It's a fundamental issue which has many symptoms.

Comment 35

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

I suggest to open another bug to land the pre-launching in LWM, it can be a separate bug and be solved while the regression be handling.

Comment 36

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

Done. Bug 1020728

Comment 37

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

I recommend we try to fix the regressions than back out complex patches, in which 2.0 is depend on.

Comment 38

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

Gregor, could your team address this issue? This is a [systemsfe] bug.

Comment 39

[Gregor Wagner [:gwagner]]

(In reply to Tim Guan-tin Chien [:timdream] (MoCo-TPE) (please ni?) from comment #37)
> I recommend we try to fix the regressions than back out complex patches, in
> which 2.0 is depend on.

What patch is so complex that we can't back out? What caused the regression here?
I disagree that we should keep this regression without a clear plan and timeframe to fix it.

Comment 40

[Etienne Segonzac (:etienne)]

Hey Mason, do you think a profile could help us identify the issue(s) here.

If we're going to workaround the debatable design of the LockScreenWindowManager/LockScreenWindow by making app launch time slower I'd like to know exactly what we need to remove.

Comment 41

[Gregor Wagner [:gwagner]]

(In reply to Etienne Segonzac (:etienne) from comment #40)
> Hey Mason, do you think a profile could help us identify the issue(s) here.
> 
> If we're going to workaround the debatable design of the
> LockScreenWindowManager/LockScreenWindow by making app launch time slower
> I'd like to know exactly what we need to remove.

There is a cleopatra profile in comment 3. Is this what you need?

Comment 42

[Etienne Segonzac (:etienne)]

(In reply to Gregor Wagner [:gwagner] from comment #41)
> (In reply to Etienne Segonzac (:etienne) from comment #40)
> > Hey Mason, do you think a profile could help us identify the issue(s) here.
> > 
> > If we're going to workaround the debatable design of the
> > LockScreenWindowManager/LockScreenWindow by making app launch time slower
> > I'd like to know exactly what we need to remove.
> 
> There is a cleopatra profile in comment 3. Is this what you need?

Thanks Gregor, I might be misreading it (so keeping the ni? on Mason) but it looks like we're doing *nothing* during ~2sec which looks suspiciously similar to the OPENING_TRANSITION_TIMEOUT of the AppTransitionController.

I think we might have a very simple logical bug, I'd like to have a look before we start backing out stuff without understanding the issue.

Comment 43

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

(In reply to Etienne Segonzac (:etienne) from comment #42)
> (In reply to Gregor Wagner [:gwagner] from comment #41)
> > (In reply to Etienne Segonzac (:etienne) from comment #40)
> > > Hey Mason, do you think a profile could help us identify the issue(s) here.
> > > 
> > > If we're going to workaround the debatable design of the
> > > LockScreenWindowManager/LockScreenWindow by making app launch time slower
> > > I'd like to know exactly what we need to remove.
> > 
> > There is a cleopatra profile in comment 3. Is this what you need?
> 
> Thanks Gregor, I might be misreading it (so keeping the ni? on Mason) but it
> looks like we're doing *nothing* during ~2sec which looks suspiciously
> similar to the OPENING_TRANSITION_TIMEOUT of the AppTransitionController.
> 
> I think we might have a very simple logical bug, I'd like to have a look
> before we start backing out stuff without understanding the issue.

The opening transition for lockscreenWindow is immediate.
I am thinking about: what if we jump from closed to opened rightaway if we see the animation name is immediate?

Comment 44

[Etienne Segonzac (:etienne)]

(In reply to Alive Kuo [:alive][NEEDINFO!] from comment #43)
> (In reply to Etienne Segonzac (:etienne) from comment #42)
> > (In reply to Gregor Wagner [:gwagner] from comment #41)
> > > (In reply to Etienne Segonzac (:etienne) from comment #40)
> > > > Hey Mason, do you think a profile could help us identify the issue(s) here.
> > > > 
> > > > If we're going to workaround the debatable design of the
> > > > LockScreenWindowManager/LockScreenWindow by making app launch time slower
> > > > I'd like to know exactly what we need to remove.
> > > 
> > > There is a cleopatra profile in comment 3. Is this what you need?
> > 
> > Thanks Gregor, I might be misreading it (so keeping the ni? on Mason) but it
> > looks like we're doing *nothing* during ~2sec which looks suspiciously
> > similar to the OPENING_TRANSITION_TIMEOUT of the AppTransitionController.
> > 
> > I think we might have a very simple logical bug, I'd like to have a look
> > before we start backing out stuff without understanding the issue.
> 
> The opening transition for lockscreenWindow is immediate.
> I am thinking about: what if we jump from closed to opened rightaway if we
> see the animation name is immediate?

Sounds promising I will try this right away :)

Comment 45

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

I made a patch before I saw your lasted comment.

Comment 46

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

Attachment: skip open/close states if animation name is immediate

I just figure out the stuff prevents us to touch the lockscreen is div.touch_blocker. It's visible during transition opening.
What we should do is just skip the opening/closing state if animation name is immediate.
Set feedback since no tests for now.

Comment 47

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

Have a patch to fix the root cause, stealing. Making unit tests.

Comment 48

[Etienne Segonzac (:etienne)]

Comment on attachment 8434780 [details]
skip open/close states if animation name is immediate

Love it!
Works like a charm and benefits from the appTransitionController instead of just shortcuting it.

Let's add some quick tests and land it :)

Comment 49

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

Attachment: https://github.com/mozilla-b2g/gaia/pull/20055/files

patch + simple tests.

Comment 51

[Etienne Segonzac (:etienne)]

Comment on attachment 8434804 [details]
https://github.com/mozilla-b2g/gaia/pull/20055/files

All good!
Small question about the fakeTimers removal but travis will tell :)

Comment 52

[Etienne Segonzac (:etienne)]

https://github.com/mozilla-b2g/gaia/commit/0130f0ebfd3deca45c1d5663332221020e698c57

Comment 53

[Gregor Wagner [:gwagner]]

Thanks guys for the quick fix!