1336734 - Navigating within preferences screens triggers unnecessary application-background/application-foreground events

Status: RESOLVED FIXED

(Firefox for Android Graveyard :: General, defect)

Description

[Jan Henning [:JanH]]

We have some code (https://dxr.mozilla.org/mozilla-central/rev/1d025ac534a6333a8170a59a95a8a3673d4028ee/mobile/android/base/java/org/mozilla/gecko/GeckoApplication.java#109) whose purpose it seems to be to make sure that Gecko is paused only when Firefox as a whole is backgrounded, as opposed to us merely switching to a different activity *within* Firefox.

I've no idea how our settings screen used to work at that time when this was originally implemented (bug 826385), however as of now it looks like this logic doesn't properly cope with navigation within the settings.
It seems like each child menu screen (General, Search, Privacy, etc., as well as any "grand children", e.g. General -> Home, and so forth) is a fresh activity and since GeckoPreferences always returns "false" for isGeckoActivityOpened(), this allows the whole if-block in onActivityPause() to run, triggering a call to GeckoThread.onPause() and all that entails, only to be immediately followed by a onActivityResume() for the new screen.

When going back up the levels of the settings we're at least saved because in that case activity.isFinishing() will be true, so we skip pausing Gecko.

Oh, and the GeckoNetworkManager stop() call is placed outside of that if-block, so currently it is paused and immediately restarted (through onActivityResume()) even more frequently, namely each time we navigate within/exit/enter the settings.

Comment 1

[Jan Henning [:JanH]]

Attachment: Bug 1336734 - Part 1 - Have GeckoPreferences properly support GeckoActivityStatus.

Currently, GeckoPreferences always returns "false" for isGeckoActivityOpened(), which means that when we're e.g. opening a new settings screen, GeckoApplication's onActivityPause() code assumes that Firefox is being backgrounded for real, calling GeckoThread.onPause(). This is then immediately followed by a call to onActivityResume() which unpauses Gecko again.

To avoid this, GeckoPreferences needs to properly implement support for GeckoActivityStatus and check the target of outgoing intents along the lines of the implementation in GeckoActivity.

Review commit: https://reviewboard.mozilla.org/r/109924/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/109924/

Comment 2

[Jan Henning [:JanH]]

Attachment: Bug 1336734 - Part 2 - Don't stop the GeckoNetworkManager unless we're really backgrounded.

Launching a new activity within our app triggers both onActivityPause() (the current activity) and onActivityResume() (the new activity) in GeckoApplication. The most prominent example at the moment are probably our preferences - entering/exiting/navigating within them always triggers a pause/resume combo. This means that currently, each time this happens the network manager is stopped only to be immediately restarted.

Review commit: https://reviewboard.mozilla.org/r/109926/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/109926/

Comment 3

[Jan Henning [:JanH]]

I need to check this, but I probably have to implement GeckoActivityStatus for the FxAccountStatusActivity as well, otherwise backgrounding the app from there would go unnoticed.

Comment 4

[Jan Henning [:JanH]]

Attachment: Bug 1336734 - Part 2 - Implement GeckoActivityStatus for the FxAccountStatusActivity.

Since we're no longer pausing Gecko when entering this activity, it must implement this interface so we can still properly pause Gecko if we get backgrounded while on the Sync preferences screen.

Most actions here are actually done via the application context (i.e. GeckoApplication), so overriding startActivity et al. and using mGeckoActivityOpened doesn't achieve all that much for most cases, but since we currently at most exit the screen (activity is finishing, so won't trigger a GeckoThread.onPause() call) and stay within our application (open a new tab in Firefox), we're still fine.

Review commit: https://reviewboard.mozilla.org/r/110172/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/110172/

Comment 5

[Jan Henning [:JanH]]

Comment on attachment 8833775 [details]
Bug 1336734 - Part 1 - Have GeckoPreferences properly support GeckoActivityStatus.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/109924/diff/1-2/

Comment 6

[Jan Henning [:JanH]]

Comment on attachment 8833776 [details]
Bug 1336734 - Part 2 - Don't stop the GeckoNetworkManager unless we're really backgrounded.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/109926/diff/1-2/

Comment 7

[Sebastian Kaspari (:sebastian; :pocmo)]

Comment on attachment 8833775 [details]
Bug 1336734 - Part 1 - Have GeckoPreferences properly support GeckoActivityStatus.

https://reviewboard.mozilla.org/r/109924/#review111964

::: mobile/android/base/java/org/mozilla/gecko/GeckoActivity.java:71
(Diff revision 2)
>      public void startActivityForResult(Intent intent, int request) {
>          mGeckoActivityOpened = checkIfGeckoActivity(intent);
>          super.startActivityForResult(intent, request);
>      }
>  
> -    private static boolean checkIfGeckoActivity(Intent intent) {
> +    public static boolean checkIfGeckoActivity(Intent intent) {

It looks like this could be moved to IntentUtils completely.

Comment 8

[Sebastian Kaspari (:sebastian; :pocmo)]

Comment on attachment 8834115 [details]
Bug 1336734 - Part 2 - Implement GeckoActivityStatus for the FxAccountStatusActivity.

https://reviewboard.mozilla.org/r/110172/#review111970

Comment 9

[Sebastian Kaspari (:sebastian; :pocmo)]

Comment on attachment 8833776 [details]
Bug 1336734 - Part 2 - Don't stop the GeckoNetworkManager unless we're really backgrounded.

https://reviewboard.mozilla.org/r/109926/#review111972

Comment 10

[Jan Henning [:JanH]]

Comment on attachment 8833775 [details]
Bug 1336734 - Part 1 - Have GeckoPreferences properly support GeckoActivityStatus.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/109924/diff/2-3/

Comment 11

[Jan Henning [:JanH]]

Comment on attachment 8834115 [details]
Bug 1336734 - Part 2 - Implement GeckoActivityStatus for the FxAccountStatusActivity.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/110172/diff/1-2/

Comment 12

[Jan Henning [:JanH]]

Comment on attachment 8833776 [details]
Bug 1336734 - Part 2 - Don't stop the GeckoNetworkManager unless we're really backgrounded.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/109926/diff/2-3/

Comment 13

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/a2b69382c9ad
Part 1 - Have GeckoPreferences properly support GeckoActivityStatus. r=sebastian
https://hg.mozilla.org/integration/autoland/rev/fa9cca3e321c
Part 2 - Implement GeckoActivityStatus for the FxAccountStatusActivity. r=sebastian
https://hg.mozilla.org/integration/autoland/rev/a7f9815aeec1
Part 3 - Don't stop the GeckoNetworkManager unless we're really backgrounded. r=sebastian

Comment 14

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

Backed out for android build bustage like https://treeherder.mozilla.org/logviewer.html#?job_id=76440293&repo=autoland

https://hg.mozilla.org/integration/autoland/rev/0c8cd30bc427

>12:51:03 <JanH> KWierso: I think you can backout bug 1336734 again from autoland - unfortunately it seems like the normal Android build does things differently than the gradle one (and fails)

Comment 15

[Jan Henning [:JanH]]

Comment on attachment 8833775 [details]
Bug 1336734 - Part 1 - Have GeckoPreferences properly support GeckoActivityStatus.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/109924/diff/3-4/

Comment 16

[Jan Henning [:JanH]]

Comment on attachment 8833776 [details]
Bug 1336734 - Part 2 - Don't stop the GeckoNetworkManager unless we're really backgrounded.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/109926/diff/3-4/

Comment 17

[Jan Henning [:JanH]]

Left out the offending part and opened a new bug for that (bug 1338673). It means that opening the sync settings will still trigger "application-background", but at least in that case that'll stick until the user leaves that screen again, whereas the current behaviour in the settings is that each navigation *into* the settings triggers a "application-background" + "application-foreground" combo.

Comment 18

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/b5d12d30e6cf
Part 1 - Have GeckoPreferences properly support GeckoActivityStatus. r=sebastian
https://hg.mozilla.org/integration/autoland/rev/be42accd3a48
Part 2 - Don't stop the GeckoNetworkManager unless we're really backgrounded. r=sebastian

Comment 19

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/b5d12d30e6cf
https://hg.mozilla.org/mozilla-central/rev/be42accd3a48