Closed Bug 797798 Opened 7 years ago Closed 7 years ago

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

Categories

(Firefox OS Graveyard :: General, defect)

ARM
Gonk (Firefox OS)
defect
Not set

Tracking

(blocking-basecamp:+, firefox18 fixed, firefox19 fixed)

RESOLVED FIXED
blocking-basecamp +
Tracking Status
firefox18 --- fixed
firefox19 --- fixed

People

(Reporter: mchen, Assigned: slee)

References

Details

Attachments

(3 files, 2 obsolete files)

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.
Blocks: 753842
Assignee: nobody → 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.
> 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?
(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
Attached patch patch (obsolete) — Splinter Review
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.
Attachment #673798 - Flags: review?(justin.lebar+bug)
Register "userproximity" event in system app.
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.
Attachment #673798 - Flags: review?(justin.lebar+bug) → review+
Attached patch patch v2 (obsolete) — Splinter Review
Attachment #673798 - Attachment is obsolete: true
Attachment #674489 - Flags: review+
Attached patch patch v2.1Splinter Review
Remove trailing whitespaces.
Attachment #674489 - Attachment is obsolete: true
Attachment #674491 - Flags: review+
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
Keywords: checkin-needed
This also needs to be fixed in the system app right?
(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
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 :(
Status: NEW → ASSIGNED
Can we get some approval love here?
blocking-basecamp: --- → ?
I think we need approval-mozilla-aurora for something that touches /dom/, no?
(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?
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.(?)
> 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.
(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.
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.
> 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?
(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.
> 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?
(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?
Gaia change also landed.
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Whiteboard: [leave open]
Let's mark this as a blocker so we don't regress.
blocking-basecamp: ? → +
Also landed on aurora. It broke our nighly builds because of an unknown permission.
https://hg.mozilla.org/releases/mozilla-aurora/rev/63cb20d6272b
Duplicate of this bug: 805966
Depends on: 805966
You need to log in before you can comment on or make changes to this bug.