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

Status: RESOLVED WORKSFORME

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

Description

[Doug Turner (:dougt)]

+++ 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

Comment 1

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

(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.

Comment 2

[Doug Turner (:dougt)]

> Let's see if this is still true.

Tested yesterday

Comment 3

[Wesley Johnston (:wesj)]

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.

Comment 4

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

I am left to assume that sensors are not tied into the docShell activation mechanism. Should they be?

Comment 5

[Doug Turner (:dougt)]

wes, is nsGlobalWindow::SuspendTimeouts being called when 'isActive = false' on the docshell?  I don't see that.

Comment 6

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

(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.

Comment 7

[Doug Turner (:dougt)]

well, is nsGlobalWindow::SetIsBackground called?

Comment 8

[Wesley Johnston (:wesj)]

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.

Comment 9

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

Assigning to Wes since he's been looking at it

Comment 10

[Sriram Ramasubramanian [:sriram]]

Attachment: Patch (1/3): Disable/Enable sensors in onApplicationPause()/Resume()

This patch moves the code to onApplicationPause() and Resume().

Comment 11

[Sriram Ramasubramanian [:sriram]]

Attachment: Patch (2/3): Remove onPause()

Because of previous patch, onPause() became empty. This can be removed.

Comment 12

[Sriram Ramasubramanian [:sriram]]

Attachment: Patch (3/3): Battery sensor cleanup.

Some little cleanup for the Battery sensor, just like how GeckoConnectivitySensor works.

Comment 13

[Sriram Ramasubramanian [:sriram]]

Ooops. Uploaded patches in wrong bug. :(

Comment 14

[Lucas Rocha (:lucasr)]

(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

Comment 15

[Lucas Rocha (:lucasr)]

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.

Comment 16

[Lucas Rocha (:lucasr)]

Attachment: Suspend timeouts on all tabs when Fennec goes to background

Comment 17

[Doug Turner (:dougt)]

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 18

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

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

Comment 19

[Doug Turner (:dougt)]

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

Comment 20

[(dormant account)]

(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.

Comment 21

[Lucas Rocha (:lucasr)]

Attachment: Suspend timeouts on all tabs when Fennec goes to background

Comment 22

[Lucas Rocha (:lucasr)]

(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.

Comment 23

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

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