If migration doesn't complete because of something like bug 735126 then we should be able to something about it rather than silently give people an empty profile. For profile reset, the new profile shouldn't be set as the default since that would effectively be total data loss. This patch just skips setting the new profile as the default if Migrate returns false but that means that the browser will continue starting up in an empty profile. I think it would be better to do one of two of the following: 1) Show a dialog stating that reset failed. 2) Switch back to the old profile (possibly requiring a restart). bsmedberg, is #2 still feasible at the point in nsAppRunner where I'm making this change? Can I just switch the directory service and the env. vars back to the values for the old profile and continue starting up? Seems like it's too late so I considered calling nsAppStartup::Quit to cause a restart but I'm not sure if that will cause problems this early in startup?

Comment on attachment 605651 [details] [diff] [review] v.1 Add r.v. to Migrate and skip setting the default profile for reset on failure > ProfileMigrator.prototype = { > migrate: function PM_migrate(aStartup, aKey) { ... > Services.ww.openWindow(null, > "chrome://browser/content/migration/migration.xul", > "_blank", > "chrome,dialog,modal,centerscreen,titlebar", > params); >+ return this._migrationEnded; > }, >+ observe: function observe(aSubject, aTopic, aData) { >+ switch (aTopic) { >+ case "Migration:Ended": >+ this._migrationEnded = true; >+ break; >+ } >+ }, Hrm, no, this isn't sync...

Oh well, I really read this wrong. Ignore my last comment: However: 1. I prefer using some kind of out-param, rather than observing Migration:Ended here. 2. If you do keep observing after all, make sure to remove the observer. 3. There's no reason for using a boolean return value to indicate success. Just throw XPCOM exceptions.

Mano, you mentioned on IRC that you prefer that I get rid of the wrappedJSObject and use a nsISupportsPRBool as an argument. I tried to do this but xpconnect seems to automatically convert it to a JSBool and copy it by value so setting it to true doesn't modify migrationEnded in ProfileMigrator.js. Am I doing something wrong? If not, how do you want me to pass the return value?

Ugh, indeed, you're right. Sorry! Just go with wrappedJSObject then. So just update the idl comment.

Comment on attachment 606007 [details] [diff] [review] v.2 Add r.v. to Migrate and skip setting the default profile for reset on failure >diff --git a/browser/components/migration/content/migration.js b/browser/components/migration/content/migration.js >--- a/browser/components/migration/content/migration.js >+++ b/browser/components/migration/content/migration.js >@@ -57,20 +57,21 @@ var MigrationWizard = { > os.addObserver(this, "Migration:ItemBeforeMigrate", false); > os.addObserver(this, "Migration:ItemAfterMigrate", false); > os.addObserver(this, "Migration:ItemError", false); > os.addObserver(this, "Migration:Ended", false); > > this._wiz = document.documentElement; > > if ("arguments" in window && window.arguments.length > 1) { >- this._source = window.arguments[0]; >- this._migrator = window.arguments[1].QueryInterface(kIMig); >- this._autoMigrate = window.arguments[2].QueryInterface(kIPStartup); >- this._skipImportSourcePage = window.arguments[3]; >+ // Return values go in window.arguments[0]. Cache it (this._retval = ...wrappedJSObject) for better readability. >diff --git a/toolkit/xre/nsAppRunner.cpp b/toolkit/xre/nsAppRunner.cpp >--- a/toolkit/xre/nsAppRunner.cpp >+++ b/toolkit/xre/nsAppRunner.cpp >@@ -3558,20 +3558,24 @@ XRE_main(int argc, char* argv[], const n > nsCOMPtr<nsIProfileMigrator> pm > (do_CreateInstance(NS_PROFILEMIGRATOR_CONTRACTID)); > if (pm) { > nsCAutoString aKey; > if (gDoProfileReset) { > // Automatically migrate from the current application if we just > // reset the profile. > aKey = MOZ_APP_NAME; >- pm->Migrate(&dirProvider, aKey); >- // Set the new profile as the default after migration. >- rv = SetCurrentProfileAsDefault(profileSvc, profD); >- if (NS_FAILED(rv)) NS_WARNING("Could not set current profile as the default"); >+ rv = pm->Migrate(&dirProvider, aKey); >+ if (NS_SUCCEEDED(rv)) { >+ // Set the new profile as the default if migration completed. >+ rv = SetCurrentProfileAsDefault(profileSvc, profD); >+ if (NS_FAILED(rv)) NS_WARNING("Could not set current profile as the default"); >+ } else { >+ NS_WARNING("Migration did not complete during profile reset"); >+ } So, before landing this, you've to figure out what to do if migration did fail. Unless I'm misinterpreting it, we're getting this wrong in two ways: 1. The new profile won't be removed 2. It'll be "activated" for one session.