Closed Bug 1173970 Opened 5 years ago Closed 5 years ago

[Linter: Wakelock] Audit wakelock usage in GeckoApp

Categories

(Firefox for Android :: General, defect)

All
Android
defect
Not set
normal

Tracking

()

RESOLVED FIXED

People

(Reporter: mcomella, Assigned: esawin)

References

(Blocks 1 open bug)

Details

Attachments

(1 file)

via android lint:

 ../../src/main/java/org/mozilla/gecko/GeckoApp.java:2403: The release() call is not always reached

2400               mWakeLocks.put(topic, wl);
2401             }
2402         } else if (!state.equals("locked-foreground") && wl != null) {
2403             wl.release();
2404             mWakeLocks.remove(topic);
2405         }

Priority: 9 / 10
Category: Performance
Severity: Warning
Explanation: Incorrect WakeLock usage.
Failing to release a wakelock properly can keep the Android device in a high power mode, which reduces battery life. There are several causes of this, such as releasing the wake lock in onDestroy() instead of in onPause(), failing to call release() in all possible code paths after an acquire(), and so on.

NOTE: If you are using the lock just to keep the screen on, you should strongly consider using FLAG_KEEP_SCREEN_ON instead. This window flag will be correctly managed by the platform as the user moves between applications and doesn't require a special permission. See http://developer.android.com/reference/android/view/WindowManager.LayoutParams.html#FLAG_KEEP_SCREEN_ON. 

---

Ensure the usage is correct (it might be - the wakelocks are stored in an array for an extended duration which could be screwing the linter up) and if so, suppress the warning with "@SuppressLint("Wakelock")".
Eugen, can you audit this (or pass it on) to make sure we're correctly taking and releasing wakelocks?

If so, add the @SuppressLint("Wakelock") annotation to the function.
Flags: needinfo?(esawin)
The condition the linter complains about is correct (although it's clear why the linter doesn't get the context of it) and there doesn't seem to be an issue with us keeping the screen wake lock when being in the background.

We could add a check in onPause to verify that we are not holding any wake locks, but that would introduce some complexity and potentially even race conditions (what to do onResume if we did hold some?).

I think this should be safe to suppress.
Flags: needinfo?(esawin)
Assignee: nobody → esawin
Comment on attachment 8626724 [details] [diff] [review]
0001-Bug-1170886-Suppress-WakeLock-linter-warning-for-non.patch

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

Sounds good to me!
Attachment #8626724 - Flags: review?(michael.l.comella) → review+
Thanks for taking care of this, Eugen!
Since Pulsebot seems dead, here is the push:
https://hg.mozilla.org/integration/mozilla-inbound/rev/5cde286271f7
Status: NEW → ASSIGNED
https://hg.mozilla.org/mozilla-central/rev/5cde286271f7
Status: ASSIGNED → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.