797798 - System App can't Receive Sensor Event During Early Suspend State.

Status: RESOLVED FIXED

(Firefox OS Graveyard :: General, defect)

Description

[Marco Chen [:mchen]]

During early suspend state, all of nsPIDOMWindow (including system app) will be set as "backgound", this prevent nsDeviceSensors from sending sensor event to foreground windows.

Use Case: During implementing bug 753842.
  1. During in call state.
  2. When phone is near to face, system app (or dial app) set device into early suspend state.
  3. When phone is away of face, system or dial app wake up the device.

Comment 1

[StevenLee[:slee]]

Hi Justin,

I have 2 plans
1. Separate these 2 attributes, background and active, in nsIDocShell. When system app is set to invisible[1], nsIDocShell::isActive is called[2]. It sets system app to background and inactive. For some apps, like system app, when it is invisible, it should able to handle some events, such as "userproximity" in this case.
Could I add one read/write attribute, say isBackground, to nsIDocShell that can set the background property of docshell?
2. In [3], |setAttribute('name', 'system') | and check the window's name in [4] to decide if this event needs to be dropped or not.

What do you think?

[1] http://mxr.mozilla.org/mozilla-central/source/b2g/chrome/content/shell.js#335
[2] http://http://mxr.mozilla.org/mozilla-central/source/dom/browser-element/BrowserElementChild.js#524
[3] http://mxr.mozilla.org/mozilla-central/source/b2g/chrome/content/shell.js#172
[4] http://mxr.mozilla.org/mozilla-central/source/dom/system/nsDeviceSensors.cpp#190

Thanks.

Comment 2

[Justin Lebar (not reading bugmail)]

> During early suspend state, all of nsPIDOMWindow (including system app) [are] set as "backgound". 
> This prevent nsDeviceSensors from sending sensor events to those windows.

What is "early suspend state", and why exactly is this happening?  Are all windows set as "background" when we turn the screen off?

> 1. Separate these 2 attributes, background and active, in nsIDocShell.

I think the idea of having both "background" and "invisible" attributes on nsIDocShell, which mean subtly different things, would be pretty confusing.

> 2. In [3], |setAttribute('name', 'system') | and check the window's name in [4] to decide if this 
> event needs to be dropped or not.

This would completely break abstraction; nsDeviceSensors should not care what an iframe's name is.  But more importantly, this doesn't work, because anybody could then set their name to "system" and receive background sensor notifications.

Could we have a "background-sensor" permission (or whatever) and check at [4] whether the window has that?

Comment 3

[StevenLee[:slee]]

(In reply to Justin Lebar [:jlebar] from comment #2)
> What is "early suspend state", and why exactly is this happening?  Are all
> windows set as "background" when we turn the screen off?
Early suspend is a suspend state in android. It's for saving the power. [1] has some information about it. 
As comment 1, during call state and the phone is near the face, system app should set device into early suspend state. mchen has implement a WIP version in system app. Please check [2]. It's a small patch.

> Could we have a "background-sensor" permission (or whatever) and check at
> [4] whether the window has that?
It sounds a good idea.
Thanks for your suggestions.

[1] http://www.thinksrc.com/2010/11/20/suspend-en.html#sec-6.2.1
[2] https://github.com/marcofreda527/gaia/commit/d072ac540a4000a44ed635d99fc8ea0a561e8238

Comment 4

[StevenLee[:slee]]

Attachment: patch

Add "background-sensors" permission. When the window is in background, nsDeviceSensor checks whether the window has "background-sensors" permission and decides to callback it or not.

Comment 5

[StevenLee[:slee]]

Attachment: Test patch in gaia

Register "userproximity" event in system app.

Comment 6

[StevenLee[:slee]]

Attachment: Add "background-sensors" permission in system app

Comment 7

[Justin Lebar (not reading bugmail)]

Comment on attachment 673798 [details] [diff] [review]
patch

This is good.  r=me with a few minor changes.

>diff --git a/dom/system/nsDeviceSensors.cpp b/dom/system/nsDeviceSensors.cpp
>--- a/dom/system/nsDeviceSensors.cpp
>+++ b/dom/system/nsDeviceSensors.cpp
>@@ -161,16 +162,37 @@ NS_IMETHODIMP nsDeviceSensors::RemoveWin
>+static bool
>+ShouldSkipEvent(nsCOMPtr<nsPIDOMWindow> aWindow)

We don't pass nsCOMPtr (or nsRefPtr or nsAutoPtr) as function arguments.

Instead, we pass a raw pointer (nsPIDOMWindow* aWindow) and rely on the fact
that our caller is holding a strong reference to aWindow.

I think all you need to change here is the function prototype; the rest of the
code should continue to work properly.

I also think this method would make a bit more sense if we called it
WindowCanReceiveSensorEvent and flipped true and false; in general, it's good
if methods don't have an implicit boolean-not baked into them, and it's often
good to provide a little more context in the name ("Sensor" and "Window", in
this case).

>+{
>+  bool skip = false;
>+  // check to see if this window is in the background.  if
>+  // it is and it does not have "background-sensors" permission,
>+  // don't send any device motion to it.

Please begin sentences with a capital letter.  This comment is good; it just
needs a couple of extra words:

s/"background-sensors" permission/the "background-sensors" permission/
s/any device motion/any device motion events/

>+  if (!aWindow || !aWindow->GetOuterWindow()) {
>+    skip = true;

      return true;

Then remove the else (because we don't do |else| after a return statement in Gecko).

>+  } else if (aWindow->GetOuterWindow()->IsBackground()) {
>+    nsCOMPtr<nsIPermissionManager> permMgr = 
>+      do_GetService(NS_PERMISSIONMANAGER_CONTRACTID);
>+    NS_ENSURE_TRUE(permMgr, false);
>+    uint32_t permission = nsIPermissionManager::DENY_ACTION;
>+    permMgr->TestPermissionFromWindow(aWindow, "background-sensors", &permission);
>+    skip = permission != nsIPermissionManager::ALLOW_ACTION;

return permission != nsIPermission::ALLOW_ACTION;

Then you can return false outside this if statement and remove |skip| altogether.

Comment 8

[StevenLee[:slee]]

Attachment: patch v2

Comment 9

[StevenLee[:slee]]

Attachment: patch v2.1

Remove trailing whitespaces.

Comment 10

[StevenLee[:slee]]

try server result: 
* try: -b do -p all -u none
  https://tbpl.mozilla.org/?tree=Try&rev=8ef2524cf34b
* try: -b d -p linux64 -u all -t none
  https://tbpl.mozilla.org/?tree=Try&rev=9ffc0ad4e3f2

Comment 11

[Kan-Ru Chen [:kanru] (UTC+9)]

This also needs to be fixed in the system app right?

Comment 12

[Kan-Ru Chen [:kanru] (UTC+9)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/b43af99c5e5b

Comment 13

[StevenLee[:slee]]

(In reply to Kan-Ru Chen [:kanru] from comment #11)
> This also needs to be fixed in the system app right?

Adding permission in system app.
https://github.com/mozilla-b2g/gaia/pull/5972

Comment 14

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/b43af99c5e5b

Comment 15

[Tim Taubert [:ttaubert] (inactive)]

This has been merged into gaia-master but hasn't been approved for Aurora. Can we please not do that? It breaks all Aurora builds if we merge it before it has been pushed to Aurora :(

Comment 16

[Justin Lebar (not reading bugmail)]

Can we get some approval love here?

Comment 17

[Tim Taubert [:ttaubert] (inactive)]

I think we need approval-mozilla-aurora for something that touches /dom/, no?

Comment 18

[Justin Lebar (not reading bugmail)]

(In reply to Tim Taubert [:ttaubert] from comment #17)
> I think we need approval-mozilla-aurora for something that touches /dom/, no?

These distinctions are pretty tiresome, I have to admit.

I think this is B2G only code?  The Android hw sensors do something different?

Comment 19

[Tim Taubert [:ttaubert] (inactive)]

Sorry don't want to say anything wrong. Just *thought* we need approval-mozilla-aurora. But, yeah this only touches B2G code so a blocker status might be sufficient.(?)

Comment 20

[Justin Lebar (not reading bugmail)]

> Sorry don't want to say anything wrong.

No, not at all.  You're not bothering me; the process is.  Sorry to suggest otherwise.  :)

> Just *thought* we need approval-mozilla-aurora. But, yeah this only touches B2G code so a blocker 
> status might be sufficient.(?)

I believe the rule is, patches with "zero risk for non-b2g" can get by with blocking-basecamp+ only.  If this is B2G-only code, then it certainly meets that criterion.

Comment 21

[Philipp von Weitershausen [:philikon]]

(In reply to Tim Taubert [:ttaubert] from comment #15)
> This has been merged into gaia-master but hasn't been approved for Aurora.
> Can we please not do that? It breaks all Aurora builds if we merge it before
> it has been pushed to Aurora :(

+1000

Folks, we need to start developing against m-a. That's what we're going to ship and what the dogfooders are using.

Comment 22

[Philipp von Weitershausen [:philikon]]

Also, what's with the [leave open] whiteboard flag? If something has landed on m-c, it's RESO/FIXED *no matter what*. m-a landing happens independently of that is tracked with the 'status-firefox19' flag.

Comment 23

[Justin Lebar (not reading bugmail)]

> m-a landing happens independently of that is tracked with the 'status-firefox19' flag.

ITYM status-firefox18.

It's not clear to me why this broke gaia.  All he did was add a permission.  What was the problem, exactly?

Comment 24

[[:fabrice] Fabrice Desré]

(In reply to Justin Lebar [:jlebar] from comment #23)
> > m-a landing happens independently of that is tracked with the 'status-firefox19' flag.
> 
> ITYM status-firefox18.
> 
> It's not clear to me why this broke gaia.  All he did was add a permission. 
> What was the problem, exactly?

We check the permissions at startup in PermissionsInstaller.jsm and fail hard when we have an unexpected permission.

Comment 25

[Justin Lebar (not reading bugmail)]

> We check the permissions at startup in PermissionsInstaller.jsm and fail hard when we have an 
> unexpected permission.

I see.  That's not very webby, is it?  Don't we normally ignore things we don't understand?

Comment 26

[Kan-Ru Chen [:kanru] (UTC+9)]

(In reply to Philipp von Weitershausen [:philikon] from comment #22)
> Also, what's with the [leave open] whiteboard flag? If something has landed
> on m-c, it's RESO/FIXED *no matter what*. m-a landing happens independently
> of that is tracked with the 'status-firefox19' flag.

I thought landing on gaia is also required to fix this bug, no?

Comment 27

[Kan-Ru Chen [:kanru] (UTC+9)]

Gaia change also landed.

Comment 28

[Andrew Overholt [:overholt]]

Let's mark this as a blocker so we don't regress.

Comment 29

[Gregor Wagner [:gwagner]]

Also landed on aurora. It broke our nighly builds because of an unknown permission.
https://hg.mozilla.org/releases/mozilla-aurora/rev/63cb20d6272b