We do not disable sensors when fennec is put into the background

RESOLVED WORKSFORME

Status

()

Firefox for Android
General
P2
normal
RESOLVED WORKSFORME
6 years ago
2 years ago

People

(Reporter: dougt, Assigned: lucasr)

Tracking

(Blocks: 1 bug, {perf})

unspecified
ARM
Android
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox12 affected, blocking-fennec1.0 +, fennec11+)

Details

Attachments

(1 attachment, 4 obsolete attachments)

(Reporter)

Description

6 years ago
+++ This bug was initially created as a clone of Bug #718364 +++

Checked battery summary after battery ran down overnight. (2-3 nights ago)


We do not disable sensors when fennec is put into the background.  This is, of course, excess background CPU use
(In reply to Doug Turner (:dougt) from comment #0)
> +++ This bug was initially created as a clone of Bug #718364 +++
> 
> Checked battery summary after battery ran down overnight. (2-3 nights ago)

Not this bug

> We do not disable sensors when fennec is put into the background.  This is,
> of course, excess background CPU use

Let's see if this is still true.
Summary: Excess background CPU use → We do not disable sensors when fennec is put into the background
(Reporter)

Comment 2

6 years ago
> Let's see if this is still true.

Tested yesterday
dougt, can you give more details about how you expect this to work? Do we need to call an API or write a new one? AFAICT, setting isActive = false on the docshell will trigger this code:

http://mxr.mozilla.org/mozilla-central/source/docshell/base/nsDocShell.cpp#6270
which tells the presShell to throttle timers and stuff:
http://mxr.mozilla.org/mozilla-central/source/layout/base/nsPresShell.cpp#9045
and fires some visibility events for the page. That works in my tests, but has nothing to do with sensors.

Sensors are deactivated when we unregister all our listeners in Hal.cpp. Listeners are removed in the DeviceSensors destructor. I... got tired of digging at that point and thought I'd just ask.
I am left to assume that sensors are not tied into the docShell activation mechanism. Should they be?
(Reporter)

Comment 5

6 years ago
wes, is nsGlobalWindow::SuspendTimeouts being called when 'isActive = false' on the docshell?  I don't see that.
(In reply to Doug Turner (:dougt) from comment #5)
> wes, is nsGlobalWindow::SuspendTimeouts being called when 'isActive = false'
> on the docshell?  I don't see that.

I don't think it would be. We don't suspend timeouts when isActive = false, we just throttle them.
(Reporter)

Comment 7

6 years ago
well, is nsGlobalWindow::SetIsBackground called?
blocking-fennec1.0: ? → +
Yep.

But I think I have seen a bug. We trigger these methods most of the time for me, but if I open a few tabs and switch back and forth a little, sometimes we stop. I'm guess that something in Android becomes confused and stops sending application-background notifications to us? Will dig in more later.
Assigning to Wes since he's been looking at it
Assignee: nobody → wjohnston
Depends on: 739747
Created attachment 612341 [details] [diff] [review]
Patch (1/3): Disable/Enable sensors in onApplicationPause()/Resume()

This patch moves the code to onApplicationPause() and Resume().
Attachment #612341 - Flags: review?(mark.finkle)
Created attachment 612342 [details] [diff] [review]
Patch (2/3): Remove onPause()

Because of previous patch, onPause() became empty. This can be removed.
Attachment #612342 - Flags: review?(mark.finkle)
Created attachment 612343 [details] [diff] [review]
Patch (3/3): Battery sensor cleanup.

Some little cleanup for the Battery sensor, just like how GeckoConnectivitySensor works.
Attachment #612343 - Flags: review?(mark.finkle)
Ooops. Uploaded patches in wrong bug. :(
Attachment #612341 - Attachment is obsolete: true
Attachment #612341 - Flags: review?(mark.finkle)
Attachment #612342 - Attachment is obsolete: true
Attachment #612342 - Flags: review?(mark.finkle)
Attachment #612343 - Attachment is obsolete: true
Attachment #612343 - Flags: review?(mark.finkle)
Assignee: wjohnston → lucasr.at.mozilla
(Assignee)

Comment 14

6 years ago
(In reply to Doug Turner (:dougt) from comment #7)
> well, is nsGlobalWindow::SetIsBackground called?

Yes, it's called here...

http://mxr.mozilla.org/mozilla-central/source/docshell/base/nsDocShell.cpp#4994

...when we set isActive on docshell here:

 https://mxr.mozilla.org/mozilla-central/source/toolkit/content/widgets/browser.xml#264
(Assignee)

Comment 15

6 years ago
After some digging, I confirm that we are disabling device orientation notifications when Fennec in background. i.e. GeckoScreenOrientationListener:disableNotifications() is called when apps goes to background.

Furthermore, we throttle (but don't suspend) timeouts on inactive tabs (http://mxr.mozilla.org/mozilla-central/source/layout/base/nsPresShell.cpp#8881), as mentioned by Finkle and Wes.

So, it seems to me that the remaining question for this bug is: do we want to go one step further and completely suspend timeouts on all tabs when Fennec goes to background? It would probably just be about calling SuspendTimeouts/ResumeTimeouts on each tab when Fennec is completely inactive. Seems like the right thing to do but I might be missing something. Mark, Doug, what do you think?

I'll submit a patch with the above-mentioned change if we agree to suspend timeouts.
(Assignee)

Comment 16

6 years ago
Created attachment 623133 [details] [diff] [review]
Suspend timeouts on all tabs when Fennec goes to background
Attachment #623133 - Flags: review?(mark.finkle)
(Reporter)

Comment 17

6 years ago
When in the background, we should suspend as much as we can.  My reasoning is that we are going to get more beaten up about power usage than some game or music player running in the background.  However, I defer to the rest of the fennec team to make this call.
Comment on attachment 623133 [details] [diff] [review]
Suspend timeouts on all tabs when Fennec goes to background

>-    let tab = BrowserApp.selectedTab;
>-    if (tab && tab.getActive() != isForeground) {
>-      tab.setActive(isForeground);

We would still want to call tab.setActive

But I don't want to suspend timers yet. Throttling them seems ok for now. If we find that suspending them is needed, we can use a variant of this patch.

If we have verified that DOM sensor events are being throttled when going into the background, then this bug is WORKSFORME
Attachment #623133 - Flags: review?(mark.finkle) → review-
Status: NEW → RESOLVED
Last Resolved: 6 years ago
Resolution: --- → WORKSFORME
(Reporter)

Comment 19

6 years ago
iirc, chrome does suspend things.  and, if so, it will kill us in terms of power uses.

Comment 20

6 years ago
(In reply to Doug Turner (:dougt) from comment #19)
> iirc, chrome does suspend things.  and, if so, it will kill us in terms of
> power uses.

I agree that it makes sense for us to match chrome behavior.
(Assignee)

Comment 21

6 years ago
Created attachment 625601 [details] [diff] [review]
Suspend timeouts on all tabs when Fennec goes to background
Attachment #625601 - Flags: review?(mark.finkle)
(Assignee)

Updated

6 years ago
Attachment #623133 - Attachment is obsolete: true
(Assignee)

Comment 22

6 years ago
(In reply to Mark Finkle (:mfinkle) from comment #18)
> Comment on attachment 623133 [details] [diff] [review]
> Suspend timeouts on all tabs when Fennec goes to background
> 
> >-    let tab = BrowserApp.selectedTab;
> >-    if (tab && tab.getActive() != isForeground) {
> >-      tab.setActive(isForeground);
> 
> We would still want to call tab.setActive

Forgot to qref my patch before submitting. My intention was to keep the setActive() call. New patch has the correct behavior. Posting here just for future reference.

Updated

6 years ago
Blocks: 757288
Comment on attachment 625601 [details] [diff] [review]
Suspend timeouts on all tabs when Fennec goes to background

We could achieve the same thing by using the preference:
"dom.min_background_timeout_value"

The preference sets the min timeout allowed when a tab is in the background (even if Fennec is still in the foreground) and setting it to a high value, like:
60 000 -> for throttling to 1 minute
3 600 000 -> for throttling to 1 hour
86 400 000 -> throttling to 1 day
Attachment #625601 - Flags: review?(mark.finkle) → review-
You need to log in before you can comment on or make changes to this bug.