Make AppLifetimeHelper a member of ApplicationImpl.

This simplifies plumbing, but more so is so that ApplicationImpl can tell it when the application is gone. Otherwise AppLifetimeHelper might access a deleted ApplicationImpl after it's deleted when the app's message loop is being destructed (since an OnConnectionError might cause the last object holding a reference to end up calling ApplicationImpl::Terminate). BUG=484234 Review URL: https://codereview.chromium.org/1158633002 Cr-Commit-Position: refs/heads/master@{#331403}
cbchan · May 26, 2015 · 1a13c5d · 1a13c5d
1 parent e47161e
commit 1a13c5d
Show file tree

Hide file tree

Showing 10 changed files with 32 additions and 15 deletions.
diff --git a/components/view_manager/native_viewport/native_viewport_application_delegate.cc b/components/view_manager/native_viewport/native_viewport_application_delegate.cc
@@ -16,14 +16,15 @@
 namespace native_viewport {
 
 NativeViewportApplicationDelegate::NativeViewportApplicationDelegate()
-    : is_headless_(false) {
+    : is_headless_(false), app_(nullptr) {
 }
 
 NativeViewportApplicationDelegate::~NativeViewportApplicationDelegate() {
 }
 
 void NativeViewportApplicationDelegate::Initialize(
     mojo::ApplicationImpl* application) {
+  app_ = application;
   tracing_.Initialize(application);
 
   base::CommandLine* command_line = base::CommandLine::ForCurrentProcess();
@@ -50,7 +51,7 @@ void NativeViewportApplicationDelegate::Create(
   if (!gpu_state_.get())
     gpu_state_ = new gles2::GpuState;
   new NativeViewportImpl(is_headless_, gpu_state_, request.Pass(),
-                         app_lifetime_helper_.CreateAppRefCount());
+                         app_->app_lifetime_helper()->CreateAppRefCount());
 }
 
 void NativeViewportApplicationDelegate::Create(

diff --git a/components/view_manager/native_viewport/native_viewport_application_delegate.h b/components/view_manager/native_viewport/native_viewport_application_delegate.h
@@ -8,7 +8,6 @@
 #include "base/macros.h"
 #include "components/view_manager/gles2/gpu_impl.h"
 #include "components/view_manager/public/interfaces/native_viewport.mojom.h"
-#include "mojo/application/public/cpp/app_lifetime_helper.h"
 #include "mojo/application/public/cpp/application_delegate.h"
 #include "mojo/application/public/cpp/interface_factory_impl.h"
 #include "mojo/common/tracing_impl.h"
@@ -50,7 +49,7 @@ class NativeViewportApplicationDelegate
   scoped_ptr<ui::PlatformEventSource> event_source_;
   bool is_headless_;
   mojo::TracingImpl tracing_;
-  mojo::AppLifetimeHelper app_lifetime_helper_;
+  mojo::ApplicationImpl* application_;
 
   DISALLOW_COPY_AND_ASSIGN(NativeViewportApplicationDelegate);
 };

diff --git a/components/view_manager/view_manager_app.cc b/components/view_manager/view_manager_app.cc
@@ -148,7 +148,7 @@ void ViewManagerApp::Create(
       is_headless_,
       gpu_state_,
       request.Pass(),
-      app_lifetime_helper_.CreateAppRefCount());
+      app_impl_->app_lifetime_helper()->CreateAppRefCount());
 }
 
 void ViewManagerApp::Create(

diff --git a/components/view_manager/view_manager_app.h b/components/view_manager/view_manager_app.h
@@ -83,7 +83,6 @@ class ViewManagerApp : public mojo::ApplicationDelegate,
   void OnConnectionError() override;
 
   mojo::ApplicationImpl* app_impl_;
-  mojo::AppLifetimeHelper app_lifetime_helper_;
   scoped_ptr<mojo::Binding<mojo::ViewManagerRoot>> view_manager_root_binding_;
   scoped_ptr<ConnectionManager> connection_manager_;
   mojo::TracingImpl tracing_;

diff --git a/mojo/application/public/cpp/app_lifetime_helper.h b/mojo/application/public/cpp/app_lifetime_helper.h
@@ -11,6 +11,7 @@
 
 namespace mojo {
 
+class ApplicationImpl;
 class AppLifetimeHelper;
 
 // A service implementation should keep this object as a member variable to hold
@@ -56,7 +57,7 @@ class AppRefCount {
 // quit with a call to mojo::ApplicationImpl::Terminate().
 class AppLifetimeHelper {
  public:
-  AppLifetimeHelper();
+  explicit AppLifetimeHelper(ApplicationImpl* app);
   ~AppLifetimeHelper();
 
   scoped_ptr<AppRefCount> CreateAppRefCount();
@@ -66,6 +67,10 @@ class AppLifetimeHelper {
   void AddRef();
   void Release();
 
+  friend ApplicationImpl;
+  void ApplicationTerminated();
+
+  ApplicationImpl* app_;
   int ref_count_;
 
   DISALLOW_COPY_AND_ASSIGN(AppLifetimeHelper);

diff --git a/mojo/application/public/cpp/application_impl.h b/mojo/application/public/cpp/application_impl.h
@@ -8,6 +8,7 @@
 #include <vector>
 
 #include "base/callback.h"
+#include "mojo/application/public/cpp/app_lifetime_helper.h"
 #include "mojo/application/public/cpp/application_connection.h"
 #include "mojo/application/public/cpp/application_delegate.h"
 #include "mojo/application/public/cpp/lib/service_registry.h"
@@ -72,6 +73,8 @@ class ApplicationImpl : public Application,
 
   const std::string& url() const { return url_; }
 
+  AppLifetimeHelper* app_lifetime_helper() { return &app_lifetime_helper_; }
+
   // Requests a new connection to an application. Returns a pointer to the
   // connection if the connection is permitted by this application's delegate,
   // or nullptr otherwise. Caller does not take ownership. The pointer remains
@@ -124,6 +127,7 @@ class ApplicationImpl : public Application,
   ShellPtr shell_;
   std::string url_;
   base::Closure termination_closure_;
+  AppLifetimeHelper app_lifetime_helper_;
 
   MOJO_DISALLOW_COPY_AND_ASSIGN(ApplicationImpl);
 };

diff --git a/mojo/application/public/cpp/lib/app_lifetime_helper.cc b/mojo/application/public/cpp/lib/app_lifetime_helper.cc
@@ -63,8 +63,8 @@ scoped_ptr<AppRefCount> AppRefCount::Clone() {
       app_lifetime_helper_, app_task_runner_));
 }
 
-AppLifetimeHelper::AppLifetimeHelper()
-    : ref_count_(0) {
+AppLifetimeHelper::AppLifetimeHelper(ApplicationImpl* app)
+    : app_(app), ref_count_(0) {
 }
 
 AppLifetimeHelper::~AppLifetimeHelper() {
@@ -82,9 +82,15 @@ void AppLifetimeHelper::AddRef() {
 
 void AppLifetimeHelper::Release() {
   if (!--ref_count_) {
-    // Disabled until network_service tests pass again http://crbug.com/484234
-    //ApplicationImpl::Terminate();
+    if (app_) {
+      // Disabled until network_service tests pass again http://crbug.com/484234
+      //ApplicationImpl::Terminate();
+    }
   }
 }
 
+void AppLifetimeHelper::ApplicationTerminated() {
+  app_ = nullptr;
+}
+
 }  // namespace mojo
diff --git a/mojo/application/public/cpp/lib/application_impl.cc b/mojo/application/public/cpp/lib/application_impl.cc
@@ -33,7 +33,8 @@ ApplicationImpl::ApplicationImpl(ApplicationDelegate* delegate,
                                  const base::Closure& termination_closure)
     : delegate_(delegate),
       binding_(this, request.Pass()),
-      termination_closure_(termination_closure) {
+      termination_closure_(termination_closure),
+      app_lifetime_helper_(this) {
 }
 
 void ApplicationImpl::ClearConnections() {
@@ -51,6 +52,7 @@ void ApplicationImpl::ClearConnections() {
 
 ApplicationImpl::~ApplicationImpl() {
   ClearConnections();
+  app_lifetime_helper_.ApplicationTerminated();
 }
 
 ApplicationConnection* ApplicationImpl::ConnectToApplication(

diff --git a/mojo/services/network/network_service_delegate.cc b/mojo/services/network/network_service_delegate.cc
@@ -11,11 +11,12 @@
 #include "base/path_service.h"
 #include "mojo/application/public/cpp/application_connection.h"
 
-NetworkServiceDelegate::NetworkServiceDelegate() {}
+NetworkServiceDelegate::NetworkServiceDelegate() : app_(nullptr) {}
 
 NetworkServiceDelegate::~NetworkServiceDelegate() {}
 
 void NetworkServiceDelegate::Initialize(mojo::ApplicationImpl* app) {
+  app_ = app;
   base::FilePath base_path;
   CHECK(PathService::Get(base::DIR_TEMP, &base_path));
   base_path = base_path.Append(FILE_PATH_LITERAL("network_service"));
@@ -43,6 +44,6 @@ void NetworkServiceDelegate::Create(
       new mojo::NetworkServiceImpl(
           connection,
           context_.get(),
-          app_lifetime_helper_.CreateAppRefCount()),
+          app_->app_lifetime_helper()->CreateAppRefCount()),
       &request);
 }
diff --git a/mojo/services/network/network_service_delegate.h b/mojo/services/network/network_service_delegate.h
@@ -31,8 +31,8 @@ class NetworkServiceDelegate
               mojo::InterfaceRequest<mojo::NetworkService> request) override;
 
  private:
+  mojo::ApplicationImpl* app_;
   scoped_ptr<mojo::NetworkContext> context_;
-  mojo::AppLifetimeHelper app_lifetime_helper_;
 };
 
 #endif  // MOJO_SERVICES_NETWORK_NETWORK_SERVICE_DELEGATE_H_