Last Comment Bug 273874 - Firefox migrator for new profiles
: Firefox migrator for new profiles
Status: VERIFIED FIXED
[qa!]
:
Product: Firefox
Classification: Client Software
Component: Migration (show other bugs)
: unspecified
: All All
: -- enhancement with 2 votes (vote)
: Firefox 12
Assigned To: Matthew N. [:MattN] (PM me if requests are blocking you)
:
: Matthew N. [:MattN] (PM me if requests are blocking you)
Mentors:
https://wiki.mozilla.org/Support/Fire...
Depends on: 700898 715315 715348 721242 721265
Blocks: 717070
  Show dependency treegraph
 
Reported: 2004-12-09 04:32 PST by Daniel Brooks [:db48x]
Modified: 2013-03-14 17:37 PDT (History)
16 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
273874-1.diff (30.34 KB, patch)
2004-12-09 04:35 PST, Daniel Brooks [:db48x]
no flags Details | Diff | Splinter Review
273874-2.diff (39.03 KB, patch)
2005-01-05 03:34 PST, Daniel Brooks [:db48x]
mconnor: review-
Details | Diff | Splinter Review
v.1 Support for bookmarks & file copies of some others (37.68 KB, patch)
2011-12-15 23:07 PST, Matthew N. [:MattN] (PM me if requests are blocking you)
no flags Details | Diff | Splinter Review
v.2 Only look for default profile & get bookmarks from places.sqlite (33.82 KB, patch)
2012-01-03 21:47 PST, Matthew N. [:MattN] (PM me if requests are blocking you)
no flags Details | Diff | Splinter Review
v.3 don't support migration via UI (34.81 KB, patch)
2012-01-04 14:46 PST, Matthew N. [:MattN] (PM me if requests are blocking you)
no flags Details | Diff | Splinter Review
v.4 address review comments (36.57 KB, patch)
2012-01-07 15:14 PST, Matthew N. [:MattN] (PM me if requests are blocking you)
mak77: review+
Details | Diff | Splinter Review
v.5 bookmark backup folder lookup (37.09 KB, patch)
2012-01-10 00:11 PST, Matthew N. [:MattN] (PM me if requests are blocking you)
mak77: review+
Details | Diff | Splinter Review

Description Daniel Brooks [:db48x] 2004-12-09 04:32:54 PST
I have a rough patch already. It works on linux, not sure about other platforms.
While this might not seem necessary at first glance, this is for a browser that
will be built off of Firefox. Normal Firefox builds will never see an option to
import a Firefox profile because if they already have one they won't see the
import dialog.
Comment 1 Daniel Brooks [:db48x] 2004-12-09 04:35:30 PST
Created attachment 168295 [details] [diff] [review]
273874-1.diff

oh, and I need to look up the location of the profiles on the other platforms.
Comment 2 Daniel Brooks [:db48x] 2004-12-09 04:37:54 PST
Comment on attachment 168295 [details] [diff] [review]
273874-1.diff

Ben, could you please glance over this patch and tell me if I'm doing anything
majorly wrong? I'm not actually requesting r= yet, I'll wait until I can test
on some other platforms first. Thanks.
Comment 3 Daniel Brooks [:db48x] 2005-01-05 03:34:41 PST
Created attachment 170335 [details] [diff] [review]
273874-2.diff

it's pretty much finished, I just need to get it working on all the other
platforms.
Comment 4 Mike Connor [:mconnor] 2005-01-15 13:47:39 PST
Comment on attachment 170335 [details] [diff] [review]
273874-2.diff

there just isn't a valid argument why Firefox would want this code.  If another
browser based on Firefox wants this, they can use it for their own builds. 
This is no different than adding code to /browser for any other functionality
that Firefox neither wants or needs.
Comment 5 Worcester12345 2006-07-27 10:24:22 PDT
(In reply to comment #0)
> ...Normal Firefox builds will never see an option to
> import a Firefox profile because if they already have one they won't see the
> import dialog.
> 
That ability would be useful for when something goes wrong, and you have a profile backed up somewhere else and you want to grab just bookmarks and cookies for instance. No?
Comment 6 Worcester12345 2006-08-25 00:27:54 PDT
What about when you start Firefox and it creates a profile, then you want to import your settings from another profile (backup stored on USB for example)? This would be far easier than find/replace entire profiles, especially when there are multiple profiles existing. Think of nightly testers using this.
Comment 7 Matthew N. [:MattN] (PM me if requests are blocking you) 2011-12-09 13:49:21 PST
I'll be working on this as part of the profile cleanup feature[1].  It will create a new profile for a user and migrate their data over.  This will also be useful for users to migrate data between profiles for different channels[2].

I have a JS implementation with support for bookmarks that I will attach shortly.

[1] https://wiki.mozilla.org/Support/Firefox_Features/Clean_up_user_profile
[2] https://wiki.mozilla.org/Features/Desktop/Ability_to_run_concurrent_channels
Comment 8 Matthew N. [:MattN] (PM me if requests are blocking you) 2011-12-15 23:07:23 PST
Created attachment 582183 [details] [diff] [review]
v.1 Support for bookmarks & file copies of some others

Here is a working version that uses the toolkit profile service which is planned to be removed in bug 214675.  I'm flagging for feedback since I'd like to hear from bsmedberg on how he'd rather me get a list of profiles?  Should I just parse profiles.ini myself?  I personally think that as long as we have profiles.ini, we should have a common API to manipulate it so I'd rather this functionality not be removed in bug 214675.  In another bug for the reset profile feature[1] I'll have to create a new profile so I'd be using toolkit profile service there too if it wasn't going away.

mak, ignoring the above, could you do a pass on this?

It supports bookmarks using the JSON backups and history, passwords, cookies by just copying the related files.  This will be the basis for the clean up feature[1] and the plan is to iterate on the individual data types to improve the error handling in follow-up bugs.  For example, instead of just copying cookies.sqlite, a follow-up would attempt to query the database and insert the valid results into a new cookie database.  This would attempt to catch cases of database corruption.

[1] https://wiki.mozilla.org/Support/Firefox_Features/Clean_up_user_profile
Comment 9 Marco Bonardo [::mak] 2011-12-17 02:55:20 PST
well, if you copy places.sqlite there is poor advantage in copying bookmarks, since they are inside places.sqlite. Maybe I'd just copy the database for now, and in future, when we'll have an history backup, we can re-evaluate that.
Comment 10 Igor Velkov 2011-12-17 04:16:52 PST
Export bookmarks to json, copy places.sqlite, import bookmarks from json.
Comment 11 Marco Bonardo [::mak] 2011-12-20 05:28:04 PST
(In reply to Igor Velkov from comment #10)
> Export bookmarks to json, copy places.sqlite, import bookmarks from json.

what's the scope, places.sqlite includes bookmarks.
Comment 12 Benjamin Smedberg [:bsmedberg] 2011-12-20 06:40:47 PST
Comment on attachment 582183 [details] [diff] [review]
v.1 Support for bookmarks & file copies of some others

Most of the point of removing the profile manager and profile service was that there won't *be* a list of named profiles. There will just be a single (default) profile directory, and if you want to use a different directory you can use the -profile flag (or a custom UI a-la the "new" standalone profile manager). So I really don't want to code in anything which assumes that there will be multiple named profiles and you have to choose one.

It is ok for now to use the profile service to get the location of the "default" profile. If we eventually have different profiles for Nightly/Aurora/Beta, we should treat those as different products, not different named profiles as in the profile manager.
Comment 13 Marco Bonardo [::mak] 2011-12-28 05:36:11 PST
MattN, please clarify if you want a review on the remaining parts, (excluding bookmarks) or you are going to make a new revision addressing bookmarks and multiple profiles concerns rised in previous comments.
Comment 14 Matthew N. [:MattN] (PM me if requests are blocking you) 2012-01-03 21:47:30 PST
Created attachment 585650 [details] [diff] [review]
v.2 Only look for default profile & get bookmarks from places.sqlite

This patch now only uses the toolkit profile service to get the default profile and import from it if it's not the currently running profile.
Instead of importing bookmarks from the json backups, I just copy the bookmark backup directory to the new profile so that it can be used to restore from in case there is a problem with places.sqlite.
Comment 15 Marco Bonardo [::mak] 2012-01-04 05:54:17 PST
(In reply to Matthew N. [:MattN] from comment #14)
> I just copy the
> bookmark backup directory to the new profile so that it can be used to
> restore from in case there is a problem with places.sqlite.

this is a really great idea!
Comment 16 Marco Bonardo [::mak] 2012-01-04 07:16:29 PST
Comment on attachment 585650 [details] [diff] [review]
v.2 Only look for default profile & get bookmarks from places.sqlite

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

::: browser/components/migration/src/ChromeProfileMigrator.js
@@ +16,4 @@
>   * The Original Code is the Browser Profile Migrator.
>   *
>   * The Initial Developer of the Original Code is the Mozilla Foundation.
>   * Portions created by the Initial Developer are Copyright (C) 2011

2012... btw actually could you may just use MPL2? http://www.mozilla.org/MPL/headers/

@@ +43,5 @@
>  const Cr = Components.results;
>  const MIGRATOR = Ci.nsIBrowserProfileMigrator;
>  
>  const LOCAL_FILE_CID = "@mozilla.org/file/local;1";
> +const TOOLKIT_PROFILE_SVC_CID = "@mozilla.org/toolkit/profile-service;1";

this is used just once, I really don't care it being a const.

@@ +122,5 @@
> +    let bookmarksBackupDir = this._sourceProfile.clone();
> +    bookmarksBackupDir.append("bookmarkbackups");
> +    if (!bookmarksBackupDir.exists()) {
> +      throw("Bookmarks backup folder does not exist." + bookmarksBackupDir.path);
> +    }

Actually, this is not a strong error condition, it may happen that the user has bookmarks but not yet a backup, or he may have disabled backups for whatever reason (it's feasible through about:config).

@@ +134,5 @@
>    {
>      this._notifyStart(MIGRATOR.BOOKMARKS);
>  
>      try {
> +      let srcBackupsDir = this._bookmarks_backup_folder;

As I said, failing here is sign there are no backups, but not that importing bookmarks failed.
we should probably ignore error and always report success, after all bookmarks are imported with places.sqlite.

@@ +268,5 @@
>     * @param   aDoingStartup
>     *          non-null if called during startup.
>     * @return  supported migration types
> +   *
> +   * @todo    make sure source databases are not in-use

is this something you have to fix here yet, or willing to follow-up? if the latter this needs a bug number.

@@ +292,5 @@
>      } catch (e) {
>        Cu.reportError(e);
>      }
>  
> +    // The following migrations may cause damage if done while the desination profile is already

typo: desination

@@ +293,5 @@
>        Cu.reportError(e);
>      }
>  
> +    // The following migrations may cause damage if done while the desination profile is already
> +    // running so only allow them when migrating during startup.

Actually, in the backups migrator, you migrate backups only if the dest profile doesn't have a backups folder... if the destination profile is already running it likely has backups, so just conditioning all migrators on aDoingStartup would not change much and avoid differences that would only hit edge cases.

if we condition everything on aDoingStartup we may check this in sourceExist and make this a migrator that can only be used on startup.

@@ +315,3 @@
>      try {
> +      let file = this._sourceProfile.clone();
> +      file.append("cookies.sqlite");

I think you should also copy permissions.sqlite for cookies.

@@ +363,5 @@
> +      for (let i = 0; i < profiles.length; i++) {
> +        result = this.getMigrateData(profiles.queryElementAt(i, Ci.nsISupportsString), false);
> +        if (result)
> +          break;
> +      }

shouldn't this check the profile we try to import in sourceProfiles (the selectedProfile), rather than any profile?
You may move that code to an helper, included the check that we are not trying to import the current profile

@@ +402,5 @@
>     * Return home page URL
>     *
>     * @return  home page URL
>     */
>    get sourceHomePageURL()

this should be a todo, I suppose?
Comment 17 Benjamin Smedberg [:bsmedberg] 2012-01-04 12:31:28 PST
Comment on attachment 585650 [details] [diff] [review]
v.2 Only look for default profile & get bookmarks from places.sqlite

Will this non-Firefox browser have the same appname (in application.ini) as Firefox? If not, the profile service will look in the wrong place for profiles.ini and you wouldn't find the data anyway, right?

I feel like this is a lot of change in code which is already poorly tested and regression-prone, but that's really for mak or some other peer to decide overall.
Comment 18 Matthew N. [:MattN] (PM me if requests are blocking you) 2012-01-04 13:06:38 PST
(In reply to Benjamin Smedberg  [:bsmedberg] from comment #17)
> Comment on attachment 585650 [details] [diff] [review]
> v.2 Only look for default profile & get bookmarks from places.sqlite
> 
> Will this non-Firefox browser have the same appname (in application.ini) as
> Firefox? If not, the profile service will look in the wrong place for
> profiles.ini and you wouldn't find the data anyway, right?

The main motivation is for this to be able to import data to a new Firefox profile from the default Firefox profile in order to clean up and fix problems [1].  The idea is to automate the manual processes that support directs people to when they are having problems with their profile[2][3] some of which will become very difficult when the profile manager UI is removed.

This patch currently replaces a large portion of the steps on [2] and will save users a lot of time.

> I feel like this is a lot of change in code which is already poorly tested
> and regression-prone, but that's really for mak or some other peer to decide
> overall.

The changes are basically the same as was done for the Chrome migrator in bug 505192 and a testing harness is in progress in bug 700898 and bug 706017.

[1] https://wiki.mozilla.org/Support/Firefox_Features/Clean_up_user_profile
[2] https://support.mozilla.com/en-US/kb/Recovering%20important%20data%20from%20an%20old%20profile
[3] https://support.mozilla.com/en-US/kb/Managing%20profiles
Comment 19 Marco Bonardo [::mak] 2012-01-04 13:18:29 PST
actually splinter review doesn't show correctly that this adds a new file implementing the migrator, doesn't modify lots of code (it shows it as modifying an existing file cause you used hg cp).
Still, the lack of tests in Migration can't surely be denied!
Comment 20 Matthew N. [:MattN] (PM me if requests are blocking you) 2012-01-04 14:46:16 PST
Created attachment 585906 [details] [diff] [review]
v.3 don't support migration via UI

(In reply to Marco Bonardo [:mak] from comment #16)
> Comment on attachment 585650 [details] [diff] [review]
> v.2 Only look for default profile & get bookmarks from places.sqlite
> 
> Review of attachment 585650 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: browser/components/migration/src/ChromeProfileMigrator.js
> @@ +16,4 @@
> >   * The Original Code is the Browser Profile Migrator.
> >   *
> >   * The Initial Developer of the Original Code is the Mozilla Foundation.
> >   * Portions created by the Initial Developer are Copyright (C) 2011
> 
> 2012... btw actually could you may just use MPL2?
> http://www.mozilla.org/MPL/headers/

My plan was to switch to MPL2 since I saw your comment in another bug but I was trying to minimize the diff from the Chrome migrator. 

> @@ +315,3 @@
> >      try {
> > +      let file = this._sourceProfile.clone();
> > +      file.append("cookies.sqlite");
> 
> I think you should also copy permissions.sqlite for cookies.

I don't think that cookie permissions are in the 80% case and blocked cookies preventing logins could be the reason a user is migrating to a new profile so I'd rather hold off on this for now.  If it is deemed necessary then I'd rather do selective migration instead of a DB copy since I've seen the default add-on permissions get lost on profiles (eg. bug 506446) so I'd rather start with good defaults.

> @@ +363,5 @@
> > +      for (let i = 0; i < profiles.length; i++) {
> > +        result = this.getMigrateData(profiles.queryElementAt(i, Ci.nsISupportsString), false);
> > +        if (result)
> > +          break;
> > +      }
> 
> shouldn't this check the profile we try to import in sourceProfiles (the
> selectedProfile), rather than any profile?
> You may move that code to an helper, included the check that we are not
> trying to import the current profile

The problem is that sourceExists is called before a profile is selected and I was avoiding coding to the assumption that sourceProfiles will only return one profile.

> @@ +402,5 @@
> >     * Return home page URL
> >     *
> >     * @return  home page URL
> >     */
> >    get sourceHomePageURL()
> 
> this should be a todo, I suppose?

I considered this a preference and I'll migrate them in a follow-up (bug 715348).  I added a comment about that.
Comment 21 Marco Bonardo [::mak] 2012-01-05 05:33:48 PST
(In reply to Matthew N. [:MattN] from comment #20)
> I don't think that cookie permissions are in the 80% case and blocked
> cookies preventing logins could be the reason a user is migrating to a new
> profile so I'd rather hold off on this for now. 

Ok, though it's worth to add a comment saying this to the cookie migrator.

> The problem is that sourceExists is called before a profile is selected

Add a comment on this, as well.
Comment 22 Marco Bonardo [::mak] 2012-01-05 05:52:27 PST
Comment on attachment 585906 [details] [diff] [review]
v.3 don't support migration via UI

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

::: browser/components/migration/src/ChromeProfileMigrator.js
@@ +7,2 @@
>  
>  const Cc = Components.classes;

Since this migrator is "special" in the sense it's migrating Firefox to itself, it's worth to add a brief multiline comment block at the top explaining the reasoning for it.

@@ +86,5 @@
> +   */
> +  get _bookmarks_backup_folder()
> +  {
> +    let bookmarksBackupDir = this._sourceProfile.clone();
> +    bookmarksBackupDir.append("bookmarkbackups");

Would be healthier to add a folderRelativePath (or something similar) to PlacesUtils.backups to help maintenability in case backups should be moved in future (most likely there will be a separate module to handle those, and doing a search would then easily find this point to be fixed).

@@ +252,1 @@
>        return result;

Just to be sure, this means we will show the option to Migrate Firefox, but show no migrator available, or the option is not shown if no migrator is available but sourceExists returns true?
If possible we should ensure Firefox is not a selectable option when migrating from the UI, otherwise would be a dead one.

@@ +261,5 @@
>      } catch (e) {
>        Cu.reportError(e);
>      }
>  
> +    // places

I prefer if you refer to it as // Bookmarks, history and favicons.

@@ +347,5 @@
> +      if (!this._profilesCache)
> +      {
> +        this._profilesCache = Cc["@mozilla.org/array;1"].createInstance(Ci.nsIMutableArray);
> +        let profileService = Cc["@mozilla.org/toolkit/profile-service;1"].getService(Ci.nsIToolkitProfileService);
> +        var profile = profileService.selectedProfile;

Add a comment on the reasoning we only support the selected profile.

@@ +366,5 @@
>     * Return home page URL
>     *
>     * @return  home page URL
> +   *
> +   * @todo    bug 715348

add also a brief comment on what is that bug supposed to handle
Comment 23 Matthew N. [:MattN] (PM me if requests are blocking you) 2012-01-07 15:14:51 PST
Created attachment 586736 [details] [diff] [review]
v.4 address review comments

(In reply to Marco Bonardo [:mak] from comment #22)
> @@ +252,1 @@
> >        return result;
> 
> Just to be sure, this means we will show the option to Migrate Firefox, but
> show no migrator available, or the option is not shown if no migrator is
> available but sourceExists returns true?
> If possible we should ensure Firefox is not a selectable option when
> migrating from the UI, otherwise would be a dead one.

If sourceExists returns false, Firefox is not shown in the list of migrators available, therefore Firefox will never be shown when migrating from the UI.
Comment 24 Marco Bonardo [::mak] 2012-01-09 03:00:43 PST
I mean, when launched from the UI, I imagine sourceExists returns true, so the Firefox entry should appear. but there is no data we can migrate because we are not in the startup path, so ideally the entry should not appear. Otherwise the user can choice Firefox but then he can't migrate anything. What happens really?
Comment 25 Marco Bonardo [::mak] 2012-01-09 08:28:50 PST
Comment on attachment 586736 [details] [diff] [review]
v.4 address review comments

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

It looks good enough to land imo.
I would just like to figure out comment 24, and a better name for the backups folder path getter, but those are minor changes.

::: browser/components/migration/src/ChromeProfileMigrator.js
@@ +2,5 @@
>   * vim: sw=2 ts=2 sts=2 et
> + * This Source Code is subject to the terms of the Mozilla Public License
> + * version 2.0 (the "License"). You can obtain a copy of the License at
> + * http://mozilla.org/MPL/2.0/.
> + */

nit: the boilerplate has been upgraded to have /* and /* in line with the licence

@@ +332,5 @@
>        let profiles = this.sourceProfiles;
>        if (profiles.length < 1)
>          return false;
> +      // check that we can get data from at least one profile since profile selection has not
> +      // happened yet

global-really-nit: proper punctuation in comments (ucfirst, end with perios, use commas)

::: toolkit/components/places/PlacesUtils.jsm
@@ +1827,5 @@
>        delete this.folder;
>        return this.folder = bookmarksBackupDir;
>      },
>  
> +    get profileRelativeFolder() {

clarify in the name that this is a path (even just by a Path suffix), otherwise it looks like an nsILocalFile
Comment 26 Matthew N. [:MattN] (PM me if requests are blocking you) 2012-01-10 00:11:27 PST
Created attachment 587256 [details] [diff] [review]
v.5 bookmark backup folder lookup

(In reply to Marco Bonardo [:mak] from comment #24)
> I mean, when launched from the UI, I imagine sourceExists returns true, so
> the Firefox entry should appear. but there is no data we can migrate because
> we are not in the startup path, so ideally the entry should not appear.
> Otherwise the user can choice Firefox but then he can't migrate anything.
> What happens really?

sourceExists only returns true when getMigrateData(..) > 0 so the user won't get to the data selection screen if there is no data to migrate.

(In reply to Marco Bonardo [:mak] from comment #22)
> ::: browser/components/migration/src/FirefoxProfileMigrator.js
> @@ +86,5 @@
> > +   */
> > +  get _bookmarks_backup_folder()
> > +  {
> > +    let bookmarksBackupDir = this._sourceProfile.clone();
> > +    bookmarksBackupDir.append("bookmarkbackups");
> 
> Would be healthier to add a folderRelativePath (or something similar) to
> PlacesUtils.backups to help maintenability in case backups should be moved
> in future (most likely there will be a separate module to handle those, and
> doing a search would then easily find this point to be fixed).

After testing patch v4 more I realized why I wasn't using PlacesUtils.backups.folder before: That code uses "profD" but when the migrator is run during startup it must use "profDS" since "profD" is not setup yet.

I changed the approach to accommodate this. Please re-review.
Comment 27 Marco Bonardo [::mak] 2012-01-10 04:52:02 PST
Comment on attachment 587256 [details] [diff] [review]
v.5 bookmark backup folder lookup

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

::: toolkit/components/places/PlacesUtils.jsm
@@ +1830,5 @@
>  
> +    get profileRelativeFolderPath() {
> +      delete this.profileRelativeFolderPath;
> +      return this.profileRelativeFolderPath = "bookmarkbackups";
> +    },

so, if this is trying to make the property readonly, seems wrong, since it's replacing the getter with a simple property, otherwise this may just be
get profileRelativeFolderPath() "bookmarkbackups",

Or I miss anything?
Comment 28 Matthew N. [:MattN] (PM me if requests are blocking you) 2012-01-10 18:36:16 PST
(In reply to Marco Bonardo [:mak] from comment #27)
> get profileRelativeFolderPath() "bookmarkbackups",
I replaced it with your version

https://hg.mozilla.org/integration/mozilla-inbound/rev/c550ffd3ba74
Comment 29 Ed Morley [:emorley] 2012-01-11 18:16:37 PST
https://hg.mozilla.org/mozilla-central/rev/c550ffd3ba74
Comment 30 Ioana (away) 2012-03-20 03:29:00 PDT
Verified as fixed on:
Mozilla/5.0 (Windows NT 6.1; rv:13.0) Gecko/20120318 Firefox/13.0a2
Mozilla/5.0 (X11; Linux i686; rv:13.0) Gecko/20120319 Firefox/13.0a2
Mozilla/5.0 (Macintosh; Intel Mac OS X 10.6; rv:13.0) Gecko/20120318 Firefox/13.0a2

Note You need to log in before you can comment on or make changes to this bug.