Restore Previous Session menu option is still enabled after Restoring Previous Session

RESOLVED FIXED in Firefox 27

Status

()

Firefox
Session Restore
RESOLVED FIXED
4 years ago
4 years ago

People

(Reporter: MihaiMorar, Assigned: ttaubert)

Tracking

({regression})

20 Branch
Firefox 27
regression
Points:
---

Firefox Tracking Flags

(firefox24 wontfix, firefox25- wontfix, firefox26- affected, firefox27- verified)

Details

Attachments

(1 attachment, 2 obsolete attachments)

(Reporter)

Description

4 years ago
STR:
1. Open FF using a new profile
2. Open 3 tabs and pin one of them
3. Restart Firefox and go to History->Restore Previous Session
4. Repeat step 3

Actual Results:
At step 4 History->Restpre Previous Session button is still enabled.

Expecred Results:
At step 4 History->Restpre Previous Session button must be disabled.

This is reproducible on all OS's starting with FF 21.0. I will provide the real causing changeset ASAP
status-firefox24: --- → wontfix
status-firefox25: --- → affected
status-firefox26: --- → affected
status-firefox27: --- → affected
tracking-firefox25: --- → ?
tracking-firefox26: --- → ?
tracking-firefox27: --- → ?
Keywords: regression, regressionwindow-wanted

Comment 1

4 years ago
Regressed window(m-c)
Good:
http://hg.mozilla.org/mozilla-central/rev/00e26dece6ea
Mozilla/5.0 (Windows NT 6.1; WOW64; rv:20.0) Gecko/20121210 Firefox/20.0 ID:20121210105701
Bad:
http://hg.mozilla.org/mozilla-central/rev/4dfe323a663d
Mozilla/5.0 (Windows NT 6.1; WOW64; rv:20.0) Gecko/20121210 Firefox/20.0 ID:20121210134422
Pushlog
http://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=00e26dece6ea&tochange=4dfe323a663d

Regressed by:
4dfe323a663d	Ehsan Akhgari — Bug 819274 - Disable the Restore Previous Session command for private windows in per-window PB builds; r=dao
Blocks: 819274
Keywords: regressionwindow-wanted
Version: 21 Branch → 20 Branch

Comment 2

4 years ago
This is an old regression that doesn't appear to be critical to our users (this is our first report). No need to track.
status-firefox25: affected → wontfix
tracking-firefox25: ? → -
tracking-firefox26: ? → -
tracking-firefox27: ? → -
(Reporter)

Updated

4 years ago
QA Contact: mihai.morar
Created attachment 820152 [details] [diff] [review]
Disable 'Restore Last Session' menuitem after restoring  the last session
Assignee: nobody → ttaubert
Status: NEW → ASSIGNED
Attachment #820152 - Flags: review?(ehsan)
Attachment #820152 - Flags: review?(dao)
Hardware: x86_64 → All
Comment on attachment 820152 [details] [diff] [review]
Disable 'Restore Last Session' menuitem after restoring  the last session

This looks like it won't disable the command across multiple windows...
Attachment #820152 - Flags: review?(dao) → review-
Comment on attachment 820152 [details] [diff] [review]
Disable 'Restore Last Session' menuitem after restoring  the last session

Review of attachment 820152 [details] [diff] [review]:
-----------------------------------------------------------------

Dao's review here should be enough.
Attachment #820152 - Flags: review?(ehsan)
(In reply to Dão Gottwald [:dao] from comment #4)
> This looks like it won't disable the command across multiple windows...

Right, sorry. I totally missed that we need to handle all windows.
Created attachment 820843 [details] [diff] [review]
Disable 'Restore Last Session' menuitem after restoring  the last session, v2
Attachment #820152 - Attachment is obsolete: true
Attachment #820843 - Flags: review?(dao)
Comment on attachment 820843 [details] [diff] [review]
Disable 'Restore Last Session' menuitem after restoring  the last session, v2

Can SessionStore.jsm's restoreLastSession send a restoring-last-session notification? It's not clear to me why the _lastSessionState setter should notify. Seems somewhat obscure.
The problem is that external callers can reset the last session state by setting ss.canRestoreLastSession = false. When purging history we throw away the last session state as well.

I could copy the send notification code to all those places where _lastSessionState might be reset. I could also introduce a tiny object that holds the last session state and we could call something like LastSessionState.clear() in SessionStore internally, that might be more obvious than having a "private" getter/setter do it?
(In reply to Tim Taubert [:ttaubert] from comment #9)
> I could also introduce a tiny object that
> holds the last session state and we could call something like
> LastSessionState.clear() in SessionStore internally, that might be more
> obvious than having a "private" getter/setter do it?

I guess so, especially since _lastSessionState is already a private property of the private SessionStoreInternal object, so _lastSessionStateInternal would add a third layer...
Created attachment 821486 [details] [diff] [review]
Disable 'Restore Last Session' menuitem after restoring  the last session, v3
Attachment #820843 - Attachment is obsolete: true
Attachment #820843 - Flags: review?(dao)
Attachment #821486 - Flags: review?(dao)
Comment on attachment 821486 [details] [diff] [review]
Disable 'Restore Last Session' menuitem after restoring  the last session, v3

>   _tabsRestoringCount: 0,
> 
>-  // The state from the previous session (after restoring pinned tabs). This
>-  // state is persisted and passed through to the next session during an app
>-  // restart to make the third party add-on warning not trash the deferred
>-  // session
>-  _lastSessionState: null,
> 

nit: remove one of the two empty lines

>+let LastSessionState = {
>+  _state: null,
>+
>+  get canRestore() {
>+    return !!this._state;
>+  },
>+
>+  get: function () {
>+    return this._state;
>+  },
>+
>+  set: function (state) {
>+    this._state = state;
>+  },
>+
>+  clear: function () {
>+    if (this._state) {
>+      this._state = null;
>+      Services.obs.notifyObservers(null, NOTIFY_LAST_SESSION_CLEARED, null);
>+    }
>+  }
>+};

I wonder if this would be a better naming scheme:

let LastSession = {
  get canRestore()...
  getState: ...
  setState: ...
  clear: ...
}
Attachment #821486 - Flags: review?(dao) → review+
(In reply to Dão Gottwald [:dao] from comment #12)
> I wonder if this would be a better naming scheme:
> 
> let LastSession = {
>   get canRestore()...
>   getState: ...
>   setState: ...
>   clear: ...
> }

Yup, I like that more. Thanks!
https://hg.mozilla.org/integration/fx-team/rev/18ccb2ac1b15
https://hg.mozilla.org/mozilla-central/rev/18ccb2ac1b15
Status: ASSIGNED → RESOLVED
Last Resolved: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 27
Verified as fixed on latest Aurora (build ID: 20131111004004) and on latest Nightly (build ID: 20131110030205).
status-firefox27: affected → verified
QA Contact: mihai.morar → cornel.ionce
You need to log in before you can comment on or make changes to this bug.