Setting chrome as the default browser is now fixed on Windows 10

chrome/browser/shell_integration.cc

Index: chrome/browser/shell_integration.cc
diff --git a/chrome/browser/shell_integration.cc b/chrome/browser/shell_integration.cc
index 2ac4b5896e9b7a6900d43331a66bd853698a581f..5460a53fa8fa3806904ac4db47e3f8ab1b1c0694 100644
--- a/chrome/browser/shell_integration.cc
+++ b/chrome/browser/shell_integration.cc
@@ -11,6 +11,7 @@
 #include "base/strings/string_util.h"
 #include "base/strings/utf_string_conversions.h"
 #include "base/threading/thread_restrictions.h"
+#include "base/timer/timer.h"
 #include "chrome/browser/policy/policy_path_parser.h"
 #include "chrome/common/chrome_paths.h"
 #include "chrome/common/chrome_switches.h"
@@ -34,7 +35,11 @@ ShellIntegration::DefaultWebClientSetPermission
     ShellIntegration::CanSetAsDefaultProtocolClient() {
   // Allowed as long as the browser can become the operating system default
   // browser.
-  return CanSetAsDefaultBrowser();
+  DefaultWebClientSetPermission permission = CanSetAsDefaultBrowser();
+
+  // Set as default asynchronous is only supported for default web browser.
+  return (permission == SET_DEFAULT_ASYNCHRONOUS) ? SET_DEFAULT_INTERACTIVE
+                                                  : permission;
 }
 
 static const struct ShellIntegration::AppModeInfo* gAppModeInfo = NULL;
@@ -124,6 +129,13 @@ bool ShellIntegration::SetAsDefaultBrowserInteractive() {
   return false;
 }
 
+#if !defined(OS_WIN)
+// static
+bool ShellIntegration::IsSetAsDefaultAsynchronous() {
+  return false;
+}
+#endif  // !defined(OS_WIN)
+
 // static
 bool ShellIntegration::SetAsDefaultProtocolClientInteractive(
     const std::string& protocol) {
@@ -152,29 +164,34 @@ bool ShellIntegration::DefaultWebClientObserver::
 
 ShellIntegration::DefaultWebClientWorker::DefaultWebClientWorker(
     DefaultWebClientObserver* observer)
-    : observer_(observer) {
-}
+    : observer_(observer) {}
 
 void ShellIntegration::DefaultWebClientWorker::StartCheckIsDefault() {
   if (observer_) {
     observer_->SetDefaultWebClientUIState(STATE_PROCESSING);
     BrowserThread::PostTask(
         BrowserThread::FILE, FROM_HERE,
-        base::Bind(
-            &DefaultWebClientWorker::ExecuteCheckIsDefault, this));
+        base::Bind(&DefaultWebClientWorker::CheckIsDefault, this));
   }
 }
 
 void ShellIntegration::DefaultWebClientWorker::StartSetAsDefault() {
+  // Cancel the already running process if another start is requested.
+  if (set_as_default_in_progress_)

[grt (UTC plus 2), 2015/09/30 17:22:32]

in looking at how set_as_default_in_progress_ is used, it seems to be used for
two distinct things: 1) ignore successive calls to
OnSetAsDefaultAttemptComplete() after the initial one, and 2) ensure that
FinalizeSetAsDefault is called when there's an outstanding call to
InitializeSetAsDefault. for this latter case ("did something happen in
InitializeSetAsDefault that needs to be cleaned up"), i think a few small
changes would improve the interaction between DefaultWebClientWorker and
DefaultBrowserWorker since, for these cases, the followon work done by
OnSetAsDefaultAttemptComplete to check for defaultness isn't appropriate.

1) InitializeSetAsDefault() should return a bool indicating "i did work that
needs a subsequent call to FinalizeSetAsDefault()".
DefaultBrowserWorker::InitializeSetAsDefault would return false for all of its
early-exit codepaths.

2) a new bool set_as_default_initialized_ should be set on line 189 based on the
return value of InitializeSetAsDefault().

3) here, ObserverDestroyed(), and ~DefaultBrowserWorker() should be changed to
"if (set_as_default_initialized_) { set_as_default_initialized_ = false;
FinalizeSetAsDefault(false); }"

4) OnSetAsDefaultAttemptComplete should do similarly.

The idea here is to ensure that FinalizeSetAsDefault is called exactly once for
every successful call to InitializeSetAsDefault. Feel free to push back if I'm
missing something, or you think this is going in the wrong direction.

Is there a need to clear set_as_default_in_progress_ in ObserverDestroyed()?

[Patrick Monette, 2015/10/01 21:12:50]

Done.

+    OnSetAsDefaultAttemptComplete(false);
+
+  set_as_default_in_progress_ = true;
   bool interactive_permitted = false;
   if (observer_) {
     observer_->SetDefaultWebClientUIState(STATE_PROCESSING);
     interactive_permitted = observer_->IsInteractiveSetDefaultPermitted();
+
+    InitializeSetAsDefault();
   }
-  BrowserThread::PostTask(
-      BrowserThread::FILE, FROM_HERE,
-      base::Bind(&DefaultWebClientWorker::ExecuteSetAsDefault, this,
-                 interactive_permitted));
+
+  BrowserThread::PostTask(BrowserThread::FILE, FROM_HERE,
+                          base::Bind(&DefaultWebClientWorker::SetAsDefault,
+                                     this, interactive_permitted));
 }
 
 void ShellIntegration::DefaultWebClientWorker::ObserverDestroyed() {
@@ -182,21 +199,20 @@ void ShellIntegration::DefaultWebClientWorker::ObserverDestroyed() {
   // our worker thread returns after the view is dead.
   DCHECK_CURRENTLY_ON(BrowserThread::UI);
   observer_ = NULL;
+  // If an attempt to set the default browser is already in progress, its result
+  // won't be posted to any observers. Manually invoke
+  // OnSetAsDefaultAttemptComplete() to ensure we free the default browser

[grt (UTC plus 2), 2015/09/30 17:22:32]

nit: "free" -> "clear"?

[Patrick Monette, 2015/10/01 21:12:51]

Done.

+  // callback and timer.
+  if (set_as_default_in_progress_)
+    OnSetAsDefaultAttemptComplete(false);
 }
 
 ///////////////////////////////////////////////////////////////////////////////
 // DefaultWebClientWorker, private:
 
-void ShellIntegration::DefaultWebClientWorker::ExecuteCheckIsDefault() {
-  DCHECK_CURRENTLY_ON(BrowserThread::FILE);
-  DefaultWebClientState state = CheckIsDefault();
-  BrowserThread::PostTask(
-      BrowserThread::UI, FROM_HERE,
-      base::Bind(
-          &DefaultWebClientWorker::CompleteCheckIsDefault, this, state));
-}
+ShellIntegration::DefaultWebClientWorker::~DefaultWebClientWorker() {}
 
-void ShellIntegration::DefaultWebClientWorker::CompleteCheckIsDefault(
+void ShellIntegration::DefaultWebClientWorker::OnCheckIsDefaultComplete(
     DefaultWebClientState state) {
   DCHECK_CURRENTLY_ON(BrowserThread::UI);
   UpdateUI(state);
@@ -208,26 +224,36 @@ void ShellIntegration::DefaultWebClientWorker::CompleteCheckIsDefault(
   }
 }
 
-void ShellIntegration::DefaultWebClientWorker::ExecuteSetAsDefault(
-    bool interactive_permitted) {
-  DCHECK_CURRENTLY_ON(BrowserThread::FILE);
-
-  bool result = SetAsDefault(interactive_permitted);
-  BrowserThread::PostTask(
-      BrowserThread::UI, FROM_HERE,
-      base::Bind(&DefaultWebClientWorker::CompleteSetAsDefault, this, result));
-}
-
-void ShellIntegration::DefaultWebClientWorker::CompleteSetAsDefault(
+void ShellIntegration::DefaultWebClientWorker::OnSetAsDefaultAttemptComplete(
     bool succeeded) {
   DCHECK_CURRENTLY_ON(BrowserThread::UI);
-  // First tell the observer what the SetAsDefault call has returned.
-  if (observer_)
-    observer_->OnSetAsDefaultConcluded(succeeded);
-  // Set as default completed, check again to make sure it stuck...
-  StartCheckIsDefault();
+  // Hold on to a reference because if this was called via the default browser
+  // callback in StartupBrowserCreator, clearing the callback in
+  // FinalizeSetAsDefault would otherwise remove the last reference and delete
+  // us in the middle of this function.
+  scoped_refptr<DefaultWebClientWorker> scoped_ref(this);
+
+  if (set_as_default_in_progress_) {
+    set_as_default_in_progress_ = false;
+
+    FinalizeSetAsDefault(succeeded);
+    if (observer_)
+      observer_->OnSetAsDefaultConcluded(succeeded);
+
+    // Start the default browser check which will notify the observer as to
+    // whether Chrome is really the default browser. This is needed because
+    // detecting that the process was successful is not 100% sure.
+    // For example, on Windows 10+, the user might have unchecked the "Always
+    // use this app" checkbox which can't be detected.
+    StartCheckIsDefault();
+  }
 }
 
+void ShellIntegration::DefaultWebClientWorker::InitializeSetAsDefault() {}
+
+void ShellIntegration::DefaultWebClientWorker::FinalizeSetAsDefault(
+    bool succeeded) {}
+
 void ShellIntegration::DefaultWebClientWorker::UpdateUI(
     DefaultWebClientState state) {
   if (observer_) {
@@ -256,30 +282,58 @@ ShellIntegration::DefaultBrowserWorker::DefaultBrowserWorker(
     : DefaultWebClientWorker(observer) {
 }
 
+ShellIntegration::DefaultBrowserWorker::~DefaultBrowserWorker() {
+  if (set_as_default_in_progress_)
+    OnSetAsDefaultAttemptComplete(false);

[grt (UTC plus 2), 2015/09/30 17:22:32]

i don't think this could ever happen. the object can't be destroyed while either
async_timer_ is alive or the callback in SBC is set since each of those holds a
reference to the worker. i think this is reasonable:

  // The worker cannot be destroyed while an operation is in progress since all
  // tasks, timers, and callbacks hold a reference to it.
  DCHECK(!set_as_default_in_progress_);

wdyt? am i missing another possible destruction path?

[Patrick Monette, 2015/10/01 21:12:50]

SGTM. Done.

+}
+
 ///////////////////////////////////////////////////////////////////////////////
 // DefaultBrowserWorker, private:
 
-ShellIntegration::DefaultWebClientState
-ShellIntegration::DefaultBrowserWorker::CheckIsDefault() {
-  return ShellIntegration::GetDefaultBrowser();
+void ShellIntegration::DefaultBrowserWorker::CheckIsDefault() {
+  DefaultWebClientState state = GetDefaultBrowser();
+  BrowserThread::PostTask(
+      BrowserThread::UI, FROM_HERE,
+      base::Bind(&DefaultBrowserWorker::OnCheckIsDefaultComplete, this, state));
 }
 
-bool ShellIntegration::DefaultBrowserWorker::SetAsDefault(
+void ShellIntegration::DefaultBrowserWorker::SetAsDefault(
     bool interactive_permitted) {
   bool result = false;
-  switch (ShellIntegration::CanSetAsDefaultBrowser()) {
-    case ShellIntegration::SET_DEFAULT_UNATTENDED:
-      result = ShellIntegration::SetAsDefaultBrowser();
+  switch (CanSetAsDefaultBrowser()) {
+    case SET_DEFAULT_NOT_ALLOWED:
+      NOTREACHED();
+      break;
+    case SET_DEFAULT_UNATTENDED:
+      result = SetAsDefaultBrowser();
       break;
-    case ShellIntegration::SET_DEFAULT_INTERACTIVE:
+    case SET_DEFAULT_INTERACTIVE:
       if (interactive_permitted)
-        result = ShellIntegration::SetAsDefaultBrowserInteractive();
+        result = SetAsDefaultBrowserInteractive();
       break;
-    default:
+    case SET_DEFAULT_ASYNCHRONOUS:
+#if defined(OS_WIN)
+      if (!interactive_permitted)
+        break;
+      if (GetDefaultBrowser() == IS_DEFAULT) {
+        // Don't start the asynchronous operation since it could result in
+        // losing the default browser status.
+        result = true;
+        break;
+      }
+      // This function will cause OnSetAsDefaultAttemptComplete() to be called
+      // asynchronously via a filter established in InitializeSetAsDefault().
+      SetAsDefaultBrowserAsynchronous();
+      return;
+#else
       NOTREACHED();
+      break;
+#endif
   }
-
-  return result;
+  BrowserThread::PostTask(
+      BrowserThread::UI, FROM_HERE,
+      base::Bind(&DefaultBrowserWorker::OnSetAsDefaultAttemptComplete, this,
+                 result));
 }
 
 ///////////////////////////////////////////////////////////////////////////////
@@ -295,28 +349,37 @@ ShellIntegration::DefaultProtocolClientWorker::DefaultProtocolClientWorker(
 ///////////////////////////////////////////////////////////////////////////////
 // DefaultProtocolClientWorker, private:
 
-ShellIntegration::DefaultWebClientState
-ShellIntegration::DefaultProtocolClientWorker::CheckIsDefault() {
-  return ShellIntegration::IsDefaultProtocolClient(protocol_);
+ShellIntegration::DefaultProtocolClientWorker::~DefaultProtocolClientWorker() {}
+
+void ShellIntegration::DefaultProtocolClientWorker::CheckIsDefault() {
+  DefaultWebClientState state = IsDefaultProtocolClient(protocol_);
+  BrowserThread::PostTask(
+      BrowserThread::UI, FROM_HERE,
+      base::Bind(&DefaultProtocolClientWorker::OnCheckIsDefaultComplete, this,
+                 state));
 }
 
-bool ShellIntegration::DefaultProtocolClientWorker::SetAsDefault(
+void ShellIntegration::DefaultProtocolClientWorker::SetAsDefault(
     bool interactive_permitted) {
   bool result = false;
-  switch (ShellIntegration::CanSetAsDefaultProtocolClient()) {
-    case ShellIntegration::SET_DEFAULT_NOT_ALLOWED:
-      result = false;
+  switch (CanSetAsDefaultProtocolClient()) {
+    case SET_DEFAULT_NOT_ALLOWED:
+      // Not allowed, do nothing.
       break;
-    case ShellIntegration::SET_DEFAULT_UNATTENDED:
-      result = ShellIntegration::SetAsDefaultProtocolClient(protocol_);
+    case SET_DEFAULT_UNATTENDED:
+      result = SetAsDefaultProtocolClient(protocol_);
       break;
-    case ShellIntegration::SET_DEFAULT_INTERACTIVE:
+    case SET_DEFAULT_INTERACTIVE:
       if (interactive_permitted) {
-        result = ShellIntegration::SetAsDefaultProtocolClientInteractive(
-            protocol_);
+        result = SetAsDefaultProtocolClientInteractive(protocol_);
       }
       break;
+    case SET_DEFAULT_ASYNCHRONOUS:
+      NOTREACHED();
+      break;
   }
-
-  return result;
+  BrowserThread::PostTask(
+      BrowserThread::UI, FROM_HERE,
+      base::Bind(&DefaultProtocolClientWorker::OnSetAsDefaultAttemptComplete,
+                 this, result));
 }