[Presentation API] Limit the number of pending Start/JoinSession

content/browser/presentation/presentation_service_impl.h

Index: content/browser/presentation/presentation_service_impl.h
diff --git a/content/browser/presentation/presentation_service_impl.h b/content/browser/presentation/presentation_service_impl.h
index aeb32f0ab924ae8ee4aa3c5291b908be5eb80a5b..3abe5e448aafd91d4ad01ba5ba6762ebf353544b 100644
--- a/content/browser/presentation/presentation_service_impl.h
+++ b/content/browser/presentation/presentation_service_impl.h
@@ -31,6 +31,10 @@ struct LoadCommittedDetails;
 struct PresentationSessionMessage;
 class RenderFrameHost;
 
+using NewSessionMojoCallback = mojo::Callback<
+    void(presentation::PresentationSessionInfoPtr,
+         presentation::PresentationErrorPtr)>;
+
 // Implementation of Mojo PresentationService.
 // It handles Presentation API requests coming from Blink / renderer process
 // and delegates the requests to the embedder's media router via
@@ -78,10 +82,14 @@ class CONTENT_EXPORT PresentationServiceImpl
       DefaultSessionStartReset);
   FRIEND_TEST_ALL_PREFIXES(PresentationServiceImplTest,
                            ReceiveSessionMessagesAfterReset);
+  FRIEND_TEST_ALL_PREFIXES(PresentationServiceImplTest,
+                           MaxPendingStartSessionRequests);
+  FRIEND_TEST_ALL_PREFIXES(PresentationServiceImplTest,
+                           MaxPendingJoinSessionRequests);
+
+  // Maximum number of queued StartSession or JoinSession requests.
+  static const int kMaxNumQueuedSessionRequests = 10;
 
-  using NewSessionMojoCallback =
-      mojo::Callback<void(presentation::PresentationSessionInfoPtr,
-          presentation::PresentationErrorPtr)>;
   using DefaultSessionMojoCallback =
       mojo::Callback<void(presentation::PresentationSessionInfoPtr)>;
   using SessionStateCallback =
@@ -133,7 +141,24 @@ class CONTENT_EXPORT PresentationServiceImpl
     scoped_ptr<PresentationSessionInfo> session_;
   };
 
-  // Context for a StartSession request.
+  // Ensures the provided NewSessionMojoCallback is invoked exactly once
+  // before it goes out of scope.

[mark a. foltz, 2015/05/14 23:03:01]

Is this necessary for production code or is it just for debugging?

[imcheng (use chromium acct), 2015/05/15 18:01:15]

This is an attempt to clean up the logic preventing dropped callbacks. It
replaces what we have in production code currently.

+  class NewSessionMojoCallbackWrapper {

[mark a. foltz, 2015/05/14 23:03:01]

Can this inherit from NewSessionMojoCallback so the rest of the code doesn't
have to know about it?

[imcheng (use chromium acct), 2015/05/15 18:01:16]

It looks like inheritance would work here as well but unless there's a strong
reason, I prefer composition over inheritance.

+   public:
+    explicit NewSessionMojoCallbackWrapper(
+        const NewSessionMojoCallback& callback);
+    ~NewSessionMojoCallbackWrapper();

[mark a. foltz, 2015/05/14 23:03:01]

virtual 

[imcheng (use chromium acct), 2015/05/15 18:01:15]

Acknowledged.

+
+    void Run(presentation::PresentationSessionInfoPtr session,
+             presentation::PresentationErrorPtr error);
+
+   private:
+    NewSessionMojoCallback callback_;
+
+    DISALLOW_COPY_AND_ASSIGN(NewSessionMojoCallbackWrapper);
+  };
+
+  // Context for a queued StartSession request.
   class CONTENT_EXPORT StartSessionRequest {
    public:
     StartSessionRequest(const std::string& presentation_url,
@@ -141,9 +166,7 @@ class CONTENT_EXPORT PresentationServiceImpl
                         const NewSessionMojoCallback& callback);
     ~StartSessionRequest();
 
-    // Retrieves the pending callback from this request, transferring ownership
-    // to the caller.
-    NewSessionMojoCallback PassCallback();
+    scoped_ptr<NewSessionMojoCallbackWrapper> PassCallback();
 
     const std::string& presentation_url() const { return presentation_url_; }
     const std::string& presentation_id() const { return presentation_id_; }
@@ -151,7 +174,9 @@ class CONTENT_EXPORT PresentationServiceImpl
    private:
     const std::string presentation_url_;
     const std::string presentation_id_;
-    NewSessionMojoCallback callback_;
+    scoped_ptr<NewSessionMojoCallbackWrapper> callback_wrapper_;
+
+    DISALLOW_COPY_AND_ASSIGN(StartSessionRequest);
   };
 
   // |render_frame_host|: The RFH this instance is associated with.
@@ -210,10 +235,11 @@ class CONTENT_EXPORT PresentationServiceImpl
   void OnDefaultPresentationStarted(const PresentationSessionInfo& session)
       override;
 

[mark a. foltz, 2015/05/14 23:03:01]

Does everything from here on down have to be public:?
The rest seem to be implementation details.

[imcheng (use chromium acct), 2015/05/15 18:01:15]

Not sure what you mean. Everything starting from L61 is private.

-  // Finds the callback from |pending_session_cbs_| using |request_session_id|.
+  // Finds the callback from |pending_join_session_cbs_| using
+  // |request_session_id|.
   // If it exists, invoke it with |session| and |error|, then erase it from
-  // |pending_session_cbs_|.
-  void RunAndEraseNewSessionMojoCallback(
+  // |pending_join_session_cbs_|.
+  void RunAndEraseJoinSessionMojoCallback(
       int request_session_id,
       presentation::PresentationSessionInfoPtr session,
       presentation::PresentationErrorPtr error);
@@ -229,21 +255,22 @@ class CONTENT_EXPORT PresentationServiceImpl
   // These functions are bound as base::Callbacks and passed to
   // embedder's implementation of PresentationServiceDelegate for later
   // invocation.
-  void OnStartOrJoinSessionSucceeded(
-      bool is_start_session,
+  void OnStartSessionSucceeded(
       int request_session_id,
       const PresentationSessionInfo& session_info);
-  void OnStartOrJoinSessionError(
-      bool is_start_session,
+  void OnStartSessionError(
+      int request_session_id,
+      const PresentationError& error);
+  void OnJoinSessionSucceeded(
+      int request_session_id,
+      const PresentationSessionInfo& session_info);
+  void OnJoinSessionError(
       int request_session_id,
       const PresentationError& error);
   void OnSendMessageCallback();
 
   // Requests delegate to start a session.
-  void DoStartSession(
-      const std::string& presentation_url,
-      const std::string& presentation_id,
-      const NewSessionMojoCallback& callback);
+  void DoStartSession(scoped_ptr<StartSessionRequest> request);
 
   // Passed to embedder's implementation of PresentationServiceDelegate for
   // later invocation when session messages arrive.
@@ -258,17 +285,18 @@ 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);
+  // Generates and returns an ID to use for a pending StartSession/JoinSession
+  // request.
+  int GetNextRequestSessionId();
+
+  // Associates a JoinSession |callback| with a unique request ID and
+  // stores it in a map.
+  // Returns a positive value on success.
+  int RegisterJoinSessionCallback(const NewSessionMojoCallback& callback);
 
   // Flushes all pending new session callbacks with error responses.
   void FlushNewSessionCallbacks();
 
-  // Invokes |callback| with an error.
-  static void InvokeNewSessionMojoCallbackWithError(
-      const NewSessionMojoCallback& callback);
-
   // Returns true if this object is associated with |render_frame_host|.
   bool FrameMatches(content::RenderFrameHost* render_frame_host) const;
 
@@ -290,11 +318,18 @@ class CONTENT_EXPORT PresentationServiceImpl
   // it is removed from head of the queue.
   std::deque<linked_ptr<StartSessionRequest>> queued_start_session_requests_;
 
-  // Indicates that a StartSession request is currently being processed.
-  bool is_start_session_pending_;
+  // For StartSession requests.
+  // Set to a positive value when a StartSession request is being processed.
+  int start_session_request_id_;
+  scoped_ptr<NewSessionMojoCallbackWrapper> pending_start_session_cb_;
+
+  // For JoinSession requests.
+  base::hash_map<int, linked_ptr<NewSessionMojoCallbackWrapper>>
+      pending_join_session_cbs_;
 
+  // Request ID counter used in StartSession/JoinSession. Incremented every
+  // time a request is processed.
   int next_request_session_id_;
-  base::hash_map<int, linked_ptr<NewSessionMojoCallback>> pending_session_cbs_;
 
   scoped_ptr<DefaultSessionStartContext> default_session_start_context_;