[Presentation API] Media Router: queue PresentationConnectionStateChanged events fired before callb…

chrome/browser/media/router/media_router_base.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 "chrome/browser/media/router/media_router_base.h"
 
 #include "base/bind.h"
 #include "base/guid.h"
 #include "base/memory/ptr_util.h"
 #include "base/stl_util.h"
@@ skipping 46 matching lines @@
   auto* callbacks = presentation_connection_state_callbacks_.get(route_id);
   if (!callbacks) {
     callbacks = new PresentationConnectionStateChangedCallbacks;
     callbacks->set_removal_callback(base::Bind(
         &MediaRouterBase::OnPresentationConnectionStateCallbackRemoved,
         base::Unretained(this), route_id));
     presentation_connection_state_callbacks_.add(route_id,
                                                  base::WrapUnique(callbacks));
   }
 
-  return callbacks->Add(callback);
+  auto subscription = callbacks->Add(callback);
+  auto queued_state_change = queued_state_changes_.find(route_id);
+  if (queued_state_change != queued_state_changes_.end()) {
+    callbacks->Notify(queued_state_change->second);
+    queued_state_changes_.erase(queued_state_change);
+  }
+  return subscription;
 }
 
 void MediaRouterBase::OnIncognitoProfileShutdown() {
   for (const auto& route_id : internal_routes_observer_->incognito_route_ids)
     TerminateRoute(route_id);
 }
 
 MediaRouterBase::MediaRouterBase() : initialized_(false) {}
 
 // static
 std::string MediaRouterBase::CreatePresentationId() {
   return "mr_" + base::GenerateGUID();
 }
 
 void MediaRouterBase::NotifyPresentationConnectionStateChange(
     const MediaRoute::Id& route_id,
     content::PresentationConnectionState state) {
   // We should call NotifyPresentationConnectionClose() for the CLOSED state.
   DCHECK_NE(state, content::PRESENTATION_CONNECTION_STATE_CLOSED);
 
-  auto* callbacks = presentation_connection_state_callbacks_.get(route_id);
-  if (!callbacks)
-    return;
-
-  callbacks->Notify(content::PresentationConnectionStateChangeInfo(state));
+  content::PresentationConnectionStateChangeInfo info(state);
+  NotifyPresentationConnectionStateChange(route_id, info);
 }
 
 void MediaRouterBase::NotifyPresentationConnectionClose(
     const MediaRoute::Id& route_id,
     content::PresentationConnectionCloseReason reason,
     const std::string& message) {
-  auto* callbacks = presentation_connection_state_callbacks_.get(route_id);
-  if (!callbacks)
-    return;
-
   content::PresentationConnectionStateChangeInfo info(
       content::PRESENTATION_CONNECTION_STATE_CLOSED);
   info.close_reason = reason;
   info.message = message;
-  callbacks->Notify(info);
+  NotifyPresentationConnectionStateChange(route_id, info);
+}
+
+void MediaRouterBase::NotifyPresentationConnectionStateChange(
+    const MediaRoute::Id& route_id,
+    const content::PresentationConnectionStateChangeInfo& info) {
+  auto* callbacks = presentation_connection_state_callbacks_.get(route_id);
+
+  if (callbacks) {
+    callbacks->Notify(info);
+    return;
+  }
+
+  auto queued_state_change = queued_state_changes_.find(route_id);
+  if (queued_state_change == queued_state_changes_.end())
+    queued_state_changes_.insert(std::make_pair(route_id, info));

[imcheng, 2016/11/01 20:09:19]

If we never added a state change callback, would this end up staying in the map
indefinitely?

There are two issues we have to address here.

1) The extension can send us state change info with bogus route ids - in those
cases queued_state_changes_ will end up accumulating entries because we will
never add callbacks for them.

2) There are increased coupling between the behaviors of MR and Presentation API
implementation. Even for legit route IDs we expect a callback to be added at
some point in order for this to not be a (browser) memory leak.

I am wondering if there is a way to address these. Another approach I  think we
can consider is to have MR signal the extension that it is ready to receive
presentation connection state changes, similar to how it is done for messages.
This means queueing state changes in the extension and discarding them after
some condition is reached (for example, reaching a terminal state)

+  else
+    queued_state_change->second = info;
 }
 
 bool MediaRouterBase::HasJoinableRoute() const {
   return internal_routes_observer_->has_route;
 }
 
 void MediaRouterBase::Initialize() {
   DCHECK(!initialized_);
   // The observer calls virtual methods on MediaRouter; it must be created
   // outside of the ctor
   internal_routes_observer_.reset(new InternalMediaRoutesObserver(this));
   initialized_ = true;
 }
 
 void MediaRouterBase::OnPresentationConnectionStateCallbackRemoved(
     const MediaRoute::Id& route_id) {
   auto* callbacks = presentation_connection_state_callbacks_.get(route_id);
   if (callbacks && callbacks->empty())
     presentation_connection_state_callbacks_.erase(route_id);
 }
 
 void MediaRouterBase::Shutdown() {
   // The observer calls virtual methods on MediaRouter; it must be destroyed
   // outside of the dtor
   internal_routes_observer_.reset();
 }
 
 }  // namespace media_router