In bug 718240, we wanted two different session restore behaviors based on whether we previously crashed or were killed. * If we crashed, we want to do a session restore the next time user opens Fennec. * If we were killed (force quit/task killer), we want to simply show about:home without doing a session restore. Since we don't currently record what caused the last crash/kill, we can't distinguish between these two cases. As described in https://bugzilla.mozilla.org/show_bug.cgi?id=718240#c22, we should be able to add this to the crash reporter.

this does not block, however if it is needed for some other blocker obviously it can be worked on.

We can detect a crash by setting a preference in the crash reporter. As confirmed with ibarlow, we only want to do a session restore if Fennec previously crashed; otherwise, if the user force quits, reboots the phone, etc., we just show about:home.

This gets rid of the arbitrary 30 second timeout and uses a pref in the crash reporter to detect a crash. To test this patch, you can open several tabs in Fennec, then enter the following command: adb shell am start -a android.intent.action.VIEW -n org.mozilla.fennec/.App -d "}" This will kill Fennec, launch the crash reporter, and restore tabs when Fennec is reopened. On the other hand, if Fennec is killed by the user (e.g., swiping to remove Fennec from Android's recent apps list), we never do a restore.

Minor cleanup.

Comment on attachment 689364 [details] [diff] [review] Only do session restore after a crash, v2 > editor.putBoolean(GeckoApp.PREFS_WAS_STOPPED, true); >+ editor.putBoolean(GeckoApp.PREFS_CRASHED, true); I was initially thinking we could reuse WAS_STOPPED, but I guess we don't want to muck around with it's true purpose. >diff --git a/mobile/android/base/GeckoApp.java b/mobile/android/base/GeckoApp.java > if (mRestoreMode == RESTORE_NONE && shouldRestoreSession()) { >- mRestoreMode = RESTORE_CRASH; >+ SharedPreferences prefs = GeckoApp.mAppContext.getSharedPreferences(PREFS_NAME, 0); >+ >+ // We record crashes in the crash reporter. If sessionstore.js >+ // exists, but we didn't flag a crash in the crash reporter, we >+ // were probably just force killed by the user, so we shouldn't do >+ // a restore. >+ if (prefs.getBoolean(PREFS_CRASHED, false)) { >+ prefs.edit().putBoolean(GeckoApp.PREFS_CRASHED, false).commit(); >+ mRestoreMode = RESTORE_CRASH; My first thought was "what is the most common code path and can we arrange the checks such that only one is needed most of the time". I figure that most people are not using "Quit" so sessionstore.js is usually around. shouldRestoreSession looks for the file existence, then we open a shared preference. Could we rearrange this to avoid the file check for the common case?

(In reply to Mark Finkle (:mfinkle) from comment #6) > Comment on attachment 689364 [details] [diff] [review] > Only do session restore after a crash, v2 > > My first thought was "what is the most common code path and can we arrange > the checks such that only one is needed most of the time". I figure that > most people are not using "Quit" so sessionstore.js is usually around. > shouldRestoreSession looks for the file existence, then we open a shared > preference. Could we rearrange this to avoid the file check for the common > case? After some benchmarking, it takes between 0-1ms to check for the existence of sessionstore.js on my Galaxy S, and it always takes 0ms to read the shared pref (probably because prefs are cached in memory once read). The difference is mostly insignificant, but I reversed the check anyway because it allows us to properly handle the edge case where the session restore pref is set to true, but there's no session file (i.e., a startup crash). With the previous patch, the pref would remain true until the session file existed and the pref is reset at some point in the future (which would cause a restore when we don't want one); with this patch, the pref is always reset at startup. https://hg.mozilla.org/integration/mozilla-inbound/rev/2fc15ea29e20

Comment on attachment 689364 [details] [diff] [review] Only do session restore after a crash, v2 [Approval Request Comment] Bug caused by (feature/regressing bug #): none; feature improvement User impact if declined: session restore behavior less predictable; sessions may not restore after a Fennec dies when users expect them to (or vice versa) Testing completed (on m-c, etc.): m-c Risk to taking this patch (and alternatives if risky): low risk String or UUID changes made by this patch: none

Comment on attachment 689364 [details] [diff] [review] Only do session restore after a crash, v2 low risk, will take on aurora.