1484528 - firefox uses CPU while in background and drains my battery

Status: VERIFIED FIXED

(Firefox for Android Graveyard :: General, defect, P1)

Description

[br]

User Agent: Mozilla/5.0 (X11; Linux x86_64; rv:61.0) Gecko/20100101 Firefox/61.0
Build ID: 20180718215440

Steps to reproduce:

1. open firefox on android
2. go back to the launcher screen so that firefox goes in background
3. turn the screen off if you want
3. open adb shell
4. type top


Actual results:

Firefox is always the first or second process, using around 10% CPU on my oneplus 3t. This doesn't seem to be a short task using CPU, I observed firefox using CPU for a whole 10 minutes.

Also, the surfaceflinger process is always in the top processes too when firefox is running (still in background, screen off), using around 5% CPU.

More generally, I noticed that when firefox is open, my battery is drained more quickly, I am assuming that it is due to the high CPU usage while sleeping.


Expected results:

Firefox shouldn't do work in background and stay at 0%. Google Chrome is always between 0% and 0.3% while in background on the same phone.

Comment 1

[Stefan Deiac]

Hi, tested your issue with LG G4 on the latest version of Firefox. Following your steps, everything works at the proper CPU. Can you retry please on the latest version? Also, do you have any specific options or additional steps? I'll wait for your update, thanks.

Comment 2

[br]

I am using the latest Firefox from the store: 61.0.2, on Android 8.0.0

I made some more tests, trying to remove everything I have on my Firefox. In the end, I tried:

- clear all Firefox's data (from Android's settings -> apps -> Firefox -> storage -> clear data)
- start Firefox (Firefox shows a welcome screen)
- press home to return to your launcher
- turn the screen off
- Firefox still uses ~10% CPU, even after a couple minutes

Maybe I can try building a debug Firefox and profiling it if you really can't reproduce this.

Comment 3

[Stefan Deiac]

Hi, retested with your steps from comment 2 and in the end, I was found two different results.
On Nokia 6 with Android 7.1.1), the CPU usage it's around 0%.
On OnePlus 5T (Android 8.1.0) and OnePlus Two (Android 6.0.1), the CPU usage it's around 1%-2%.
Can you please try to build a debug Firefox and post here your results?

Comment 4

[br]

Alright, this is the first time I am doing that, so tell me if I missed something or if I did something wrong.

I built android from the mercurial repo as the tutorial explained. It's an artifact build. I ran it through android studio's profiler and managed to reproduce the issue.

I didn't find any way to export the profiling data though.

I first profiled using "Sampled (Native)". What seems to be the culprit is the first thread (seems like the main thread) on which runs what seems like an android internal called onWindowFocusChangedNative once every 20~30ms. As before, firefox was not focused and the phone's screen was off.

I also profiled with the mode "Sampled (Java)". I am not sure if it's the cause or the consequence, but that same thread runs calls to Handler.dispatchMessage and MessageQueue.next.

Most of the other threads are just sleeping.

I will attach a screenshot of both stacks, not sure it helps though.

Comment 5

[br]

Attachment: Profile using "Sampled (Native)"

Comment 6

[br]

Attachment: Profile using "Sampled (Java)"

Comment 7

[Stefan Deiac]

Petru, can you take a look on this issue or someone from your team? From my capacities, I can't take a proper decision at this point.

Comment 8

[Petru-Mugurel Lingurar [:petru]]

Attachment: fennec - cpu consumption in background.png

(In reply to blastrock from comment #0)

`top` shows CPU usage as a percentage of a single cpu by default.
So in the case of a OnePlus 3T with an 8 core cpu it could show a load of up to 800%.

As noted in Android documentation [1] onWindowFocusChanged()
> provides information about global focus state, which is managed independently of activity lifecycles
> an activity that is stopped will not generally get window focus
So while not intended this callback could indeed fire.

Profiled the latest Nightly and saw negligible CPU activity while on background (see attachment).

With this in mind I'm closing the ticket, but if you have new information you can reopen it or create a new one.

[1] https://developer.android.com/reference/android/app/Activity#onWindowFocusChanged(boolean)

Comment 9

[br]

I reported the bug to oneplus (since it seems to only happen on their phones), but I haven't had news for some time now.

I have spent some more time on this and managed to find a fix. I am not sure at all if this is the correct fix though. Here's the patch on current mercurial repository:

diff -r 8eff0a4f5d8f mobile/android/base/java/org/mozilla/gecko/drawable/ShiftDrawable.java
--- a/mobile/android/base/java/org/mozilla/gecko/drawable/ShiftDrawable.java    Tue Nov 20 18:47:30 2018 +0200
+++ b/mobile/android/base/java/org/mozilla/gecko/drawable/ShiftDrawable.java    Tue Nov 20 22:31:04 2018 +0100
@@ -82,7 +82,7 @@
     @Override
     public boolean setVisible(final boolean visible, final boolean restart) {
         final boolean result = super.setVisible(visible, restart);
-        if (isVisible()) {
+        if (visible) {
             mAnimator.start();
         } else {
             mAnimator.end();

This class seems to be instantiated from AnimatedProgressBar. Is it the progress bar that appears on the top when a page is loading?

For some reason, isVisible returns true when the widget is not visible, so the animation is never stopped. So callbacks keep being called even when the screen is off.

Relying on the visible argument instead seems to work. It turns to true when the widget appears and to false when it disappears.

Comment 10

[Petru-Mugurel Lingurar [:petru]]

Interesting find, I'll look into that.

Comment 11

[Petru-Mugurel Lingurar [:petru]]

Attachment: Bug 1484528 - Prevent page loading indicator running continuously; r?JanH

The page loading indicator should be animated while it is visible.
Previously for controlling the animation the visibility state of the parent
Drawable - ShiftDrawable would be checked, which in all cases would return
true, so the animation would run indefinitely.

The new method this patch adds - isChildVisible() complements setVisible() which
sets the visibility for the progress drawable and allow querying it to know
exactly if the animation should run or not.
Does not keep the original method name from Drawable - isVisible() like in the
case of the overridden setVisible() because this is a final method.

Comment 12

[Jan Henning [:JanH]]

The fix will be simple enough that we should get this at lest into Beta to avoid unnecessarily wasting CPU cycles.

Comment 13

[Petru-Mugurel Lingurar [:petru]]

Comment on attachment 9026711 [details]
Bug 1484528 - Prevent page loading indicator running continuously; r?JanH

[Beta/Release Uplift Approval Request]

Feature/Bug causing the regression: Bug 1366672

User impact if declined: No visible impact but wastes CPU cycles.

Is this code covered by automated tests?: No

Has the fix been verified in Nightly?: No

Needs manual test from QE?: No

If yes, steps to reproduce: 

List of other uplifts needed: None

Risk to taking this patch: Low

Why is the change risky/not risky? (and alternatives if risky): Simple fix

String changes made/needed:

Comment 14

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/e23085e6b51f
Prevent page loading indicator running continuously; r=JanH

Comment 15

[Daniel Varga [:dvarga]]

https://hg.mozilla.org/mozilla-central/rev/e23085e6b51f

Comment 16

[Ryan VanderMeulen [:RyanVM]]

Comment on attachment 9026711 [details]
Bug 1484528 - Prevent page loading indicator running continuously; r?JanH

[Triage Comment]
Fixes needless battery drain caused by the page loading indicator running continuously. Approved for 64.0b13.

Comment 17

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-beta/rev/8d5ec415316b

Comment 18

[br]

I confirm that the last fix (diff 38029) fixes my issue. Thank you!

Comment 19

[Petru-Mugurel Lingurar [:petru]]

(In reply to blastrock from comment #18)
> I confirm that the last fix (diff 38029) fixes my issue. Thank you!

Great! Thanks for also investigating this!

Comment 20

[Stefan Deiac]

Based on comment 18, I'll close this issue, thanks.

Comment 21

[Kevin Brosnan [Ex-Mozilla]]

Release Note Request (optional, but appreciated)
[Why is this notable]: Fixed an issue that resulted in the loading indicator using too much CPU
[Affects Firefox for Android]: only
[Suggested wording]: Fixed an issue that resulted in the loading indicator using too much CPU. 
[Links (documentation, blog post, etc)]: