347336 - [SessionStore] Preserve the list of recently closed tabs during one session

Status: RESOLVED FIXED

(Firefox :: Session Restore, defect)

Description

[Simon Bünzli]

... so that Undo Close Tab is still available after a crash.

Comment 1

[Simon Bünzli]

Attachment: fix

Comment 2

[Dietrich Ayala (:dietrich)]

>     
>     if (aOverwriteTabs) {
>       this.restoreWindowFeatures(aWindow, winData, root.opener || null);
>+      if (winData._closedTabs) {
>+        this._windows[aWindow.__SSi]._closedTabs = winData._closedTabs;
>+      }
>     }

Why are you making restoration of this data depend on aOverwriteTabs being true? IIRC, if an app update was downloaded pre-crash and then applied when restarting post-crash, aOverwriteTabs would be false, and we'd want to restore this data in that scenario.

Comment 3

[Simon Bünzli]

Attachment: fix (restores the list at startup and when overwriting a window)

Indeed, at startup we've got root._firstTabs but not aOverwriteTabs set for the very first window. Thanks for catching that.

Comment 4

[Simon Bünzli]

Drivers: This simple patch ensures that the list of recently closed tabs isn't lost after a crash and also allows extensions to restore that list when restoring an arbitrary session (which so far isn't possible). The patch's risk is low.

Comment 5

[Mike Schroepfer]

Comment on attachment 232672 [details] [diff] [review]
fix (restores the list at startup and when overwriting a window)

a=schrep for drivers - approving all [181approval pending] bugs now that tree is open.

Comment 6

[Dietrich Ayala (:dietrich)]

On branch, the patch for this bug caused the regression noted in bug 343871 comment #15.

Comment 7

[Dietrich Ayala (:dietrich)]

Attachment: initialize windows before restoring them

This patch fixes a race condition wherein we assumed that the domwindowopened notification would always be received before a window's contents would be restored. The fix initializes windows prior to attempting restoration, for windows have not yet been initialized.

Comment 8

[Mike Connor [:mconnor]]

Comment on attachment 235504 [details] [diff] [review]
initialize windows before restoring them

good catch, thanks!

Comment 9

[Dietrich Ayala (:dietrich)]

Checking in nsSessionStore.js;
/cvsroot/mozilla/browser/components/sessionstore/src/nsSessionStore.js,v  <--  nsSessionStore.js
new revision: 1.41; previous revision: 1.40
done

Comment 10

[Dietrich Ayala (:dietrich)]

Comment on attachment 235504 [details] [diff] [review]
initialize windows before restoring them

Drivers: This fixes a serious problem in which secondary restored windows have tabs but no content.

Comment 11

[Simon Bünzli]

Comment on attachment 235504 [details] [diff] [review]
initialize windows before restoring them

Dietrich: What happens when restoreWindow is called before the expected onLoad call?

AFAICT: onLoad is called now and it will be called later again - thus overwriting the window's __SSi which will effectively clear the list of recently closed tabs and all other data directly added to _windows[aWindow.__SSi]. You'll thus have to add the same check to onLoad as well.

Comment 12

[Gennady Feldman]

I guess that this patch hasn't landed yet, because I still see the problem with todays nightly ( Mozilla/5.0 (Windows; U; Windows NT 5.0; en-US; rv:1.8.1b2) Gecko/20060828 BonEcho/2.0b2 ). 

P.S. This bug is really annoying and discouraging for testers, because we lose the contents of other windows if we had >1 open before the update.

Comment 13

[Dietrich Ayala (:dietrich)]

Attachment: add window initialization check to onLoad

This addresses Simon's comment by not initializing windows that have already been initialized.

Comment 14

[Mike Connor [:mconnor]]

Comment on attachment 235779 [details] [diff] [review]
add window initialization check to onLoad

r+a=me for this, let's get these landed

Comment 15

[ronin.achilles]

Will this also restore the list of Closed Tabs if the user has selected  the option of restoring his session (tabs and windows) when starting Firefox. Its the expected behavior.
If not, should I file a separate bug for the same?

Comment 16

[Simon Bünzli]

(In reply to comment #15)
This patch indeed also restores the list when resuming a session regularly (without a crash).