Bug 1319816 - fix default browser determination on Windows 7 and below in the automigration case, r=jaws a=jcristau a=gchang FIREFOX_51_0b4_BUILD1 FIREFOX_51_0b4_RELEASE
authorGijs Kruitbosch <gijskruitbosch@gmail.com>
Wed, 23 Nov 2016 16:06:00 +0000
changeset 358974 1b954c82dd04faf1926804d89c0d130dc6b9ab93
parent 358973 4164d5620eac1957830e904190d837835f62064d
child 358975 0742e181e22f650c7c4f18a72489665b28e90129
push id1324
push usermtabara@mozilla.com
push dateMon, 16 Jan 2017 13:07:44 +0000
treeherdermozilla-release@a01c49833940 [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewersjaws, jcristau, gchang
bugs1319816
milestone51.0
Bug 1319816 - fix default browser determination on Windows 7 and below in the automigration case, r=jaws a=jcristau a=gchang As noted on the bug, because we call getMigratorKeyForDefaultBrowser() multiple times, its value no longer reflects the (deleted) registry key for subsequent calls. While we can fix this for the automigrate case by just passing the default we determined a few lines earlier (and that seems worth doing to avoid busywork), there are 2 small problems with this: 1) if the default browser has no data, `migratorKey` won't be set, and so we'll call the same method again anyway, and the message reported in the error console will be that we can't migrate from Firefox, when the real problem is that we can't migrate from the original default browser. 2) there are other callers besides AutoMigrate. Specifically, migration.js also calls this method. To deal with these, I've fixed getMigratorKeyForDefaultBrowser() to return the same registry-based value for its lifetime if we hit the 'the default is firefox, go look for an earlier default' case. I've verified that either the s/aMigratorKey/migratorKey/ or the change to getMigratorKeyForDefaultBrowser() are sufficient to make this work properly in the automigration case. While I was here, I also updated one of the error messages to be more explicit. MozReview-Commit-ID: GeUNTfScMMB
browser/components/migration/AutoMigrate.jsm
browser/components/migration/MigrationUtils.jsm
--- a/browser/components/migration/AutoMigrate.jsm
+++ b/browser/components/migration/AutoMigrate.jsm
@@ -167,17 +167,17 @@ const AutoMigrate = {
       migratorKey = defaultKey;
     }
     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.");
+      throw new Error("Migrator specified or a default was found, but the migrator object is not available (or has no data).");
     }
     return {migrator, pickedKey: migratorKey};
   },
 
   /**
    * Pick a source profile (from the original browser) to use.
    *
    * @param {Migrator} migrator     the migrator object to use
--- a/browser/components/migration/MigrationUtils.jsm
+++ b/browser/components/migration/MigrationUtils.jsm
@@ -31,16 +31,17 @@ XPCOMUtils.defineLazyModuleGetter(this, 
 XPCOMUtils.defineLazyModuleGetter(this, "TelemetryStopwatch",
                                   "resource://gre/modules/TelemetryStopwatch.jsm");
 XPCOMUtils.defineLazyModuleGetter(this, "WindowsRegistry",
                                   "resource://gre/modules/WindowsRegistry.jsm");
 
 var gMigrators = null;
 var gProfileStartup = null;
 var gMigrationBundle = null;
+var gPreviousDefaultBrowserKey = "";
 
 XPCOMUtils.defineLazyGetter(this, "gAvailableMigratorKeys", function() {
   if (AppConstants.platform == "win") {
     return [
       "firefox", "edge", "ie", "chrome", "chromium", "360se",
       "canary"
     ];
   }
@@ -706,29 +707,39 @@ this.MigrationUtils = Object.freeze({
     catch (ex) {
       Cu.reportError("Could not detect default browser: " + ex);
     }
 
     // "firefox" is the least useful entry here, and might just be because we've set
     // ourselves as the default (on Windows 7 and below). In that case, check if we
     // have a registry key that tells us where to go:
     if (key == "firefox" && AppConstants.isPlatformAndVersionAtMost("win", "6.2")) {
-      const kRegPath = "Software\\Mozilla\\Firefox";
-      let oldDefault = WindowsRegistry.readRegKey(
-          Ci.nsIWindowsRegKey.ROOT_KEY_CURRENT_USER, kRegPath, "OldDefaultBrowserCommand");
-      if (oldDefault) {
-        // Remove the key:
-        WindowsRegistry.removeRegKey(
-          Ci.nsIWindowsRegKey.ROOT_KEY_CURRENT_USER, kRegPath, "OldDefaultBrowserCommand");
-        try {
-          let file = Cc["@mozilla.org/file/local;1"].createInstance(Ci.nsILocalFileWin);
-          file.initWithCommandLine(oldDefault);
-          key = APP_DESC_TO_KEY[file.getVersionInfoField("FileDescription")] || key;
-        } catch (ex) {
-          Cu.reportError("Could not convert old default browser value to description.");
+      // Because we remove the registry key, reading the registry key only works once.
+      // We save the value for subsequent calls to avoid hard-to-trace bugs when multiple
+      // consumers ask for this key.
+      if (gPreviousDefaultBrowserKey) {
+        key = gPreviousDefaultBrowserKey;
+      } else {
+        // We didn't have a saved value, so check the registry.
+        const kRegPath = "Software\\Mozilla\\Firefox";
+        let oldDefault = WindowsRegistry.readRegKey(
+            Ci.nsIWindowsRegKey.ROOT_KEY_CURRENT_USER, kRegPath, "OldDefaultBrowserCommand");
+        if (oldDefault) {
+          // Remove the key:
+          WindowsRegistry.removeRegKey(
+            Ci.nsIWindowsRegKey.ROOT_KEY_CURRENT_USER, kRegPath, "OldDefaultBrowserCommand");
+          try {
+            let file = Cc["@mozilla.org/file/local;1"].createInstance(Ci.nsILocalFileWin);
+            file.initWithCommandLine(oldDefault);
+            key = APP_DESC_TO_KEY[file.getVersionInfoField("FileDescription")] || key;
+            // Save the value for future callers.
+            gPreviousDefaultBrowserKey = key;
+          } catch (ex) {
+            Cu.reportError("Could not convert old default browser value to description.");
+          }
         }
       }
     }
     return key;
   },
 
   // Whether or not we're in the process of startup migration
   get isStartupMigration() {
@@ -890,17 +901,17 @@ this.MigrationUtils = Object.freeze({
       }
     }
 
     let isRefresh = migrator && skipSourcePage &&
                     migratorKey == AppConstants.MOZ_APP_NAME;
 
     if (!isRefresh && AutoMigrate.enabled) {
       try {
-        AutoMigrate.migrate(aProfileStartup, aMigratorKey, aProfileToMigrate);
+        AutoMigrate.migrate(aProfileStartup, migratorKey, aProfileToMigrate);
         return;
       } catch (ex) {
         // If automigration failed, continue and show the dialog.
         Cu.reportError(ex);
       }
     }
 
     let migrationEntryPoint = this.MIGRATION_ENTRYPOINT_FIRSTRUN;