Closed
Bug 868914
Opened 12 years ago
Closed 12 years ago
Prevent a background app/page to call mozLockOrientation
Categories
(Firefox OS Graveyard :: General, defect)
Tracking
(blocking-b2g:tef+, firefox21 wontfix, firefox22 wontfix, firefox23 fixed, b2g18 fixed, b2g18-v1.0.0 wontfix, b2g18-v1.0.1 fixed)
RESOLVED
FIXED
blocking-b2g | tef+ |
People
(Reporter: alive, Assigned: alive)
References
(Blocks 1 open bug)
Details
(Whiteboard: [games:p?])
Attachments
(2 files, 3 obsolete files)
(deleted),
patch
|
Details | Diff | Splinter Review | |
(deleted),
patch
|
Details | Diff | Splinter Review |
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 | ||
Updated•12 years ago
|
Assignee: nobody → alive
Assignee | ||
Comment 1•12 years ago
|
||
Patch v1: prevent background page to lock orientation
Mounir, could you please review this? Thanks!
Attachment #745809 -
Flags: review?(mounir)
Updated•12 years ago
|
blocking-b2g: tef? → tef+
Comment 2•12 years ago
|
||
Thanks you for the quick Gecko bug fix!
Comment 3•12 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.
Attachment #745809 -
Flags: review?(mounir) → review+
Comment 4•12 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.
Comment 6•12 years ago
|
||
Alive, did you mean to carry to r+ on the refined patch and ask for a checkin?
Flags: needinfo?(alive)
Assignee | ||
Comment 7•12 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.
Flags: needinfo?(alive)
Updated•12 years ago
|
Whiteboard: [games:p?] → [status: has r+'d patch, awaiting try results][games:p?]
Assignee | ||
Comment 8•12 years ago
|
||
Assignee | ||
Comment 9•12 years ago
|
||
checkin needed patch(r+d)
Assignee | ||
Updated•12 years ago
|
Keywords: checkin-needed
Assignee | ||
Comment 10•12 years ago
|
||
Thanks :rlin for try server help ;)
Updated•12 years ago
|
Attachment #746892 -
Attachment is obsolete: true
Comment 11•12 years ago
|
||
Keywords: checkin-needed
Whiteboard: [status: has r+'d patch, awaiting try results][games:p?] → [games:p?]
Comment 12•12 years ago
|
||
Status: NEW → RESOLVED
Closed: 12 years ago
Resolution: --- → FIXED
Comment 13•12 years ago
|
||
https://hg.mozilla.org/releases/mozilla-b2g18/rev/236f9b1f3569
https://hg.mozilla.org/releases/mozilla-b2g18_v1_0_1/rev/92166697330f
status-b2g18:
--- → fixed
status-b2g18-v1.0.0:
--- → wontfix
status-b2g18-v1.0.1:
--- → fixed
status-firefox21:
--- → wontfix
status-firefox22:
--- → wontfix
status-firefox23:
--- → fixed
Comment 14•12 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
Comment 15•12 years ago
|
||
Alive, it seems that nsIDocument::Hidden() doesn't exist in b2g18. You should use nsIDOMDocument::GetHidden(bool*) for b2g18.
Assignee | ||
Comment 16•12 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....
Assignee | ||
Updated•12 years ago
|
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Assignee | ||
Comment 17•12 years ago
|
||
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 18•12 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?
Attachment #747746 -
Flags: review?(mounir) → review+
Comment 19•12 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.
Status: REOPENED → RESOLVED
Closed: 12 years ago → 12 years ago
Resolution: --- → FIXED
Comment 20•12 years ago
|
||
Comment 21•12 years ago
|
||
Updated•12 years ago
|
Attachment #747746 -
Attachment is obsolete: true
Comment 22•12 years ago
|
||
Comment 23•12 years ago
|
||
Can you please provide steps to verify this fix - as we will blackbox test from the UI?
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
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
Updated•11 years ago
|
Blocks: gecko-games
Comment 26•11 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?
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.
You need to log in
before you can comment on or make changes to this bug.
Description
•