[Presentation API] Fix potential callback leaks in PSImpl.

Background: mojo rolled out a new version that requires callbacks (as
defined by the => syntax in mojom) must be invoked before they go out
of scope, otherwise it indicates a connection error on the callback's
recipient, which will destroy mojom implementations that are strongly
bound to the connection.

Note that a partial fix was added in crrev.com/1019173002. This patch
addresses the remaining points.

BUG=468575

Review URL: https://codereview.chromium.org/996173006

Cr-Commit-Position: refs/heads/master@{#322299}

content/browser/presentation/presentation_service_impl.cc

@@ -25,6 +25,7 @@ PresentationServiceImpl::PresentationServiceImpl(
     : WebContentsObserver(web_contents),
       render_frame_host_(render_frame_host),
       delegate_(delegate),
+      next_request_session_id_(0),
       weak_factory_(this) {
   DCHECK(render_frame_host_);
   DCHECK(web_contents);
@@ -133,10 +134,7 @@ void PresentationServiceImpl::StartSession(
     const NewSessionMojoCallback& callback) {
   DVLOG(2) << "StartSession";
   if (!delegate_) {
-    callback.Run(
-        presentation::PresentationSessionInfoPtr(),
-        presentation::PresentationError::From(
-            PresentationError(PRESENTATION_ERROR_UNKNOWN, "")));
+    InvokeNewSessionMojoCallbackWithError(callback);
     return;
   }
 
@@ -152,23 +150,20 @@ void PresentationServiceImpl::JoinSession(
     const NewSessionMojoCallback& callback) {
   DVLOG(2) << "JoinSession";
   if (!delegate_) {
-    callback.Run(
-        presentation::PresentationSessionInfoPtr(),
-        presentation::PresentationError::From(
-            PresentationError(PRESENTATION_ERROR_UNKNOWN, "")));
+    InvokeNewSessionMojoCallbackWithError(callback);
     return;
   }
 
+  int request_session_id = RegisterNewSessionCallback(callback);
   delegate_->JoinSession(
       render_frame_host_->GetProcess()->GetID(),
       render_frame_host_->GetRoutingID(),
       presentation_url,
       presentation_id,
-      // TODO(imcheng): These callbacks may be dropped. http://crbug.com/468575
       base::Bind(&PresentationServiceImpl::OnStartOrJoinSessionSucceeded,
-                 weak_factory_.GetWeakPtr(), false, callback),
+                 weak_factory_.GetWeakPtr(), false, request_session_id),
       base::Bind(&PresentationServiceImpl::OnStartOrJoinSessionError,
-                 weak_factory_.GetWeakPtr(), false, callback));
+                 weak_factory_.GetWeakPtr(), false, request_session_id));
 }
 
 void PresentationServiceImpl::HandleQueuedStartSessionRequests() {
@@ -183,27 +178,36 @@ void PresentationServiceImpl::HandleQueuedStartSessionRequests() {
   }
 }
 
+int PresentationServiceImpl::RegisterNewSessionCallback(
+    const NewSessionMojoCallback& callback) {
+  ++next_request_session_id_;
+  pending_session_cbs_[next_request_session_id_].reset(
+      new NewSessionMojoCallback(callback));
+  return next_request_session_id_;
+}
+
 void PresentationServiceImpl::DoStartSession(
     const std::string& presentation_url,
     const std::string& presentation_id,
     const NewSessionMojoCallback& callback) {
+  int request_session_id = RegisterNewSessionCallback(callback);
   delegate_->StartSession(
       render_frame_host_->GetProcess()->GetID(),
       render_frame_host_->GetRoutingID(),
       presentation_url,
       presentation_id,
-      // TODO(imcheng): These callbacks may be dropped. http://crbug.com/468575
       base::Bind(&PresentationServiceImpl::OnStartOrJoinSessionSucceeded,
-                 weak_factory_.GetWeakPtr(), true, callback),
+                 weak_factory_.GetWeakPtr(), true, request_session_id),
       base::Bind(&PresentationServiceImpl::OnStartOrJoinSessionError,
-                 weak_factory_.GetWeakPtr(), true, callback));
+                 weak_factory_.GetWeakPtr(), true, request_session_id));
 }
 
 void PresentationServiceImpl::OnStartOrJoinSessionSucceeded(
     bool is_start_session,
-    const NewSessionMojoCallback& callback,
+    int request_session_id,
     const PresentationSessionInfo& session_info) {
-  callback.Run(
+  RunAndEraseNewSessionMojoCallback(
+      request_session_id,
       presentation::PresentationSessionInfo::From(session_info),
       presentation::PresentationErrorPtr());
   if (is_start_session)
@@ -212,15 +216,29 @@ void PresentationServiceImpl::OnStartOrJoinSessionSucceeded(
 
 void PresentationServiceImpl::OnStartOrJoinSessionError(
     bool is_start_session,
-    const NewSessionMojoCallback& callback,
+    int request_session_id,
     const PresentationError& error) {
-  callback.Run(
+  RunAndEraseNewSessionMojoCallback(
+      request_session_id,
       presentation::PresentationSessionInfoPtr(),
       presentation::PresentationError::From(error));
   if (is_start_session)
     HandleQueuedStartSessionRequests();
 }
 
+void PresentationServiceImpl::RunAndEraseNewSessionMojoCallback(
+    int request_session_id,
+    presentation::PresentationSessionInfoPtr session,
+    presentation::PresentationErrorPtr error) {
+  auto it = pending_session_cbs_.find(request_session_id);
+  if (it == pending_session_cbs_.end())
+    return;
+
+  DCHECK(it->second.get());
+  it->second->Run(session.Pass(), error.Pass());
+  pending_session_cbs_.erase(it);
+}
+
 void PresentationServiceImpl::DoSetDefaultPresentationUrl(
     const std::string& default_presentation_url,
     const std::string& default_presentation_id) {
@@ -332,12 +350,26 @@ void PresentationServiceImpl::Reset() {
 
   default_presentation_url_.clear();
   default_presentation_id_.clear();
-  for (const auto& context : availability_contexts_) {
-    context.second->OnScreenAvailabilityChanged(false);
+  for (const auto& context_entry : availability_contexts_) {
+    context_entry.second->OnScreenAvailabilityChanged(false);
   }
   availability_contexts_.clear();
-  // TODO(imcheng): This may drop callbacks. See http://crbug.com/468575.
+  for (auto& request_ptr : queued_start_session_requests_) {
+    InvokeNewSessionMojoCallbackWithError(request_ptr->callback);
+  }
   queued_start_session_requests_.clear();
+  for (auto& pending_entry : pending_session_cbs_) {
+    InvokeNewSessionMojoCallbackWithError(*pending_entry.second);
+  }
+  pending_session_cbs_.clear();
+}
+
+void PresentationServiceImpl::InvokeNewSessionMojoCallbackWithError(
+    const NewSessionMojoCallback& callback) {
+  callback.Run(
+        presentation::PresentationSessionInfoPtr(),
+        presentation::PresentationError::From(
+            PresentationError(PRESENTATION_ERROR_UNKNOWN, "Internal error")));
 }
 
 void PresentationServiceImpl::OnDelegateDestroyed() {

content/browser/presentation/presentation_service_impl.h

@@ -186,6 +186,14 @@ class CONTENT_EXPORT PresentationServiceImpl
   // PresentationServiceDelegate::Observer
   void OnDelegateDestroyed() override;
 
+  // Finds the callback from |pending_session_cbs_| using |request_session_id|.
+  // If it exists, invoke it with |session| and |error|, then erase it from
+  // |pending_session_cbs_|.
+  void RunAndEraseNewSessionMojoCallback(
+      int request_session_id,
+      presentation::PresentationSessionInfoPtr session,
+      presentation::PresentationErrorPtr error);
+
   // Sets |default_presentation_url_| to |presentation_url| and informs the
   // delegate of new default presentation URL and ID.
   void DoSetDefaultPresentationUrl(
@@ -201,11 +209,11 @@ class CONTENT_EXPORT PresentationServiceImpl
   // invocation.
   void OnStartOrJoinSessionSucceeded(
       bool is_start_session,
-      const NewSessionMojoCallback& callback,
+      int request_session_id,
       const PresentationSessionInfo& session_info);
   void OnStartOrJoinSessionError(
       bool is_start_session,
-      const NewSessionMojoCallback& callback,
+      int request_session_id,
       const PresentationError& error);
 
   // Requests delegate to start a session.
@@ -220,6 +228,14 @@ class CONTENT_EXPORT PresentationServiceImpl
   // the first one in the queue.
   void HandleQueuedStartSessionRequests();
 
+  // Associates |callback| with a unique request ID and stores it in a map.
+  int RegisterNewSessionCallback(
+    const NewSessionMojoCallback& callback);
+
+  // Invokes |callback| with an error.
+  void InvokeNewSessionMojoCallbackWithError(
+      const NewSessionMojoCallback& callback);
+
   // Gets the ScreenAvailabilityContext for |presentation_url|, or creates one
   // if it does not exist.
   ScreenAvailabilityContext* GetOrCreateAvailabilityContext(
@@ -240,6 +256,9 @@ class CONTENT_EXPORT PresentationServiceImpl
   // it is removed from head of the queue.
   std::deque<linked_ptr<StartSessionRequest>> queued_start_session_requests_;
 
+  int next_request_session_id_;
+  base::hash_map<int, linked_ptr<NewSessionMojoCallback>> pending_session_cbs_;
+
   // NOTE: Weak pointers must be invalidated before all other member variables.
   base::WeakPtrFactory<PresentationServiceImpl> weak_factory_;