701092 - Limit session restore attempts

Status: RESOLVED FIXED

(Firefox for Android Graveyard :: General, defect, P3)

Description

[Kevin Brosnan [Ex-Mozilla]]

I have Firefox in a state where it is crashing on Gecko start up. I cannot get out of this state because of session restore.

Comment 1

[Naoki Hirata :nhirata (please use needinfo instead of cc)]

Attachment: logcat

I also got into this state on a Honeycomb tablet with 20111109 Nightly Birch Build.  See attached logcat.

Comment 2

[Naoki Hirata :nhirata (please use needinfo instead of cc)]

Work around is to stop and clear the data in the settings-> applications -> <app>

Comment 3

[Kevin Brosnan [Ex-Mozilla]]

There is no way a user is going to perform that. We need session store not to be dumb.

Comment 4

[Brad Lassey [:blassey] (use needinfo?)]

create a SUMO page for it then

Comment 5

[Mark Finkle (:mfinkle) (use needinfo?)]

(In reply to Kevin Brosnan [:kbrosnan] from comment #0)
> I have Firefox in a state where it is crashing on Gecko start up. I cannot
> get out of this state because of session restore.

Just curious. Why do we think it is session store? Trying to figure out why this is happening at all. Fennec XUL session store doesn't do this, does it?

Comment 6

[Naoki Hirata :nhirata (please use needinfo instead of cc)]

I believe he thinks it is session store, because once you crash and try to relaunch the application, you go back to the same crash and keep crashing.

In the XUL version, it goes to about:home when you crash.

Comment 7

[Matt Brubeck (:mbrubeck)]

XUL Fennec does try to restore your session after a crash.  However, most content-related crashes in XUL Fennec happen in the child process, and XUL Fennec will offer to close the tab and return the home page when this happens.

Desktop Firefox tries to restore your session after a crash, but if the crash happens during session restore, the next time Firefox starts it will display the "well, this is embarassing" page and let you choose which tabs to restore.

Comment 8

[Kevin Brosnan [Ex-Mozilla]]

This is the behavior I am seeing. http://people.mozilla.com/~kbrosnan/tmp/701092.webm

Comment 9

[Brad Lassey [:blassey] (use needinfo?)]

what does desktop do? I think they detect a start up crash some how and do the "tabs from last time" screen instead.

Madhava?

Comment 10

[Kevin Brosnan [Ex-Mozilla]]

Desktop restores to about:sessionrestore, which could use some work to make it finger friendly. The displaying of about:sessionrestore is controlled by browser.sessionstore.max_resumed_crashes. http://mxr.mozilla.org/mozilla-central/ident?i=TAB_STATE_NEEDS_RESTORE

Comment 11

[Brad Lassey [:blassey] (use needinfo?)]

Assigned to Madhava for UX

Comment 12

[Madhava Enros [:madhava]]

If we crash we try to do session restore, which makes sense.

If we crash on startup, we can try to do session restore the first time.

The desktop approach is, IIRC, if the second time we crash immediately on startup, we show the "Well, this is embarrassing" screen, which explains that we probably shouldn't try to restore all your stuff immediately and lets you pick from the list of what to restore and not restore.

I could see us just opening to the Start page (about:home) if we crash a second time on startup.

Comment 13

[Madhava Enros [:madhava]]

Another approach would be to show the "tabs from last time" list in the start page if we were unsuccessful in doing a session restore (i.e., purposefully not doing it because it crashed last time).

Comment 14

[Madhava Enros [:madhava]]

mfinkle, via IRC: "can we do a simple trigger or rule that says "if we crash again after a restore, we skip the restore next time and only show the 'tabs from last time' in the home page as the way to reopen tabs" ?"

This sounds about right to me.

Comment 15

[Brian Nicholson (:bnicholson)]

Attachment: patch

I created the "browser.sessionstore.recent_crashes" pref to keep track of the number of recent crashes (is there a procedure to follow when introducing new preferences?). I also added "browser.sessionstore.max_resumed_crashes", which is used in desktop.

Comment 16

[Brian Nicholson (:bnicholson)]

Also, desktop does not make any distinction whether the crash occurs during startup. If it crashes, is restored, and crashes again at any point without being closed cleanly, this is considered successive crashing. I've used the same approach here.

Comment 17

[Mark Finkle (:mfinkle) (use needinfo?)]

Comment on attachment 591231 [details] [diff] [review]
patch


>diff --git a/mobile/android/chrome/content/browser.js b/mobile/android/chrome/content/browser.js

>+    // determine if we crashed last time (and not just killed from android OOM)
>+    let recentCrash = !restoreSession && ss.shouldRestore();
>+    if (recentCrash)
>+      ss.flagRecentCrash();

Can we move this into SessionStore.js and drop the need for a new method in IDL while we are at it?

>+      // if we detect a crash loop, show about:home instead of doing a session restore
>+      if (recentCrash) {
>+        if (url && url != "about:home")
>+          this.addTab("about:home");
>+      }

Can't we move this into SessionStore.restoreLastSession() ?

>diff --git a/mobile/android/components/SessionStore.idl b/mobile/android/components/SessionStore.idl

>+   * Indicate that a crash recently occurred; helps determine whether to restore
>+   */
>+  void flagRecentCrash();

No need for this

>diff --git a/mobile/android/components/SessionStore.js b/mobile/android/components/SessionStore.js

>+  flagRecentCrash: function ss_flagRecentCrash() {
>+    let maxCrashes = Services.prefs.getIntPref("browser.sessionstore.max_resumed_crashes");
>+    let recentCrashes;
>+    try {
>+      recentCrashes = Services.prefs.getIntPref("browser.sessionstore.recent_crashes") + 1;
>+    } catch (e) {
>+      recentCrashes = 1;
>+    }
>+
>+    if (recentCrashes > maxCrashes)
>+      this._shouldRestore = false;
>+    Services.prefs.setIntPref("browser.sessionstore.recent_crashes", recentCrashes);
>+  },

If we want to move the crash-loop-stopper into restoreLastSession, we need to keep _shouldRestore=true, but we need a new variable to move 99% of the method into restoreLastSession. the only line that doesn't get moved would be setting the recent_crashes pref.

Does this idea make any sense to you or am I crazy? The overriding issue I have is SessionStore has all it needs to handle this on its own - no need to add code to poor startup() in browser.js

Comment 18

[Brian Nicholson (:bnicholson)]

Attachment: patch v2

Comment 19

[Brian Nicholson (:bnicholson)]

(In reply to Mark Finkle (:mfinkle) from comment #17)
> Comment on attachment 591231 [details] [diff] [review]
> patch
> 
> Does this idea make any sense to you or am I crazy? The overriding issue I
> have is SessionStore has all it needs to handle this on its own - no need to
> add code to poor startup() in browser.js

SessionStore has everything it needs except the ability to determine whether we had a recent crash, which is true if 1) ss.shouldRestore() is true, and 2) we are not restoring from the Android bundle. I just passed this in as an argument to restoreLastSession().

Comment 20

[Mark Finkle (:mfinkle) (use needinfo?)]

Comment on attachment 591907 [details] [diff] [review]
patch v2


>diff --git a/mobile/android/chrome/content/browser.js b/mobile/android/chrome/content/browser.js

>       // Start the restore
>-      ss.restoreLastSession(restoreToFront);
>+      ss.restoreLastSession(restoreToFront, !restoreSession);

Let's change "restoreSession" -> "forceRestore" since that is really what it means. Also, don't use !forceRestore now, since we do not want to invert the boolean

>diff --git a/mobile/android/components/SessionStore.idl b/mobile/android/components/SessionStore.idl

>+   * @param aRecentCrash   whether we are recovering from a crash

aForceRestore  whether we need to force a restore, regardless of the recent crash situation

>+  void restoreLastSession(in boolean aBringToFront, in boolean aRecentCrash);

aRecentCrash -> aForceRestore

>diff --git a/mobile/android/components/SessionStore.js b/mobile/android/components/SessionStore.js

>+  restoreLastSession: function ss_restoreLastSession(aBringToFront, aRecentCrash) {

aRecentCrash -> aForceRestore

>+    if (aRecentCrash) {

      if (!aForceRestore) {

r+ if that makes sense, otherwise let's discuss

Comment 21

[Brian Nicholson (:bnicholson)]

It would make sense to use "forceRestore" in browser.js since that's what we're using it for, but it's being used in SessionStore.js for a different reason: to determine whether we crashed (note that the | if (aRecentCrash) {...} | statement in SessionStore.js is used only to determine whether we've exceeded the max crashes - and to abort the restore if so).

We're actually forcing the session restore simply by calling ss.restoreLastSession(). Normally, we would do ss.restoreLastSession() if just ss.shouldRestore() is true, but we do ss.restoreLastSession() if ss.shouldRestore() is true *or* if restoreSession is true (which is why forceRestore would be a better name in browser.js).

Comment 22

[Mark Finkle (:mfinkle) (use needinfo?)]

In ss.restoreLastSession, we check for recent crashes all the time, unless Java tells us to force a restore. Checking for recent crashes means we might _not_ do a restore - we might bail and let about:home be displayed.

However, if Java tells us to force a restore, we skip the recent crashes check, right?

Comment 23

[Brian Nicholson (:bnicholson)]

(In reply to Mark Finkle (:mfinkle) from comment #22)
> In ss.restoreLastSession, we check for recent crashes all the time, unless
> Java tells us to force a restore. Checking for recent crashes means we might
> _not_ do a restore - we might bail and let about:home be displayed.
> 
> However, if Java tells us to force a restore, we skip the recent crashes
> check, right?

Ok, I think I see what you're saying now. Yes, that's what's happening - I think we just interpreted it differently. I wasn't considering an OOM kill a "crash" since it's a normal part of the Android activity lifecycle.

Comment 24

[Brian Nicholson (:bnicholson)]

Attachment: patch v3

So I was thinking about how you said most users won't use Quit. This would be problem for the previous patch because the recent_crashes pref was reset to 0 only when quitting. As a result, crashing/Force Quit once would restore the tabs; crashing/Force Quit again would show about:home with "tabs from last time". After that, a crash would *never* restore tabs again unless the user had chosen Quit to reset the recent_crashes pref at some point.

This patch resets the recent_crashes pref whenever we go into the background. This should work since crash loops generally only happen when crashing in the foreground. I would much rather put this in SessionStore.js instead of nsAppShell.cpp, but we don't send any notifications when we receive an ACTIVITY_PAUSE event.

With this patch, the only thing I'd be concerned about would be a "Force Quit loop":
- A user visits some page that freezes Gecko--but doesn't crash.
- As a result, he wants to kill Fennec. He pushes Home, then goes to Settings > Force Quit. By pushing Home first, Fennec's pause is triggered, causing the recent_crashes pref to be reset to 0.
- Fennec is reopened. The problematic site is restored since recent_crashes is 0.
- Rinse and repeat.

I'm not sure whether this is actually a realistic scenario. Sending Fennec to the background might not actually reset the recent_crashes pref to 0 in this case if Gecko is frozen since it'll be too busy.

Comment 25

[Brian Nicholson (:bnicholson)]

Another option: reset recent_crashes to 0 *after* we resume from a bundle, since this means we just paused and were killed by OOM (which is obviously impossible to determine beforehand).

Comment 26

[Mark Finkle (:mfinkle) (use needinfo?)]

Comment on attachment 592328 [details] [diff] [review]
patch v3

>+  restoreLastSession: function ss_restoreLastSession(aBringToFront, aForceRestore) {

>     // The previous session data has already been renamed to the backup file
>     if (!this._sessionFileBackup.exists()) {
>-      notifyObservers("fail")
>+      notifyObservers("fail");

Add this line above the notifyObservers call:

        Services.prefs.setIntPref("browser.sessionstore.recent_crashes", 0);

That will handle reseting the crash loop flag

>diff --git a/widget/android/nsAppShell.cpp b/widget/android/nsAppShell.cpp

Remove these changes

r+ with those changes

Comment 27

[Mark Finkle (:mfinkle) (use needinfo?)]

Comment on attachment 592328 [details] [diff] [review]
patch v3

>diff --git a/widget/android/nsAppShell.cpp b/widget/android/nsAppShell.cpp

>         if (prefs) {
>+            // Indicate that we shut down cleanly
>+            nsCOMPtr<nsIPrefBranch> prefBranch;
>+            prefs->GetBranch("browser.sessionstore.", getter_AddRefs(prefBranch));
>+            if (prefBranch)
>+                prefBranch->SetIntPref("recent_crashes", 0);
>+

Keep this but change the comment. We are not shutting down cleanly, we are "resetting the crash loop state"

Comment 28

[Brian Nicholson (:bnicholson)]

Landed on inbound:
http://hg.mozilla.org/integration/mozilla-inbound/rev/ededf3963a98

Comment 29

[Joe Drew (not getting mail)]

https://hg.mozilla.org/mozilla-central/rev/ededf3963a98

Comment 30

[Brad Lassey [:blassey] (use needinfo?)]

Brian, request approval for aurora

Comment 31

[Alex Keybl [:akeybl]]

Comment on attachment 592328 [details] [diff] [review]
patch v3

[Triage Comment]
Mobile only - approved for Beta 11.

Comment 32

[Brad Lassey [:blassey] (use needinfo?)]

https://hg.mozilla.org/releases/mozilla-beta/rev/8a81092f109d