Closed Bug 932698 Opened 11 years ago Closed 10 years ago

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

Categories

(Core Graveyard :: Widget: Gonk, defect)

ARM
Gonk (Firefox OS)
defect
Not set
normal

Tracking

(firefox34 wontfix, firefox35 wontfix, firefox36 fixed, b2g-v1.4 wontfix, b2g-v2.0 affected, b2g-v2.0M affected, b2g-v2.1 fixed, b2g-v2.2 fixed)

RESOLVED FIXED
mozilla36
Tracking Status
firefox34 --- wontfix
firefox35 --- wontfix
firefox36 --- fixed
b2g-v1.4 --- wontfix
b2g-v2.0 --- affected
b2g-v2.0M --- affected
b2g-v2.1 --- fixed
b2g-v2.2 --- fixed

People

(Reporter: mchen, Assigned: viralwang)

References

Details

(Whiteboard: [u=devices c=power p=])

Attachments

(1 file, 11 obsolete files)

3.37 KB, patch
mwu
: review+
Details | Diff | Splinter Review
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.
Whiteboard: [u=devices c=power p=]
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.
Attachment #8443369 - Flags: review?(dhylands)
I'm linking to a handful of bugs which are most likely related to this. Many are likely duplicates.
See Also: → 1013066, 919903, 937288, 1020885, 1026026
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.
Attachment #8443369 - Flags: review?(dhylands) → review-
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 :)
Assignee: nobody → vwang
Attachment #8443369 - Attachment is obsolete: true
Attachment #8462436 - Flags: review?(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.
Attachment #8462436 - Flags: review?(dhylands) → review+
Hi Dave,

Thanks again for your suggestion.
Could you please help to review this patch again?
Thanks!
Attachment #8462436 - Attachment is obsolete: true
Attachment #8463199 - Flags: review?(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
Attachment #8463199 - Flags: review?(dhylands) → review+
Hi Dave,

Thank you for your feedback.
Need your help to review it, thanks :)
Attachment #8463199 - Attachment is obsolete: true
Attachment #8463798 - Flags: review?(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.
Attachment #8463798 - Flags: review?(dhylands) → review+
[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.
blocking-b2g: --- → 1.4?
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.
Attachment #8464600 - Flags: review?(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.
Attachment #8464600 - Flags: review?(dhylands) → review+
Hi Dave,

Updated patch need your help to review.
Thank you!
Attachment #8463798 - Attachment is obsolete: true
Attachment #8464600 - Attachment is obsolete: true
Attachment #8465300 - Flags: review?(dhylands)
Comment on attachment 8465300 [details] [diff] [review]
hold a wakelock when we receive the powey key to wake up device (v6)

wrong mail :(
Attachment #8465300 - Flags: review?(dhylands) → review?(dhylands)
blocking-b2g: 1.4? → 1.4+
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.
Attachment #8465300 - Flags: review?(dhylands) → review+
Hi Bhavana,

I'm thinking this bug also need to land in 2.0.
Any suggestion?
Flags: needinfo?(bbajaj)
https://hg.mozilla.org/integration/b2g-inbound/rev/07c91b928cd7

Was the Try push build-only because there's no automated test coverage for this?
Flags: in-testsuite?
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/07c91b928cd7
Status: NEW → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Depends on: 1047618
Backed out for causing bug 1047618.
https://hg.mozilla.org/mozilla-central/rev/6551323d96e6
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Dolphin won't have this issue, should this still be 1.4+?
[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.
blocking-b2g: 1.4+ → 2.0?
Its a long standing issue and given the risk of regression and 2.0 timeline, lets get this in 2.1
blocking-b2g: 2.0? → ---
Flags: needinfo?(bbajaj)
Component: General → GonkIntegration
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.
Flags: needinfo?(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?
Flags: needinfo?(jsmith) → needinfo?(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.
Flags: needinfo?(dhylands)
Component: GonkIntegration → Widget: Gonk
Product: Firefox OS → Core
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.
Attachment #8465300 - Attachment is obsolete: true
Attachment #8498004 - Flags: review?(dhylands)
Attachment #8498004 - Attachment is patch: true
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.
Attachment #8498004 - Flags: review?(mwu)
Attachment #8498004 - Flags: review?(dhylands)
Attachment #8498004 - Flags: review+
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.
Attachment #8498004 - Flags: review?(mwu) → review-
(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!
Flags: needinfo?(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.
Flags: needinfo?(mwu)
Hi Michael,

Any feedback I can have in this patch?
I will do more testing and then request your review later.
Thanks.
Attachment #8500374 - Flags: feedback?(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.
Attachment #8500374 - Flags: feedback?(mwu)
Hi Michael,

Thank you for your suggestion.
Could you please help to review this updated patch?
Thanks!
Attachment #8498004 - Attachment is obsolete: true
Attachment #8500374 - Attachment is obsolete: true
Attachment #8500972 - Flags: review?(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?
Attachment #8500972 - Flags: review?(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.
Hi Michael,

Thank you for your feedback, could you please help on this updated patch?
Attachment #8500972 - Attachment is obsolete: true
Attachment #8504639 - Flags: review?(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.
Attachment #8504639 - Flags: review?(mwu)
Hi Michael,

Thanks again for your response.
Update few parts as your suggestion.
Attachment #8504639 - Attachment is obsolete: true
Attachment #8505340 - Flags: review?(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.
Attachment #8505340 - Flags: review?(mwu) → review+
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?
Attachment #8505340 - Attachment is obsolete: true
Attachment #8506819 - Flags: review?(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.
Attachment #8506819 - Flags: review?(mwu)
Attachment #8506819 - Flags: review+
https://hg.mozilla.org/mozilla-central/rev/6250e897cd5d
Status: REOPENED → RESOLVED
Closed: 10 years ago10 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla36
blocking-b2g: --- → 2.0M?
No longer blocks: Woodduck
blocking-b2g: 2.0M? → ---
See Also: → 1096311
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
Attachment #8506819 - Flags: approval-mozilla-b2g34?
Attachment #8506819 - Flags: approval-mozilla-b2g34? → approval-mozilla-b2g34+
Product: Core → Core Graveyard
You need to log in before you can comment on or make changes to this bug.