880609 - [System] WakeLock needs to be released when app switching

Status: RESOLVED FIXED

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

Description

[Leo]

We need a solution for a case when an app acquired screen WakeLock by requestWakeLock('screen') and does not unlock it. This can cause the device to have the screen ON forever.

Mozilla build ID: 20130518070206
Gaia Revision : 9380ceb81b3eac45861b8d1be07ab7f748ed52a3
Personal email id:  [e-mail]

Suggestion: 
When homescreen is brought to foreground, we cancel the screen WakeLock. Between app switching, we store the screen WakeLock state for the app before hiding. And restore it when the app is brought to foreground.

Comment 1

[Leo]

Alive, what do you think?

Thanks

Comment 2

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

I am confused. What's the real use case here? Is there any existing code make screen turn on forever?

Comment 3

[Leo]

There is an existing code causing the problem.

https://bugzilla.mozilla.org/show_bug.cgi?id=871472
https://github.com/mozilla-b2g/gaia/pull/9717/files

Having the patch above, screenTimeout function is the only function that calls screenLock.unlock();. And the if condition is not satisfied when Home key is pressed when Camera app is on foreground. Therefore it locks the device screen ON forever..

Comment 4

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

(In reply to Leo from comment #3)
> There is an existing code causing the problem.
> 
> https://bugzilla.mozilla.org/show_bug.cgi?id=871472
> https://github.com/mozilla-b2g/gaia/pull/9717/files
> 
> Having the patch above, screenTimeout function is the only function that
> calls screenLock.unlock();. And the if condition is not satisfied when Home
> key is pressed when Camera app is on foreground. Therefore it locks the
> device screen ON forever..

So the problem should be: 'Why camera doesn't get visibilitychange when back to home'?

Comment 5

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

(In reply to Alive Kuo [:alive] from comment #4)
> (In reply to Leo from comment #3)
> 
> So the problem should be: 'Why camera doesn't get visibilitychange when back
> to home'?

The "visilibilitychange" event should be recieved by app correctly besides it's an inline activity frame. Otherwise, we should fix the incorrect visibility state, instead of releasing the wake lock for app. App should do this by itself.

I don't know what will happen if an app is crashed without releasing wake lock. But this should be manipulated in gecko when the window is removed IMO.

Comment 6

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

Leo, this is good catch.
I'm c.c.ing some platform guys to this, though I still don't think we could solve this from gaia, because bothg gaia/gecko won't know who(which frame) request the wake lock, so your proposal doesn't work..

Comment 7

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

:kanru told me that https://mxr.mozilla.org/mozilla-b2g18/source/hal/HalWakeLock.cpp#126 will handle(release wake lock): crash, close...cases when app window is removed.

So the only problem: where do you see the visibilitystate isn't fired?
https://github.com/leob2g/gaia/blob/7d17edae81d0821e65d53dd6007a22dff02e257f/apps/camera/js/camera.js#L1081

This handler will go to release wake lock, isn't it?

Comment 8

[Leo]

(In reply to Alive Kuo [:alive] from comment #4)
> (In reply to Leo from comment #3)

Camera app does get visibilitychange when back to home. The problem in this patch was the following 'if' condition. (returnToCamera was true when back to home.) Therefore it wasn't calling 'screenLock.unlock();'.

if (screenLock && !returnToCamera) {


Now, what we want to do is that we want to cover those simple mistakes in system by canceling the wake lock when minimized and restoring it when re-opened.

Comment 9

[Leo]

(In reply to Alive Kuo [:alive] from comment #7)

We have observed the wake lock is released when closed. But we think it's not enough. How about we release the lock when minimized until it is reopened?

Comment 10

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

(In reply to Leo from comment #9)
> (In reply to Alive Kuo [:alive] from comment #7)
> 
> We have observed the wake lock is released when closed. But we think it's
> not enough. How about we release the lock when minimized until it is
> reopened?

If there's no special reason to keep the wakelock in background, this looks a camera app bug to me.

Leo, again, that, I*system app don't "who" request the wake lock, so if a background app is requesting the wake lock, I couldn't tell. Do you know what I mean? It's a global API not app specific.

Comment 11

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

Dale, see comment 10. Could you specify what's the reason the keep wake lock when camera app in background? thanks.

Comment 12

[Leo]

> (In reply to Alive Kuo [:alive] from comment #11)
> Dale, see comment 10. Could you specify what's the reason the keep wake lock
> when camera app in background? thanks.

This is a regression issue caused because of linking camera wakelock to fimlmstrip preview.

Here is the reason because of the camera wakelock runs even when camera app is minimized.
1. Wakelock in camera app will be created when camera preview starts
2. when any item in filmstrip is selected, it opens the preview and wakelock will be unlocked.
3. And when camera visibilty change occurs it stops camera preview and releases the wakelock. Here comes the problems: before releasing the wakelock it checks a condition (screenLock && !returnToCamera). This conflict condition which holds the camera wakelock.

I have a solution for this. I will upload the patch by regestering a new bug.

Comment 13

[Kan-Ru Chen [:kanru] (UTC+9)]

This might be related to bug 809671. Could you test this patch https://hg.mozilla.org/mozilla-central/rev/191155ed17b0 and see if it fix this problem?

Comment 14

[Leo]

(In reply to Alive Kuo [:alive] from comment #10)

Yeah, it is a camera app bug and we have a fix for it. However, we want more than just fixing the camera app bug..

so you mean, the WakeLock status of an app when going to background might not be the status of the specific app, since the requestWakeLock is a global API?

p.s. The Leo comment right above was written by someone else. 

Hanjkim

Comment 15

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

Just confirmed with :kanru,
If app A(camera) request wake lock--->we couldn't unlock it in app B(system).

Comment 16

[Leo]

(In reply to Kan-Ru Chen (:kanru) (Mozilla Corporation) from comment #13)
> This might be related to bug 809671. Could you test this patch
> https://hg.mozilla.org/mozilla-central/rev/191155ed17b0 and see if it fix
> this problem?

I have applied your gecko patch. Tested with camera app and it is working.

Comment 17

[Dale Harvey (:daleharvey)]

Clearing needinfo as Leo answered it in #11

Comment 18

[hanj.kim25]

(In reply to Leo from comment #16)
Let me verify one more time.

Comment 19

[hanj.kim25]

(In reply to Kan-Ru Chen (:kanru) (VAC Jun 10-12) (Mozilla Corporation) from comment #13)
> This might be related to bug 809671. Could you test this patch
> https://hg.mozilla.org/mozilla-central/rev/191155ed17b0 and see if it fix
> this problem?

I have tested the camera app with the patch and observed a new behavior. the screen goes off in step 5 below. However the screen also goes off when it shouldn't in step 9.

steps to repro:
1. Launch Camera app.
2. Press power button to screen-off.
3. Press power button to screen-on.
4. Unlock lockscreen.
5. Press home button.
   * observed the screen goes off in step 5.

6. Hold home button and select camera app in cardview.
7. Press power button to screen-off.
8. Press power button to screen-on.
9. Unlock lockscreen (the camera app is on foreground.)
   * screen also goes off when it shouldn't

The patch has a problem, right?

Comment 20

[Kan-Ru Chen [:kanru] (UTC+9)]

I also observe the strange behavior in comment 19 step 9. I could confirm that when in step 9 there are two foreground "screen" wake lock. @alive could you find out if we handle the wake lock events correctly in system app?

Comment 21

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

This looks to me another camera app bug that it doesn't unlock and request another lock when it's brought from background to foreground.

Comment 22

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

What happens:

(In reply to hanj.kim25 from comment #19)
> (In reply to Kan-Ru Chen (:kanru) (VAC Jun 10-12) (Mozilla Corporation) from
> comment #13)
> > This might be related to bug 809671. Could you test this patch
> > https://hg.mozilla.org/mozilla-central/rev/191155ed17b0 and see if it fix
> > this problem?
> 
> I have tested the camera app with the patch and observed a new behavior. the
> screen goes off in step 5 below. However the screen also goes off when it
> shouldn't in step 9.
> 
> steps to repro:
> 1. Launch Camera app.

returnToCamera: true
visibilityState: true

> 2. Press power button to screen-off.

visibilityState: false

> 3. Press power button to screen-on.

visibilityState: false

> 4. Unlock lockscreen.

visibilityState: true

> 5. Press home button.

visibilityState: false
=> unlock doesn't happen because returnToCamera=true.
=> But gecko knows the camera is in background so the wakelock state is background.

>    * observed the screen goes off in step 5.
> 
> 6. Hold home button and select camera app in cardview.

visibilityState: true

> 7. Press power button to screen-off.

visibilityState: false

> 8. Press power button to screen-on.

visibilityState: false

> 9. Unlock lockscreen (the camera app is on foreground.)

visibilityState: true
=> Request another screenlock in 
```js
screenWakeLock: function camera_screenWakeLock() {
    if (!screenLock && returnToCamera) {
      screenLock = navigator.requestWakeLock('screen');
    }
  },
```

So there're 2 wake lock of screen in camera app now.

>    * screen also goes off when it shouldn't
> 
> The patch has a problem, right?

Comment 23

[Kan-Ru Chen [:kanru] (UTC+9)]

(In reply to Alive Kuo [:alive] from comment #22)
> So there're 2 wake lock of screen in camera app now.

Yes, that matches what I saw. However those two wake locks are active & foreground, why system app still turns off the screen?

Comment 24

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

(In reply to Kan-Ru Chen (:kanru) (Mozilla Corporation) from comment #23)
> (In reply to Alive Kuo [:alive] from comment #22)
> > So there're 2 wake lock of screen in camera app now.
> 
> Yes, that matches what I saw. However those two wake locks are active &
> foreground, why system app still turns off the screen?

Yes..this is a bug in system/screen_manager.
And before the gecko bug is fixed(the screen wake lock is kept even the app is backgrounded), nobody would see this.

The simpler STR:

1. Open camera
2. Press power key
3. Press powey key again:
   Screen manager turns screen on and set a short idle timer because lockscreen needs a shorter idler timeout.
4. Unlock the lockscreen
   Now the visibility of camera is true. Therefore it request a wake lock.
   System app sees wakelock event with state="locked-foreground"
5. Unlock transition ends:
   The unlock event triggers the short idle timeout callback in step 3 and override the idle timer of step (4) discard of screen wakelock.

The solution may be to cancel the timer registered in step 3 in step 4.

Comment 25

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

leo? since the screen wake lock of camera app is failed after screen is off and on.

Comment 26

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

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

Patch proposal: Use will-unlock event to set short idle timer.
Leo, please check if this solves the problem(I tested and the wake lock works after unlock).

Comment 27

[hanj.kim25]

(In reply to Alive Kuo [:alive] from comment #26)

Confirmed that these fixes the wake lock issue. Can we get these into v1-train?

https://hg.mozilla.org/mozilla-central/rev/191155ed17b0
https://github.com/mozilla-b2g/gaia/pull/10366


BTW, what does it mean by having the wakelock state in background? Is it held on hold until the caller app comes on foreground?

Thanks!

Comment 28

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

(In reply to hanj.kim25 from comment #27)
> (In reply to Alive Kuo [:alive] from comment #26)
> 
> Confirmed that these fixes the wake lock issue. Can we get these into
> v1-train?

Once r+ed.

> 
> https://hg.mozilla.org/mozilla-central/rev/191155ed17b0
> https://github.com/mozilla-b2g/gaia/pull/10366
> 
> 
> BTW, what does it mean by having the wakelock state in background? Is it
> held on hold until the caller app comes on foreground?
> 

It depends on the strategy we want.
Currently screen-manager doesn't keep the screen lock when wake lock requester is at background.

> Thanks!

Comment 29

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

Comment on attachment 761983 [details]
https://github.com/mozilla-b2g/gaia/pull/10366

r+, f+ based on comment 27.

Comment 30

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

master
https://github.com/mozilla-b2g/gaia/commit/81a31ebc00c2d918c1f1868f04fa271b345516f4

v1-train
https://github.com/mozilla-b2g/gaia/commit/65197db55d1db5e01b8bd71a34ef1c8f96ed0903

Comment 31

[John Ford [:jhford] CET/CEST Berlin Time]

1.1hd: 65197db55d1db5e01b8bd71a34ef1c8f96ed0903