Closed Bug 1072376 Opened 5 years ago Closed 5 years ago

Regression: Open/Close animation of the tabs tray and menu is sluggish

Categories

(Firefox for Android :: General, defect)

35 Branch
ARM
Android
defect
Not set

Tracking

()

VERIFIED FIXED
Firefox 35
Tracking Status
firefox35 --- verified
fennec 35+ ---

People

(Reporter: aaronmt, Assigned: wesj)

References

Details

(Keywords: regression, reproducible)

Attachments

(3 files)

This is currently on fx-team, opening and closing the tabs tray is incredibly slow.

Possibly a regression from

8329fcecefdc	Martyn Haigh — Bug 1056920 - Create grid based layout for tabs (r=lucasr)

Martyn, do you see this on fx-team? Can you confirm if it's that recent landing?
Flags: needinfo?(mhaigh)
Yeah, that landed on morning of 24th GMT.  However, eyeballing it - I'm not seeing any different between my dev build and beta running on an N10.
Flags: needinfo?(mhaigh)
Attached video video.mp4
Sluggish animation
(In reply to Martyn Haigh (:mhaigh) from comment #1)
> Yeah, that landed on morning of 24th GMT.  However, eyeballing it - I'm not
> seeing any different between my dev build and beta running on an N10.

This is on phones.
Martyn, could you have a look at this to see what's causing the jank here?
Assignee: nobody → mhaigh
I'm able to reproduce the janky animation in latest m-c on a Nexus 7. I backed out bug 1072376 locally and it doesn't make any difference. We need a regression window here. This is a serious regression.
Also the menu is affected.
Summary: Regression: Open/Close animation of the tabs tray is sluggish → Regression: Open/Close animation of the tabs tray and menu is sluggish
(In reply to Aaron Train [:aaronmt] from comment #8)
> Also the menu is affected.

What menu? How exactly?
Flags: needinfo?(aaron.train)
Last good revision: 29c35d8e3d90
First bad revision: f1a36cb2bba6

Pushlog:
https://hg.mozilla.org/integration/fx-team/pushloghtml?fromchange=29c35d8e3d90&tochange=f1a36cb2bba6

wjohnston@mozilla.com
Mon Sep 22 06:01:28 2014 +0000	f1a36cb2bba6	Wes Johnston — Bug 815682 - Lockscreen widget for guest sessions. r=bnicholson, rnewman
mhammond@skippinet.com.au
(In reply to Lucas Rocha (:lucasr) from comment #9)
> (In reply to Aaron Train [:aaronmt] from comment #8)
> > Also the menu is affected.
> 
> What menu? How exactly?

Browser menu, latency on opening and closing
Flags: needinfo?(aaron.train)
Seems unrelated to mhaigh patch. Unassigning for now.
Assignee: mhaigh → nobody
There's a change there in GeckoProfile.get that will cause us to check the keyguard state. Maybe its getting hit here and delaying things? The rest of these changes are mostly in onCreate or onNewIntent. I don't have any devices that reproduce this on me right now (and I'm stuck home sick), so someone else will have to look into this (or at least back out the patch and see if it helps).
(In reply to Wesley Johnston (:wesj) from comment #14)
> There's a change there in GeckoProfile.get that will cause us to check the
> keyguard state. Maybe its getting hit here and delaying things? The rest of
> these changes are mostly in onCreate or onNewIntent. I don't have any
> devices that reproduce this on me right now (and I'm stuck home sick), so
> someone else will have to look into this (or at least back out the patch and
> see if it helps).

I've tracked this down: GuestSession.configureWindow() is the culprit here.
Well, configureWindow just sets some lock screen flags on the window dealing with the keyguard. I'm digging through the Android source a bit, but I have a strange feeling that doing so resets hardware acceleration for the window? Strangely, the dialer screen does this completely differently:
http://androidxref.com/4.2.2_r1/xref/packages/apps/Phone/src/com/android/phone/InCallScreen.java#468

If you can test, you might try adding FLAG_HARDWARE_ACCELERATED as well here:
http://mxr.mozilla.org/mozilla-central/source/mobile/android/base/GuestSession.java#74
(those addFlags calls should just be one call anyway. I'm not sure why I used two).

Hardware acceleration listed in the manifest is set when the window is attached:
http://androidxref.com/4.2.2_r1/xref/frameworks/base/core/java/android/app/Activity.java#5090

All passing that does is set a flag in the window, which is only used when setting up a subwindow here:
http://androidxref.com/4.2.2_r1/xref/frameworks/base/core/java/android/view/Window.java#531

I don't know enough about windows and subwindows to know who that will affect though. The flags set via window.set/addFlags() seem to be stored separately, but trickle through to

http://androidxref.com/4.2.2_r1/xref/frameworks/base/core/java/android/view/WindowManagerGlobal.java#269

where they are applied to the root view of an activity. I don't understand the window/subwindow stuff well enough yet to see the hole, but at this point I'd probably just check that adding the hardware accel flag to configureWindow works to see if I'm on the right track at all. If you don't have time, I'll see if MTV QA has a device tomorrow.
Tried enabling hardware acceleration but it didn't make any difference. Logcat is showing a bunch of strictmode exceptions on every layout pass in the UI:

D/StrictMode( 3221): StrictMode policy violation; ~duration=25 ms: android.os.StrictMode$StrictModeDiskReadViolation: policy=287 violation=2
D/StrictMode( 3221): 	at android.os.StrictMode$AndroidBlockGuardPolicy.onReadFromDisk(StrictMode.java:1135)
D/StrictMode( 3221): 	at android.database.sqlite.SQLiteConnection.applyBlockGuardPolicy(SQLiteConnection.java:1041)
D/StrictMode( 3221): 	at android.database.sqlite.SQLiteConnection.executeForCursorWindow(SQLiteConnection.java:842)
D/StrictMode( 3221): 	at android.database.sqlite.SQLiteSession.executeForCursorWindow(SQLiteSession.java:836)
D/StrictMode( 3221): 	at android.database.sqlite.SQLiteQuery.fillWindow(SQLiteQuery.java:62)
D/StrictMode( 3221): 	at android.database.sqlite.SQLiteCursor.fillWindow(SQLiteCursor.java:144)
D/StrictMode( 3221): 	at android.database.sqlite.SQLiteCursor.getCount(SQLiteCursor.java:133)
D/StrictMode( 3221): 	at android.database.AbstractCursor.moveToPosition(AbstractCursor.java:197)
D/StrictMode( 3221): 	at android.database.AbstractCursor.moveToFirst(AbstractCursor.java:237)
D/StrictMode( 3221): 	at com.android.server.LockSettingsService.readFromDb(LockSettingsService.java:420)
D/StrictMode( 3221): 	at com.android.server.LockSettingsService.getBoolean(LockSettingsService.java:202)
D/StrictMode( 3221): 	at com.android.internal.widget.ILockSettings$Stub.onTransact(ILockSettings.java:94)
D/StrictMode( 3221): 	at android.os.Binder.execTransact(Binder.java:404)
D/StrictMode( 3221): 	at android.os.BinderProxy.transact(Native Method)
D/StrictMode( 3221): 	at com.android.internal.policy.IKeyguardService$Stub$Proxy.isSecure(IKeyguardService.java:253)
D/StrictMode( 3221): 	at com.android.internal.policy.impl.keyguard.KeyguardServiceWrapper.isSecure(KeyguardServiceWrapper.java:53)
D/StrictMode( 3221): 	at com.android.internal.policy.impl.keyguard.KeyguardServiceDelegate.isSecure(KeyguardServiceDelegate.java:190)
D/StrictMode( 3221): 	at com.android.internal.policy.impl.PhoneWindowManager.isKeyguardSecure(PhoneWindowManager.java:4397)
D/StrictMode( 3221): 	at com.android.internal.policy.impl.PhoneWindowManager.finishPostLayoutPolicyLw(PhoneWindowManager.java:3534)
D/StrictMode( 3221): 	at com.android.server.wm.WindowManagerService.performLayoutAndPlaceSurfacesLockedInner(WindowManagerService.java:9026)
D/StrictMode( 3221): 	at com.android.server.wm.WindowManagerService.performLayoutAndPlaceSurfacesLockedLoop(WindowManagerService.java:8179)
D/StrictMode( 3221): 	at com.android.server.wm.WindowManagerService.performLayoutAndPlaceSurfacesLocked(WindowManagerService.java:8121)
D/StrictMode( 3221): 	at com.android.server.wm.WindowManagerService.relayoutWindow(WindowManagerService.java:3016)
D/StrictMode( 3221): 	at com.android.server.wm.Session.relayout(Session.java:190)
D/StrictMode( 3221): 	at android.view.IWindowSession$Stub.onTransact(IWindowSession.java:235)
D/StrictMode( 3221): 	at com.android.server.wm.Session.onTransact(Session.java:125)
D/StrictMode( 3221): 	at android.os.Binder.execTransact(Binder.java:404)
D/StrictMode( 3221): 	at dalvik.system.NativeStart.run(Native Method)

It seems the UI is making blocking calls on the KeyguardService in the main thread. FLAG_DISMISS_KEYGUARD is what's causing this regression btw.
Assignee: nobody → wjohnston
tracking-fennec: ? → 35+
There doesn't seem to be a Nexus 7 available in MTV right now (which is silly). I ordered one.
Attached patch PatchSplinter Review
Finally managed to repro this. This makes us only set these flags when you're running over the keyguard. That can happen if your running in Guest mode, or if your keyguard is just not a secure one (i.e. swipe to unlock!).

In the later case, onResume is actually called twice, and the second time the keyguard is no longer showing so it actually removes these flags there (but the second onResume call never happens if we don't set the flags once). So really, you'll only see the jank if

1.) The keyguard is showing and
2.) You're in guest mode.

That's not perfect, but its better than what we had... (and it basically gets us back to parity with builds that didn't support running over the keyguard at all).
Attachment #8495550 - Flags: review?(lucasr.at.mozilla)
Comment on attachment 8495550 [details] [diff] [review]
Patch

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

Have you tried always removing the DISMISS_KEYGUARD flags in onResume() to see if you can avoid the performance issues in all cases? DISMISS_KEYGUARD looks like a flags that will be only used when the activity starts. It's a dangerous assumption to make but maybe worth trying?

::: mobile/android/base/BrowserApp.java
@@ +750,5 @@
> +            // is not showing since it affects Android's layout of the window.
> +            if (manager.isKeyguardLocked() && (GeckoProfile.get(this).inGuestMode() || !manager.isKeyguardSecure())) {
> +                GuestSession.configureWindow(getWindow());
> +            } else {
> +                GuestSession.unconfigureWindow(getWindow());

I remember you mentioned onResume being called twice when keyguard is not secure. Maybe document this here?
Attachment #8495550 - Flags: review?(lucasr.at.mozilla) → review+
(In reply to Lucas Rocha (:lucasr) from comment #21)
> Have you tried always removing the DISMISS_KEYGUARD flags in onResume() to
> see if you can avoid the performance issues in all cases? DISMISS_KEYGUARD
> looks like a flags that will be only used when the activity starts. It's a
> dangerous assumption to make but maybe worth trying?

I can try just that one. I removed both at one point (in DelayedStartup) and saw the keyguard reappear.
(In reply to Lucas Rocha (:lucasr) from comment #21)
> Have you tried always removing the DISMISS_KEYGUARD flags in onResume() to
> see if you can avoid the performance issues in all cases? DISMISS_KEYGUARD
> looks like a flags that will be only used when the activity starts. It's a
> dangerous assumption to make but maybe worth trying?

Flipping this causes the activity to be hidden. Will land as is for now.
https://hg.mozilla.org/mozilla-central/rev/828069050685
Status: NEW → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 35
Status: RESOLVED → VERIFIED
You need to log in before you can comment on or make changes to this bug.