Closed Bug 896992 Opened 7 years ago Closed 7 years ago

Activity looping if GeckoApp has been killed and Settings is launched from notification

Categories

(Firefox for Android :: General, defect, major)

23 Branch
ARM
Android
defect
Not set
major

Tracking

()

VERIFIED FIXED
Firefox 25
Tracking Status
firefox22 --- unaffected
firefox23 - unaffected
firefox24 + verified
firefox25 + verified
fennec 24+ ---

People

(Reporter: ioana.chiorean, Assigned: liuche)

References

Details

(Keywords: regression)

Attachments

(2 files, 5 obsolete files)

Build: Firefox 23.0b8 07/23
Device: LG Nexus 4, Galaxy Nexus (4.2.2)

Steps to reproduce:
1. Install app - Open it
2. Wait to get notification about data & stats 
3. Swipe off Nightly to remove it from the foreground
4. Access data & stats from the notification shortcut
5. Press back (both from upper left corner or device's back)

Expected Results: 
- You should be back in about:home

Actual Results:
- The page is very slow, tapping is almost not recognized etc

Note:
- See video here www.youtube.com/watch?v=KbpYke1vs1U
- Not reproducible on other devices that we have in the office
- You are not able to run the app until you clean data from Device's Settings.

Might be Regression from Bug 873072
> - You are not able to run the app until you clean data from Device's
> Settings.

And kill the app. Opening it from recently used apps will open the settings and you will find yourself in the same loop.
> Might be Regression from Bug 873072
Confirmed. 
Patch from Bug 973072 c#8 produced this https://hg.mozilla.org/mozilla-central/rev/91cc9c75c3d1
Whiteboard: Regression
tracking-fennec: --- → ?
Keywords: regression
Whiteboard: Regression
we'll track a backout of bug 873072 for FF23 and hope for a forward fix in FF24 if possible, otherwise backout again.
I was only able to repro this on a Galaxy Nexus if I checked the developer option "Don't keep activities."

Is that setting selected on the phones you're using, Ioana?

If so, I think I can put together a quick fix for this. Since this is a notoriously problematic developer option that most users shouldn't even have turned on, I would really prefer to land and uplift a fix w/o backing out the original patch.

Lukas, I'll add the backout patch to bug 873072 but I'll also try to get a patch for the developer options case tonight.
Flags: needinfo?(ioana.chiorean)
Summary: Launching settings from Android system notification will keep the app in a loop → Activity looping if GeckoApp has been killed and Settings is launched from notification
This adds one line of handling if GeckoApp has been killed from under Settings.
Assignee: nobody → liuche
Status: NEW → ASSIGNED
Attachment #780811 - Flags: review?(bugmail.mozilla)
Attachment #780812 - Flags: review?(bugmail.mozilla)
Attachment #780812 - Flags: review?(bugmail.mozilla)
Ioana: Please try out the build at http://people.mozilla.org/~liuche/bug-896992/

Let me know if you still see the problem on your devices.
(In reply to Chenxia Liu [:liuche] from comment #8)
> Ioana: Please try out the build at
> http://people.mozilla.org/~liuche/bug-896992/

The build works fine on both LG Nexus 4 and Galaxy Nexus (also in both case - with & without Keep activity checked in Developer options. )
Flags: needinfo?(ioana.chiorean)
Comment on attachment 780811 [details] [diff] [review]
Beta patch: handle restoring GeckoApp

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

So if I'm understanding the code correctly, these checks will prevent the data notification settings pane from opening in some cases even when it should. I tried the following scenario with the APK you posted a couple of comments above:

1. Set "Don't keep activities" in the options
2. Start Fennec
3. Open the awesome bar (this will destroy GeckoApp)
4. Close the awesome bar (this will open GeckoApp with mIsRestoringActivity true)
5. Open the data notification from the android notification bar

These steps will run through the "onNewIntent" codepath while mIsRestoringActivity is set to true, and so the data notification thing won't open when in fact it should. Here's another scenario:

1. Set "Don't keep activities" in the options
2. Start Fennec
3. Put Fennec in the background (GeckoApp will be killed)
4. Open the data notification from the android notification bar

This will run through the initialize() code path with mIsRestoringActivity set to true, and so will not open the data notification pane.

I'm not entirely sure what caused the original problem - is that when Android re-creates the GeckoApp activity, it re-delivers the intent to it? If so, maybe a better solution is to either null out the intent in GeckoApp or if that doesn't work, add a flag on the intent saying that it has been processed and check that instead of mIsRestoringActivity.

::: mobile/android/base/GeckoApp.java
@@ +1973,5 @@
>              String uri = getURIFromIntent(intent);
>              GeckoAppShell.sendEventToGecko(GeckoEvent.createURILoadEvent(uri));
>          } else if (ACTION_ALERT_CALLBACK.equals(action)) {
>              processAlertCallback(intent);
> +        } if (ACTION_LAUNCH_SETTINGS.equals(action) && !mIsRestoringActivity) {

Also: This is missing an "else" between the } and the "if"
Attachment #780811 - Flags: review?(bugmail.mozilla) → review-
This sets a flag to note when saving state that we have already launched settings and should clear that action.

The apk is the beta2.apk at http://people.mozilla.com/~liuche/bug-896992/
Attachment #780811 - Attachment is obsolete: true
Attachment #780812 - Attachment is obsolete: true
Attachment #780815 - Attachment is obsolete: true
Attachment #781060 - Flags: review?(bugmail.mozilla)
Comment on attachment 781060 [details] [diff] [review]
Patch: Beta patch: handle settings intent with flag v2

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

This looks ok to me. That being said I would advise against uplifting this to beta this late in the cycle unless you're sure you've tested every possible scenario. Patches that touch our startup codepath have a history of breaking things subtly because our startup is pretty brittle. Sometimes even adding innocuous lines of code causes a previously latent race condition to trigger.
Attachment #781060 - Flags: review?(bugmail.mozilla) → review+
Triage agrees with Kats. Let's only uplift this to Fx24.
tracking-fennec: ? → 24+
Attached patch Patch: fix looping with flag (obsolete) — Splinter Review
This patch fixes the looping by adding flag to be tracked until GeckoApp is properly destroyed, but there seems to be some padding issue with the recreated GeckoApp, where the urlbar overlaps the content.
Attachment #781060 - Attachment is obsolete: true
Lukas, should I remove tracking for 23 since we decided we're not going to uplift to beta at this late in the stage, and is there anything else I need to do since 23 is going to ship with this bug?
Flags: needinfo?(lsblakk)
Attachment #781377 - Attachment description: Patch: m-c WIP - fixes looping → Patch: fix looping with flag
Attachment #781377 - Flags: review?(bugmail.mozilla)
Leaving this tracked for FF24 in order to get a forward fix that doesn't bring back bug 873072
Flags: needinfo?(lsblakk)
Comment on attachment 781377 [details] [diff] [review]
Patch: fix looping with flag

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

LGTM. Some nits below but I don't feel strongly about them.

::: mobile/android/base/GeckoApp.java
@@ +135,5 @@
>      public static final String ACTION_BOOKMARK      = "org.mozilla.gecko.BOOKMARK";
>      public static final String ACTION_LOAD          = "org.mozilla.gecko.LOAD";
>      public static final String ACTION_LAUNCH_SETTINGS = "org.mozilla.gecko.SETTINGS";
>      public static final String ACTION_INIT_PW       = "org.mozilla.gecko.INIT_PW";
> +    public static final String SAVED_STATE_ACTION_MAIN = "setActionMain";

nit: I would rename this SAVED_STATE_INTENT_HANDLED for consistency with the boolean variable. Change the value likewise.

@@ +160,5 @@
>      private static GeckoThread sGeckoThread;
>      private GeckoProfile mProfile;
>      public static int mOrientation;
>      protected boolean mIsRestoringActivity;
> +    private boolean mIntentHandled = false;

No need to initialize this; boolean fields are initialized to false by default.

@@ +637,5 @@
>  
>          outState.putBoolean(SAVED_STATE_IN_BACKGROUND, isApplicationInBackground());
>          outState.putString(SAVED_STATE_PRIVATE_SESSION, mPrivateBrowsingSession);
> +
> +        // Bug 896992 - Replace intent action with ACTION_MAIN on restart. 

nit: trailing whitespace
Attachment #781377 - Flags: review?(bugmail.mozilla) → review+
Attachment #781377 - Attachment is obsolete: true
[Approval Request Comment]
Bug caused by (feature/regressing bug #): 873072

User impact if declined: Devices with low memory or "Don't keep activities" dev option turned on will be looped infinitely to the settings screen if they tap on the datareporting notification if Fennec has been killed

Testing completed (on m-c, etc.): Manually tested, try build: https://tbpl.mozilla.org/?tree=Try&rev=9194ed28deaf

Risk to taking this patch (and alternatives if risky): low risk, adding a single flag to startup code to be tracked between restarts
 
String or IDL/UUID changes made by this patch: none
Attachment #783242 - Flags: approval-mozilla-aurora?
https://hg.mozilla.org/mozilla-central/rev/dd1908156c2f
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 25
Attachment #783242 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Target Milestone: Firefox 25 → Firefox 24
Target milestone tracks m-c landing. Tracking flags track branch uplifts.
Target Milestone: Firefox 24 → Firefox 25
Status: RESOLVED → VERIFIED
You need to log in before you can comment on or make changes to this bug.