Only make the Windows COM server an AppServer singleton.

Making all App instances singletons appears to have been
a mistake. Singletons in Chromium are typically leaky
singletons, and thus, destructors for App instance don't
run, breaking expectations and creating confusion.

This CL makes all the App instances normal ref counted
class instances, execept for the Windows COM server, which
remains a singleton because of a design quirk: coclass
instances created by COM need a way to refer the COM
App server class (to talk to the update service, for example).
We could not find a way to pass the App instance to
coclasses, and so, the need for the singleton.

TBR: waffles@chromium.org
Bug: 1064498
Change-Id: I2872aa1821fcf27b74f22236483cfc11ea7c3059
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2252462Reviewed-by: Sorin Jianu <sorin@chromium.org>
Commit-Queue: Sorin Jianu <sorin@chromium.org>
Cr-Commit-Position: refs/heads/master@{#780057}

chrome/updater/app/app.h

@@ -13,19 +13,21 @@
 namespace updater {
 
 // Creates a ref-counted singleton instance of the type T. Use this function
-// to get instances of classes derived from updater::App.
-// TODO(crbug.com/1064498) - provide non-leaky creation for callers where
-// a singleton is not needed.
+// to get instances of classes derived from |updater::App|, only if a
+// singleton design is needed.
 template <typename T>
-scoped_refptr<T> AppInstance() {
+scoped_refptr<T> AppSingletonInstance() {
   static base::NoDestructor<scoped_refptr<T>> instance{
       base::MakeRefCounted<T>()};
   return *instance;
 }
 
 // An App is an abstract class used as a main processing mode for the updater.
-// Instances of classes derived from App must be accessed as singletons by
-// calling the function template AppInstance<T>.
+// Prefer creating non-singleton instances of |App| using |base::MakeRefCounted|
+// then use |updater::AppSingletonInstance| only when a singleton instance is
+// required by the design. Typically, |App| instances are not singletons but
+// there are cases where a singleton is needed, such as the Windows RPC
+// server app instance.
 class App : public base::RefCountedThreadSafe<App> {
  public:
   // Starts the thread pool and task executor, then runs a runloop on the main

chrome/updater/app/app_uninstall.cc

@@ -36,8 +36,8 @@ void AppUninstall::FirstTaskRun() {
       base::BindOnce(&AppUninstall::Shutdown, this));
 }
 
-scoped_refptr<App> AppUninstallInstance() {
-  return AppInstance<AppUninstall>();
+scoped_refptr<App> MakeAppUninstall() {
+  return base::MakeRefCounted<AppUninstall>();
 }
 
 }  // namespace updater

chrome/updater/app/app_uninstall.h

@@ -11,7 +11,7 @@ namespace updater {
 
 class App;
 
-scoped_refptr<App> AppUninstallInstance();
+scoped_refptr<App> MakeAppUninstall();
 
 }  // namespace updater
 

chrome/updater/app/app_wake.cc

@@ -54,8 +54,8 @@ void AppWake::FirstTaskRun() {
           base::BindOnce(&AppWake::Shutdown, this)));
 }
 
-scoped_refptr<App> AppWakeInstance() {
-  return AppInstance<AppWake>();
+scoped_refptr<App> MakeAppWake() {
+  return base::MakeRefCounted<AppWake>();
 }
 
 }  // namespace updater

chrome/updater/app/app_wake.h

@@ -11,7 +11,7 @@ namespace updater {
 
 class App;
 
-scoped_refptr<App> AppWakeInstance();
+scoped_refptr<App> MakeAppWake();
 
 }  // namespace updater
 

chrome/updater/app/server/mac/server.h

@@ -11,7 +11,7 @@ namespace updater {
 
 class App;
 
-scoped_refptr<App> AppServerInstance();
+scoped_refptr<App> MakeAppServer();
 
 }  // namespace updater
 

chrome/updater/app/server/mac/server.mm

@@ -77,8 +77,8 @@ void AppServer::FirstTaskRun() {
   }
 }
 
-scoped_refptr<App> AppServerInstance() {
-  return AppInstance<AppServer>();
+scoped_refptr<App> MakeAppServer() {
+  return base::MakeRefCounted<AppServer>();
 }
 
 }  // namespace updater

chrome/updater/app/server/win/com_classes.cc

@@ -59,7 +59,7 @@ HRESULT UpdaterImpl::UpdateAll(IUpdaterObserver* observer) {
   using IUpdaterObserverPtr = Microsoft::WRL::ComPtr<IUpdaterObserver>;
 
   // Invoke the in-process |update_service| on the main sequence.
-  auto com_server = ComServerApp::Instance();
+  scoped_refptr<ComServerApp> com_server = AppServerSingletonInstance();
   com_server->main_task_runner()->PostTask(
       FROM_HERE,
       base::BindOnce(

chrome/updater/app/server/win/com_classes_legacy.cc

@@ -103,7 +103,7 @@ STDMETHODIMP LegacyOnDemandImpl::initialize() {
 // callbacks to a sequenced task runner. |obj| is bound to this object.
 STDMETHODIMP LegacyOnDemandImpl::checkForUpdate() {
   using LegacyOnDemandImplPtr = Microsoft::WRL::ComPtr<LegacyOnDemandImpl>;
-  auto com_server = ComServerApp::Instance();
+  scoped_refptr<ComServerApp> com_server = AppServerSingletonInstance();
   com_server->main_task_runner()->PostTask(
       FROM_HERE,
       base::BindOnce(

chrome/updater/app/server/win/server.cc

@@ -34,8 +34,8 @@
 namespace updater {
 
 // Returns a leaky singleton of the App instance.
-scoped_refptr<App> AppServerInstance() {
-  return AppInstance<ComServerApp>();
+scoped_refptr<ComServerApp> AppServerSingletonInstance() {
+  return AppSingletonInstance<ComServerApp>();
 }
 
 ComServerApp::ComServerApp()
@@ -133,7 +133,7 @@ void ComServerApp::Stop() {
   UnregisterClassObjects();
   main_task_runner_->PostTask(
       FROM_HERE, base::BindOnce([]() {
-        auto this_server = ComServerApp::Instance();
+        scoped_refptr<ComServerApp> this_server = AppServerSingletonInstance();
         this_server->config_->GetPrefService()->CommitPendingWrite();
         this_server->service_ = nullptr;
         this_server->config_ = nullptr;

chrome/updater/app/server/win/server.h

@@ -21,9 +21,6 @@ namespace updater {
 class Configurator;
 class UpdateService;
 
-// Returns an application object bound to this COM server.
-scoped_refptr<App> AppServerInstance();
-
 // The COM objects involved in this server are free threaded. Incoming COM calls
 // arrive on COM RPC threads. Outgoing COM calls are posted from a blocking
 // sequenced task runner in the thread pool. Calls to the update service occur
@@ -46,11 +43,6 @@ class ComServerApp : public App {
  public:
   ComServerApp();
 
-  // Returns the singleton instance of this ComServerApp.
-  static scoped_refptr<ComServerApp> Instance() {
-    return static_cast<ComServerApp*>(AppServerInstance().get());
-  }
-
   scoped_refptr<base::SequencedTaskRunner> main_task_runner() {
     return main_task_runner_;
   }
@@ -99,6 +91,9 @@ class ComServerApp : public App {
   scoped_refptr<Configurator> config_;
 };
 
+// Returns a singleton application object bound to this COM server.
+scoped_refptr<ComServerApp> AppServerSingletonInstance();
+
 }  // namespace updater
 
 #endif  // CHROME_UPDATER_APP_SERVER_WIN_SERVER_H_

chrome/updater/mac/setup/app_swap.cc

@@ -40,12 +40,12 @@ void AppUninstallCandidate::FirstTaskRun() {
 
 }  // namespace
 
-scoped_refptr<App> AppPromoteCandidateInstance() {
-  return AppInstance<AppPromoteCandidate>();
+scoped_refptr<App> MakeAppPromoteCandidate() {
+  return base::MakeRefCounted<AppPromoteCandidate>();
 }
 
-scoped_refptr<App> AppUninstallCandidateInstance() {
-  return AppInstance<AppUninstallCandidate>();
+scoped_refptr<App> MakeAppUninstallCandidate() {
+  return base::MakeRefCounted<AppUninstallCandidate>();
 }
 
 }  // namespace updater

chrome/updater/mac/setup/app_swap.h

@@ -11,8 +11,8 @@ namespace updater {
 
 class App;
 
-scoped_refptr<App> AppPromoteCandidateInstance();
-scoped_refptr<App> AppUninstallCandidateInstance();
+scoped_refptr<App> MakeAppPromoteCandidate();
+scoped_refptr<App> MakeAppUninstallCandidate();
 
 }  // namespace updater
 

chrome/updater/mac/setup/install_app.cc

@@ -28,8 +28,8 @@ void AppInstall::FirstTaskRun() {
 
 }  // namespace
 
-scoped_refptr<App> AppInstallInstance() {
-  return AppInstance<AppInstall>();
+scoped_refptr<App> MakeAppInstall() {
+  return base::MakeRefCounted<AppInstall>();
 }
 
 }  // namespace updater

chrome/updater/mac/setup/install_app.h

@@ -11,7 +11,7 @@ namespace updater {
 
 class App;
 
-scoped_refptr<App> AppInstallInstance();
+scoped_refptr<App> MakeAppInstall();
 
 }  // namespace updater
 

chrome/updater/test/test_app/test_app.cc

@@ -112,8 +112,8 @@ void TestApp::FirstTaskRun() {
   ParseCommandLine();
 }
 
-scoped_refptr<App> TestAppInstance() {
-  return AppInstance<TestApp>();
+scoped_refptr<App> MakeTestApp() {
+  return base::MakeRefCounted<TestApp>();
 }
 
 }  // namespace
@@ -124,7 +124,7 @@ int TestAppMain(int argc, const char** argv) {
   base::CommandLine::Init(argc, argv);
   updater::InitLogging(FILE_PATH_LITERAL("test_app.log"));
 
-  return TestAppInstance()->Run();
+  return MakeTestApp()->Run();
 }
 
 }  // namespace updater

chrome/updater/updater.cc

@@ -90,7 +90,12 @@ int HandleUpdaterCommands(const base::CommandLine* command_line) {
   }
 
   if (command_line->HasSwitch(kServerSwitch)) {
-    return AppServerInstance()->Run();
+#if defined(OS_WIN)
+    // By design, Windows uses a leaky singleton server for its RPC server.
+    return AppServerSingletonInstance()->Run();
+#else
+    return MakeAppServer()->Run();
+#endif
   }
 
 #if defined(OS_WIN)
@@ -99,20 +104,20 @@ int HandleUpdaterCommands(const base::CommandLine* command_line) {
 #endif  // OS_WIN
 
   if (command_line->HasSwitch(kInstallSwitch))
-    return AppInstallInstance()->Run();
+    return MakeAppInstall()->Run();
 
 #if defined(OS_MACOSX)
   if (command_line->HasSwitch(kPromoteCandidateSwitch))
-    return AppPromoteCandidateInstance()->Run();
+    return MakeAppPromoteCandidate()->Run();
   if (command_line->HasSwitch(kUninstallCandidateSwitch))
-    return AppUninstallCandidateInstance()->Run();
+    return MakeAppUninstallCandidate()->Run();
 #endif  // OS_MACOSX
 
   if (command_line->HasSwitch(kUninstallSwitch))
-    return AppUninstallInstance()->Run();
+    return MakeAppUninstall()->Run();
 
   if (command_line->HasSwitch(kWakeSwitch)) {
-    return AppWakeInstance()->Run();
+    return MakeAppWake()->Run();
   }
 
   VLOG(1) << "Unknown command line switch.";

chrome/updater/win/install_app.cc

@@ -766,12 +766,12 @@ void AppInstall::SetupDone(int result) {
       app_id, base::BindOnce(&AppInstall::Shutdown, this));
 }
 
-scoped_refptr<App> AppInstallInstance() {
+scoped_refptr<App> MakeAppInstall() {
   // TODO(sorin) "--install" must be run with "--single-process" until
   // crbug.com/1053729 is resolved.
   DCHECK(
       base::CommandLine::ForCurrentProcess()->HasSwitch(kSingleProcessSwitch));
-  return AppInstance<AppInstall>();
+  return base::MakeRefCounted<AppInstall>();
 }
 
 }  // namespace updater

chrome/updater/win/install_app.h

@@ -13,7 +13,7 @@ namespace updater {
 
 class App;
 
-scoped_refptr<App> AppInstallInstance();
+scoped_refptr<App> MakeAppInstall();
 
 }  // namespace updater