1043920 - Reader mode (ambient light detection) prevents device from sleeping (holds CPU wakelock)

Status: VERIFIED FIXED

(Firefox for Android Graveyard :: Reader View, defect)

Description

[Kamil Páral]

User Agent: Mozilla/5.0 (X11; Linux x86_64; rv:30.0) Gecko/20100101 Firefox/30.0 (Beta/Release)
Build ID: 20140611060104

Steps to reproduce:

Lately my tablet seems to have been "randomly" depleting a lot of battery during sleep (e.g. 10-15% of capacity lost during night instead of usual 3%). I don't know if it is a recent regression, but I have identified the issue to be caused by Reader Mode in Firefox.

I have spent two days carefully measuring it (using Battery Monitor Widget) and the measurements are absolutely consistent and I can reproduce them any time and every time.


Actual results:

If Firefox has at least one tab opened in Reader Mode, the device drains 4 times more energy during sleep).

It doesn't matter whether the tab is active or not. It doesn't matter whether Firefox was the currently active app before putting device to sleep, or whether it has just been in the "recently used apps" list.

If I look at integrated Battery usage dialog in Settings, I see "Android OS" to be the primary cause of energy consumption (e.g. 40%, more than Screen, which is usually the top offender). If I look into Android OS process details, I see "Keep awake: 10 hours" and similar values. Firefox process itself is not marked as using energy nor keeping the device awake. But I can consistently reproduce this behavior by opening or closing a tab in Reader Mode, letting the device sleep for a while, and then looking at the changed numbers.

I have read there there are two types of "wakelocks" - display and CPU. The display lock is clearly not held, because the device automatically turns it off and locks itself. But it seems that Reader Mode is holding the CPU lock for as long as the tab is opened, even if Firefox is not an active app.

I have seen this problem with many different web pages toggled into Reader Mode, so I don't expect to be URL-specific. But to be sure, here's an example URL that causes the problem every time I test it:
http://www.lupa.cz/clanky/p2p-pujcky-proc-se-zadluzit-bance-kdyz-vam-muze-pujcit-dav/
If I open it as a normal web page and put the device to sleep, it doesn't consume extra power and Android OS process doesn't show increasing numbers in "Keep awake" (the numbers are actually decreasing, for some reason). If I toggle that web page into Reader Mode and put the device to sleep, a lot of power is drained and Android OS process shows rapidly increasing time in Keep Awake.



Expected results:

Reader Mode should not keep the device awake. I don't know whether I'm the only one affected (due to some specific configuration), or everyone with my hardware (Nexus 7) or completely everyone. But at least for me, this bug is quite critical. If I don't pay a lot of attention and don't carefully close all Reader Mode tabs before putting the device to sleep, my battery is depleted very quickly.

If you need some specific information to help you debug the issue, please tell me. My tablet is not rooted, by I can work with adb shell. Thanks.

Comment 1

[Mark Finkle (:mfinkle) (use needinfo?)]

The Reader view using a web API to monitor the ambient lighting levels and adjust the background color from light to dark. It only does this if the "color scheme" is set to "auto", which is the default.

Can you try tapping the "Aa" button in Reader and changing the color scheme to "light" or "dark" and see if that changes the power usage?

Comment 2

[Kamil Páral]

Bingo! It's definitely caused by light conditions autodetection. When I switch the theme manually, the energy consumption is normal and the device sleeps properly. When I switch it back to autodetection, the consumption is up again and the device does not sleep. Should this get reassigned somewhere else? Do I understand correctly it might affect all websites using this API, not just the reader mode?

Comment 3

[Mark Finkle (:mfinkle) (use needinfo?)]

(In reply to Kamil Páral from comment #2)
> Bingo! It's definitely caused by light conditions autodetection. When I
> switch the theme manually, the energy consumption is normal and the device
> sleeps properly. When I switch it back to autodetection, the consumption is
> up again and the device does not sleep. Should this get reassigned somewhere
> else? Do I understand correctly it might affect all websites using this API,
> not just the reader mode?

Thank you for testing this out. Let's leave the bug here for now. We can try to fix this by listening for "application-foreground" and "application-background" notifications. We can hook and unhook the event listener as appropriate.

A larger question is whether the Gecko platform should be aware of this situation for all events and throttle them back when Fennec is not in the foreground.

Comment 4

[Mark Finkle (:mfinkle) (use needinfo?)]

Attachment: devicelight-pagevisible v0.1

This patch uses "visibilitychange" to monitor when the page goes into the background. It works when switching tabs. It also works when putting the app into the background and then back to foreground.

If using "auto", the patch will remove the "devicelight" event listener when the page is not visible. It will add the event listener back when the page becomes visible again.

Comment 5

[:Margaret Leibovic]

Comment on attachment 8462873 [details] [diff] [review]
devicelight-pagevisible v0.1

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

Nice, visibilitychange is definitely what we want to use here. I'd just like to see us make this logic a bit better before landing it.

::: mobile/android/chrome/content/aboutReader.js
@@ +415,5 @@
> +    if (this._doc.hidden) {
> +      this._win.removeEventListener("devicelight", this, false);
> +    } else {
> +      this._win.addEventListener("devicelight", this, false);
> +    }

I don't really like how this means we now add/remove this listener in two different places, since I'm worried we could end up accidentally adding the listener twice. Like, what happens if the user opens a reader mode tab in the background? Perhaps instead of using the color scheme pref, we could keep track of whether or not we have an active listener. And then instead of always adding this visibilitychange listener, you could only add it when we're in auto color mode.

At the very least, I think you should factor out the event listener code into a helper method like _setLightDetectionEnabled(true/false), which adds/removes the listener and also initializes/deletes _luxValues/_totalLux as appropriate. The user's ambient light situation may have changed while the reader mode tab was in the background, and resetting these values would lead to us updating the color scheme immediately, as opposed to waiting for the average to update.

Comment 6

[Mark Finkle (:mfinkle) (use needinfo?)]

Attachment: devicelight-pagevisible v0.2

This version unifies the code to turn device light tracking. I tested with the debugger and it removes the event handler when appropriate. Light/dark transitions work correctly.

Comment 7

[:Margaret Leibovic]

Comment on attachment 8463547 [details] [diff] [review]
devicelight-pagevisible v0.2

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

::: mobile/android/chrome/content/aboutReader.js
@@ +411,5 @@
> +      return;
> +    }
> +
> +    // Turn off the ambient light sensor if the page is hidden
> +    this._enableAmbientLighting(!this._doc.hidden)

Nit: Add a semicolon.

Comment 8

[Mark Finkle (:mfinkle) (use needinfo?)]

https://hg.mozilla.org/integration/fx-team/rev/877cd1bb9912

I think we should uplift this since it can impact battery life.

Comment 9

[Aaron Train [:aaronmt]]

Kamil, it would be nice to do the same comparative testing again for verification purposes when this makes it's way to Nightly.

Comment 10

[Kamil Páral]

Sure, I will. Just please tell me once it is available at http://nightly.mozilla.org/. And thanks for the quick fix.

Comment 11

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/877cd1bb9912

Comment 12

[Aaron Train [:aaronmt]]

(In reply to Kamil Páral from comment #10)
> Sure, I will. Just please tell me once it is available at
> http://nightly.mozilla.org/. And thanks for the quick fix.

This is in Nightly now as of the 30th of July.

Comment 13

[Kamil Páral]

I have downloaded today's Nightly and tested it, it seems to be fixed. I don't see any increased battery consumption even with light conditions autodetection. Great job!

Comment 14

[Aaron Train [:aaronmt]]

re: aurora/beta

Comment 15

[Mark Finkle (:mfinkle) (use needinfo?)]

(In reply to Aaron Train [:aaronmt] from comment #14)
> re: aurora/beta

Given the verification in comment 13, yes, we should uplift as far as we can. Reducing battery consumption is a big deal.

Comment 16

[Mark Finkle (:mfinkle) (use needinfo?)]

Comment on attachment 8463547 [details] [diff] [review]
devicelight-pagevisible v0.2

Approval Request Comment
[Feature/regressing bug #]:
[User impact if declined]: Increased battery consumption
[Describe test coverage new/current, TBPL]: Reduced power usage was verified
[Risks and why]: Low. Functionality still seems to work fine.
[String/UUID change made/needed]: None

Comment 17

[Mark Finkle (:mfinkle) (use needinfo?)]

https://hg.mozilla.org/releases/mozilla-aurora/rev/b578163affd0
https://hg.mozilla.org/releases/mozilla-beta/rev/a59d3af0c000