(inactive after 6/18) Alive Kuo [:alive] Assignee
	Description • 11 years ago

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.

	(inactive after 6/18) Alive Kuo [:alive] Assignee
	Comment 1 • 11 years ago

Patch v1: prevent background page to lock orientation

Mounir, could you please review this? Thanks!

	Tim Guan-tin Chien [:timdream] (please needinfo)
	Comment 2 • 11 years ago

Thanks you for the quick Gecko bug fix!

	Mounir Lamouri (:mounir)
	Comment 3 • 11 years ago

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.

	Mounir Lamouri (:mounir)
	Comment 4 • 11 years ago

Also, this might prevent the system application to change the screen orientation when the screen is off but I guess that should unlikely happen.

	(inactive after 6/18) Alive Kuo [:alive] Assignee
	Comment 5 • 11 years ago

refined patch.

	Milan Sreckovic [:milan] (needinfo for best results)
	Comment 6 • 11 years ago

Alive, did you mean to carry to r+ on the refined patch and ask for a checkin?

	(inactive after 6/18) Alive Kuo [:alive] Assignee
	Comment 7 • 11 years ago

(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.

	(inactive after 6/18) Alive Kuo [:alive] Assignee
	Comment 8 • 11 years ago

try:
https://tbpl.mozilla.org/?tree=Try&rev=97d47c92b8d7

	(inactive after 6/18) Alive Kuo [:alive] Assignee
	Comment 9 • 11 years ago

checkin needed patch(r+d)

	(inactive after 6/18) Alive Kuo [:alive] Assignee
	Comment 10 • 11 years ago

Thanks :rlin for try server help ;)

	Ryan VanderMeulen [:RyanVM]
	Comment 11 • 11 years ago

https://hg.mozilla.org/projects/birch/rev/82ab0af69c16

	Ryan VanderMeulen [:RyanVM]
	Comment 12 • 11 years ago

https://hg.mozilla.org/mozilla-central/rev/82ab0af69c16

	Ryan VanderMeulen [:RyanVM]
	Comment 13 • 11 years ago

https://hg.mozilla.org/releases/mozilla-b2g18/rev/236f9b1f3569
https://hg.mozilla.org/releases/mozilla-b2g18_v1_0_1/rev/92166697330f

	Ryan VanderMeulen [:RyanVM]
	Comment 14 • 11 years ago

Backed out for bustage. b2g18 doesn't have Hidden() apparently.
https://hg.mozilla.org/releases/mozilla-b2g18/rev/498418a4cf8d
https://hg.mozilla.org/releases/mozilla-b2g18_v1_0_1/rev/2d275685ee7e

	Mounir Lamouri (:mounir)
	Comment 15 • 11 years ago

Alive, it seems that nsIDocument::Hidden() doesn't exist in b2g18. You should use nsIDOMDocument::GetHidden(bool*) for b2g18.

	(inactive after 6/18) Alive Kuo [:alive] Assignee
	Comment 16 • 11 years ago

(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....

	(inactive after 6/18) Alive Kuo [:alive] Assignee
	Comment 17 • 11 years ago

I am not sure what's the process here.
Seems I need one patch for mc, one for b2g18?

	Mounir Lamouri (:mounir)
	Comment 18 • 11 years ago

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?

	Ryan VanderMeulen [:RyanVM]
	Comment 19 • 11 years ago

Don't reopen bugs unless they're backed out from m-c. I don't see bugs needing uplift unless they're resolved.

	Ryan VanderMeulen [:RyanVM]
	Comment 20 • 11 years ago

https://hg.mozilla.org/releases/mozilla-b2g18/rev/355d6ceeadb1
https://hg.mozilla.org/releases/mozilla-b2g18_v1_0_1/rev/c8f807dc9f1f

	Ryan VanderMeulen [:RyanVM]
	Comment 22 • 11 years ago

Autopilot fail.
https://hg.mozilla.org/releases/mozilla-b2g18/rev/c8e6a9297701
https://hg.mozilla.org/releases/mozilla-b2g18_v1_0_1/rev/1e9a618862b4

	sarsenyev
	Comment 23 • 11 years ago

Can you please provide steps to verify this fix - as we will blackbox test from the UI?

	sarsenyev
	Comment 24 • 11 years ago

As per description, Game Pack app switch between games stops reply and needs to restart the whole app to make it working again

	sarsenyev
	Comment 25 • 11 years ago

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

	John Lu [:mnjul][Please NI or I'll miss]
	Comment 26 • 10 years ago

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?

	Tim Guan-tin Chien [:timdream] (please needinfo)
	Comment 27 • 10 years ago

John, let's use bug 1015073 to continue investigate recent breakage. We don't do that on already closed bugs.