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

chrome/browser/media/router/media_router_base.cc

Index: chrome/browser/media/router/media_router_base.cc
diff --git a/chrome/browser/media/router/media_router_base.cc b/chrome/browser/media/router/media_router_base.cc
index 60f85c88b331990f8b00d668b2b9a904895a6fce..a65f812e09f28df199b7acfcdce573fcb6d02b51 100644
--- a/chrome/browser/media/router/media_router_base.cc
+++ b/chrome/browser/media/router/media_router_base.cc
@@ -64,7 +64,13 @@ MediaRouterBase::AddPresentationConnectionStateChangedCallback(
                                                  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() {
@@ -85,26 +91,36 @@ void MediaRouterBase::NotifyPresentationConnectionStateChange(
   // 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 {