Last Comment Bug 739913 - Use kernel wake lock when the "cpu" topic is locked
: Use kernel wake lock when the "cpu" topic is locked
Status: RESOLVED FIXED
:
Product: Core
Classification: Components
Component: DOM: Device Interfaces (show other bugs)
: unspecified
: All Gonk (Firefox OS)
: -- normal (vote)
: mozilla14
Assigned To: Kan-Ru Chen [:kanru] (UTC+8)
:
Mentors:
Depends on:
Blocks: 708964
  Show dependency treegraph
 
Reported: 2012-03-28 01:58 PDT by Kan-Ru Chen [:kanru] (UTC+8)
Modified: 2012-04-17 07:43 PDT (History)
8 users (show)
ryanvm: in‑testsuite-
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Add cpuMightSleep attribute to mozPower. (10.54 KB, patch)
2012-04-11 00:57 PDT, Kan-Ru Chen [:kanru] (UTC+8)
no flags Details | Diff | Review
Add cpuSleepAllowed attribute to mozPower. v1.1 (10.87 KB, patch)
2012-04-12 02:15 PDT, Kan-Ru Chen [:kanru] (UTC+8)
cjones.bugs: review+
Details | Diff | Review
Add cpuSleepAllowed attribute to mozPower. v2 (10.66 KB, patch)
2012-04-12 21:39 PDT, Kan-Ru Chen [:kanru] (UTC+8)
cjones.bugs: review+
Details | Diff | Review
Add cpuSleepAllowed attribute to mozPower. v2.1 (10.74 KB, patch)
2012-04-15 23:05 PDT, Kan-Ru Chen [:kanru] (UTC+8)
no flags Details | Diff | Review

Description Kan-Ru Chen [:kanru] (UTC+8) 2012-03-28 01:58:13 PDT
To provide a method for background app to keep the CPU on while the screen is off, we should write kernel wake lock to /sys/power/wake_lock on Gonk platform when "cpu" wake lock is acquired.
Comment 1 Justin Lebar (not reading bugmail) 2012-03-28 08:39:44 PDT
Are we going to have a whitelist of sites which are allowed to acquire this wakelock?
Comment 2 Kan-Ru Chen [:kanru] (UTC+8) 2012-03-28 16:47:02 PDT
(In reply to Justin Lebar [:jlebar] from comment #1)
> Are we going to have a whitelist of sites which are allowed to acquire this
> wakelock?

Android uses a single WAKE_LOCK permission to govern the whole wake lock usage. If we want a whitelist for the cpu, I think we should have a list for each lockable resource as well.

I have another concern that if we lock the cpu in Gecko then it is impossible to suspend the cpu from the content. Thus it take some of the policy management power off from the content.

Maybe we should do the other way around, add a suspendAllowed attribute to mozPower?
Comment 3 Justin Lebar (not reading bugmail) 2012-03-29 12:08:14 PDT
(In reply to Kan-Ru Chen [:kanru] from comment #2)
> If we want a whitelist for the cpu, I think we should have a list for
> each lockable resource as well.

Yes, I think so.  It seems to me that the CPU lock is pretty separate from the screen lock.

> I have another concern that if we lock the cpu in Gecko then it is
> impossible to suspend the cpu from the content. Thus it take some of the
> policy management power off from the content.

I don't understand what you mean.  Could you rephrase this?

> Maybe we should do the other way around, add a suspendAllowed attribute to
> mozPower?

I also don't understand what this would be useful for.  Is the point to allow the content to figure out whether its CPU wakelock will do anything?
Comment 4 Kan-Ru Chen [:kanru] (UTC+8) 2012-03-29 16:06:45 PDT
(In reply to Justin Lebar [:jlebar] from comment #3)
> > I have another concern that if we lock the cpu in Gecko then it is
> > impossible to suspend the cpu from the content. Thus it take some of the
> > policy management power off from the content.
> 
> I don't understand what you mean.  Could you rephrase this?

I mean we shouldn't make the policy decision in gecko. If we enable kernel wake lock for cpu then we also have to handle the foreground and background case for it.

> > Maybe we should do the other way around, add a suspendAllowed attribute to
> > mozPower?
> 
> I also don't understand what this would be useful for.  Is the point to
> allow the content to figure out whether its CPU wakelock will do anything?

Note this is also a privileged api. Because now turning off the screen implies the CPU may also be suspended. So we have to have (1) a way to control this behavior or (2) decouple them and add a suspend() method. I think (1) is good because suspend will not always success (depends on whether there are other kernel wake locks). We could just explicit don't allow it to suspend or allow it and know it may fail.
Comment 5 Justin Lebar (not reading bugmail) 2012-03-31 11:47:02 PDT
> I mean we shouldn't make the policy decision in gecko.

Ah, probably not.  But then what we need is for the "Gaia" back-end API to receive notifications of which origins are holding wake locks, right?

Like, maybe the API would be

 WakeLockChanged(lockTopic, data), where data is an array of objects, one for each origin which holds a lock:

   [{origin: "http://mozilla.org", visible: true}, {origin: ..., visible: false}]

Using a list of objects like this would let us send other data for each wake lock in the future, if we wanted to.

Your beautiful API, gone before we had a chance to use it!  :)
Comment 6 Justin Lebar (not reading bugmail) 2012-03-31 11:47:30 PDT
> > I mean we shouldn't make the policy decision in gecko.

> Ah, probably not.

I mean, I agree with you, we shouldn't make this policy decision in Gecko.
Comment 7 Rob Arnold [:robarnold] 2012-04-03 09:06:00 PDT
I am a little wary of adding an explicit wake lock since it seems rather easy to forget to remove it when it is no longer necessary. Such a permission is also privileged on Android yet most apps ask for it. Would it be possible to infer from the page's API usage (ex: playing audio/video) whether or not it needs a wake lock to be held on behalf of it?
Comment 8 Justin Lebar (not reading bugmail) 2012-04-03 09:45:29 PDT
> Would it be possible to infer from the page's API usage (ex: playing audio/video) whether or not it 
> needs a wake lock to be held on behalf of it?

That's in general very difficult.  For example, a turn-by-turn navigation app would want to stay alive in the background.  The only resource it would be using is GPS.

But using GPS isn't enough to prove that the app really wants to stay alive.  For example, my Yelp app watches GPS, but it doesn't need to stay alive while the phone is asleep.

I'm afraid that if we tried to infer "stay alive", pages which wanted to stay alive would resort to inefficient hacks, like playing a silent, infinite-length audio file in the background.

> Such a permission is also privileged on Android yet most apps ask for it.

We're going to have a better system, in which the user will be able to say "no" to a request like this, and the app should continue to work.

That of course doesn't mitigate the harm caused by pages which hold the lock longer than they need.  We could list the apps which are holding a CPU wake lock somewhere in the UI, or something, but that whole discussion is orthogonal to this.
Comment 9 Jonas Sicking (:sicking) PTO Until July 5th 2012-04-03 11:07:23 PDT
I definitely share the concern about people forgetting to release wake locks.

However given the use-cases I don't think we can infer them really.

One of the use-cases I had in mind is for the camera application. When you take a picture the camera application will likely want to do some post-processing on the picture, such as JPEG compressing, HDR computations, stitching together pictures into panorama pictures etc. After that the camera application will want to save the resulting file to disk. While all of that is happening we don't want to shut down the CPU since we want to ensure that the user's picture is saved.

Many of these operations will be purely in-memory operations. Others might involve several distinct IO operations (for example loading the separate HDR pictures, or the various pictures that will make up a panorama).

I don't see how we could infer how long to keep wake locks for these use cases.

What I think we can do is to use some heuristics though. For example we can add a timeout for how long a page is allowed to keep a wake lock. And if the timeout runs out we can allow pending IO operations to finish before shutting down the CPU. We could even reset the timeout timer on any IO operation if we wanted to.
Comment 10 Justin Lebar (not reading bugmail) 2012-04-03 11:09:36 PDT
> What I think we can do is to use some heuristics though. For example we can add a timeout for how 
> long a page is allowed to keep a wake lock

How would this work with a music app?  You'd simply kill it after an hour?

You could modify this heuristic: Kill it after an hour if it's not playing music.  But then my turn-by-turn navigation app will get killed, unless it plays a silent audio file...
Comment 11 Jonas Sicking (:sicking) PTO Until July 5th 2012-04-03 13:54:06 PDT
For music-players we could either use some heuristics like "don't timeout while playing music" yeah. Though for a music-player app I would imagine that we'd also need something like a "audio lock" since I think that by default we should turn off audio when the user presses the sleep button. So we could add the presence of that lock as additional information to the heuristics.

For a turn-by-turn navigation application I would imagine that we won't need to use the "cpu" lock at all. Wouldn't the application simply grab a lock which prevents the device from going into sleep mode at all since we'd want the screen to stay on? Whereas if the user presses the sleep button, I would expect the turn-by-turn application to fully go to sleep.
Comment 12 Justin Lebar (not reading bugmail) 2012-04-03 14:03:33 PDT
On my Android phone, I can turn off the screen and my turn-by-turn navigation program continues to run.  It checks GPS and speaks to me occasionally when it wants me to turn.

I think expiring the CPU lock after a period of time is really scary.  I do not think you're going to be able to get the heuristic right, and if we insist on using a heuristic anyway, we're just begging to be worked around in inefficient ways when someone tries to do something we haven't thought of.

> Though for a music-player app I would imagine that we'd also need something like a "audio lock" 
> since I think that by default we should turn off audio when the user presses the sleep button.

I think we should think about the CPU lock as "allow me to continue doing whatever I want when I'm in the background."  That is, if a page can play audio while it's in the foreground, then it can play audio while it's in the background, if it has the CPU lock.

Otherwise we're going to start introducing a bunch of background activity locks: background-audio, background-gps, background-bluetooth, background-network.  What's meaningful to me isn't that application X can use bluetooth while it's in the background, but that X can use bluetooth, and can stay alive while it's in the background.

Can you think of an app which I explicitly want to allow to run in the background, but which I only want to allow to use some API (e.g. audio, geolocation) while it's in the foreground, or otherwise while the device is awake?
Comment 13 Jonas Sicking (:sicking) PTO Until July 5th 2012-04-03 21:22:16 PDT
> On my Android phone, I can turn off the screen and my turn-by-turn
> navigation program continues to run.  It checks GPS and speaks to me
> occasionally when it wants me to turn.

Interesting. This is especially surprising to me given how much battery GPS uses. I personally would want to be in fairly tight control of when the app goes to sleep here.

I suspect the answer is to allow applications to choose what they want to do here.

> I think expiring the CPU lock after a period of time is really scary.  I do
> not think you're going to be able to get the heuristic right, and if we
> insist on using a heuristic anyway, we're just begging to be worked around
> in inefficient ways when someone tries to do something we haven't thought of.

At the same time, I think that simply exposing a lock/unlock API, people will lead to applications locking and the forgetting to unlock.

> > Though for a music-player app I would imagine that we'd also need something 
> > like a "audio lock" since I think that by default we should turn off audio when
> > the user presses the sleep button.
> 
> I think we should think about the CPU lock as "allow me to continue doing
> whatever I want when I'm in the background."  That is, if a page can play
> audio while it's in the foreground, then it can play audio while it's in the
> background, if it has the CPU lock.

This was something that I was pondering a lot originally too. Someone (I think it was you) pointed out that creating orthogonal locks is a simpler model.

Would you want to keep the wifi on while the "cpu" lock is held for example?

> Can you think of an app which I explicitly want to allow to run in the
> background, but which I only want to allow to use some API (e.g. audio,
> geolocation) while it's in the foreground, or otherwise while the device is
> awake?

The camera-app example is a good one I think. The camera app might very well want to turn on the GPS as well as orientation sensors in order to put meta-data into pictures as they are taken. As well as turn on the camera of course. However all of these seems like they aren't needed once the user presses the 'sleep' button and the only thing the camera does is to store process pixel data and store it to disk.

It's not a matter of "*allow* to use some API" really. It's a matter of what we turn off by default. Pages should still be allowed to grab locks which keep these APIs alive.

But I agree it's really annoying if applications will have to grab a ton of locks just to be able to finish whatever task they were doing.
Comment 14 Justin Lebar (not reading bugmail) 2012-04-03 22:02:07 PDT
> I suspect the answer is to allow applications to choose what they want to do here.

Yes, precisely.  :)

> At the same time, I think that simply exposing a lock/unlock API, people will lead to applications 
> locking and the forgetting to unlock.

If this is a primary concern, then I think we shouldn't have a lock/unlock API at all.  Having a lock-but-we-might-revoke-this-lock-if-we-think-you're-not-actually-using-it API is a poor compromise.

I have no idea what this non-locking API would look like, or if one is even possible.  I tend to think not.  :)

> Would you want to keep the wifi on while the "cpu" lock is held for example?

You'd probably have to hold the CPU lock plus the network lock if you wanted to do this.  Or perhaps the "background network lock" implies the CPU lock.

The difference is, for geolocation or audio, you actively frob an API when you want to use it.  For the network, you don't -- you just make network requests.  So the app needs a way to say "hey, I'm going to be doing network things now, and I really want them to work!".

There's a separate policy question of, do we want to allow users to give an app permission to use geolocation, but only in the foreground?  I think this is unnecessary granularity, personally.  But my main point is that we don't *need* a background-geolocation lock in the same way that we need a background wifi lock.

> It's not a matter of "*allow* to use some API" really. It's a matter of what we turn off by 
> default. Pages should still be allowed to grab locks which keep these APIs alive.

I don't understand.  I thought your proposal is that we should cut locks if we think the page isn't using it.  For example, if the page doesn't appear to be using background CPU for something useful (e.g. playing audio), I thought you were saying we should revoke its CPU lock.

> The camera-app example is a good one I think.

Yeah, the case of "allow this app to finish some things up" is pretty different from the CPU lock, as I've thought about it.  Maybe we need some separate policy to handle this.

But in any case, suppose the camera app acquired the CPU lock here, and suppose that implied access to geolocation etc.  The app does its thing, then gives up the CPU lock.  Boom, done.  We allowed geolocation etc for a few seconds, and then it's over.  Not a big deal.
Comment 15 Kan-Ru Chen [:kanru] (UTC+8) 2012-04-05 21:25:31 PDT
(In reply to Justin Lebar [:jlebar] from comment #5)
> > I mean we shouldn't make the policy decision in gecko.
> 
> Ah, probably not.  But then what we need is for the "Gaia" back-end API to
> receive notifications of which origins are holding wake locks, right?
> 
> Like, maybe the API would be
> 
>  WakeLockChanged(lockTopic, data), where data is an array of objects, one
> for each origin which holds a lock:
> 
>    [{origin: "http://mozilla.org", visible: true}, {origin: ..., visible:
> false}]
> 
> Using a list of objects like this would let us send other data for each wake
> lock in the future, if we wanted to.
> 
> Your beautiful API, gone before we had a chance to use it!  :)

In fact we already have a little PowerManager in shell.js ;-)

I think it depends on when we check the permission and the permission model we are building. If we check the permission of each allowed topic at requestWakeLock time then we don't have to expose the origin/permission list to the content.

What I want to address in this bug is to provide a means for content to control whether the device is allowed to go to sleep automatically. Add a cpuSuspendable attribute which resembles the screenEnabled attribute seems reasonable to me.
Comment 16 Justin Lebar (not reading bugmail) 2012-04-06 09:16:55 PDT
> If we check the permission of each allowed topic at requestWakeLock time then we don't have to 
> expose the origin/permission list to the content.

Do you mean, content calls requestWakeLock, and then we call a new API method in the back-end API which asks "do you want to allow this call?"  That might work.

> Add a cpuSuspendable attribute which resembles the screenEnabled attribute seems reasonable to me.

Okay, so this is the back-end of the CPU wake lock.  We can address the front-end permissions issue separately, if you'd like!
Comment 17 Kan-Ru Chen [:kanru] (UTC+8) 2012-04-08 19:06:07 PDT
(In reply to Justin Lebar [:jlebar] from comment #16)
> > If we check the permission of each allowed topic at requestWakeLock time
> > then we don't have to expose the origin/permission list to the content.
> 
> Do you mean, content calls requestWakeLock, and then we call a new API
> method in the back-end API which asks "do you want to allow this call?" 
> That might work.

Sort of like that. I image we could use the permission API here.

> 
> > Add a cpuSuspendable attribute which resembles the screenEnabled attribute
> > seems reasonable to me.
> 
> Okay, so this is the back-end of the CPU wake lock.  We can address the
> front-end permissions issue separately, if you'd like!

Sure, I will bring this to the webapi ml.
Comment 18 Kan-Ru Chen [:kanru] (UTC+8) 2012-04-11 00:57:33 PDT
Created attachment 613894 [details] [diff] [review]
Add cpuMightSleep attribute to mozPower.
Comment 19 Justin Lebar (not reading bugmail) 2012-04-11 09:27:43 PDT
I don't like "cpuMightSleep".  "cpuMaySleep" might be better [1], but I'm not wild about that either.

+    /**
+     * Is it possible that the device's CPU sleep after the screen is disabled?
+     * Setting this attribute to false will prevent the device entering suspend
+     * state.
+     */

"Is it possible that the device's CPU /will/ sleep".  And does it happen /after/ the screen is disabled, or /when/ the screen is disabled?

[1] The difference between "may" and "might" is subtle.  http://topics.blogs.nytimes.com/2009/07/14/may-might-muddle/  In this case, I'd read "may" in the "has permission to" sense, but you could also read it in the "more-likely-to-happen-than-might" sense.
Comment 20 Kan-Ru Chen [:kanru] (UTC+8) 2012-04-11 19:17:35 PDT
(In reply to Justin Lebar [:jlebar] from comment #19)
> I don't like "cpuMightSleep".  "cpuMaySleep" might be better [1], but I'm
> not wild about that either.
> 
> +    /**
> +     * Is it possible that the device's CPU sleep after the screen is
> disabled?
> +     * Setting this attribute to false will prevent the device entering
> suspend
> +     * state.
> +     */
> 
> "Is it possible that the device's CPU /will/ sleep".  And does it happen
> /after/ the screen is disabled, or /when/ the screen is disabled?

After. There is a short time interval before the CPU is actually suspended and many wake up event can interrupt the suspend.

> [1] The difference between "may" and "might" is subtle. 
> http://topics.blogs.nytimes.com/2009/07/14/may-might-muddle/  In this case,
> I'd read "may" in the "has permission to" sense, but you could also read it
> in the "more-likely-to-happen-than-might" sense.

I choose "might" because the uncertainty natural of the suspend process. Even cpuMightSleep is true, the suspend process might be interrupted by, for example, a MMC write. But I'm not wild about that too, a better name is always welcome.
Comment 21 Justin Lebar (not reading bugmail) 2012-04-11 19:39:25 PDT
How about cpuSleepDisabled?
Comment 22 Kan-Ru Chen [:kanru] (UTC+8) 2012-04-11 20:03:16 PDT
(In reply to Justin Lebar [:jlebar] from comment #21)
> How about cpuSleepDisabled?

Not bad. How about cpuSleepEnabled? So that it feels like the screenEnabled attribute.
Comment 23 Justin Lebar (not reading bugmail) 2012-04-11 20:09:05 PDT
Except cpuSleepEnabled = true sounds like the CPU is currently asleep, which isn't right.  Whereas cpuSleepDisabled = true can be correctly interpreted as the CPU is not asleep and the CPU won't be going to sleep.
Comment 24 Justin Lebar (not reading bugmail) 2012-04-11 20:10:40 PDT
Maybe cpuSleepOnScreenOff or cpuSleepOnScreenDisabled?  If it's just a short delay between screen off and CPU sleep, that could be OK...
Comment 25 Kan-Ru Chen [:kanru] (UTC+8) 2012-04-11 20:18:42 PDT
I don't want to tie the behavior to screen too much. So let's go for cpuSleepDisabled.
Comment 26 Chris Jones [:cjones] inactive; ni?/f?/r? if you need me 2012-04-12 00:42:22 PDT
I vote for |cpuSleepAllowed|, defaulting to true.  enabled/disabled often implies a binary state, where !disabled => enabled and !enabled => disabled.  So one might think that  |cpuSleepDisabled = false| => cpu goes to sleep.

Here, we're enabling or disabling a *policy*, not a device state.  So I'd prefer the "Allowed" language, which suggests "we'll let the CPU go to sleep if it wants to, or not let it."
Comment 27 Chris Jones [:cjones] inactive; ni?/f?/r? if you need me 2012-04-12 00:51:40 PDT
Comment on attachment 613894 [details] [diff] [review]
Add cpuMightSleep attribute to mozPower.

(The review below is of course independent of the bikeshedding.
Please re-test after updating for name changes. :) )

>diff --git a/b2g/chrome/content/shell.js b/b2g/chrome/content/shell.js

>+    if (topic == "cpu") {
>+      if (state != "unlocked") {
>+        navigator.mozPower.cpuMightSleep = false;
>+      } else {
>+        navigator.mozPower.cpuMightSleep = true;
>+      }

The possible states here are "locked" and "unlocked" right?  I sort of prefer this logic

  cpuSleepAllowed = (state == "locked");

That is, we only disable sleep if the CPU has been explicitly locked.
It doesn't really matter here but it's a good habit to be in.  (CPU
sleep disabled is the more "dangerous" state because it will drain
battery.)

>diff --git a/hal/Hal.cpp b/hal/Hal.cpp

>+bool GetCpuMightSleep()
>+{
>+  AssertMainThread();
>+  RETURN_PROXY_IF_SANDBOXED(GetCpuMightSleep());
>+}
>+

Since this is a privileged interface, the synchronous getter is OK.
For future reference, interfaces that are accessible by normal web
content should cache their results in Hal.cpp and be notified on state
changes, like what the battery hal:: API does.

Please add a comment to this effect.

>diff --git a/hal/gonk/GonkHal.cpp b/hal/gonk/GonkHal.cpp

>+// We can read wakeLockFilename to find out whether the cpu wake lock
>+// is already acquired, but reading and parsing it is a lot more work
>+// than track it ourselves, and it won't be accurate anyway (kernel

"tracking"

This looks good, but I'd like to take another look at the version that
includes the naming change.
Comment 28 Kan-Ru Chen [:kanru] (UTC+8) 2012-04-12 01:10:13 PDT
(In reply to Chris Jones [:cjones] [:warhammer] from comment #27)
> The possible states here are "locked" and "unlocked" right?

"locked" has two variants, namely "locked-foreground" and "locked-background". I would like to allow all locked case in the beginning.

> I sort of prefer this logic
> 
>   cpuSleepAllowed = (state == "locked");
> 
> That is, we only disable sleep if the CPU has been explicitly locked.
> It doesn't really matter here but it's a good habit to be in.  (CPU
> sleep disabled is the more "dangerous" state because it will drain
> battery.)

Sure, I can check the two locked state explicitly.
Comment 29 Kan-Ru Chen [:kanru] (UTC+8) 2012-04-12 01:56:55 PDT
>   cpuSleepAllowed = (state == "locked");
> 
> That is, we only disable sleep if the CPU has been explicitly locked.
> It doesn't really matter here but it's a good habit to be in.  (CPU
> sleep disabled is the more "dangerous" state because it will drain
> battery.)

I'm confused. Did you want to say:

    cpuSleepAllowed = (state == "unlocked");
Comment 30 Kan-Ru Chen [:kanru] (UTC+8) 2012-04-12 02:15:28 PDT
Created attachment 614302 [details] [diff] [review]
Add cpuSleepAllowed attribute to mozPower. v1.1

With name changed and above comments addressed.
Comment 31 Jonas Sicking (:sicking) PTO Until July 5th 2012-04-12 02:33:55 PDT
(In reply to Justin Lebar [:jlebar] from comment #14)
> > I suspect the answer is to allow applications to choose what they want to do here.
> 
> Yes, precisely.  :)
> 
> > At the same time, I think that simply exposing a lock/unlock API, people will lead to applications 
> > locking and the forgetting to unlock.
> 
> If this is a primary concern, then I think we shouldn't have a lock/unlock
> API at all.  Having a
> lock-but-we-might-revoke-this-lock-if-we-think-you're-not-actually-using-it
> API is a poor compromise.

I would definitely say it's a concern. But we have to weigh it against the cost of not having the ability to lock the CPU awake at all and the resulting applications that can't be written.

But I guess I'm ok with not adding any heuristics for now. If we add heuristics in the future they should be clever enough to be mostly transparent anyway.

> > Would you want to keep the wifi on while the "cpu" lock is held for example?
> 
> You'd probably have to hold the CPU lock plus the network lock if you wanted
> to do this.  Or perhaps the "background network lock" implies the CPU lock.
> 
> The difference is, for geolocation or audio, you actively frob an API when
> you want to use it.  For the network, you don't -- you just make network
> requests.  So the app needs a way to say "hey, I'm going to be doing network
> things now, and I really want them to work!".

Not necessarily. I believe that geolocation has a mode where you tell it "give me continuous updates" which is easy to have left on longer than needed when grabbing a CPU lock just to save a file.

Same thing with other sensors which can be put in a "give me continuous updates" state.

> There's a separate policy question of, do we want to allow users to give an
> app permission to use geolocation, but only in the foreground?

This is a fine question, but one that I think we shouldn't debate here. There are definitely concerns that allowing a page to keep accelerators on when the user puts the device to sleep an in his pocket, allows the app to track a user's movement through a home or city. But again, I think this is off topic for this bug and more related to security reviews of the individual sensors (geolocation being one of them).

> > It's not a matter of "*allow* to use some API" really. It's a matter of what we turn off by 
> > default. Pages should still be allowed to grab locks which keep these APIs alive.
> 
> I don't understand.  I thought your proposal is that we should cut locks if
> we think the page isn't using it.  For example, if the page doesn't appear
> to be using background CPU for something useful (e.g. playing audio), I
> thought you were saying we should revoke its CPU lock.

I think this was on the topic of "what do we keep on by default when the CPU lock is held and the user puts the device in sleep mode". So orthogonal to the question of how long the CPU lock can be held.
Comment 32 Chris Jones [:cjones] inactive; ni?/f?/r? if you need me 2012-04-12 03:58:23 PDT
(In reply to Kan-Ru Chen [:kanru] from comment #29)
> >   cpuSleepAllowed = (state == "locked");
> > 
> > That is, we only disable sleep if the CPU has been explicitly locked.
> > It doesn't really matter here but it's a good habit to be in.  (CPU
> > sleep disabled is the more "dangerous" state because it will drain
> > battery.)
> 
> I'm confused. Did you want to say:
> 
>     cpuSleepAllowed = (state == "unlocked");

Er, yes.
Comment 33 Justin Lebar (not reading bugmail) 2012-04-12 06:05:44 PDT
>> The difference is, for geolocation or audio, you actively frob an API when
>> you want to use it.  For the network, you don't -- you just make network
>> requests.  So the app needs a way to say "hey, I'm going to be doing network
>> things now, and I really want them to work!".
>
> Not necessarily. I believe that geolocation has a mode where you tell it "give me continuous 
> updates" which is easy to have left on longer than needed when grabbing a CPU lock just to save a 
> file.
> 
> Same thing with other sensors which can be put in a "give me continuous updates" state.

Perhaps "actively" is the wrong modifier.  You have to touch a Special API, at some point, in order to get data out of geolocation et al, which makes it different from the network, where there are a ton of ways, implicit and explicit, to generate network traffic.

In general, if the CPU lock implies that you can continue accessing sensors while you're in the background, then yes, it's easy to keep a sensor on that you don't actually need.  We could protect against this by having separate background-geolocation, background-accelerometer, etc. locks.  At least this way, you'd have to explicitly ask to keep something on in the background.

I'd be OK with that, but the question was originally about *permissions* -- should permission to lock the CPU plus permission to play audio in the foreground imply permission to play audio in the background?  We can discuss that elsewhere, if you like, but that's separate from how we structure the locks themselves.

I think we all agree that the CPU lock is ill-suited to the "imaletyoufinish, but first I want to save a file" case.  It's error-prone and gives the app way more privilege than it actually wants.  Maybe we'd add a "short CPU lock" which expires after a few seconds.  Or maybe we'd not put the device to sleep if we're in the middle of file I/O.  Or, there are a bunch of options we can discuss not in this bug.  :)

I like cpuSleepAllowed.  </bikeshed>
Comment 34 Kan-Ru Chen [:kanru] (UTC+8) 2012-04-12 06:20:43 PDT
> I think we all agree that the CPU lock is ill-suited to the
> "imaletyoufinish, but first I want to save a file" case.  It's error-prone
> and gives the app way more privilege than it actually wants.  Maybe we'd add
> a "short CPU lock" which expires after a few seconds.  Or maybe we'd not put
> the device to sleep if we're in the middle of file I/O.  Or, there are a
> bunch of options we can discuss not in this bug.  :)

A single write operation will be protected by the kernel. But multiple write still have to be protected, just like protecting the critical section in multi-thread program.

How about a lock that automatically times out, but also receives timeout event so that it can extend the life time by requesting a new lock?
Comment 35 Justin Lebar (not reading bugmail) 2012-04-12 06:25:15 PDT
We should figure out this short-CPU-lock elsewhere, probably only once we have someone who wants to use it.
Comment 36 Ryan VanderMeulen [:RyanVM] 2012-04-12 16:28:22 PDT
https://hg.mozilla.org/integration/mozilla-inbound/rev/b589fe2441a5

Also, to make life easier for those checking in patches for you, please follow the guidelines given in the link below (in this case, it was missing a commit message). Thanks!
https://developer.mozilla.org/en/Mercurial_FAQ#How_can_I_generate_a_patch_for_somebody_else_to_check-in_for_me.3F
Comment 37 Ryan VanderMeulen [:RyanVM] 2012-04-12 17:04:03 PDT
Backed out due to OSX 10.7 bustage.
http://mozillamemes.tumblr.com/post/19498220636/try-server-takes-the-beatings-so-mozilla-inbound

10.7 bustage:
https://tbpl.mozilla.org/php/getParsedLog.php?id=10854921&tree=Mozilla-Inbound
Undefined symbols for architecture i386:
  "mozilla::hal_impl::SetCpuSleepAllowed(bool)", referenced from:
      mozilla::hal::SetCpuSleepAllowed(bool) in Hal.o
  "mozilla::hal_impl::GetCpuSleepAllowed()", referenced from:
      mozilla::hal::GetCpuSleepAllowed()     in Hal.o
ld: symbol(s) not found for architecture i386
collect2: ld returned 1 exit status

https://hg.mozilla.org/integration/mozilla-inbound/rev/1ae36c75ffb3
Comment 38 Chris Jones [:cjones] inactive; ni?/f?/r? if you need me 2012-04-12 19:54:25 PDT
Kan-Ru, let's make a hal/fallback/FallbackWakeLocks.cpp file and use it everywhere non-gonk for now.
Comment 39 Kan-Ru Chen [:kanru] (UTC+8) 2012-04-12 21:39:37 PDT
Created attachment 614676 [details] [diff] [review]
Add cpuSleepAllowed attribute to mozPower. v2

Added patch header and use FallbackWakeLocks.cpp for non-gonk build.

Try build: https://tbpl.mozilla.org/?tree=Try&rev=b07e1b59f830
Comment 40 Chris Jones [:cjones] inactive; ni?/f?/r? if you need me 2012-04-13 07:50:44 PDT
Comment on attachment 614676 [details] [diff] [review]
Add cpuSleepAllowed attribute to mozPower. v2

>+    if (topic == "cpu") {
>+      navigator.mozPower.cpuSleepAllowed = (state == "unlocked");
>+    }
>   }

r=me if you revert this change ;).  (I prefer the logic from the last patch.)
Comment 41 Kan-Ru Chen [:kanru] (UTC+8) 2012-04-15 23:05:35 PDT
Created attachment 615250 [details] [diff] [review]
Add cpuSleepAllowed attribute to mozPower. v2.1
Comment 42 Ryan VanderMeulen [:RyanVM] 2012-04-16 15:36:56 PDT
https://hg.mozilla.org/integration/mozilla-inbound/rev/ad0867e065d8
Comment 43 Marco Bonardo [::mak] 2012-04-17 07:43:13 PDT
https://hg.mozilla.org/mozilla-central/rev/ad0867e065d8

Note You need to log in before you can comment on or make changes to this bug.