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)

ARM
Gonk (Firefox OS)
defect
Not set
critical

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+
Tracking Status
firefox21 --- wontfix
firefox22 --- wontfix
firefox23 --- fixed
b2g18 --- fixed
b2g18-v1.0.0 --- wontfix
b2g18-v1.0.1 --- fixed

People

(Reporter: alive, Assigned: alive)

References

(Blocks 1 open bug)

Details

(Whiteboard: [games:p?])

Attachments

(2 files, 3 obsolete files)

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.
Attached patch refined patch (obsolete) (deleted) — 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?]
Attached patch checkin patch (deleted) — Splinter Review
checkin needed patch(r+d)
Thanks :rlin for try server help ;)
Attachment #746892 - Attachment is obsolete: true
Keywords: checkin-needed
Whiteboard: [status: has r+'d patch, awaiting try results][games:p?] → [games:p?]
Status: NEW → RESOLVED
Closed: 12 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 → ---
Attached patch b2g18 patch (obsolete) (deleted) — 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: 12 years ago12 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
Blocks: gecko-games
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.

Attachment

General

Created:
Updated:
Size: