833943 - Migrate open windows and tabs when resetting Firefox

Status: VERIFIED FIXED

(Firefox :: Migration, defect)

Description

[Verdi [:verdi] Best to slack me]

Now that we only load the active tab when doing a session restore it should be safe to restore tabs and tab groups when resetting Firefox. The feedback on the reset feature has been very positive but the biggest disincentive to using it is that you loose your current tabs and tab groups. Adding this to the reset will greatly improve it's utility and create a nearly seamless recovery for users.

Comment 1

[Alex Limi (:limi) — Firefox UX Team]

Wanted to chime in here since I'm the one that suggested this change. :)

If we're worried that an open page will crash Firefox after restarting when doing a reset, we can open an about:newtab blank tab that has focus.

Nuking the tabs and tab groups shouldn't be done as part of the reset anymore. This will make it a lot more likely to be used.

Comment 2

[Matthew N. [:MattN]]

There have been quite a few problems with bad sessionstore data breaking things so I suggest that we only migrate the tab URLs (and possibly titles and groups) but not the entire sessionstore.json. (There may be other important field that I'm missing)

(In reply to Alex Limi (:limi) — Firefox UX Team from comment #1)
> If we're worried that an open page will crash Firefox after restarting when
> doing a reset, we can open an about:newtab blank tab that has focus.

Keep in mind that about:newtab would have no thumbnails in the new profile so about:home is probably better.

Comment 3

[Alex Limi (:limi) — Firefox UX Team]

(In reply to Matthew N. [:MattN] from comment #2)
> There have been quite a few problems with bad sessionstore data breaking
> things so I suggest that we only migrate the tab URLs (and possibly titles
> and groups) but not the entire sessionstore.json. (There may be other
> important field that I'm missing)

Right, that makes sense. Groups need to be included for the Panorama users out there.

> (In reply to Alex Limi (:limi) — Firefox UX Team from comment #1)
> > If we're worried that an open page will crash Firefox after restarting when
> > doing a reset, we can open an about:newtab blank tab that has focus.
> 
> Keep in mind that about:newtab would have no thumbnails in the new profile
> so about:home is probably better.

Yeah, that works too.

Comment 4

[Asa Dotzler [:asa]]

(In reply to Verdi [:verdi] from comment #0)
> Now that we only load the active tab when doing a session restore it should
> be safe to restore tabs and tab groups when resetting Firefox.

This is a short term hack that needs to go away. It's a huge responsiveness regression for most users who never have more than a handfull of tabs open at a time. We should not be building new features based on our broken tab restoring behavior. 

Also, with this change, it seems to me we'd be reducing the Reset Firefox to something like a "kill add-ons" feature because we are preserving pretty much everything else. Is that really the direction we want this feature to evolve? 

(In reply to Alex Limi (:limi) — Firefox UX Team from comment #3)
> Right, that makes sense. Groups need to be included for the Panorama users
> out there.

I disagree with this. We should not be investing further design or engineering effort in Panorama. There are far too many bugs that impact most Firefox users to continue pumping resources into this feature.

Comment 5

[Tim Taubert [:ttaubert] (inactive)]

(In reply to Alex Limi (:limi) — Firefox UX Team from comment #3)
> (In reply to Matthew N. [:MattN] from comment #2)
> > There have been quite a few problems with bad sessionstore data breaking
> > things so I suggest that we only migrate the tab URLs (and possibly titles
> > and groups) but not the entire sessionstore.json. (There may be other
> > important field that I'm missing)
> 
> Right, that makes sense. Groups need to be included for the Panorama users
> out there.

It should be quite easy to retain the current session and just purge history entries other than the currently active one from each tab. That way we'd also keep Tab Groups without additional overhead.

Comment 6

[:Gavin Sharp [email: [e-mail]]]

(In reply to Asa Dotzler [:asa] from comment #4)
> Also, with this change, it seems to me we'd be reducing the Reset Firefox to
> something like a "kill add-ons" feature because we are preserving pretty
> much everything else. Is that really the direction we want this feature to
> evolve?

That's a good question. If "losing tab groups" and "losing large existing session" are the biggest reported annoyances with the feature, I wonder whether this feedback is representative of the right set of users. It may be that we should focus on making "Reset Firefox" more visible to other users who could benefit from it (i.e. those who don't have a lot of tabs open and don't know about Panorama), rather than optimizing for the set of users who've already discovered it.

Having a "reset" feature that restores transient state like "open tabs" also feels kind of weird to me. There are workarounds either way ("bookmark all tabs"/close all tabs before resetting), so it's more about which particular case we care to optimize, and trading off the risk of porting over "bad" sessionstore data with the annoyance of losing your session.

Comment 7

[Florian Bender]

Yeah, let's make "Reset Profile" save all open Tabs as Bookmarks in a "Pre-reset Open Tabs" folder, and e. g. have a checkbox just above the Reset button ("Save open tabs as bookmarks.", when clicking the reset button, or after reset, a message will appear where the user can find his tabs). That seems to me like a sensible workaround. That way people may also cut down on tabs they no longer need to have opened ;-).

Comment 8

[David Lang]

the fact that tabs and windows are saved if the system crashes, but not if you hit reset profile is a problem. Opening to the same restore window that you get after a crash would be perfectly fine. If the problem is with a tab, the user can experiment to find a which one and start without it.

If it's safe to carry over history, you should be able to carry over the per-tab history (whatever cleanup is done to the main history should be done to the per-tab history)

While we are at it, why does it delete the old profile instead of mearly making the new one the default?

Comment 9

[Alex Limi (:limi) — Firefox UX Team]

After talking this through with MattN, how about we:

- Show about:sessionrestore after a reset (but only restore titles, URLs, groups)
- Update the text+icon so it doesn't look like an error

Text would be something like "You just reset Firefox, here are the tabs you had open, you can restore them or start fresh."

Comment 10

[David Bruant]

(In reply to Alex Limi (:limi) — Firefox UX Team from comment #9)
> After talking this through with MattN, how about we:
> 
> - Show about:sessionrestore after a reset (but only restore titles, URLs,
> groups)
> - Update the text+icon so it doesn't look like an error
> 
> Text would be something like "You just reset Firefox, here are the tabs you
> had open, you can restore them or start fresh."
I doubt this would result in a good user experience.
Currently, when I click on "reset Firefox", I have a dialog telling me what will be saved (history, bookmark, cookies, form data, passwords) and a message in bold saying "everything else will be deleted" (I have the UI in French, so sorry if that's not the exact en-US message). This implies tabs&groups are lost.
It's a good thing if we can recover tabs as a sessionrestore type of thing, but it's confusing in my opinion. Especially for something like Reset which will typically generates some anxiety, I think we'd better be very upfront on what can be saved and what cannot.

Comment 11

[Matthew N. [:MattN]]

(In reply to David Bruant from comment #10)
> Currently, when I click on "reset Firefox", I have a dialog telling me what
> will be saved (history, bookmark, cookies, form data, passwords) and a
> message in bold saying "everything else will be deleted" (I have the UI in
> French, so sorry if that's not the exact en-US message). This implies
> tabs&groups are lost.

That list would be updated to reflect the fact that tabs will be migrated.

Comment 12

[:Gijs (he/him)]

Attachment: Part 1: Add an API to get a minimal session store file to nsISessionStore, trigger restoring it via about:sessionstore

Comment 13

[:Gijs (he/him)]

Comment on attachment 735407 [details] [diff] [review]
Part 1: Add an API to get a minimal session store file to nsISessionStore, trigger restoring it via about:sessionstore

Just talked with Matt, and we want to do this differently, adapting and migrating the existing sessionstore.js file instead of creating a separate minimal copy.

Comment 14

[Caspy7]

(In reply to :Gijs Kruitbosch from comment #13)

Comment #2 from Matt indicated that "There have been quite a few problems with bad sessionstore data breaking things"
I take it this indicates a high degree of confidence that this adapted sessionstore.js does not carry with it the corruption or any of the negative attributes associated with a "bad" profile?
Also, if we're able to "clean" sessionstore.js like this, how about we do it every few months or something?  (If it strips out history or something I imagine that won't work so well.)

On a separate note how about we take the opportunity, if the user has more than N tabs open, to warn them that "Having many tabs open may affect performance."  Throw that into the session restore tab.

Comment 15

[:Gijs (he/him)]

(In reply to Mark S. from comment #14)
> (In reply to :Gijs Kruitbosch from comment #13)
> 
> Comment #2 from Matt indicated that "There have been quite a few problems
> with bad sessionstore data breaking things"
> I take it this indicates a high degree of confidence that this adapted
> sessionstore.js does not carry with it the corruption or any of the negative
> attributes associated with a "bad" profile?

We will probably want to warn the user about having many tabs affecting performance and give them the option of restoring (or not, or restore only a subset of their tabs). See also comment #9 and your own suggestion later in the comment. :-)

> Also, if we're able to "clean" sessionstore.js like this, how about we do it
> every few months or something?  (If it strips out history or something I
> imagine that won't work so well.)

Yes, it is slightly lossy (specifically, pinned tabs and groups info will not make it, as well as form data, subdocument (frame) data etc. etc.). If the store is completely corrupted, we won't be able to do anything. And even if we did keep some more info than I am planning to do, proper session restore requires some of the tricky info which is currently in there (eg. form information, child frames/documents).

Comment 16

[David Lang]

There is already a setting that lets you say 'warn about performance if opening more than X tabs', we don't need another one.

Besides, how do you determine how many tabs are too many? what will slow down a raspberry Pi won't bother most desktops, and what will slow most desktops won't bother a machine with gobs of ram and CPU on a fast network. (at least, it won't unless you trip over one of the bugs that seem to be easier to hit with lots of tabls)

Comment 17

[Tyler Downer [He/Him]]

If we can land this in 23 it would be nice, since we already landed Bug 834034. We can include this in our release notes that Firefox Reset is less destructive, update all our documentation at once, and this will make Firefox Reset much more powerful and desirable, which means we can then start working on efforts to make it more visible and recommend it to more users.

Comment 18

[:Gijs (he/him)]

Attachment: Part 1: Add an API to session storage to do a migration of a minimal subset of data

Comment 19

[:Gijs (he/him)]

Attachment: Part 2: Add about:welcomeback

Comment 20

[:Gijs (he/him)]

Attachment: Part 3: Make the migrator actually migrate stuff, adapt migration strings

Comment 21

[:Gijs (he/him)]

Attachment: Work around bug 863570

This part wouldn't be necessary if it wasn't for bug 863570. I'm still waiting for Benjamin to reply to my concerns there; I'd rather fix OS.Constants.profileDir and make it available earlier in startup. If that's not happening, I think this is the best solution.

Comment 22

[:Gijs (he/him)]

Attachment: about:welcomeback screenshot

Alex, I've tried to do more or less what you suggested in comment 9. Note that because of technical issues with how groups are stored, we *won't* restore groups, but we *will* restore whether or not a tab was pinned or not. The tabs also have their back/forward history intact.

The idea of naming it "about:welcomeback" was Matt Brubeck's, and I thought it sounded nice, so I went with that.

Comment 23

[Alex Limi (:limi) — Firefox UX Team]

Comment on attachment 742261 [details]
about:welcomeback screenshot

In general, this looks good.

I do think we should take the time to implement restoring the groups too, as it's hard to explain that "we will restore your tabs but drop your tab groups".

Nice work!

Comment 24

[Matthew N. [:MattN]]

I'm not sure we should migrate groups for the following reasons:
1) The value is low because of low usage of the feature
2) It's somewhat dirty to implement because we need to know about the contents of extdata which should be opaque to us
3) Implementing the UI for groups will cause it diverge from about:sessionrestore more and add another level of nesting making things more complicated.
3) It's possible users have groups they no longer care about and this makes it easier for them to continue to forget about them and have them take up memory and slow down operations which iterate all tabs.
* Yes, they could uncheck them on about:welcomeback but if we make it too easy to keep all tabs, users will probably just the restore button and not even evaluate whether they're needed.

On a separate note, are we fine with this level of prettiness for the page. It seems like we should make it more welcoming with artwork and colours. Gijs, do you have a UI/UX design on the radar of the UX team? Were you planning on doing that in a separate bug?

Comment 25

[:Gijs (he/him)]

(In reply to Matthew N. [:MattN] from comment #24)
> I'm not sure we should migrate groups for the following reasons:
> 1) The value is low because of low usage of the feature

I defer to Alex on this one. If he says we should care, I trust him on that call.

> 2) It's somewhat dirty to implement because we need to know about the
> contents of extdata which should be opaque to us

Agreed about it being mildly dirty, but I don't think that by itself is enough reason not to do it. It's about 10 lines on top of the first patch, as far as I can tell (although I haven't yet tested it, working on that).

> 3) Implementing the UI for groups will cause it diverge from
> about:sessionrestore more and add another level of nesting making things
> more complicated.

How do we need UI for this? If session store doesn't have UI for it, I was not planning on adding any. Not doing anything in particular also helps with 4, IMHO.

> 4) It's possible users have groups they no longer care about and this makes
> it easier for them to continue to forget about them and have them take up
> memory and slow down operations which iterate all tabs.
> * Yes, they could uncheck them on about:welcomeback but if we make it too
> easy to keep all tabs, users will probably just the restore button and not
> even evaluate whether they're needed.

This is a reasonable point. Alex, I should have brought this up but forgot about it - sorry. Does it change the equation for keeping the groups as far as you are concerned?

> On a separate note, are we fine with this level of prettiness for the page.
> It seems like we should make it more welcoming with artwork and colours.
> Gijs, do you have a UI/UX design on the radar of the UX team? Were you
> planning on doing that in a separate bug?

No, and no. I don't think we need this to be much more than this, personally, but again, I defer to UI/UX. If they feel otherwise, I'm happy to take that to a followup bug. I don't believe it should hold back this bug.

Comment 26

[David Bruant]

(In reply to Matthew N. [:MattN] from comment #24)
> 1) The value is low because of low usage of the feature
I haven't tried Reset Firefox on my girlfriend's dying Firefox, because she doesn't want to loose her Panorama group tabs.
Hmm... I haven't tried Reset Firefox on my own Firefox for the exact same reason actually...

> 4) It's possible users have groups they no longer care about and this makes
> it easier for them to continue to forget about them and have them take up
> memory and slow down operations which iterate all tabs.
One tab group is for job websites she regularly looks at. She doesn't spend her browsing life on this group, but certainly does care about it.

I don't mind if groups aren't migrated, however, please provide a way to not loose the URLs. If you want to turn a group into a bookmark folder before the reset (I believe there is an addon doing pretty much that [1]?), so be it, but please do not loose the information, that's a pretty bad betrayal to your users.

You wouldn't want your email provider to drop all your 1+ year old emails without telling you just because they take up space and you don't look at them regularly. "Keep my information" is an very important contract between a user and a software.

[1] https://addons.mozilla.org/fr/firefox/addon/the-grim-tab-reaper/

Comment 27

[:Gijs (he/him)]

(In reply to David Bruant from comment #26)
> I don't mind if groups aren't migrated, however, please provide a way to not
> loose the URLs. If you want to turn a group into a bookmark folder before
> the reset (I believe there is an addon doing pretty much that [1]?), so be
> it, but please do not loose the information, that's a pretty bad betrayal to
> your users.

All the tabs in groups would, with the current patch, end up as "normal" tabs. They are not lost. It is only the group information itself that is lost.

Comment 28

[David Bruant]

(In reply to :Gijs Kruitbosch from comment #27)
> All the tabs in groups would, with the current patch, end up as "normal"
> tabs. They are not lost. It is only the group information itself that is
> lost.
Oh ok. Cool then :-) Sorry for misunderstanding.

Comment 29

[Verdi [:verdi] Best to slack me]

(In reply to Matthew N. [:MattN] from comment #24)
> I'm not sure we should migrate groups for the following reasons:
> 1) The value is low because of low usage of the feature
Bug 836758 will remove panorama from Firefox and turn it into an add-on.

Comment 30

[Szabolcs Hubai (:xabolcs)]

(In reply to Verdi [:verdi] from comment #29)
> (In reply to Matthew N. [:MattN] from comment #24)
> > I'm not sure we should migrate groups for the following reasons:
> > 1) The value is low because of low usage of the feature
> Bug 836758 will remove panorama from Firefox and turn it into an add-on.

Are there any chance to a patch named "Add an API to notify Tab Group addon to do a migration of a minimal subset of data"?
In that case the grouping informations would be saved by the addon itself.

Comment 31

[Alex Limi (:limi) — Firefox UX Team]

That the code is a little "dirty" shouldn't matter here. It's a function that is a last-resort troubleshooting step, and we should take every effort to bring back everything we can. The fact that Panorama might be going away in the future shouldn't stop us from doing the right thing here. The code gets run once, it's not like it's a function you use 400 times per day.

If it's really really hard to migrate the groups, I could live with just all the URLs being loaded as tabs, but this should *only* be the case if we don't think we can pull off restoring the groups. Remember, most people won't have Panorama groups, but for the ones that do, those groups are arguably even more important than the current set of open tabs, since people use them as bookmarks, reminders and task lists.

Comment 32

[:Gijs (he/him)]

Comment on attachment 742248 [details] [diff] [review]
Part 1: Add an API to session storage to do a migration of a minimal subset of data

This needs work to do the tab groups. Not sure when I will have time, but I will try to get this done this week.

Comment 33

[:Gijs (he/him)]

Attachment: Part 1, v1.1 - sessionstore changes

Comment 34

[:Gijs (he/him)]

Attachment: Part 2, v1.1: Add about:welcomeback

Comment 35

[:Gijs (he/him)]

Attachment: Part 3, v1.1 - Migration changes

Comment 36

[:Gijs (he/him)]

Attachment: Part 4 - OS.File/profiledir-getting workarounds

Alright, this is all unbitrotted and this now successfully migrates tab groups. There is no UI (just like in the current about:sessionrestore) but if you hit 'restore', all your groups do get restored properly.


Gavin, Matt, Tim, I believe the idea is to try to get this into 24. I know you are all very busy, but it would be awesome if we could indeed get to this. Thanks!

Comment 37

[:Gavin Sharp [email: [e-mail]]]

Comment on attachment 759972 [details] [diff] [review]
Part 2, v1.1: Add about:welcomeback

>diff --git a/browser/locales/en-US/chrome/browser/aboutWelcomeBack.dtd b/browser/locales/en-US/chrome/browser/aboutWelcomeBack.dtd

How about just putting these strings in aboutSessionStore.dtd instead? (clearly separated, with a comment indicating their use in a different context, etc.)

>diff --git a/browser/themes/shared/aboutWelcomeBack.css b/browser/themes/shared/aboutWelcomeBack.css

>+%if 0
>+/* This Source Code Form is subject to the terms of the Mozilla Public
>+ * License, v. 2.0. If a copy of the MPL was not distributed with this
>+ * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
>+%endif

Not a fan of preprocessing license headers, particularly when they're the only thing that requires preprocessing in the file. Build time is reduced when you reduce the use of preprocessing, and our new license headers are short enough that it's not necessary to worry about space efficiency.

I kind of wonder if we couldn't just have about:sessionstore serve both purposes (depending on some query flag or something), actually. Have you considered that?

If you don't go that route, you may want to add about:welcomeback to gInitialPages?

Comment 38

[Matthew N. [:MattN]]

(In reply to :Gavin Sharp (use [e-mail] for email) from comment #37)
> I kind of wonder if we couldn't just have about:sessionstore serve both
> purposes (depending on some query flag or something), actually. Have you
> considered that?

We're going to be prettifying this page in a follow-up very shortly so the XHTML may change as part of that. It's hard to predict until we have designs though. The design may also apply to about:sessionrestore but that is not a goal.

Comment 39

[:Gijs (he/him)]

(In reply to Matthew N. [:MattN] from comment #38)
> (In reply to :Gavin Sharp (use [e-mail] for email) from comment
> #37)
> > I kind of wonder if we couldn't just have about:sessionstore serve both
> > purposes (depending on some query flag or something), actually. Have you
> > considered that?
> 
> We're going to be prettifying this page in a follow-up very shortly so the
> XHTML may change as part of that. It's hard to predict until we have designs
> though. The design may also apply to about:sessionrestore but that is not a
> goal.

This was the most important reason, yeah. If we're changing it more soon, it seemed simpler to give it its own page rather than trying to keep it all together in one place.  I'll incorporate all the other suggestions, including gInitialPages and upload an updated patch for reference in the(/my) morning.

Thanks for the super-quick review!

Comment 40

[Matthew N. [:MattN]]

Comment on attachment 759973 [details] [diff] [review]
Part 3, v1.1 - Migration changes

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

::: browser/components/migration/src/FirefoxProfileMigrator.js
@@ +63,1 @@
>    let getFileResource = function(aMigrationType, aFileNames) {

Nit: I think we generally prefer .bind(this) when possible.

@@ +99,5 @@
> +                                                                sessionFile.path);
> +        migrationPromise.then(function(err) {
> +          if (!err) {
> +            let buildID = Services.appinfo.platformBuildID;
> +            let mstone = Services.appinfo.platformVersion;

This doesn't exactly give the correct behaviour because these values should come from the old profile. Consider the following:
1) User is running Firefox version N
2) Update to Firefox version N+1 is staged in the background
3) User resets Firefox
This user wouldn't see the what's new or similar page for the update. I'm not sure this is an edge-case worth solving since the solution would require either eval'ing or regex matching the old pref file and looking for the two specific prefs.

@@ +102,5 @@
> +            let buildID = Services.appinfo.platformBuildID;
> +            let mstone = Services.appinfo.platformVersion;
> +            Services.prefs.setBoolPref("browser.sessionstore.resume_session_once", true);
> +            Services.prefs.setCharPref("browser.startup.homepage_override.mstone", mstone);
> +            Services.prefs.setCharPref("browser.startup.homepage_override.buildID", buildID);

Add a comment explaining why we're setting the homepage_override prefs. Wouldn't hurt for resume_session_once either.

::: browser/locales/en-US/chrome/browser/migration/migration.properties
@@ +28,5 @@
>  
>  4_ie=Browsing History
>  4_safari=Browsing History
>  4_chrome=Browsing History
> +4_firefox_session=Windows, Tabs, Browsing History and Bookmarks

The tabs should be their own data type on the IDL IMO. I think it could be used for migrating from other browsers as well.

I think combining all of these on one line make the list imbalanced and makes it harder to quickly scan visually.

Comment 41

[Matthew N. [:MattN]]

Comment on attachment 759972 [details] [diff] [review]
Part 2, v1.1: Add about:welcomeback

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

Nit: make aboutWelcomeBack.xmhtl an hg copy to preserve blame and make it easier to see how they differ.

::: browser/locales/en-US/chrome/browser/aboutWelcomeBack.dtd
@@ +7,5 @@
> +<!-- LOCALIZATION NOTE: The title is intended to be welcoming and congratulatory,
> +     expressing joy that the user has successfully migrated their stuff and hope
> +     that now they have a better experience.  -->
> +<!ENTITY welcomeback.pageTitle      "Welcome Back!">
> +<!ENTITY welcomeback.pageInfo    "&brandShortName; successfully reset your profile. Below is a list of windows and tabs you had open; you can restore them or start with a clean slate.">

Nit: align the strings (whether they are here or in the sessionrestore dtd as proposed by Gavin)

Comment 42

[:Gijs (he/him)]

(In reply to Matthew N. [:MattN] from comment #40)
> Comment on attachment 759973 [details] [diff] [review]
> Part 3, v1.1 - Migration changes
> @@ +99,5 @@
> > +                                                                sessionFile.path);
> > +        migrationPromise.then(function(err) {
> > +          if (!err) {
> > +            let buildID = Services.appinfo.platformBuildID;
> > +            let mstone = Services.appinfo.platformVersion;
> 
> This doesn't exactly give the correct behaviour because these values should
> come from the old profile. Consider the following:
> 1) User is running Firefox version N
> 2) Update to Firefox version N+1 is staged in the background
> 3) User resets Firefox
> This user wouldn't see the what's new or similar page for the update. I'm
> not sure this is an edge-case worth solving since the solution would require
> either eval'ing or regex matching the old pref file and looking for the two
> specific prefs.

First of all, yes, I think this is an edge-case. :-)

Second, what would the 'correct' solution be? Because as it is, if I were to implement changes as you suggest, you would see the what's new pages, and you wouldn't get your session back in an obvious way. I think when the user resets their profile, it's more important to show them their session than to show them the 'what's new' page. If they don't immediately see a page validating that they've reset and/or presenting them with their session, instead saying "hey look, Firefox got an update!", they will be confused about what happened to their windows/tabs. 

There is a significant amount of extra work in making a mix of an upgrade and a reset an actually good experience, I think. I don't think that should hold back this bug. If we're genuinely worried about this case, I actually think an acceptable option would be replacing the help menu item for reset (once that replaces Safe Mode) and/or the button on about:support, if that still exists, with "Update Firefox Now" if an update is staged. Updates do also solve problems, and I'd easily recommend doing that as a first step, before a profile reset.

Also, we've been talking about doing an automatic reset for "old" profiles, where this case is perhaps more likely (if other users w/ profiles have used and updated Firefox on that machine, for instance). If/when that gets done, I'd argue it should *not* migrate the tabs/windows, because they are unlikely to be of use to the user if (s)he hasn't opened them in 6 months or whatever our limit is going to be.

Comment 43

[:Gijs (he/him)]

Attachment: Part 2, v1.2: Add about:welcomeback

This fixes all the nits from gavin and MattN's reviews, I believe.

Carrying over r=gavin

Comment 44

[:Gijs (he/him)]

Attachment: Part 3, v1.2 - Migration changes

This addresses everything apart from the edgecase you cited, Matt. The order of "Open Tabs and Windows" being after "Other Data" in the post-migration dialog is a little weird now, but as that order already didn't match the reset profile dialog's order, I'm not sure we want to change that code and I left it like this for now.

Comment 45

[:Gijs (he/him)]

Attachment: Part 4 v1.1 - OS.File/profileDir-compat changes in sessionstore

This needed to be unbitrotted now.

Comment 46

[:Gijs (he/him)]

Attachment: Part 5, add about:welcomeback to gInitialPages

Forgot this part. Gavin, I'm guessing this is r=you, too, but am just doublechecking. :-)

Comment 47

[Tim Taubert [:ttaubert] (inactive)]

Comment on attachment 759970 [details] [diff] [review]
Part 1, v1.1 - sessionstore changes

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

I'm not sure I really like modifying _SessionFile.jsm to take an alternative path as all SessionFile.read() does is just read a file using the OS.File API. Equally, modifying nsSessionStartup to read and parse a file containing JSON data seems not right as well - even if it's related to sessionstore.

If we had something like AsyncFileUtils.read() that uses OS.File and returns a promise that would be useful in a couple of places. The whole nsSessionStartup._getStateFromString() method can actually be replaced by JSON.parse() as we're not supporting Firefox < 3.0 anymore. The whole patch could then be reduced to just read the file and parse its JSON contents. Using SessionStore to write the new file seems also a little overkill - we don't need to create a backup and write telemetry values. We could use a generic method like AsyncFileUtils.write(), even _SessionFile.write() does way more than we want.

Instead of putting the migration code into SessionStore.jsm, how about having a separate file for it? Maybe call it _SessionMigration.jsm or the like?

::: browser/components/sessionstore/src/SessionStore.jsm
@@ +3964,5 @@
> +    let migrated = Promise.defer();
> +    let self = this;
> +    gSessionStartup.getStateFromOtherPath(aPath).then(function(aStateObj) {
> +      if (!aStateObj) {
> +        migrated.resolve("Error: no valid session object could be created.");

Shouldn't this use migrated.reject()?

::: browser/components/sessionstore/src/_SessionFile.jsm
@@ +235,5 @@
>     * In case sessionstore.js file does not exist or is corrupted (something
>     * happened between backup and write), attempt to read the sessionstore.bak
>     * instead.
>     */
> +  read: function ssfi_read(aAltPath) {

read: function (aAltPath = this.path) {

::: browser/components/sessionstore/src/nsSessionStartup.js
@@ +90,5 @@
>      string.data = aData;
>      return string;
>    },
>  
> +  _getStateFromString: function sss_getStateFromString(aStateString) {

Nit: we don't need to name function expressions anymore.

@@ +188,5 @@
>        gOnceInitializedDeferred.resolve();
>      }
>    },
>  
> +  getStateFromOtherPath: function sss_getStateFromOtherPath(otherPath) {

Nit: we don't need to name function expressions anymore.

@@ +191,5 @@
>  
> +  getStateFromOtherPath: function sss_getStateFromOtherPath(otherPath) {
> +    let self = this;
> +    let readState = Promise.defer();
> +    _SessionFile.read(otherPath).then(function(aData) {

Nit: could use a fat-arrow function here to avoid 'self = this'.

Comment 48

[Matthew N. [:MattN]]

Comment on attachment 760106 [details] [diff] [review]
Part 3, v1.2 - Migration changes

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

Looks good. r=me assuming savePrefFile(null) works.

sr?(mak) for the IDL change.

::: browser/components/migration/src/FirefoxProfileMigrator.js
@@ +24,5 @@
>  function FirefoxProfileMigrator() { }
>  
>  FirefoxProfileMigrator.prototype = Object.create(MigratorPrototype);
>  
> +FirefoxProfileMigrator.prototype._getFileObject = function getFileObject(dir, fileName) {

Nit: drop the function name for consistency with getResources below.

@@ +106,5 @@
> +            Services.prefs.setCharPref("browser.startup.homepage_override.mstone", mstone);
> +            Services.prefs.setCharPref("browser.startup.homepage_override.buildID", buildID);
> +            let newPrefsFile = currentProfileDir.clone();
> +            newPrefsFile.append("prefs.js");
> +            Services.prefs.savePrefFile(newPrefsFile);

These three lines are normally equivalent to Services.prefs.savePrefFile(null). Why was "null" not suitable as an argument to savePrefFile?[1] Is the pref service not in a good state? If there is a good reason, please add a comment.

[1] https://developer.mozilla.org/en-US/docs/XPCOM_Interface_Reference/nsIPrefService#savePrefFile%28%29

::: browser/locales/en-US/chrome/browser/migration/migration.properties
@@ +49,5 @@
>  64_safari=Other Data
>  64_chrome=Other Data
>  64_firefox_other=Other Data
> +
> +128_firefox=Open Windows and Tabs

I keep going back-and-forth on whether "Open" should be part of the string shown to users. I understand that it is more explicit because we are intentionally throwing away closed tabs/windows but I'm not sure regular users will noticed the subtlety and so it could just be adding noise. Since it's kinda dirty to change the property names for migration (see getLocalizedString), I'd like to get agreement on this with UX & matej.

Comment 49

[Matthew N. [:MattN]]

I like wording/UX feedback on the last paragraph of comment 48 because it's a little dirty to change this string later. Thanks!

Comment 50

[Marco Bonardo [:mak] (Away Apr 25 - May 5)]

I'm sorry, but I'm not a superreviewer, afaik. I may feedback it if you wish.

Comment 51

[Matthew N. [:MattN]]

Comment on attachment 760106 [details] [diff] [review]
Part 3, v1.2 - Migration changes

(In reply to Marco Bonardo [:mak] from comment #50)
> I'm sorry, but I'm not a superreviewer, afaik. I may feedback it if you wish.

Sorry, I thought you gave sr on the last change to this interface but I see that was Gavin.

Comment 52

[:Gavin Sharp [email: [e-mail]]]

Comment on attachment 760106 [details] [diff] [review]
Part 3, v1.2 - Migration changes

no need to bump the IID as discussed on IRC

Comment 53

[Matthew N. [:MattN]]

Adding verifyme keyword since some of this code won't have automated tests and should be manually tested.

Comment 54

[Matej Novak [:matej]]

Sorry, I'm not clear which copy you'd like me to have a look at in comment 48. Can you paste it into a comment? Thanks.

Comment 55

[:Gijs (he/him)]

Attachment: Part 1, v2 - session migration jsm

I'm not entirely sure this is what you wanted. Tim, can you confirm this is more or less what you were thinking of?

Comment 56

[:Gijs (he/him)]

(In reply to Matej Novak [:matej] from comment #54)
> Sorry, I'm not clear which copy you'd like me to have a look at in comment
> 48. Can you paste it into a comment? Thanks.

(In reply to Matthew N. [:MattN] from comment #48)
> ::: browser/locales/en-US/chrome/browser/migration/migration.properties
> @@ +49,5 @@
> >  64_safari=Other Data
> >  64_chrome=Other Data
> >  64_firefox_other=Other Data
> > +
> > +128_firefox=Open Windows and Tabs
> 
> I keep going back-and-forth on whether "Open" should be part of the string
> shown to users. I understand that it is more explicit because we are
> intentionally throwing away closed tabs/windows but I'm not sure regular
> users will noticed the subtlety and so it could just be adding noise. Since
> it's kinda dirty to change the property names for migration (see
> getLocalizedString), I'd like to get agreement on this with UX & matej.

These strings are shown in the migration/reset dialog.

Comment 57

[Matej Novak [:matej]]

Oh, wow, not sure how I missed that. Sorry.

I agree that "open" doesn't seem to be adding much there. It seems fairly self-explanatory that we're talking about open ones. I'd support removing it.

Comment 58

[Tim Taubert [:ttaubert] (inactive)]

Comment on attachment 768739 [details] [diff] [review]
Part 1, v2 - session migration jsm

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

This looks good. I think we should be using SessionFile.write() though, writing to the current profile's session file is really what we want to do. The only thing we need to re-implement is reading the file.

Comment 59

[:Gijs (he/him)]

Attachment: Part 1, v2.1 - session migration jsm to do the migration

The profile dir info isn't available when the migration runs. This means we have to pass paths around to do both the saving of the session store and that of the prefs (see Part 3), hence not using SessionFile.write. :-(

This is the same patch but without the parameter for the wrapping URL, as for migration that's just always going to be about:welcomeback. As sessionstore also has references to this, I figured there wasn't much point making it a parameter. Otherwise, I just added a bunch of comments which hopefully clarifies how it works / why it does what it does.

Comment 60

[:Gijs (he/him)]

Attachment: Part 3, v1.3 - Migration changes

This time without "open" windows and tabs, without rev'ing the interface, using SessionMigration.jsm instead of SessionStore, and with a comment to explain why I'm using the pref service this way - I did try again to pass null, but that didn't work in my testing. :-(

Carrying over the sr+, just checking with Matt that this is (still) OK.

Comment 61

[:Gijs (he/him)]

Comment on attachment 760107 [details] [diff] [review]
Part 4 v1.1 - OS.File/profileDir-compat changes in sessionstore

Note that the meat of the current method of doing this (SessionMigration.jsm) is quite amenable to tests; I'd really like to land this ASAP and add tests in a follow-up.

Comment 62

[Tim Taubert [:ttaubert] (inactive)]

Comment on attachment 768981 [details] [diff] [review]
Part 1, v2.1 - session migration jsm to do the migration

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

r=me in general, but I'd really like to see the sessionstore change again before signing it off.

::: browser/components/sessionstore/src/SessionMigration.jsm
@@ +59,5 @@
> +      });
> +      // There are various tabgroup-related attributes that we need to get out
> +      // of the session restore data for the window, too.
> +      if (oldWin.extData) {
> +        for (var k in oldWin.extData) {

Maybe iterate over Object.keys(oldWin.extData)?

@@ +60,5 @@
> +      // There are various tabgroup-related attributes that we need to get out
> +      // of the session restore data for the window, too.
> +      if (oldWin.extData) {
> +        for (var k in oldWin.extData) {
> +          if (k.indexOf("tabview-") == 0) {

k.startsWith("tabview-")

::: browser/components/sessionstore/src/SessionStore.jsm
@@ +4085,5 @@
> +        return false;
> +      }
> +      if (singleURL == "about:welcomeback") {
> +        winData[0].tabs[0].entries[0].url = "about:sessionrestore";
> +        return false;

I don't like that _needsRestorePage() has such a side-effect. Can we maybe add an else clause to the part the calls this function? It would also be great to have a comment that explains why exactly we're replacing about:welcomeback here.

To not have to repeat the whole if condition checking for a single tab with a URL, how about adding a helper function like hasSingleTabWithURL(aURL)?

Comment 63

[Matthew N. [:MattN]]

Comment on attachment 768984 [details] [diff] [review]
Part 3, v1.3 - Migration changes

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

::: browser/components/migration/src/MigrationUtils.jsm
@@ +332,5 @@
>      FORMDATA:   Ci.nsIBrowserProfileMigrator.FORMDATA,
>      PASSWORDS:  Ci.nsIBrowserProfileMigrator.PASSWORDS,
>      BOOKMARKS:  Ci.nsIBrowserProfileMigrator.BOOKMARKS,
> +    OTHERDATA:  Ci.nsIBrowserProfileMigrator.OTHERDATA,
> +    SESSION:    Ci.nsIBrowserProfileMigrator.SESSION

Nit: add a trailing comma to help with blame for next time.

Comment 64

[:Gijs (he/him)]

Attachment: Part 1, v2.2 - session migration with refactored sessionstore code

Comment 65

[Tim Taubert [:ttaubert] (inactive)]

Comment on attachment 769105 [details] [diff] [review]
Part 1, v2.2 - session migration with refactored sessionstore code

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

::: browser/components/sessionstore/src/SessionStore.jsm
@@ +4108,5 @@
> +   */
> +  _hasSingleTabWithURL: function(aWinData, aURL) {
> +    if (aWinData && aWinData.length == 1 && aWinData[0].tabs &&
> +        aWinData[0].tabs.length == 1 && aWinData[0].tabs[0].entries &&
> +        aWinData[0].tabs[0].entries.length == 1) {

Nit: please put one condition per line.

@@ +4110,5 @@
> +    if (aWinData && aWinData.length == 1 && aWinData[0].tabs &&
> +        aWinData[0].tabs.length == 1 && aWinData[0].tabs[0].entries &&
> +        aWinData[0].tabs[0].entries.length == 1) {
> +      let singleURL = aWinData[0].tabs[0].entries[0].url;
> +      return aURL == singleURL;

Nit: that variable seems superfluous.

Comment 66

[:Gijs (he/him)]

Pushed to inbound, thanks everyone!

https://hg.mozilla.org/integration/mozilla-inbound/rev/c205141f8c8f
https://hg.mozilla.org/integration/mozilla-inbound/rev/2abe2b21106b
https://hg.mozilla.org/integration/mozilla-inbound/rev/23b5e3cb21aa

Comment 67

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/c205141f8c8f
https://hg.mozilla.org/mozilla-central/rev/2abe2b21106b
https://hg.mozilla.org/mozilla-central/rev/23b5e3cb21aa

Comment 68

[bhavana bajaj [:bajaj]]

Asa requested to add this to the change section on the feature notes

Comment 69

[Ioana (away)]

Verified as fixed on Windows 7 64bit - 09/01 Aurora on, Ubuntu 13.04 32bit - 09/02 Aurora, Mac OSX 10.7.5 - 09/03 Aurora.

Tested with one and multiple windows, dialogs, private windows, simple tabs, pinned tabs, tab groups, tabs with rich content, with plugins, with logins, with completed forms, searches perfomed, multiple resets, resets with tabs not loaded etc.

There are no major issues, but I did find some areas where I am not sure if there are bugs, or that's the design:

- Windows sizes are not imported to the new profile. Even small dialogs (like the Persona login one) are opened with the default window size.

- I searched directions on Google maps. The directions are kept when simply restarting Firefox with the "Show windows and tabs ..." option, but they are not kept when resetting Firefox.

- The SocialAPI sidebar is not imported on the new profile although I was active on the old one.

- The completed signup form here http://www.emag.ro/user/login is kept completed after restarts, but not after the reset (restart with "show windows and tabs from last time" selected).

- Panorama groups are kept, but there is no visual feedback of that in Firefox (the Panorama button). The button doesn't appear even after navigating between the groups multiple times.

- I reset Firefox, then, while it was on the about:sessionrestore page, I reset it again. An empty about:sessionrestore page was displayed at the second reset.

- Shouldn't this "Aurora successfully ... had open; you can restore them or start with a clean slate" be "Aurora successfully ... had open. you can restore them or start with a clean slate"?

Comment 70

[:Gijs (he/him)]

(In reply to Ioana Budnar, QA [:ioana] from comment #69)
> Verified as fixed on Windows 7 64bit - 09/01 Aurora on, Ubuntu 13.04 32bit -
> 09/02 Aurora, Mac OSX 10.7.5 - 09/03 Aurora.
> 
> Tested with one and multiple windows, dialogs, private windows, simple tabs,
> pinned tabs, tab groups, tabs with rich content, with plugins, with logins,
> with completed forms, searches perfomed, multiple resets, resets with tabs
> not loaded etc.
> 
> There are no major issues, but I did find some areas where I am not sure if
> there are bugs, or that's the design:
> 
> - Windows sizes are not imported to the new profile. Even small dialogs
> (like the Persona login one) are opened with the default window size.

By design. We don't keep window size information.

> - I searched directions on Google maps. The directions are kept when simply
> restarting Firefox with the "Show windows and tabs ..." option, but they are
> not kept when resetting Firefox.

Which Google Maps (new or old)? If the directions are kept as part of form data rather than in the URL, they will not be persisted. Anything in the URL of the page will be persisted, anything else won't.

> - The SocialAPI sidebar is not imported on the new profile although I was
> active on the old one.

By design. We don't keep any of the user's preference data, apart from their homepage, so the Social API code will need to be reinitialized.

> - The completed signup form here http://www.emag.ro/user/login is kept
> completed after restarts, but not after the reset (restart with "show
> windows and tabs from last time" selected).

By design. We don't keep form data.

> - Panorama groups are kept, but there is no visual feedback of that in
> Firefox (the Panorama button). The button doesn't appear even after
> navigating between the groups multiple times.

I don't understand this. How are you navigating between groups if there is no feedback? Can you file a bug with more exact steps to reproduce?

> - I reset Firefox, then, while it was on the about:sessionrestore page, I
> reset it again. An empty about:sessionrestore page was displayed at the
> second reset.

When you say "about:sessionrestore page" do you really mean the "about:welcomeback" page? Or actually "about:sessionrestore" (i.e. you then triggered the normal session restore page by crashing the browser? Either way, can you file a new bug with more exact steps to reproduce, if you can reproduce this issue? Thank you!

> - Shouldn't this "Aurora successfully ... had open; you can restore them or
> start with a clean slate" be "Aurora successfully ... had open. you can
> restore them or start with a clean slate"?

I *think* you're saying there should be a new sentence rather than a semicolon? Or did you change something else? I think either works as far as grammatical English or comprehensibility is concerned, but if you feel strongly you could ask :matej. Note that changing this right now on Aurora isn't possible anymore because of string freeze.

Comment 71

[Ioana (away)]

(In reply to :Gijs Kruitbosch from comment #70)
> Which Google Maps (new or old)? If the directions are kept as part of form
> data rather than in the URL, they will not be persisted. Anything in the URL
> of the page will be persisted, anything else won't.
Old ones, but the directions are not stored in the URL.

> > - Panorama groups are kept, but there is no visual feedback of that in
> > Firefox (the Panorama button). The button doesn't appear even after
> > navigating between the groups multiple times.
> 
> I don't understand this. How are you navigating between groups if there is
> no feedback? Can you file a bug with more exact steps to reproduce?
I used the Panorama shortcut: Ctrl+Shift+E. Filed bug 915132.
 
> > - I reset Firefox, then, while it was on the about:sessionrestore page, I
> > reset it again. An empty about:sessionrestore page was displayed at the
> > second reset.
> 
> When you say "about:sessionrestore page" do you really mean the
> "about:welcomeback" page? Or actually "about:sessionrestore" (i.e. you then
> triggered the normal session restore page by crashing the browser? Either
> way, can you file a new bug with more exact steps to reproduce, if you can
> reproduce this issue? Thank you!
Mea culpa. I did mean about:welcomeback. Filed bug 915126.

> > - Shouldn't this "Aurora successfully ... had open; you can restore them or
> > start with a clean slate" be "Aurora successfully ... had open. you can
> > restore them or start with a clean slate"?
> 
> I *think* you're saying there should be a new sentence rather than a
> semicolon? Or did you change something else? I think either works as far as
> grammatical English or comprehensibility is concerned, but if you feel
> strongly you could ask :matej. Note that changing this right now on Aurora
> isn't possible anymore because of string freeze.

I did only change the semicolon. This is just an aesthetics matter I'd say. I won't be filing anything here since the string freeze already happened.

Thanks for all the information!

Comment 72

[negociat.ro]

I am satisfied I will solve the problem by negociat.ro