Oops, managed to hit the submit button too soon. Anyway, we only update the "Tabs from last time" with the tabs from the last session when we're not restoring tabs automatically, i.e. if the user has opted to "Never restore", or if the crash loop protection from bug 1263110 kicks in. This means that the rest of the time, those tabs will become stale pretty soon, so we should stop showing them when they become out of date.

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

sessionstore.bak, which powers the "tabs from last time" display, is only updated with the data of the previous session if the user chooses to "never restore" tabs, or if we've temporarily disabled session restoring because of too many successive crashes in a row. In all other cases, we restore tabs automatically so sessionstore.bak is never updated with fresh data, meaning its contents get stale pretty soon. With this patch, we clean up old copies of sessionstore.bak when doing an automatic restore, so they don't linger indefinitely in the "tabs from last time" section of the recent tabs panel. Review commit: https://reviewboard.mozilla.org/r/48247/diff/#index_header See other reviews: https://reviewboard.mozilla.org/r/48247/

Comment on attachment 8744012 [details] MozReview Request: Bug 1266339 - Part 1 - Move session store file names in GeckoProfile into named constants. r=margaret https://reviewboard.mozilla.org/r/48245/#review44991

Comment on attachment 8744013 [details] MozReview Request: Bug 1266339 - Part 2 - Delete outdated sessionstore.bak files when restoring tabs automatically. r=margaret https://reviewboard.mozilla.org/r/48247/#review44995 ::: mobile/android/base/java/org/mozilla/gecko/GeckoApp.java:1538 (Diff revision 1) > if (!mShouldRestore) { > getProfile().moveSessionFile(); > + } else { > + // If we *are* restoring, clean up old copies of sessionstore.bak, > + // so we don't show stale "tabs from last time". > + getProfile().removeOldSessionFile(); Is this happening on the main thread? I see we're already doing file I/O for `moveSessionFile`, but that seems like a problem that we're doing this on the main thread during startup. ::: mobile/android/base/java/org/mozilla/gecko/GeckoProfile.java:795 (Diff revision 1) > + */ > + public void removeOldSessionFile() { > + File sessionFileBackup = getFile(SESSION_FILE_BACKUP); > + if (sessionFileBackup != null && sessionFileBackup.exists() && > + System.currentTimeMillis() - sessionFileBackup.lastModified() > MAX_BACKUP_FILE_AGE) { > + sessionFileBackup.delete(); Let's ask antlam about the desired UX here. This sounds reasonable to me, but I'm not sure what users are expecting for their "Tabs from last time". Also, what happens to the UI if there is no sessionstore.bak? Do we just hide that "Tabs from last time" section?

https://reviewboard.mozilla.org/r/48247/#review44995 > Is this happening on the main thread? I see we're already doing file I/O for `moveSessionFile`, but that seems like a problem that we're doing this on the main thread during startup. The only problem with moving it to a background thread is that I assume it would be no longer strictly guaranteed that the file moving etc. has finished before the recent tabs panel initialises and tries reading from sessionstore.bak (although in practice it might usually still work?). To be absolutely certain, it looks like it should be possible to store a flag (restoring yes/no) in GeckoProfiles and do the file moving just before [the recent tabs panel tries reading the previous session](https://dxr.mozilla.org/mozilla-central/rev/0891f0fa044cba28024849803e170ed7700e01e0/mobile/android/base/java/org/mozilla/gecko/home/RecentTabsPanel.java#332), as that bit of code runs in a background thread anyway, if [this comment](https://dxr.mozilla.org/mozilla-central/rev/0891f0fa044cba28024849803e170ed7700e01e0/mobile/android/base/java/org/mozilla/gecko/home/RecentTabsPanel.java#294) is to be believed. On the other hand though, in that case how large is the remaining margin between the recent tabs panel having initialised itself and Gecko itself being up and running? Once the session store is active, it'll overwrite the old session data in sessionstore.js, so when not restoring, its contents **must** be copied to sessionstore.bak before that point. Alternatively, I could bring the file moving forward in time to run in the background task spun off off onCreate. It means we normally wouldn't notice `mShouldRestore` reverting back to false [after an error](https://dxr.mozilla.org/mozilla-central/rev/0891f0fa044cba28024849803e170ed7700e01e0/mobile/android/base/java/org/mozilla/gecko/GeckoApp.java#1488), but as that is normally caused by a corrupt sessionstore.js, we wouldn't be any worse off: Today: 1. mShouldRestore = true 2. restoring the session from sessionstore.js fails 3. mShouldRestore = false 4. We move sessionstore.js to sessionstore.bak, so when we later try accessing the tabs from last time, parsing sessionstore.bak will most probably fail, too. As proposed: 1. mShouldRestore = true 2. We leave sessionstore.js alone and only delete sessionstore.bak if it's too old 3. restoring the session from sessionstore.js fails 4. mShouldRestore = false 5. If sessionstore.bak wasn't too old, we can at least still show some tabs from "last" time Plus a few lines above, we're already creating the tabs stubs for the UI on the main thread anyway when doing a restore, which feels like a heavier operation to me because it involves actually reading and parsing the session file - here on the other hand we're at most either renaming a file, or accessing its last modified data and possibly deleting it. Is the impact really that bad? > Let's ask antlam about the desired UX here. This sounds reasonable to me, but I'm not sure what users are expecting for their "Tabs from last time". > > Also, what happens to the UI if there is no sessionstore.bak? Do we just hide that "Tabs from last time" section? Yes, that section is simply hidden if sessionstore.bak doesn't contain any tabs/is empty/missing/out for lunch :-). I think there are three main scenarios here: 1. A user with the tab restore setting set to "Never restore" changes her/his mind and sets it to "Always restore", so the tabs from last time stop being updated. 2. We've temporarily deactivated session restore because of too many successive crashes in a row. Instead, the old session is shown as "tabs from last time", so the user can manually restore tabs as needed. Once we've managed a successful background-foreground-background cycle without crashing, we turn session restore back on, so sooner or later the "tabs from last time" aren't needed any more. 3. Session restore is turned off, but we've temporarily activated it for one time only because we've been upgraded to a new version, OOM killed etc. All old tabs which would normally be shown as "Tabs from last time" are automatically restored.

(In reply to Jan Henning [:JanH] from comment #6) > https://reviewboard.mozilla.org/r/48247/#review44995 > > > Is this happening on the main thread? I see we're already doing file I/O for `moveSessionFile`, but that seems like a problem that we're doing this on the main thread during startup. > > The only problem with moving it to a background thread is that I assume it > would be no longer strictly guaranteed that the file moving etc. has > finished before the recent tabs panel initialises and tries reading from > sessionstore.bak (although in practice it might usually still work?). To be > absolutely certain, it looks like it should be possible to store a flag > (restoring yes/no) in GeckoProfiles and do the file moving just before [the > recent tabs panel tries reading the previous > session](https://dxr.mozilla.org/mozilla-central/rev/ > 0891f0fa044cba28024849803e170ed7700e01e0/mobile/android/base/java/org/ > mozilla/gecko/home/RecentTabsPanel.java#332), as that bit of code runs in a > background thread anyway, if [this > comment](https://dxr.mozilla.org/mozilla-central/rev/ > 0891f0fa044cba28024849803e170ed7700e01e0/mobile/android/base/java/org/ > mozilla/gecko/home/RecentTabsPanel.java#294) is to be believed. > > On the other hand though, in that case how large is the remaining margin > between the recent tabs panel having initialised itself and Gecko itself > being up and running? Once the session store is active, it'll overwrite the > old session data in sessionstore.js, so when not restoring, its contents > **must** be copied to sessionstore.bak before that point. > > Alternatively, I could bring the file moving forward in time to run in the > background task spun off off onCreate. It means we normally wouldn't notice > `mShouldRestore` reverting back to false [after an > error](https://dxr.mozilla.org/mozilla-central/rev/ > 0891f0fa044cba28024849803e170ed7700e01e0/mobile/android/base/java/org/ > mozilla/gecko/GeckoApp.java#1488), but as that is normally caused by a > corrupt sessionstore.js, we wouldn't be any worse off: > Today: > 1. mShouldRestore = true > 2. restoring the session from sessionstore.js fails > 3. mShouldRestore = false > 4. We move sessionstore.js to sessionstore.bak, so when we later try > accessing the tabs from last time, parsing sessionstore.bak will most > probably fail, too. > > As proposed: > 1. mShouldRestore = true > 2. We leave sessionstore.js alone and only delete sessionstore.bak if it's > too old > 3. restoring the session from sessionstore.js fails > 4. mShouldRestore = false > 5. If sessionstore.bak wasn't too old, we can at least still show some tabs > from "last" time > > Plus a few lines above, we're already creating the tabs stubs for the UI on > the main thread anyway when doing a restore, which feels like a heavier > operation to me because it involves actually reading and parsing the session > file - here on the other hand we're at most either renaming a file, or > accessing its last modified data and possibly deleting it. Is the impact > really that bad? Fair point, although just beacuse we're already doing bad things doesn't mean we should continue to do more :) This thread conversation is probably outside the scope of this. In general, we avoid doing any disk I/O on the main thread, since that can freeze the UI, but perhaps that's a risk we're willing to take in order to populate the UI immediately. It sounds like our UI logic would need a decent amount of reworking if we were to move this work to a background thread.

Chiming in from the UX side of things here. This seems to challenge and somewhat break the model we're setting up with our new History panel. I'm having trouble following along the nuances and it seems like its really complicated to grasp. Seeing as how we default to Restore tabs "always" right now, I think this is also going to be seen less and less. On top of that, I fear that seeing this placed next to the History items, Synced tabs, and recently closed tabs makes the user question how it's different to everything else. Maybe, our telemetry could tell us more here but I'd like to suggest that we remove "Tabs from last time" because it's creating more confusion and doesn't seem as useful anymore. Barbara, thoughts?

I'm a bit hesitant about removing it entirely as long as we still offer the option to switch the tab restore setting back to never - plus over in bug 1263110 I've repurposed the "tabs from last time" panel to act similarly to desktop's "Well, this is embarrassing" dialogue, i.e. to allow a user to manually restore tabs if we couldn't restore them automatically because of too many crashes in a row. What this patch does is make sure that once we turn automatic tab restoring back on (or if a user the settings option from "never" to "always"), the "tabs from last time" section continues to be shown for at most 24 h. After that, the tabs from last time section disappears and will never reappear as long as - we don't get into a crash loop, and - the user doesn't switch the tab restore settings back to never.

Comment on attachment 8744012 [details] MozReview Request: Bug 1266339 - Part 1 - Move session store file names in GeckoProfile into named constants. r=margaret Review request updated; see interdiff: https://reviewboard.mozilla.org/r/48245/diff/1-2/

Comment on attachment 8744013 [details] MozReview Request: Bug 1266339 - Part 2 - Delete outdated sessionstore.bak files when restoring tabs automatically. r=margaret Review request updated; see interdiff: https://reviewboard.mozilla.org/r/48247/diff/1-2/

Comment on attachment 8744013 [details] MozReview Request: Bug 1266339 - Part 2 - Delete outdated sessionstore.bak files when restoring tabs automatically. r=margaret https://reviewboard.mozilla.org/r/48247/#review49878 ::: mobile/android/base/java/org/mozilla/gecko/GeckoProfile.java:774 (Diff revision 2) > - * from last time"). Normally, sessionstore.js is moved to sessionstore.bak > - * on a clean quit, but this doesn't happen if Fennec crashed. Thus, this > - * method should be called after a crash so sessionstore.bak correctly > - * holds the previous session. > + * If we're not restoring tabs automatically, sessionstore.js needs to be moved to > + * sessionstore.bak, so we can display the correct "tabs from last time". > + * If we *are* restoring tabs, we need to delete outdated copies of sessionstore.bak, > + * so we don't continue showing stale "tabs from last time" indefinitely. > */ > - public void moveSessionFile() { > + public void updateSessionFile(Boolean aRestore) { Nit: Drop the "a" prefix - you could also call this something more boolean-ish like `shouldRestore`. Why `Boolean` instead of `boolean`?

https://reviewboard.mozilla.org/r/48247/#review49878 > Nit: Drop the "a" prefix - you could also call this something more boolean-ish like `shouldRestore`. > > Why `Boolean` instead of `boolean`? Blame Java for allowing both - thanks for catching it.

Comment on attachment 8744012 [details] MozReview Request: Bug 1266339 - Part 1 - Move session store file names in GeckoProfile into named constants. r=margaret Review request updated; see interdiff: https://reviewboard.mozilla.org/r/48245/diff/2-3/

Comment on attachment 8744013 [details] MozReview Request: Bug 1266339 - Part 2 - Delete outdated sessionstore.bak files when restoring tabs automatically. r=margaret Review request updated; see interdiff: https://reviewboard.mozilla.org/r/48247/diff/2-3/

https://hg.mozilla.org/integration/fx-team/rev/582c4b721a4f https://hg.mozilla.org/integration/fx-team/rev/1633fe6e9c81

https://hg.mozilla.org/integration/fx-team/rev/582c4b721a4f https://hg.mozilla.org/integration/fx-team/rev/1633fe6e9c81

https://hg.mozilla.org/mozilla-central/rev/582c4b721a4f https://hg.mozilla.org/mozilla-central/rev/1633fe6e9c81

Release Note Request (optional, but appreciated) [Why is this notable]: [Suggested wording]: Cleaned up "Tabs from last time" to hide outdated tabs when restore tabs setting is set to "Always restore" [Links (documentation, blog post, etc)]:

Relnote 49+ added "Outdated tabs are now hidden when restore tabs setting is set to "Always restore"

Verified as fixed in build 49.0a2 (2016-06-16); Device: - LG G4 (Android 5.1); - Nexus 5 (Android 6.0.1).