The default bug view has changed. See this FAQ.

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
3 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
(Assignee)

Comment 3

4 years ago
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)
(Assignee)

Updated

4 years ago
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)
(Assignee)

Comment 6

4 years ago
(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.
(Assignee)

Comment 7

4 years ago
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.
(Assignee)

Comment 9

4 years ago
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...
(Assignee)

Comment 11

4 years ago
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+
(Assignee)

Comment 13

4 years ago
(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!
(Assignee)

Comment 14

4 years ago
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.