932698 - [Power] The processing flow on power key should be protected by WakeLock.

Status: RESOLVED FIXED

(Core Graveyard :: Widget: Gonk, defect)

Description

[Marco Chen [:mchen]]

Refer to https://bugzilla.mozilla.org/show_bug.cgi?id=898707#c22.

If Linux kernel didn't help to hold a unknow_wakelock, then there is a chance of system go back to suspend before gecko/gaia processing the power key.

So we should use wakelock to protect the entire processing flow of power key.

Comment 1

[viral [:viralwang]]

Attachment: hold a wakelock when we receive the powey key to wake up device

Hi Dave,

We now only have a wakelock in eventhub.c when we receive power key and wakelock will release once all the events are capatured.
There's a chance eventhub receive the power key but we can not wake up device in time if all other wakelocks expired and device suspend again.
I think we can hold another wakelock in eventhub and release it until screen on, it should fix the bug that power key can not always wake up device.
Thanks.

Comment 2

[Gabriele Svelto [:gsvelto]]

I'm linking to a handful of bugs which are most likely related to this. Many are likely duplicates.

Comment 3

[Dave Hylands [:dhylands]]

Comment on attachment 8443369 [details] [diff] [review]
hold a wakelock when we receive the powey key to wake up device

Review of attachment 8443369 [details] [diff] [review]:
-----------------------------------------------------------------

So I think that "what" is being done looks ok, but I'm objecting to "how" its implemented.

So I'm giving this an r- for now. I'd like to see it reworked as per my comments below.

::: hal/gonk/GonkHal.cpp
@@ +95,5 @@
>  #ifndef BATTERY_FULL_ARGB
>  #define BATTERY_FULL_ARGB 0x0000FF00
>  #endif
>  
> +static const char *Power_WAKE_LOCK_ID = "PowerKeyEvent";

nit: This should probably be called kPower_WAKE_LOCK_ID

(as per: https://developer.mozilla.org/en-US/docs/Mozilla/Developer_guide/Coding_Style#Prefixes)

@@ +517,5 @@
>  void
>  SetScreenEnabled(bool enabled)
>  {
>    GetGonkDisplay()->SetEnabled(enabled);
> +  release_wake_lock(Power_WAKE_LOCK_ID);

Whenever possible, we should avoid using acquire/release semantics, and instead use RAII (resource allocation is initialization).

So I think it would be better to create a helper class.

The helper class constructor should acquire the wake lock, and the destructor should release it.
Then the Power_WAKE_LOCK_ID will then get encapsulated in the helper class (rather than duplicated).

Use a reference counted pointer to hold the pointer to the new class, and you'd just assign nullptr to the reference counted pointer to release the wakelock.

This then accomplishes the following:

- eliminates mismatched calls to acquire/release
- encapsulates knowledge of how the wakelock is implemented to within the class (i.e. no duplcation of constants) and the caller doesn't have to know how to use wakelocks, they just need to instantiate release a power-wake-lock object.

::: widget/gonk/libui/EventHub.cpp
@@ +67,5 @@
>  #define INDENT "  "
>  #define INDENT2 "    "
>  #define INDENT3 "      "
>  
> +static const char *Power_WAKE_LOCK_ID = "PowerKeyEvent";

Why is this duplicated? This seems bad to me.

Shouldn't this go into a header file?

@@ +860,5 @@
>                          event->type = iev.type;
>                          event->code = iev.code;
>                          event->value = iev.value;
> +                        if (event->code == KEY_POWER && event->value
> +                          && !hal::GetScreenEnabled()) {

Why do we care about whether the screen is enabled or not? Shouldn't we just acquire the wake lock regardless?

Otherwise couldn't we run into a situation where pressing the power button to turn off the screen doesn't get properly processed?

I think its a mistake to equate screen-on with phone doesn't go to sleep.

I know our phones don't do this, but some phones sleep during vertical blanking on the LCD, so I think its important to keep the two concepts separated.

Comment 4

[viral [:viralwang]]

Attachment: bug932698 - hold a wakelock when we receive the powey key to wake up device.

Hi Dave,

Thank you for your kindly feedback and suggestion.

I think we should still check if screen is enable or not.
We have 3 user cases to press power key.
1) when device is suspend, press power key to wake up(resume) device
2) when screen is on, press power key to suspend device
3) long press power key to find shut down menu.
This bug should protect case 1 and don't need to hold wakelock for case 2 and 3.
That's why I would like to check the screen status.
Thanks :)

Comment 5

[Dave Hylands [:dhylands]]

Comment on attachment 8462436 [details] [diff] [review]
bug932698 - hold a wakelock when we receive the powey key to wake up device.

Review of attachment 8462436 [details] [diff] [review]:
-----------------------------------------------------------------

This looks much better. There are still a few minor nits.

r=me with nits addressed.

::: hal/HalWakeLock.h
@@ +27,5 @@
>  
> +class PowerWakelock
> +{
> +private:
> +  const char *kPower_WAKE_LOCK_ID = "PowerKeyEvent";

nit: swap * and space. const char* kPower_WAKE...

nit: Put the public declarations at the beginning and the private ones at the end.

@@ +34,5 @@
> +  PowerWakelock();
> +  ~PowerWakelock();
> +};
> +
> +extern nsRefPtr <PowerWakelock> mPowerWakelock;

Since this isn't a member variable, it shouldn't use the m prefix.

It looks like its a global, so it should use the 'g' prefix.
https://developer.mozilla.org/en-US/docs/Mozilla/Developer_guide/Coding_Style#Prefixes

::: hal/gonk/GonkHal.cpp
@@ +667,5 @@
>  void
>  SetScreenEnabled(bool aEnabled)
>  {
>    GetGonkDisplay()->SetEnabled(aEnabled);
> +  if (aEnabled && mPowerWakelock)

nit: Use braces, even on single line statements.

You also don't need the test for mPowerWakeLock here. You can just check aEnabled and assign the nullptr. If it's already null then it won't do any harm.

Actually, I'm not even sure you need the if at all. I think you can just arbitrarily assign mPowerWakeLock to be nullptr.

Comment 6

[viral [:viralwang]]

Attachment: hold a wakelock when we receive the powey key to wake up device (v3)

Hi Dave,

Thanks again for your suggestion.
Could you please help to review this patch again?
Thanks!

Comment 7

[Dave Hylands [:dhylands]]

Comment on attachment 8463199 [details] [diff] [review]
hold a wakelock when we receive the powey key to wake up device (v3)

Review of attachment 8463199 [details] [diff] [review]:
-----------------------------------------------------------------

::: widget/gonk/libui/EventHub.cpp
@@ +57,2 @@
>  
> +nsRefPtr <PowerWakelock> gPowerWakelock;

Sorry - I missed this earlier, but because this is global it should use StaticRefPtr instead of nsRefPtr.
http://dxr.mozilla.org/mozilla-central/source/xpcom/base/StaticPtr.h?from=StaticRefPtr#99

I also think that this global should be in the same .cpp file as the constructor/destructor. i.e. HalWakeLock.cpp

Comment 8

[viral [:viralwang]]

Attachment: hold a wakelock when we receive the powey key to wake up device (v4)

Hi Dave,

Thank you for your feedback.
Need your help to review it, thanks :)

Comment 9

[Dave Hylands [:dhylands]]

Comment on attachment 8463798 [details] [diff] [review]
hold a wakelock when we receive the powey key to wake up device (v4)

Review of attachment 8463798 [details] [diff] [review]:
-----------------------------------------------------------------

Looks good to me.

Comment 10

[viral [:viralwang]]

[Blocking Requested - why for this release]:
Normally we use power key to wake up device when device in suspend mode, sometimes kernel need more time to wake device up and pass power key to EventHub too late.
We can't wake up device in this case since we can't handle power key event in time and device enter suspend mode again before device wake up.
We never do any protection for this case and the bug will happened by device since it depends on the kernel resume time.

Comment 11

[viral [:viralwang]]

Attachment: hold a wakelock when we receive the powey key to wake up device (v5)

Hi Dave,

Sorry for bothering you again.
I met some build break in try server and error log shows below:
"Cannot open include file: 'hardware_legacy/power.h': No such file or directory"

I think I should not new the PowerWakeLock class in HalWakeLock.h and should use it for Gonk only.
Could you please help to review the updated patch?
Thank you in advance.

Comment 12

[Dave Hylands [:dhylands]]

Comment on attachment 8464600 [details] [diff] [review]
hold a wakelock when we receive the powey key to wake up device (v5)

Review of attachment 8464600 [details] [diff] [review]:
-----------------------------------------------------------------

Looking good. I think that a few more tweaks are needed.

r=me with issues addressed.

::: widget/gonk/PowerWakeLock.cpp
@@ +3,5 @@
> + * License, v. 2.0. If a copy of the MPL was not distributed with this file,
> + * You can obtain one at http://mozilla.org/MPL/2.0/. */
> +
> +#include "mozilla/StaticPtr.h"
> +#include "PowerWakeLock.h"

nit: #include "PowerWakeLock.h" as the first #include. This will ensure that PowerWakeLock.h #includes everything that it needs.

@@ +12,5 @@
> +{
> +  acquire_wake_lock(PARTIAL_WAKE_LOCK, kPower_WAKE_LOCK_ID);
> +}
> +
> +PowerWakelock:: ~PowerWakelock()

nit: no space before ~

::: widget/gonk/PowerWakeLock.h
@@ +8,5 @@
> +
> +#include "mozilla/StaticPtr.h"
> +#include "nsISupportsImpl.h"
> +
> +class PowerWakelock

I think that we should put this inside the mozilla::hal_impl namespace

@@ +13,5 @@
> +{
> +public:
> +  NS_INLINE_DECL_REFCOUNTING(PowerWakelock);
> +  PowerWakelock();
> +  ~PowerWakelock();

Make the destructor private (this way it can only be destructed by the ref count going to zero).

@@ +15,5 @@
> +  NS_INLINE_DECL_REFCOUNTING(PowerWakelock);
> +  PowerWakelock();
> +  ~PowerWakelock();
> +private:
> +  const char* kPower_WAKE_LOCK_ID = "PowerKeyEvent";

This doesn't really need to be a private member. I'd be inclined to just put it at the top of PowerWakeLock.cpp.

Comment 13

[viral [:viralwang]]

Attachment: hold a wakelock when we receive the powey key to wake up device (v6)

Hi Dave,

Updated patch need your help to review.
Thank you!

Comment 14

[viral [:viralwang]]

Comment on attachment 8465300 [details] [diff] [review]
hold a wakelock when we receive the powey key to wake up device (v6)

wrong mail :(

Comment 15

[Dave Hylands [:dhylands]]

Comment on attachment 8465300 [details] [diff] [review]
hold a wakelock when we receive the powey key to wake up device (v6)

Review of attachment 8465300 [details] [diff] [review]:
-----------------------------------------------------------------

Excellent.

Comment 16

[viral [:viralwang]]

https://tbpl.mozilla.org/?tree=Try&rev=8a4314d87b66

Comment 17

[viral [:viralwang]]

Hi Bhavana,

I'm thinking this bug also need to land in 2.0.
Any suggestion?

Comment 18

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/b2g-inbound/rev/07c91b928cd7

Was the Try push build-only because there's no automated test coverage for this?

Comment 19

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/07c91b928cd7

Comment 20

[Ryan VanderMeulen [:RyanVM]]

Backed out for causing bug 1047618.
https://hg.mozilla.org/mozilla-central/rev/6551323d96e6

Comment 21

[thomas tsai]

Dolphin won't have this issue, should this still be 1.4+?

Comment 22

[Wayne Chang [:wchang]]

[Blocking Requested - why for this release]:

Since this doesn't impact dolphin and has cased regression, but the fix is ultimately desired for FxOS - pushing to subsequent release.

Comment 23

[bhavana bajaj [:bajaj]]

Its a long standing issue and given the risk of regression and 2.0 timeline, lets get this in 2.1

Comment 25

[Dave Hylands [:dhylands]]

Jason, why are you changing these to GonkIntegration?

The description for GonkIntegration says:

"Issues related to integrating the Gonk build system (based on the Android build system) and its helper scripts."

We either need to change the description of GonkIntegration or leave these in General.

Comment 26

[Jason Smith [:jsmith]]

I was trying to move this out of General since bugs typically can get lost in the General component. Originally I thought GonkIntegration was appropriate until I realized the scope of that component focuses on the Gonk build system. Should we use Core --> Widget: Gonk instead then?

Comment 27

[Dave Hylands [:dhylands]]

I think that you could argue for Widget:Gonk for this particular bug.

I've been using General for AutoMounter/MTP/USB Mass Storage type stuff (things which go in gecko, but which are specific to FirefoxOS), since none of the other categories fit.

Comment 28

[viral [:viralwang]]

Attachment: hold a wakelock when we receive the powey key to wake up device. (v7)

Hi Dave,

I think we can hold a wakelock every time we press power key.
wakelock will release when screen turn on/off.
It should fix this bug and will not cause the bug 1047618.

Comment 29

[Dave Hylands [:dhylands]]

Comment on attachment 8498004 [details] [diff] [review]
hold a wakelock when we receive the powey key to wake up device. (v7)

Review of attachment 8498004 [details] [diff] [review]:
-----------------------------------------------------------------

This looks reasonable to me, but because it touches stuff in widget/gonk, I'd like mwu to take a look as well.

Comment 30

[Michael Wu [:mwu]]

Comment on attachment 8498004 [details] [diff] [review]
hold a wakelock when we receive the powey key to wake up device. (v7)

Review of attachment 8498004 [details] [diff] [review]:
-----------------------------------------------------------------

As far as I can tell, Android *always* holds the wakelock while events are processed - so there is no need to acquire a wakelock in EventHub.cpp. It can be done within the handler in nsAppShell.cpp which runs on the EventHub thread. It's only when we pass the event to the main thread that we might lose all wakelocks.

I don't think we even need to special case the locking for the power button - we can always ensure there exists a wakelock when there are key events queued to process, and then drop the wakelock when the queue is empty.

Comment 31

[viral [:viralwang]]

(In reply to Michael Wu [:mwu] from comment #30)
> Comment on attachment 8498004 [details] [diff] [review]
> hold a wakelock when we receive the powey key to wake up device. (v7)
> 
> Review of attachment 8498004 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> As far as I can tell, Android *always* holds the wakelock while events are
> processed - so there is no need to acquire a wakelock in EventHub.cpp. It
> can be done within the handler in nsAppShell.cpp which runs on the EventHub
> thread. It's only when we pass the event to the main thread that we might
> lose all wakelocks.
> 
> I don't think we even need to special case the locking for the power button
> - we can always ensure there exists a wakelock when there are key events
> queued to process, and then drop the wakelock when the queue is empty.

Hi Michael,

If we drop the wakelock after key event queue is empty, we still have chance that screen_manager.js didn't turn on the screen in time. (there's no wakelock hold until the screen on, isn't it?)

It should be fine if we hold the wakelock in GeckoInputDispatcher when key event comes, but not sure the proper timing to release the wakelock. Any suggestion I can have?
Thank you!

Comment 32

[Michael Wu [:mwu]]

screen_manager.js runs in the main process, so I think it should be synchronously processing key events. So as long as you check the event queue after events are dispatched, screen_manager.js should've had a chance to turn on the screen. This means that any point after dispatchOnce() in nsAppShell::ProcessNextNativeEvent is probably fine. Doing it at the end of the function is probably good to ensure we do whatever other work we have before going back to sleep.

Comment 33

[viral [:viralwang]]

Attachment: hold a wakelock when we receive the powey key to wake up device. (v8)

Hi Michael,

Any feedback I can have in this patch?
I will do more testing and then request your review later.
Thanks.

Comment 34

[Michael Wu [:mwu]]

Comment on attachment 8500374 [details] [diff] [review]
hold a wakelock when we receive the powey key to wake up device. (v8)

Review of attachment 8500374 [details] [diff] [review]:
-----------------------------------------------------------------

::: widget/gonk/nsAppShell.cpp
@@ +658,5 @@
>      data.deviceId = args->deviceId;
>      data.key.keyCode = args->keyCode;
>      data.key.scanCode = args->scanCode;
> +    if (args->scanCode == KEY_POWER && !args->action)
> +        gPowerWakelock = new PowerWakelock();

I'm not sure if it's safe to set gPowerWakelock on different threads. Also, the wakelock object doesn't have any data, so this allocation serves no purpose.

It should be more straightforward to have a bool field indicate whether the wakelock is being held. I think you can take advantage of the mQueueLock to protect that field.

Comment 35

[viral [:viralwang]]

Attachment: hold a wakelock when we receive the powey key to wake up device. (v9)

Hi Michael,

Thank you for your suggestion.
Could you please help to review this updated patch?
Thanks!

Comment 36

[Michael Wu [:mwu]]

Comment on attachment 8500972 [details] [diff] [review]
hold a wakelock when we receive the powey key to wake up device. (v9)

Review of attachment 8500972 [details] [diff] [review]:
-----------------------------------------------------------------

::: widget/gonk/nsAppShell.cpp
@@ +101,5 @@
>  
>  // Amount of time in MS before an input is considered expired.
>  static const uint64_t kInputExpirationThresholdMs = 1000;
>  
> +bool gPowerWakelock = false;

Move this to GeckoInputDispatcher - all its users are in there.

@@ +102,5 @@
>  // Amount of time in MS before an input is considered expired.
>  static const uint64_t kInputExpirationThresholdMs = 1000;
>  
> +bool gPowerWakelock = false;
> +const char* kPower_WAKE_LOCK_ID = "PowerKeyEvent";

Make this a static const char array.

@@ +668,5 @@
>      data.key.scanCode = args->scanCode;
>      {
>          MutexAutoLock lock(mQueueLock);
>          mEventQueue.push(data);
> +        if (data.key.scanCode == KEY_POWER && !data.action) {

Instead of looking for the power key going down, can this always ensure that a wakelock is acquired if there are key events to process?

Comment 37

[Michael Wu [:mwu]]

Comment on attachment 8500972 [details] [diff] [review]
hold a wakelock when we receive the powey key to wake up device. (v9)

Review of attachment 8500972 [details] [diff] [review]:
-----------------------------------------------------------------

::: widget/gonk/nsAppShell.cpp
@@ +639,5 @@
>          dispatcher.Dispatch();
>          break;
>      }
>      }
> +    {

This block isn't necessary since we're at the end of the function.

Comment 38

[viral [:viralwang]]

Attachment: Bug932698 - hold a wakelock when we receive key events.

Hi Michael,

Thank you for your feedback, could you please help on this updated patch?

Comment 39

[Michael Wu [:mwu]]

Comment on attachment 8504639 [details] [diff] [review]
Bug932698 - hold a wakelock when we receive key events.

Review of attachment 8504639 [details] [diff] [review]:
-----------------------------------------------------------------

Almost there.

::: widget/gonk/nsAppShell.cpp
@@ +547,5 @@
>      nsRefPtr<GeckoTouchDispatcher> mTouchDispatcher;
>  
>      int mKeyDownCount;
>      bool mKeyEventsFiltered;
> +    bool gPowerWakelock;

This is a member field, not a global, so it should be prefixed with a m rather than g.

@@ +640,5 @@
>          break;
>      }
>      }
> +    MutexAutoLock lock(mQueueLock);
> +    if (gPowerWakelock) {

I recommend also checking if the queue is empty before dropping the wake lock.

@@ +667,5 @@
>      {
>          MutexAutoLock lock(mQueueLock);
>          mEventQueue.push(data);
> +        gPowerWakelock =
> +            acquire_wake_lock(PARTIAL_WAKE_LOCK, kPower_WAKE_LOCK_ID);

We should only need to acquire a wakelock here if mPowerWakelock == false.

Comment 40

[viral [:viralwang]]

Attachment: Bug932698 - hold a wakelock when we receive key events.

Hi Michael,

Thanks again for your response.
Update few parts as your suggestion.

Comment 41

[Michael Wu [:mwu]]

Comment on attachment 8505340 [details] [diff] [review]
Bug932698 - hold a wakelock when we receive key events.

Review of attachment 8505340 [details] [diff] [review]:
-----------------------------------------------------------------

Looks good, thanks!

::: widget/gonk/nsAppShell.cpp
@@ +100,5 @@
>  static int32_t sMicrophoneState;
>  
>  // Amount of time in MS before an input is considered expired.
>  static const uint64_t kInputExpirationThresholdMs = 1000;
> +static const char kPower_WAKE_LOCK_ID[] = "PowerKeyEvent";

Not sure if this is the right name anymore for the wakelock, but it doesn't bother me too much.

Comment 42

[viral [:viralwang]]

Attachment: Bug932698 - hold a wakelock when we receive key events.

Hi Michael,

Looks like EventHub already use 'KeyEvents' as wakelock id. (http://dxr.mozilla.org/mozilla-central/source/widget/gonk/libui/EventHub.cpp#70)
How do you feel about using 'GeckoKeyEvent' in nsAppShell?

Comment 43

[Michael Wu [:mwu]]

Comment on attachment 8506819 [details] [diff] [review]
Bug932698 - hold a wakelock when we receive key events.

Review of attachment 8506819 [details] [diff] [review]:
-----------------------------------------------------------------

Looks good to me.

Comment 44

[viral [:viralwang]]

try server:
https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=96599111d31f
https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=d812f8490c8d

Comment 45

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/6250e897cd5d

Comment 46

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/6250e897cd5d

Comment 47

[Michael Wu [:mwu]]

Flags are clearly wrong.

Comment 48

[Michael Wu [:mwu]]

Comment on attachment 8506819 [details] [diff] [review]
Bug932698 - hold a wakelock when we receive key events.

NOTE: Please see https://wiki.mozilla.org/Release_Management/B2G_Landing to better understand the B2G approval process and landings.

[Approval Request Comment]
Bug caused by (feature/regressing bug #): This is a long standing bug.
User impact if declined: Screen occasionally fails to turn on when pressing power button and another press is required. Looks and feels bad. Some vendors have worked around this issue, but this patch is the only true fix.
Testing completed: Baked on trunk for a few weeks now.
Risk to taking this patch (and alternatives if risky): Low risk - we make sure to release the wakelock whenever we're done processing events, and not releasing the wakelock is the main risk.
String or UUID changes made by this patch: None

Comment 49

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-b2g34_v2_1/rev/cb5b7901200e