Closed Bug 959399 Opened 6 years ago Closed 6 years ago

Firefox users who switch to metroFx and close the browser will lose their sessions

Categories

(Firefox for Metro Graveyard :: Browser, defect, P2)

x86_64
Windows 8.1
defect

Tracking

(firefox28 verified, firefox29 verified)

VERIFIED FIXED
Firefox 29
Tracking Status
firefox28 --- verified
firefox29 --- verified

People

(Reporter: TimAbraldes, Assigned: TimAbraldes)

References

Details

(Whiteboard: [defect] [beta28] p=3)

Attachments

(1 file, 3 obsolete files)

In the following situation, Firefox users who try out metroFx can experience data loss:
  A user of desktop Firefox (especially one who keeps a large number of open tabs and persists her/his session across multiple launches of desktop Firefox) notices that there is a new metro version of Firefox and decides to try it out. At the end of her/his session, the user closes metro Firefox normally (through "close" gesture, alt+f4, etc). The next time the user launches Firefox, it launches metroFx with a blank session, with no way to recover all those tabs that the user has carefully curated for who-knows-how-long. The user then optionally files strongly-worded, disparaging bugs.
Whiteboard: [triage] → [triage] [defect] p=0
We took the restore session option out a while back in a triage with Asa, so I think this is by design. If we want to bring that back we can, but I think it's too late to consider for fx28 rollout.
(In reply to Jim Mathies [:jimm] from comment #2)
> We took the restore session option out a while back in a triage with Asa, so
> I think this is by design. If we want to bring that back we can, but I think
> it's too late to consider for fx28 rollout.

Hmm, actually, first comments in bug 917020 indicate "show tabs from last time" should be the default setting. Maybe some where along the way we lost that in the profile work?
Asa, do you remember what the final decision on this was? I'm having a hard time finding the bug trail. I could have sworn we decided to default to about:start on restarts.
Flags: needinfo?(asa)
I believe that at the time, the final decision was that explicit app closes (Close Gesture + alt+f4), for any reason, closes the session out as well. So new restarts will show the start page.  This also matches what other Metro apps are supposed to do. 

A user that wants to carry over his session, at least for v1, would just not do an explicit close to do so.  A carried over session would also happen for a lite close on 8.1 where you only drag down for a second (also known as a win 8.0 close)

We could check what the other 2 browsers are doing though.  But I think this bug just comes down to a feature that was not planned for v1.  I think Karen may want it matched to Tim's expectation in Comment 0 though so you should check with her for final say.
I'm wondering if it's possible that bug 959396 is caused by the "lite close". My session seems to always restore (I'm on win 8.0).

I quickly tried this on Chrome and they seem to honour the 'restore my tabs' option that you can set on desktop which is option A) from this thread https://mail.mozilla.org/pipermail/metro/2014-January/000388.html that some of us agreed on.

I think this is what we should ultimately be doing. But if we do restore sessions after a lite close then perhaps users are less likely to blow their session away and I wouldn't mind keeping this as a lower priority for v1.
We did indeed remove the pref and the UI for selecting "show tabs from last time." That happened in bug 917020, and the decision was to follow the metro UX guidelines available on MSDN. Those guidelines say that, after a clean app close, the app should start up with a fresh session.

The major difference between the discussion that happened back then and the discussion we're having now is that metroFx and desktopFx now share sessions. This means that long-time desktopFx users who try out metroFx will have their *desktop* sessions discarded without warning and without a way to recover their lost data. That was never an issue back when metroFx and desktopFx had separate profiles and separate sessions, but is now a pathway for desktop users to lose their desktop data.

The fact that a lite close acts differently from a hard close is, in my mind, unimportant: Any user that restarts her/his machine while metroFx is running (something that Windows does on its own frequently and without user intervention) will see the "hard close" behavior. Any user that uses alt+f4 to shut down metroFx will also see the "hard close" behavior.

I don't really mind whether we call this a feature or a defect, but what I am interested in is whether this is a blocker for v1. I believe it should be, due to the data loss issue that I've mentioned previously.
Ah I see, makes sense Tim thanks for explaining.
I agree with Tim's comment #7
Flags: needinfo?(asa)
Assignee: nobody → tabraldes
Blocks: metrov1it22
No longer blocks: metrov1backlog
Status: NEW → ASSIGNED
Priority: -- → P2
QA Contact: jbecerra
Whiteboard: [triage] [defect] p=0 → [defect] p=0
Whiteboard: [defect] p=0 → [defect] p=2
Whiteboard: [defect] p=2 → [defect] p=3
Attached patch Patch v1 (obsolete) — Splinter Review
So far so good. I'll do a little more testing/cleaning of this patch before requesting review, but I think this is definitely the approach we'll take
Attached patch Patch v2 (obsolete) — Splinter Review
This is a cleaned up patch (removed debugging messages). With this patch applied, metroFx appears to correctly decide when it should restore tabs, but I've noticed that my sessionStore.js is always the same as a blank session which means that, even when restoring, I launch to a blank session. I'll try to track down the cause of this (might be a separate bug?)
Attachment #8361268 - Attachment is obsolete: true
Attached patch Patch v3 (obsolete) — Splinter Review
I don't thing anything actually changed between v2 and v3, but re-attaching just in case.

Tested this patch by doing the following:
  In desktopFx set "show home page"
  Switched to metroFx, made session non-blank by visiting a page, verified that "browser.sessionstore.resume_session_once" was false (to avoid bug 959396), quit metroFx, re-launched, verified that metroFx started a blank session

  In desktopFx set "show tabs from last time"
  Switched to metroFx, made session non-blank by visiting a page, verified that "browser.sessionstore.resume_session_once" was false (to avoid bug 959396), quit metroFx, re-launched, verified that metroFx started previous session
Attachment #8361283 - Attachment is obsolete: true
Attachment #8361301 - Flags: review?(mbrubeck)
Comment on attachment 8361301 [details] [diff] [review]
Patch v3

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

::: browser/metro/base/content/browser.js
@@ +184,5 @@
>  
>        // Should we restore the previous session (crash or some other event)
> +      let ss =
> +            Cc["@mozilla.org/browser/sessionstore;1"]
> +            .getService(Ci.nsISessionStore);

Can we reduce this from 3 lines to 2?  Just to conserve precious vertical space, and make my head hurt less?

::: browser/metro/profile/metro.js
@@ +133,5 @@
>  /* session history */
>  pref("browser.sessionhistory.max_entries", 50);
>  
> +// On startup, don't automatically restore tabs
> +pref("browser.startup.page", 0);

Should this default to 1 (show home page), like desktop Firefox?  I know we don't have honor the browser.startup.homepage pref (yet?) but we do, kind of, open a "home page" on startup.  I guess it doesn't actually matter with the current code...
Attachment #8361301 - Flags: review?(mbrubeck) → review+
Attached patch Patch v4Splinter Review
Review comments addressed, carrying forward r+

Will test locally with "mach mochitest-metro" and then land on fx-team
Attachment #8361301 - Attachment is obsolete: true
Attachment #8361345 - Flags: review+
Tests ran locally with no issue.

Landed on fx-team:
  https://hg.mozilla.org/integration/fx-team/rev/af860898eb79
Whiteboard: [defect] p=3 → [fixed-in-fx-team][defect] p=3
https://hg.mozilla.org/mozilla-central/rev/af860898eb79
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Whiteboard: [fixed-in-fx-team][defect] p=3 → [defect] p=3
Target Milestone: --- → Firefox 29
Whiteboard: [defect] p=3 → [defect] [beta28] p=3
Comment on attachment 8361345 [details] [diff] [review]
Patch v4

[Approval Request Comment]
Bug caused by (feature/regressing bug #): In bug 917020 we removed the ability to "show tabs from last time" in metroFx, which made sense until we also started sharing a profile with desktopFx - bug 924911 and others.
User impact if declined: Any desktop user that switches to metroFx and closes the browser will discover that her/his session is not restored.
Testing completed (on m-c, etc.): This was briefly tested locally, was on fx-team for two days, and has been on m-c since January 17th
Risk to taking this patch (and alternatives if risky): This patch mostly only touches metro. There is one change to toolkit/modules/WindowsPrefSync.jsm that adds one extra pref to our whitelist of prefs that sync from desktop to metro. This is a low-risk change.
String or IDL/UUID changes made by this patch: none
Attachment #8361345 - Flags: approval-mozilla-aurora?
Attachment #8361345 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Mozilla/5.0 (Windows NT 6.3; WOW64; rv:28.0) Gecko/20100101 Firefox/28.0
Mozilla/5.0 (Windows NT 6.3; WOW64; rv:29.0) Gecko/20100101 Firefox/29.0

Verified as fixed on latest Nightly (build ID: ) and latest Aurora (build ID: 20140122004004) using the STR from comment 12.
Status: RESOLVED → VERIFIED
Forgot to paste the Nightly build ID: 20140122030521
You need to log in before you can comment on or make changes to this bug.