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

Status: VERIFIED FIXED

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

Description

[Tim Abraldes [:TimAbraldes] [:tabraldes]]

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.

Comment 1

[Tim Abraldes [:TimAbraldes] [:tabraldes]]

These two threads include discussion about this issue and its potential implementations:
  https://mail.mozilla.org/pipermail/metro/2013-December/000368.html
  https://mail.mozilla.org/pipermail/metro/2014-January/000388.html

Comment 2

[Jim Mathies [:jimm]]

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.

Comment 3

[Jim Mathies [:jimm]]

(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?

Comment 4

[Jim Mathies [:jimm]]

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.

Comment 5

[Brian R. Bondy [:bbondy]]

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.

Comment 6

[Marina Samuel [:emtwo]]

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.

Comment 7

[Tim Abraldes [:TimAbraldes] [:tabraldes]]

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.

Comment 8

[Brian R. Bondy [:bbondy]]

Ah I see, makes sense Tim thanks for explaining.

Comment 9

[Asa Dotzler [:asa]]

I agree with Tim's comment #7

Comment 10

[Tim Abraldes [:TimAbraldes] [:tabraldes]]

Attachment: Patch v1

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

Comment 11

[Tim Abraldes [:TimAbraldes] [:tabraldes]]

Attachment: Patch v2

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?)

Comment 12

[Tim Abraldes [:TimAbraldes] [:tabraldes]]

Attachment: Patch v3

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

Comment 13

[Matt Brubeck (: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...

Comment 14

[Tim Abraldes [:TimAbraldes] [:tabraldes]]

Attachment: Patch v4

Review comments addressed, carrying forward r+

Will test locally with "mach mochitest-metro" and then land on fx-team

Comment 15

[Tim Abraldes [:TimAbraldes] [:tabraldes]]

Tests ran locally with no issue.

Landed on fx-team:
  https://hg.mozilla.org/integration/fx-team/rev/af860898eb79

Comment 16

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/af860898eb79

Comment 17

[Tim Abraldes [:TimAbraldes] [:tabraldes]]

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

Comment 18

[Matt Brubeck (:mbrubeck)]

https://hg.mozilla.org/releases/mozilla-aurora/rev/6e306ae9e1cb

Comment 19

[Cornel Ionce [:noni] [Hubs QA]]

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.

Comment 20

[Cornel Ionce [:noni] [Hubs QA]]

Forgot to paste the Nightly build ID: 20140122030521