Bug 1289229 - disable automigration undo if people create/change bookmarks/logins, r?mak draft
authorGijs Kruitbosch <gijskruitbosch@gmail.com>
Tue, 26 Jul 2016 12:48:37 +0100
changeset 392857 bf4fd2a706c4467b9c8b9f5b82e908d5dcaf9e4e
parent 391305 e0bc88708ffed39aaab1fbc0ac461d93561195de
child 392858 1acd63faad086f5c523d9d94fe4f22f379216b66
push id24138
push usergijskruitbosch@gmail.com
push dateTue, 26 Jul 2016 12:20:08 +0000
reviewersmak
bugs1289229
milestone50.0a1
Bug 1289229 - disable automigration undo if people create/change bookmarks/logins, r?mak MozReview-Commit-ID: DAoNV9H71Yv
browser/app/profile/firefox.js
browser/components/migration/AutoMigrate.jsm
browser/components/migration/MigrationUtils.jsm
browser/components/migration/content/migration.js
browser/components/migration/tests/unit/test_automigration.js
browser/components/nsBrowserGlue.js
--- a/browser/app/profile/firefox.js
+++ b/browser/app/profile/firefox.js
@@ -1445,17 +1445,17 @@ pref("toolkit.pageThumbs.minHeight", 190
 
 // Enable speech synthesis
 pref("media.webspeech.synth.enabled", true);
 
 pref("browser.esedbreader.loglevel", "Error");
 
 pref("browser.laterrun.enabled", false);
 
-pref("browser.migration.automigrate", false);
+pref("browser.migrate.automigrate.enabled", false);
 
 // Enable browser frames for use on desktop.  Only exposed to chrome callers.
 pref("dom.mozBrowserFramesEnabled", true);
 
 pref("extensions.pocket.enabled", true);
 
 pref("signon.schemeUpgrades", true);
 
--- a/browser/components/migration/AutoMigrate.jsm
+++ b/browser/components/migration/AutoMigrate.jsm
@@ -3,83 +3,128 @@
  * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
 
 "use strict";
 
 this.EXPORTED_SYMBOLS = ["AutoMigrate"];
 
 const { classes: Cc, interfaces: Ci, results: Cr, utils: Cu } = Components;
 
-const kAutoMigrateStartedPref = "browser.migrate.automigrate-started";
-const kAutoMigrateFinishedPref = "browser.migrate.automigrate-finished";
+const kAutoMigrateEnabledPref = "browser.migrate.automigrate.enabled";
+
+const kAutoMigrateStartedPref = "browser.migrate.automigrate.started";
+const kAutoMigrateFinishedPref = "browser.migrate.automigrate.finished";
+const kAutoMigrateBrowserPref = "browser.migrate.automigrate.browser";
+
+const kPasswordManagerTopic = "passwordmgr-storage-changed";
+const kPasswordManagerTopicTypes = new Set([
+  "addLogin",
+  "modifyLogin",
+]);
 
 Cu.import("resource:///modules/MigrationUtils.jsm");
+Cu.import("resource://gre/modules/Preferences.jsm");
 Cu.import("resource://gre/modules/PlacesUtils.jsm");
 Cu.import("resource://gre/modules/Services.jsm");
 Cu.import("resource://gre/modules/Task.jsm");
 Cu.import("resource://gre/modules/XPCOMUtils.jsm");
 
 const AutoMigrate = {
   get resourceTypesToUse() {
     let {BOOKMARKS, HISTORY, PASSWORDS} = Ci.nsIBrowserProfileMigrator;
     return BOOKMARKS | HISTORY | PASSWORDS;
   },
 
+  init() {
+    this.enabled = Preferences.get(kAutoMigrateEnabledPref, false);
+    if (this.enabled) {
+      this.maybeInitUndoObserver();
+    }
+  },
+
+  maybeInitUndoObserver() {
+    // Check sync if we really need to do this:
+    if (!this.getUndoRange()) {
+      return;
+    }
+    // Now register places and password observers:
+    this.onItemAdded = this.onItemMoved = this.onItemChanged =
+      this.removeUndoOption;
+    PlacesUtils.addLazyBookmarkObserver(this, true);
+    Services.obs.addObserver(this, kPasswordManagerTopic, true);
+  },
+
+  observe(subject, topic, data) {
+    // As soon as any login gets added or modified, disable undo:
+    // (Note that this ignores logins being removed as that doesn't
+    //  impair the 'undo' functionality of the import.)
+    if (kPasswordManagerTopicTypes.has(data)) {
+      this.removeUndoOption();
+    }
+  },
+
   /**
    * Automatically pick a migrator and resources to migrate,
    * then migrate those and start up.
    *
    * @throws if automatically deciding on migrators/data
    *         failed for some reason.
    */
   migrate(profileStartup, migratorKey, profileToMigrate) {
-    let histogram = Services.telemetry.getHistogramById("FX_STARTUP_MIGRATION_AUTOMATED_IMPORT_PROCESS_SUCCESS");
+    let histogram = Services.telemetry.getHistogramById(
+      "FX_STARTUP_MIGRATION_AUTOMATED_IMPORT_PROCESS_SUCCESS");
     histogram.add(0);
-    let migrator = this.pickMigrator(migratorKey);
+    let {migrator, pickedKey} = this.pickMigrator(migratorKey);
     histogram.add(5);
 
     profileToMigrate = this.pickProfile(migrator, profileToMigrate);
     histogram.add(10);
 
     let resourceTypes = migrator.getMigrateData(profileToMigrate, profileStartup);
     if (!(resourceTypes & this.resourceTypesToUse)) {
       throw new Error("No usable resources were found for the selected browser!");
     }
     histogram.add(15);
 
     let sawErrors = false;
-    let migrationObserver = function(subject, topic, data) {
+    let migrationObserver = (subject, topic, data) => {
       if (topic == "Migration:ItemError") {
         sawErrors = true;
       } else if (topic == "Migration:Ended") {
         histogram.add(25);
         if (sawErrors) {
           histogram.add(26);
         }
         Services.obs.removeObserver(migrationObserver, "Migration:Ended");
         Services.obs.removeObserver(migrationObserver, "Migration:ItemError");
         Services.prefs.setCharPref(kAutoMigrateStartedPref, startTime.toString());
         Services.prefs.setCharPref(kAutoMigrateFinishedPref, Date.now().toString());
+        Services.prefs.setCharPref(kAutoMigrateBrowserPref, pickedKey);
+        // Need to manually start listening to new bookmarks/logins being created,
+        // because when we were initialized there wasn't the possibility to
+        // 'undo' anything.
+        this.maybeInitUndoObserver();
       }
     };
 
     Services.obs.addObserver(migrationObserver, "Migration:Ended", false);
     Services.obs.addObserver(migrationObserver, "Migration:ItemError", false);
     // We'll save this when the migration has finished, at which point the pref
     // service will be available.
     let startTime = Date.now();
     migrator.migrate(this.resourceTypesToUse, profileStartup, profileToMigrate);
     histogram.add(20);
   },
 
   /**
    * Pick and return a migrator to use for automatically migrating.
    *
    * @param {String} migratorKey   optional, a migrator key to prefer/pick.
-   * @returns                      the migrator to use for migrating.
+   * @returns {Object}             an object with the migrator to use for migrating, as
+   *                               well as the key we eventually ended up using to obtain it.
    */
   pickMigrator(migratorKey) {
     if (!migratorKey) {
       let defaultKey = MigrationUtils.getMigratorKeyForDefaultBrowser();
       if (!defaultKey) {
         throw new Error("Could not determine default browser key to migrate from");
       }
       migratorKey = defaultKey;
@@ -87,17 +132,17 @@ const AutoMigrate = {
     if (migratorKey == "firefox") {
       throw new Error("Can't automatically migrate from Firefox.");
     }
 
     let migrator = MigrationUtils.getMigrator(migratorKey);
     if (!migrator) {
       throw new Error("Migrator specified or a default was found, but the migrator object is not available.");
     }
-    return migrator;
+    return {migrator, pickedKey: migratorKey};
   },
 
   /**
    * Pick a source profile (from the original browser) to use.
    *
    * @param {Migrator} migrator     the migrator object to use
    * @param {String}   suggestedId  the id of the profile to migrate, if pre-specified, or null
    * @returns                       the profile to migrate, or null if migrating
@@ -122,18 +167,18 @@ const AutoMigrate = {
       throw new Error("Don't know how to pick a profile when more than 1 profile is present.");
     }
     return profiles ? profiles[0] : null;
   },
 
   getUndoRange() {
     let start, finish;
     try {
-      start = parseInt(Services.prefs.getCharPref(kAutoMigrateStartedPref), 10);
-      finish = parseInt(Services.prefs.getCharPref(kAutoMigrateFinishedPref), 10);
+      start = parseInt(Preferences.get(kAutoMigrateStartedPref, "0"), 10);
+      finish = parseInt(Preferences.get(kAutoMigrateFinishedPref, "0"), 10);
     } catch (ex) {
       Cu.reportError(ex);
     }
     if (!finish || !start) {
       return null;
     }
     return [new Date(start), new Date(finish)];
   },
@@ -182,14 +227,32 @@ const AutoMigrate = {
     histogram.add(20);
 
     try {
       Services.logins.removeAllLogins();
     } catch (ex) {
       // ignore failure.
     }
     histogram.add(25);
-    Services.prefs.clearUserPref("browser.migrate.automigrate-started");
-    Services.prefs.clearUserPref("browser.migrate.automigrate-finished");
+    this.removeUndoOption();
     histogram.add(30);
   }),
+
+  removeUndoOption() {
+    // Remove observers, and ensure that exceptions doing so don't break
+    // removing the pref.
+    try {
+      Services.obs.removeObserver(this, kPasswordManagerTopic);
+    } catch (ex) {}
+    try {
+      PlacesUtils.removeLazyBookmarkObserver(this);
+    } catch (ex) {}
+    Services.prefs.clearUserPref(kAutoMigrateStartedPref);
+    Services.prefs.clearUserPref(kAutoMigrateFinishedPref);
+    Services.prefs.clearUserPref(kAutoMigrateBrowserPref);
+  },
+
+  QueryInterface: XPCOMUtils.generateQI(
+    [Ci.nsIObserver, Ci.nsINavBookmarkObserver, Ci.nsISupportsWeakReference]
+  ),
 };
 
+AutoMigrate.init();
--- a/browser/components/migration/MigrationUtils.jsm
+++ b/browser/components/migration/MigrationUtils.jsm
@@ -447,16 +447,46 @@ this.MigrationUtils = Object.freeze({
     aKey = OVERRIDES[aKey] || aKey;
 
     if (aReplacements === undefined)
       return getMigrationBundle().GetStringFromName(aKey);
     return getMigrationBundle().formatStringFromName(
       aKey, aReplacements, aReplacements.length);
   },
 
+  _getLocalePropertyForBrowser(browserId) {
+    switch (browserId) {
+      case "edge":
+        return "sourceNameEdge";
+      case "ie":
+        return "sourceNameIE";
+      case "safari":
+        return "sourceNameSafari";
+      case "canary":
+        return "sourceNameCanary";
+      case "chrome":
+        return "sourceNameChrome";
+      case "chromium":
+        return "sourceNameChromium";
+      case "firefox":
+        return "sourceNameFirefox";
+      case "360se":
+        return "sourceName360se";
+    }
+    return null;
+  },
+
+  getBrowserName(browserId) {
+    let prop = this._getLocalePropertyForBrowser(browserId);
+    if (prop) {
+      return this.getLocalizedString(prop);
+    }
+    return null;
+  },
+
   /**
    * Helper for creating a folder for imported bookmarks from a particular
    * migration source.  The folder is created at the end of the given folder.
    *
    * @param sourceNameStr
    *        the source name (first letter capitalized).  This is used
    *        for reading the localized source name from the migration
    *        bundle (e.g. if aSourceNameStr is Mosaic, this will try to read
@@ -711,18 +741,17 @@ this.MigrationUtils = Object.freeze({
         this.finishMigration();
         return;
       }
     }
 
     let isRefresh = migrator && skipSourcePage &&
                     migratorKey == AppConstants.MOZ_APP_NAME;
 
-    if (!isRefresh &&
-        Services.prefs.getBoolPref("browser.migration.automigrate")) {
+    if (!isRefresh && AutoMigrate.enabled) {
       try {
         AutoMigrate.migrate(aProfileStartup, aMigratorKey, aProfileToMigrate);
         return;
       } catch (ex) {
         // If automigration failed, continue and show the dialog.
         Cu.reportError(ex);
       }
     }
--- a/browser/components/migration/content/migration.js
+++ b/browser/components/migration/content/migration.js
@@ -309,48 +309,24 @@ var MigrationWizard = {
     document.getElementById("homePageImportDesc").setAttribute("value", pageDesc);
 
     this._wiz._adjustWizardHeader();
 
     var singleStart = document.getElementById("homePageSingleStart");
     singleStart.setAttribute("label", mainStr);
     singleStart.setAttribute("value", "DEFAULT");
 
-    var source = null;
-    switch (this._source) {
-      case "ie":
-        source = "sourceNameIE";
-        break;
-      case "safari":
-        source = "sourceNameSafari";
-        break;
-      case "canary":
-        source = "sourceNameCanary";
-        break;
-      case "chrome":
-        source = "sourceNameChrome";
-        break;
-      case "chromium":
-        source = "sourceNameChromium";
-        break;
-      case "firefox":
-        source = "sourceNameFirefox";
-        break;
-      case "360se":
-        source = "sourceName360se";
-        break;
-    }
+    var appName = MigrationUtils.getBrowserName(this._source);
 
     // semi-wallpaper for crash when multiple profiles exist, since we haven't initialized mSourceProfile in places
     this._migrator.getMigrateData(this._selectedProfile, this._autoMigrate);
 
     var oldHomePageURL = this._migrator.sourceHomePageURL;
 
-    if (oldHomePageURL && source) {
-      var appName = MigrationUtils.getLocalizedString(source);
+    if (oldHomePageURL && appName) {
       var oldHomePageLabel =
         brandBundle.getFormattedString("homePageImport", [appName]);
       var oldHomePage = document.getElementById("oldHomePage");
       oldHomePage.setAttribute("label", oldHomePageLabel);
       oldHomePage.setAttribute("value", oldHomePageURL);
       oldHomePage.removeAttribute("hidden");
     }
     else {
--- a/browser/components/migration/tests/unit/test_automigration.js
+++ b/browser/components/migration/tests/unit/test_automigration.js
@@ -1,10 +1,11 @@
 Cu.import("resource:///modules/MigrationUtils.jsm");
 Cu.import("resource://gre/modules/PlacesUtils.jsm");
+Cu.import("resource://gre/modules/Preferences.jsm");
 Cu.import("resource://testing-common/TestUtils.jsm");
 Cu.import("resource://testing-common/PlacesTestUtils.jsm");
 let AutoMigrateBackstage = Cu.import("resource:///modules/AutoMigrate.jsm");
 
 let gShimmedMigratorKeyPicker = null;
 let gShimmedMigrator = null;
 
 const kUsecPerMin = 60 * 1000000;
@@ -155,19 +156,19 @@ add_task(function* checkUndoPrecondition
                      "getMigrateData called with 'null' as a profile");
 
   let {BOOKMARKS, HISTORY, PASSWORDS} = Ci.nsIBrowserProfileMigrator;
   let expectedTypes = BOOKMARKS | HISTORY | PASSWORDS;
   Assert.deepEqual(gShimmedMigrator._migrateArgs, [expectedTypes, "startup", null],
                    "migrate called with 'null' as a profile");
 
   yield migrationFinishedPromise;
-  Assert.ok(Services.prefs.getPrefType("browser.migrate.automigrate-started"),
+  Assert.ok(Services.prefs.getPrefType("browser.migrate.automigrate.started"),
             "Should have set start time pref");
-  Assert.ok(Services.prefs.getPrefType("browser.migrate.automigrate-finished"),
+  Assert.ok(Services.prefs.getPrefType("browser.migrate.automigrate.finished"),
             "Should have set finish time pref");
   Assert.ok((yield AutoMigrate.canUndo()), "Should be able to undo migration");
 
   let [beginRange, endRange] = AutoMigrate.getUndoRange();
   let stringRange = `beginRange: ${beginRange}; endRange: ${endRange}`;
   Assert.ok(beginRange <= endRange,
             "Migration should have started before or when it ended " + stringRange);
 
@@ -175,17 +176,16 @@ add_task(function* checkUndoPrecondition
   Assert.ok(true, "Should be able to finish an undo cycle.");
 });
 
 /**
  * Fake a migration and then try to undo it to verify all data gets removed.
  */
 add_task(function* checkUndoRemoval() {
   let startTime = "" + Date.now();
-  Services.prefs.setCharPref("browser.migrate.automigrate-started", startTime);
 
   // Insert a login and check that that worked.
   let login = Cc["@mozilla.org/login-manager/loginInfo;1"].createInstance(Ci.nsILoginInfo);
   login.init("www.mozilla.org", "http://www.mozilla.org", null, "user", "pass", "userEl", "passEl");
   Services.logins.addLogin(login);
   let storedLogins = Services.logins.findLogins({}, "www.mozilla.org",
                                                 "http://www.mozilla.org", null);
   Assert.equal(storedLogins.length, 1, "Should have 1 login");
@@ -217,17 +217,18 @@ add_task(function* checkUndoRemoval() {
   let visits = PlacesUtils.history.executeQuery(query, opts);
   visits.root.containerOpen = true;
   Assert.equal(visits.root.childCount, 2, "Should have 2 visits");
   // Clean up:
   visits.root.containerOpen = false;
 
   // Now set finished pref:
   let endTime = "" + Date.now();
-  Services.prefs.setCharPref("browser.migrate.automigrate-finished", endTime);
+  Preferences.set("browser.migrate.automigrate.started", startTime);
+  Preferences.set("browser.migrate.automigrate.finished", endTime);
 
   // Verify that we can undo, then undo:
   Assert.ok(yield AutoMigrate.canUndo(), "Should be possible to undo migration");
   yield AutoMigrate.undo();
 
   // Check that the undo removed the history visits:
   visits = PlacesUtils.history.executeQuery(query, opts);
   visits.root.containerOpen = true;
@@ -239,13 +240,49 @@ add_task(function* checkUndoRemoval() {
   Assert.ok(!bookmark, "Should have no bookmarks after undo");
 
   // Check that the undo removed the passwords:
   storedLogins = Services.logins.findLogins({}, "www.mozilla.org",
                                             "http://www.mozilla.org", null);
   Assert.equal(storedLogins.length, 0, "Should have no logins");
 
   // Finally check prefs got cleared:
-  Assert.ok(!Services.prefs.getPrefType("browser.migrate.automigrate-started"),
+  Assert.ok(!Services.prefs.getPrefType("browser.migrate.automigrate.started"),
             "Should no longer have pref for migration start time.");
-  Assert.ok(!Services.prefs.getPrefType("browser.migrate.automigrate-finished"),
+  Assert.ok(!Services.prefs.getPrefType("browser.migrate.automigrate.finished"),
             "Should no longer have pref for migration finish time.");
 });
+
+add_task(function* checkUndoDisablingByBookmarksAndPasswords() {
+  let startTime = "" + Date.now();
+  Services.prefs.setCharPref("browser.migrate.automigrate.started", startTime);
+  // Now set finished pref:
+  let endTime = "" + (Date.now() + 1000);
+  Services.prefs.setCharPref("browser.migrate.automigrate.finished", endTime);
+  AutoMigrate.maybeInitUndoObserver();
+
+  ok((yield AutoMigrate.canUndo()), "Should be able to undo.");
+
+  // Insert a login and check that that disabled undo.
+  let login = Cc["@mozilla.org/login-manager/loginInfo;1"].createInstance(Ci.nsILoginInfo);
+  login.init("www.mozilla.org", "http://www.mozilla.org", null, "user", "pass", "userEl", "passEl");
+  Services.logins.addLogin(login);
+
+  ok(!(yield AutoMigrate.canUndo()), "Should no longer be able to undo.");
+  Services.prefs.setCharPref("browser.migrate.automigrate.started", startTime);
+  Services.prefs.setCharPref("browser.migrate.automigrate.finished", endTime);
+  ok((yield AutoMigrate.canUndo()), "Should be able to undo.");
+  AutoMigrate.maybeInitUndoObserver();
+
+  // Insert a bookmark and check that that disabled undo.
+  yield PlacesUtils.bookmarks.insert({
+    parentGuid: PlacesUtils.bookmarks.toolbarGuid,
+    url: "http://www.example.org/",
+    title: "Some example bookmark",
+  });
+  ok(!(yield AutoMigrate.canUndo()), "Should no longer be able to undo.");
+
+  try {
+    Services.logins.removeAllLogins();
+  } catch (ex) {}
+  yield PlacesUtils.bookmarks.eraseEverything();
+});
+
--- a/browser/components/nsBrowserGlue.js
+++ b/browser/components/nsBrowserGlue.js
@@ -793,16 +793,19 @@ BrowserGlue.prototype = {
     Feeds.init();
     ContentPrefServiceParent.init();
 
     LoginManagerParent.init();
     ReaderParent.init();
 
     SelfSupportBackend.init();
 
+    // Ensure we keep track of places/pw-mananager undo by init'ing this early.
+    Cu.import("resource:///modules/AutoMigrate.jsm");
+
     if (!AppConstants.RELEASE_BUILD) {
       let themeName = gBrowserBundle.GetStringFromName("deveditionTheme.name");
       let vendorShortName = gBrandBundle.GetStringFromName("vendorShortName");
 
       LightweightThemeManager.addBuiltInTheme({
         id: "firefox-devedition@mozilla.org",
         name: themeName,
         headerURL: "resource:///chrome/browser/content/browser/defaultthemes/devedition.header.png",