Get rid of KeepAlive. Quit shell when all urls run directly by Context are closed.

mojo/shell/dynamic_application_loader.cc

 // Copyright 2014 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #include "mojo/shell/dynamic_application_loader.h"
 
 #include "base/bind.h"
 #include "base/command_line.h"
 #include "base/file_util.h"
 #include "base/files/file_path.h"
 #include "base/memory/scoped_ptr.h"
 #include "base/message_loop/message_loop.h"
 #include "mojo/common/common_type_converters.h"
 #include "mojo/common/data_pipe_utils.h"
 #include "mojo/services/public/interfaces/network/url_loader.mojom.h"
 #include "mojo/shell/context.h"
-#include "mojo/shell/keep_alive.h"
 #include "mojo/shell/switches.h"
 #include "net/base/filename_util.h"
 
 namespace mojo {
 namespace shell {
 
-namespace {
-
-void RunLibraryComplete(DynamicServiceRunner* runner,
-                        const base::FilePath& temp_file) {
-  delete runner;
-  if (!temp_file.empty())
-    base::DeleteFile(temp_file, false);
-}
-
-}  // namespace
-
 DynamicApplicationLoader::DynamicApplicationLoader(
     Context* context,
     scoped_ptr<DynamicServiceRunnerFactory> runner_factory)
     : context_(context),
       runner_factory_(runner_factory.Pass()),
       weak_ptr_factory_(this) {
 }
 
 DynamicApplicationLoader::~DynamicApplicationLoader() {
 }
@@ skipping 91 matching lines @@
                                 file,
                                 callbacks,
                                 kDeleteFileAfter));
 }
 
 void DynamicApplicationLoader::RunLibrary(
     const base::FilePath& path,
     scoped_refptr<LoadCallbacks> callbacks,
     bool delete_file_after,
     bool path_exists) {
-  // TODO(aa): We need to create a runner, even if we're not going to use it,
-  // because it getting destroyed is what causes shell to shut down. If we don't
-  // create this, in the case where shell never successfully creates even one
-  // app, then shell will never shut down, because no runners are ever
-  // destroyed.
-  scoped_ptr<DynamicServiceRunner> runner(runner_factory_->Create(context_));
+
+  ScopedMessagePipeHandle shell_handle = callbacks->RegisterApplication();
+  if (!shell_handle.is_valid())
+    return;
+
   if (!path_exists) {
     DVLOG(1) << "Library not started because library path '"
              << path.value() << "' does not exist.";
     return;
   }
 
-  ScopedMessagePipeHandle shell_handle = callbacks->RegisterApplication();
-  if (!shell_handle.is_valid())
-    return;
+  scoped_ptr<DynamicServiceRunner> runner =
+      runner_factory_->Create(context_).Pass();
+  runner->Start(path,
+                shell_handle.Pass(),
+                base::Bind(&DynamicApplicationLoader::RunLibraryComplete,
+                           weak_ptr_factory_.GetWeakPtr(),
+                           base::Unretained(runner.get()),

[tim (not reviewing), 2014/08/23 00:14:27]

Hm. So if I have this right, a DynamicServiceRunner could be destroyed either a)
by OnRunLibraryComplete or b) by ~DynamicApplicationLoader. If we use
ApplicationManager::TerminateShellConnections when shutting down though,
shouldn't it be possible to say that the Runners are *always* destroyed by
OnRunLibraryComplete after the library exits (because Terminate "forces" the
libraries to exit the standard way)?

In that case, maybe you could you use base::Passed(runner) here so that
OnRunLibraryComplete accepts a scoped_ptr<DynamicServiceRunner> and avoid the
need for |runners_| altogether?

[DaveMoore, 2014/08/24 22:31:11]

We could if we made TerminateShellConnections block on the Joins(). I thought it
was cleaner to have the DAL own the DSR reference and be the only thing that
deleted it (through the scoped_ptr).

On 2014/08/23 00:14:27, timsteele wrote:

[tim (not reviewing), 2014/08/25 16:56:47]

Your patch makes it so we won't Quit the shell MessageLoop until no apps remain,
so that's an implicit wait right?  I guess the issue is the other apps that
you're not tracking with app_urls in the context?

On 2014/08/24 22:31:11, DaveMoore wrote:

+                           delete_file_after ? path : base::FilePath()));
+  runners_.push_back(runner.release());
+}
 
-  DynamicServiceRunner* runner_raw = runner.release();
-  runner_raw->Start(path,
-                    shell_handle.Pass(),
-                    base::Bind(&RunLibraryComplete,
-                               base::Unretained(runner_raw),
-                               delete_file_after ? path : base::FilePath()));
+void DynamicApplicationLoader::RunLibraryComplete(
+    DynamicServiceRunner* runner,
+    const base::FilePath& temp_file) {
+  for (ScopedVector<DynamicServiceRunner>::iterator it = runners_.begin();
+       it != runners_.end(); ++it) {
+    if (*it == runner) {
+      runners_.erase(it);
+      if (!temp_file.empty())
+        base::DeleteFile(temp_file, false);
+      return;
+    }
+  }
 }
 
 void DynamicApplicationLoader::OnServiceError(ApplicationManager* manager,
                                               const GURL& url) {
   // TODO(darin): What should we do about service errors? This implies that
   // the app closed its handle to the service manager. Maybe we don't care?
 }
 
 }  // namespace shell
 }  // namespace mojo