Bug 1319816 - fix default browser determination on Windows 7 and below in the automigration case, r=jaws
authorGijs Kruitbosch <gijskruitbosch@gmail.com>
Wed, 23 Nov 2016 16:06:00 +0000
changeset 324071 4153ce5276d5f44c77a928836453343d89a78b5d
parent 324070 052fdfd28e7a4a710c12c027fd43a55eb9f6b181
child 324072 c72334e5c900c88684a083dc04e725873e428a67
push id24
push usermaklebus@msu.edu
push dateTue, 20 Dec 2016 03:11:33 +0000
reviewersjaws
bugs1319816
milestone53.0a1
Bug 1319816 - fix default browser determination on Windows 7 and below in the automigration case, r=jaws 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"
     ];
   }
@@ -714,29 +715,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() {
@@ -900,17 +911,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;