Closed Bug 739729 Opened 9 years ago Closed 8 years ago

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

Categories

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

ARM
Android
defect

Tracking

()

RESOLVED WORKSFORME
Tracking Status
firefox12 --- affected
blocking-fennec1.0 --- +
fennec 11+ ---

People

(Reporter: dougt, Assigned: lucasr)

References

(Blocks 1 open bug)

Details

(Keywords: perf)

Attachments

(1 file, 4 obsolete files)

+++ 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
> 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?
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.
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
This patch moves the code to onApplicationPause() and Resume().
Attachment #612341 - Flags: review?(mark.finkle)
Attached patch Patch (2/3): Remove onPause() (obsolete) — Splinter Review
Because of previous patch, onPause() became empty. This can be removed.
Attachment #612342 - Flags: review?(mark.finkle)
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
(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
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.
Attachment #623133 - Flags: review?(mark.finkle)
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
Closed: 8 years ago
Resolution: --- → WORKSFORME
iirc, chrome does suspend things.  and, if so, it will kill us in terms of power uses.
(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.
Attachment #623133 - Attachment is obsolete: true
(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.
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.