Prevent a background app/page to call mozLockOrientation

RESOLVED FIXED

Status

defect
--
critical
RESOLVED FIXED
6 years ago
5 years ago

People

(Reporter: alive, Assigned: alive)

Tracking

(Blocks 1 bug)

unspecified
ARM
Gonk (Firefox OS)
Dependency tree / graph

Firefox Tracking Flags

(blocking-b2g:tef+, firefox21 wontfix, firefox22 wontfix, firefox23 fixed, b2g18 fixed, b2g18-v1.0.0 wontfix, b2g18-v1.0.1 fixed)

Details

(Whiteboard: [games:p?])

Attachments

(2 attachments, 3 obsolete attachments)

See https://bugzilla.mozilla.org/show_bug.cgi?id=851642#c20

STR:
1. Have an app with a button like this:
document.getElementById('lol').onclick = function() {
  setTimeout(function() {
    var r = screen.mozLockOrientation('landscape-primary');
    console.log('[alive]', r);
  }, 5000);
}

2. Click the button and then press home button.

3. Wait 5 sec.

Expected:
Homescreen doesn't change the orientation

Actual:
Homescreen orientation is changed by a background app after 5sec..

Note:
The orientation webAPI needs to improve to solve this.
My thought:
1. check page visible state before call hal::LockScreenOrientation(orientation)
or 2. Add new API for system app to block the orientation API of mozbrowser iframe.

+++ This bug was initially created as a clone of Bug #851642 +++

This happens occasionally when debugging apps, that homescreen suddenly freely rotates freely between landscape and portrait.

screen.lockOrientation seems to unlock the screen at one point, I had the same problem in my app.

What happens:
At one point (not sure when), lockOrientation stops working and the screen feely rotates. This can be seen on homescreen but also apps using lockOrientation.

Steps to reproduce:
Install https://marketplace.firefox.com/app/game-pack/ , it uses screen.lockOrientation to lock orientation for each game. Switch between games and at one point lock orientation will stop working, only working again after restarting the whole app.

I see this happen a lot and it really breaks homescreen and apps UX. Therefor marking critical, maybe even blocker.
Assignee: nobody → alive
Patch v1: prevent background page to lock orientation

Mounir, could you please review this? Thanks!
Attachment #745809 - Flags: review?(mounir)
blocking-b2g: tef? → tef+
Thanks you for the quick Gecko bug fix!
No longer depends on: 851642
Comment on attachment 745809 [details] [diff] [review]
Patch: prevent background page to lock orientation

Review of attachment 745809 [details] [diff] [review]:
-----------------------------------------------------------------

::: dom/base/nsScreen.cpp
@@ +242,5 @@
>  
>    nsCOMPtr<nsIDOMDocument> domDoc;
>    owner->GetDocument(getter_AddRefs(domDoc));
>    nsCOMPtr<nsIDocument> doc = do_QueryInterface(domDoc);
>    if (!doc) {

Just do:
if (!doc || doc->Hidden()) {
  return LOCK_DENIED;
}

But I wonder if we couldn't have solved that issue by saving the state of each application and simply ignore locking made while not visible but apply it as soon as the app is visible again.

I guess this is good enough for the moment though.
Attachment #745809 - Flags: review?(mounir) → review+
Also, this might prevent the system application to change the screen orientation when the screen is off but I guess that should unlikely happen.
Posted patch refined patch (obsolete) — Splinter Review
refined patch.
Attachment #745809 - Attachment is obsolete: true
Alive, did you mean to carry to r+ on the refined patch and ask for a checkin?
Flags: needinfo?(alive)
(In reply to Milan Sreckovic [:milan] from comment #6)
> Alive, did you mean to carry to r+ on the refined patch and ask for a
> checkin?

Yes, I mistakenly remove the r+ flag...
but  AFAIK I need to pass try server before checkin-need...I am asking my colleague to help me on try server stuff. Thanks.
Flags: needinfo?(alive)
Whiteboard: [games:p?] → [status: has r+'d patch, awaiting try results][games:p?]
Posted patch checkin patchSplinter Review
checkin needed patch(r+d)
Thanks :rlin for try server help ;)
Attachment #746892 - Attachment is obsolete: true
https://hg.mozilla.org/projects/birch/rev/82ab0af69c16
Keywords: checkin-needed
Whiteboard: [status: has r+'d patch, awaiting try results][games:p?] → [games:p?]
https://hg.mozilla.org/mozilla-central/rev/82ab0af69c16
Status: NEW → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Alive, it seems that nsIDocument::Hidden() doesn't exist in b2g18. You should use nsIDOMDocument::GetHidden(bool*) for b2g18.
(In reply to Mounir Lamouri (:mounir) from comment #15)
> Alive, it seems that nsIDocument::Hidden() doesn't exist in b2g18. You
> should use nsIDOMDocument::GetHidden(bool*) for b2g18.

Seems my first patch works....
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Posted patch b2g18 patch (obsolete) — Splinter Review
I am not sure what's the process here.
Seems I need one patch for mc, one for b2g18?
Attachment #747746 - Flags: review?(mounir)
Comment on attachment 747746 [details] [diff] [review]
b2g18 patch

Review of attachment 747746 [details] [diff] [review]:
-----------------------------------------------------------------

r=me but make sure it compiles in b2g18 before asking for a checkin.

::: dom/base/nsScreen.cpp
@@ +245,5 @@
>    nsCOMPtr<nsIDocument> doc = do_QueryInterface(domDoc);
>    if (!doc) {
>      return LOCK_DENIED;
>    }
> +  

nit: trailing spaces.

@@ +251,5 @@
> +  bool hidden = false;
> +  domDoc->GetHidden(&hidden);
> +  if (hidden) {
> +    return LOCK_DENIED;
> +  })

I guess the ')' here is a typo?
Attachment #747746 - Flags: review?(mounir) → review+
Don't reopen bugs unless they're backed out from m-c. I don't see bugs needing uplift unless they're resolved.
Status: REOPENED → RESOLVED
Closed: 6 years ago6 years ago
Resolution: --- → FIXED
Attachment #747746 - Attachment is obsolete: true
Can you please provide steps to verify this fix - as we will blackbox test from the UI?
As per description, Game Pack app switch between games stops reply and needs to restart the whole app to make it working again
Unagi:
Environmental  Variables:
Build ID: 20130620230208
Gecko: http://hg.mozilla.org/releases/mozilla-b2g18/rev/a34f6d62cb05
Gaia: de211a86e1df621415942e8b02acea77efafd864
Platform Version: 18.0

Inari:
Environmental  Variables:
Build ID: 20130621070204
Gecko: http://hg.mozilla.org/releases/mozilla-b2g18_v1_0_1/rev/9c62297d11b0
Gaia: 93241eb6c5d6c110710fad8da3ccd4423312b0c9
Platform Version: 18.0
Hi everyone,

I was investigating bug 1008486 and I came to think that this issue is still present in v1.3/v1.4, i.e. Screen.mozLockOrientation apparently still grants/executes orientation-lock requests from background apps: (the following test commits are based on v1.3 branch)

Please take a look at my test app (at test_apps/template) at https://github.com/mnjul/gaia/commit/536d40c395a098fcd006e4e314ae590d11aa7bdb , which relentlessly tries to lock device orientation at landscape.

Steps for anomaly (ref https://www.youtube.com/watch?v=db8Ls9TwU3M ):
0. (Turn on lock screen in settings)
1. Launch the template app.
2. Lock the screen with power button. Note that in this step, mozLockOrientation is called in LockScreen.lock(); and it returns true, meaning that the orientation was successfully locked into portrait mode.
3. Right now, if one uses app manager from Fx nightly to investigate system app's screen orientation, one would observe that the orientation is landscape -- probably having been changed by the "background" template app
4. Wake up the screen with power button.
5. Boom! Lock screen is in landscape mode (which I think is against lockscreen spec, ref @gweng)

It appears that in step 4, the mozLockOrientation call in LockScreen.lock() (https://github.com/mnjul/gaia/commit/536d40c395a098fcd006e4e314ae590d11aa7bdb#diff-a79cb8e42a7726b3c9a9d0624218fab0R660) fails, as it returns false; I did further experiment and it appeared that in step 4, if we wait for about one second and then call mozLockOrientation, the call would succeed. See https://github.com/mnjul/gaia/commit/dc8d9da21750a51719cc0b30443f7d9cd6d7a35a : Usually, the call would return true after around three or four tries (on Buri). See the video: https://www.youtube.com/watch?v=Zsr3VVJQC9c ; when the device is waken, lockscreen is first shown in landscape orientation, and in about one second it is rotated back to portrait.

Note that the "fix" in that dc8d9d commit does not really fix, or even work around, bug 1008486. It appears that the market place app mentioned in that bug somehow locks the screen orientation more "aggressively", that even after the lockscreen is rotated back to portrait as in dc8d9d, it is rotated back to landscape, presumably by the marketplace app.

To summarize: 
1. Background app's calls to Screen.mozLockOrientation would succeed, and interfere with foreground's calls -- this is unwanted behavior.
2. Why would LockScren.lock()'s call to mozLockOrientation always fail?

BTW it is not feasible for lockscreen to continuously lock orientation to portrait; I tried this, and I only got a black screen, and console logs showed screen orientation randomly alternating between landscape and portrait (the black screen is probably because gecko is overloaded or something similar).

Oh and bonus: my template app will also leave card view and homescreen in landscape orientation. See: https://www.youtube.com/watch?v=T2ZsHWAX6Hk . Unwanted too?
John, let's use bug 1015073 to continue investigate recent breakage. We don't do that on already closed bugs.
You need to log in before you can comment on or make changes to this bug.