Closed Bug 701092 Opened 9 years ago Closed 8 years ago

Limit session restore attempts


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




Firefox 12
Tracking Status
firefox11 --- fixed
blocking-fennec1.0 --- beta+
fennec 11+ ---


(Reporter: kbrosnan, Assigned: bnicholson)




(2 files, 2 obsolete files)

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.
Attached file logcat
I also got into this state on a Honeycomb tablet with 20111109 Nightly Birch Build.  See attached logcat.
Work around is to stop and clear the data in the settings-> applications -> <app>
Closed: 9 years ago
Resolution: --- → INVALID
There is no way a user is going to perform that. We need session store not to be dumb.
Resolution: INVALID → ---
create a SUMO page for it then
Closed: 9 years ago9 years ago
Resolution: --- → WONTFIX
(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?
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.
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.
This is the behavior I am seeing.
what does desktop do? I think they detect a start up crash some how and do the "tabs from last time" screen instead.

Resolution: WONTFIX → ---
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.
Assigned to Madhava for UX
Assignee: nobody → madhava
tracking-fennec: --- → 11+
Priority: -- → P3
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.
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).
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.
Assignee: madhava → bnicholson
Summary: need a way to get out of a Gecko start up crash → Limit session restore attempts
Attached patch patch (obsolete) — Splinter Review
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.
Attachment #591231 - Flags: review?(mark.finkle)
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 on attachment 591231 [details] [diff] [review]

>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
Attachment #591231 - Flags: review?(mark.finkle) → review-
Attached patch patch v2 (obsolete) — Splinter Review
Attachment #591231 - Attachment is obsolete: true
Attachment #591907 - Flags: review?(mark.finkle)
(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 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
Attachment #591907 - Flags: review?(mark.finkle) → review+
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).
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?
(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.
Attached patch patch v3Splinter Review
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.
Attachment #591907 - Attachment is obsolete: true
Attachment #592328 - Flags: review?(mark.finkle)
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 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
Attachment #592328 - Flags: review?(mark.finkle) → review+
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"
Closed: 9 years ago8 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 12
Brian, request approval for aurora
Attachment #592328 - Flags: approval-mozilla-aurora?
Comment on attachment 592328 [details] [diff] [review]
patch v3

[Triage Comment]
Mobile only - approved for Beta 11.
Attachment #592328 - Flags: approval-mozilla-aurora? → approval-mozilla-beta+
blocking-fennec1.0: --- → beta+
See Also: → 1263110
You need to log in before you can comment on or make changes to this bug.