Group ConnectToApplication-related info into a params struct.

mojo/shell/application_instance.cc

 // Copyright 2015 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/application_instance.h"
 
 #include "base/bind.h"
 #include "base/stl_util.h"
 #include "mojo/application/public/interfaces/content_handler.mojom.h"
 #include "mojo/common/common_type_converters.h"
@@ skipping 15 matching lines @@
     if (it->first == "*")
       canonicalized[it->first] = it->second;
     else
       canonicalized[GURL(it->first).spec()] = it->second;
   }
   return canonicalized;
 }
 
 }  // namespace
 
-ApplicationInstance::QueuedClientRequest::QueuedClientRequest()
-    : originator(nullptr) {}
-
-ApplicationInstance::QueuedClientRequest::~QueuedClientRequest() {
-}
-
 ApplicationInstance::ApplicationInstance(
     ApplicationPtr application,
     ApplicationManager* manager,
     const Identity& originator_identity,
     const Identity& identity,
     const CapabilityFilter& filter,
     uint32_t requesting_content_handler_id,
     const base::Closure& on_application_end)
     : manager_(manager),
       originator_identity_(originator_identity),
       identity_(identity),
       filter_(CanonicalizeFilter(filter)),
       allow_any_application_(filter.size() == 1 && filter.count("*") == 1),
       requesting_content_handler_id_(requesting_content_handler_id),
       on_application_end_(on_application_end),
       application_(application.Pass()),
       binding_(this),
       queue_requests_(false) {
   binding_.set_connection_error_handler([this]() { OnConnectionError(); });
 }
 
 ApplicationInstance::~ApplicationInstance() {
   STLDeleteElements(&queued_client_requests_);

[sky, 2015/09/02 22:38:26]

Do you need to also run the callbacks in queued_client_requests_?

[yzshen1, 2015/09/03 00:06:38]

Good catch. :)

 }
 
 void ApplicationInstance::InitializeApplication() {
   ShellPtr shell;
   binding_.Bind(GetProxy(&shell));
   application_->Initialize(shell.Pass(), identity_.url.spec());
 }
 
 void ApplicationInstance::ConnectToClient(
-    ApplicationInstance* originator,
-    const GURL& requested_url,
-    const GURL& requestor_url,
-    InterfaceRequest<ServiceProvider> services,
-    ServiceProviderPtr exposed_services,
-    const CapabilityFilter& filter,
-    const ConnectToApplicationCallback& callback) {
-  callback.Run(requesting_content_handler_id_);
+    scoped_ptr<ConnectToApplicationParams> params) {
   if (queue_requests_) {
-    QueuedClientRequest* queued_request = new QueuedClientRequest();
-    queued_request->originator = originator;
-    queued_request->requested_url = requested_url;
-    queued_request->requestor_url = requestor_url;
-    queued_request->services = services.Pass();
-    queued_request->exposed_services = exposed_services.Pass();
-    queued_request->filter = filter;
-    queued_client_requests_.push_back(queued_request);
+    queued_client_requests_.push_back(params.release());
     return;
   }
 
-  CallAcceptConnection(originator, requestor_url, services.Pass(),
-                       exposed_services.Pass(), requested_url);
-}
-
-AllowedInterfaces ApplicationInstance::GetAllowedInterfaces(
-    const Identity& identity) const {
-  // Start by looking for interfaces specific to the supplied identity.
-  auto it = filter_.find(identity.url.spec());
-  if (it != filter_.end())
-    return it->second;
-
-  // Fall back to looking for a wildcard rule.
-  it = filter_.find("*");
-  if (filter_.size() == 1 && it != filter_.end())
-    return it->second;
-
-  // Finally, nothing is allowed.
-  return AllowedInterfaces();
+  CallAcceptConnection(params.Pass());
 }
 
 // Shell implementation:
 void ApplicationInstance::ConnectToApplication(
     URLRequestPtr app_request,
     InterfaceRequest<ServiceProvider> services,
     ServiceProviderPtr exposed_services,
     CapabilityFilterPtr filter,
     const ConnectToApplicationCallback& callback) {
   std::string url_string = app_request->url.To<std::string>();
@@ skipping 18 matching lines @@
 }
 
 void ApplicationInstance::QuitApplication() {
   queue_requests_ = true;
   application_->OnQuitRequested(
       base::Bind(&ApplicationInstance::OnQuitRequestedResult,
                  base::Unretained(this)));
 }
 
 void ApplicationInstance::CallAcceptConnection(
-    ApplicationInstance* originator,
-    const GURL& requestor_url,
-    InterfaceRequest<ServiceProvider> services,
-    ServiceProviderPtr exposed_services,
-    const GURL& requested_url) {
+    scoped_ptr<ConnectToApplicationParams> params) {
+  params->connect_callback().Run(requesting_content_handler_id_);
   AllowedInterfaces interfaces;
   interfaces.insert("*");
-  if (originator)
-    interfaces = originator->GetAllowedInterfaces(identity_);
-  application_->AcceptConnection(requestor_url.spec(),
-                                 services.Pass(),
-                                 exposed_services.Pass(),
-                                 Array<String>::From(interfaces).Pass(),
-                                 requested_url.spec());
+  if (!params->originator_identity().is_null())
+    interfaces = GetAllowedInterfaces(params->originator_filter(), identity_);
+
+  application_->AcceptConnection(
+      params->originator_identity().url.spec(), params->TakeServices(),
+      params->TakeExposedServices(), Array<String>::From(interfaces).Pass(),
+      params->app_url().spec());
 }
 
 void ApplicationInstance::OnConnectionError() {
-  std::vector<QueuedClientRequest*> queued_client_requests;
+  std::vector<ConnectToApplicationParams*> queued_client_requests;
   queued_client_requests_.swap(queued_client_requests);
   auto manager = manager_;
   manager_->OnApplicationInstanceError(this);
   //|this| is deleted.
 
   // If any queued requests came to shell during time it was shutting down,
   // start them now.
   for (auto request : queued_client_requests) {
-    mojo::URLRequestPtr url(mojo::URLRequest::New());
-    url->url = mojo::String::From(request->requested_url.spec());
-    ApplicationInstance* originator =
-        manager->GetApplicationInstance(originator_identity_);
-    manager->ConnectToApplication(
-        originator, url.Pass(), std::string(), request->requestor_url,
-        request->services.Pass(), request->exposed_services.Pass(),
-        request->filter, base::Closure(), EmptyConnectCallback());
+    // Unfortunately, it is possible that |request->original_request| is null at
+    // this point. Consider the following sequence:
+    // 1) connect_request_1 arrives at the application manager; the manager
+    //    decides to fetch the app.
+    // 2) connect_request_2 arrives for the same app; because the app is not
+    //    running yet, the manager decides to fetch the app again.
+    // 3) The fetch for step (1) completes and an application instance app_a is
+    //    registered.
+    // 4) app_a goes into two-phase shutdown.
+    // 5) The fetch for step (2) completes; the manager finds that there is a
+    //    running app already, so it connects to app_a.
+    // 6) connect_request_2 is queued (and eventually gets here), but its
+    //    original_request field was already lost to NetworkFetcher at step (2).
+    //
+    // TODO(yzshen): It seems we should register a pending application instance
+    // before starting the fetch. So at step (2) the application manager knows
+    // that it can wait for the first fetch to complete instead of doing a
+    // second one directly.

[Ben Goodger (Google), 2015/09/02 22:20:33]

sgtm

+    if (!request->app_url_request()) {
+      URLRequestPtr url_request = mojo::URLRequest::New();
+      url_request->url = request->app_url().spec();
+      request->SetURLInfo(url_request.Pass());
+    }
+    manager->ConnectToApplication(make_scoped_ptr(request));
   }
+
   STLDeleteElements(&queued_client_requests);
 }
 
 void ApplicationInstance::OnQuitRequestedResult(bool can_quit) {
   if (can_quit)
     return;
 
   queue_requests_ = false;
-  for (auto request : queued_client_requests_) {
-    CallAcceptConnection(
-        request->originator, request->requestor_url, request->services.Pass(),
-        request->exposed_services.Pass(), request->requested_url);
-  }
+  for (auto request : queued_client_requests_)
+    CallAcceptConnection(make_scoped_ptr(request));
+
   STLDeleteElements(&queued_client_requests_);

[sky, 2015/09/02 22:38:26]

Shouldn't you remove this line now since the call on 171 takes ownership of each
item in the vector?

[yzshen1, 2015/09/03 00:06:39]

Wow... good catch++.

 }
 
 }  // namespace shell
 }  // namespace mojo