Closed Bug 273874 Opened 20 years ago Closed 12 years ago

Firefox migrator for new profiles

Categories

(Firefox :: Migration, enhancement)

enhancement
Not set
normal

Tracking

()

VERIFIED FIXED
Firefox 12

People

(Reporter: db48x, Assigned: MattN)

References

(Depends on 2 open bugs, )

Details

(Whiteboard: [qa!])

Attachments

(1 file, 6 obsolete files)

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.
Attached patch 273874-1.diff (obsolete) — Splinter Review
oh, and I need to look up the location of the profiles on the other platforms.
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.
Attachment #168295 - Flags: review?(bugs)
Attached patch 273874-2.diff (obsolete) — Splinter Review
it's pretty much finished, I just need to get it working on all the other
platforms.
Attachment #168295 - Attachment is obsolete: true
Attachment #170335 - Flags: review?(bugs)
Attachment #168295 - Flags: review?(bugs)
Attachment #170335 - Flags: review?(bugs) → review?(mconnor)
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.
Attachment #170335 - Flags: review?(mconnor) → review-
(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?
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.
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
Assignee: db48x → mnoorenberghe+bmo
Status: NEW → ASSIGNED
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
Attachment #582183 - Flags: review?(mak77)
Attachment #582183 - Flags: feedback?(benjamin)
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.
Export bookmarks to json, copy places.sqlite, import bookmarks from json.
(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 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.
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.
Attachment #582183 - Flags: feedback?(benjamin)
Attachment #582183 - Flags: review?(mak77)
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.
Attachment #582183 - Attachment is obsolete: true
Attachment #585650 - Flags: review?(mak77)
Attachment #585650 - Flags: feedback?(benjamin)
(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 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 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.
(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
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!
Summary: Firefox Migrator → Firefox migrator for new profiles
Depends on: 700898
Depends on: 715315
Depends on: 715348
(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.
Attachment #585650 - Attachment is obsolete: true
Attachment #585906 - Flags: review?(mak77)
Attachment #585650 - Flags: review?(mak77)
Attachment #585650 - Flags: feedback?(benjamin)
(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 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
Attached patch v.4 address review comments (obsolete) — Splinter Review
(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.
Attachment #585906 - Attachment is obsolete: true
Attachment #586736 - Flags: review?(mak77)
Attachment #585906 - Flags: review?(mak77)
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 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
Attachment #586736 - Flags: review?(mak77) → review+
(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.
Attachment #170335 - Attachment is obsolete: true
Attachment #586736 - Attachment is obsolete: true
Attachment #587256 - Flags: review?(mak77)
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?
Attachment #587256 - Flags: review?(mak77) → review+
Blocks: 717070
(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
Target Milestone: --- → Firefox 12
https://hg.mozilla.org/mozilla-central/rev/c550ffd3ba74
Status: ASSIGNED → RESOLVED
Closed: 12 years ago
Resolution: --- → FIXED
Depends on: 721242
Depends on: 721265
Depends on: 725904
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
Status: RESOLVED → VERIFIED
Whiteboard: [qa!]
Blocks: 750979
Depends on: 834034
Depends on: 833943
No longer depends on: 833943
No longer depends on: 834034
No longer depends on: 725904
No longer blocks: 750979
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: