[DIAL MRP] Don't send "stop" receiver action if a request is pending

When the DIAL MRP sends a request to stop casting to a receiver, it also
notifies the sender of the receiver action. This may cause some senders
to submit another stop request, causing a loop that continues until the
app is actually terminated on the receiver. This CL prevents that.

Bug: 958529
Change-Id: Ife55002096bd5d57b689021c4901fad4f4c4100c
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1610531Reviewed-by: mark a. foltz <mfoltz@chromium.org>
Commit-Queue: Takumi Fujimoto <takumif@chromium.org>
Cr-Commit-Position: refs/heads/master@{#662260}

chrome/browser/media/router/providers/dial/dial_activity_manager.cc

@@ -176,7 +176,6 @@ void DialActivityManager::LaunchApp(
                                                      : launch_info.post_data;
   DVLOG(2) << "Launching app on " << route_id;
 
-  // TODO(https://crbug.com/816628): Add metrics to record launch success/error.
   auto fetcher =
       CreateFetcher(base::BindOnce(&DialActivityManager::OnLaunchSuccess,
                                    base::Unretained(this), route_id),
@@ -188,25 +187,27 @@ void DialActivityManager::LaunchApp(
           std::move(fetcher), std::move(callback));
 }
 
+std::pair<base::Optional<std::string>, RouteRequestResult::ResultCode>
+DialActivityManager::CanStopApp(const MediaRoute::Id& route_id) const {
+  auto record_it = records_.find(route_id);
+  if (record_it == records_.end())
+    return {"Activity not found", RouteRequestResult::ROUTE_NOT_FOUND};
+
+  if (record_it->second->pending_stop_request) {
+    return {"A pending request already exists",
+            RouteRequestResult::UNKNOWN_ERROR};
+  }
+  return {base::nullopt, RouteRequestResult::OK};
+}
+
 void DialActivityManager::StopApp(
     const MediaRoute::Id& route_id,
     mojom::MediaRouteProvider::TerminateRouteCallback callback) {
   DCHECK_CALLED_ON_VALID_SEQUENCE(sequence_checker_);
-
   auto record_it = records_.find(route_id);
-  if (record_it == records_.end()) {
-    DVLOG(2) << "Activity not found: " << route_id;
-    std::move(callback).Run("Activity not found",
-                            RouteRequestResult::ROUTE_NOT_FOUND);
-    return;
-  }
-
-  auto& record = record_it->second;
-  if (record->pending_stop_request) {
-    std::move(callback).Run("A pending request already exists",
-                            RouteRequestResult::UNKNOWN_ERROR);
-    return;
-  }
+  DCHECK(record_it != records_.end());
+  std::unique_ptr<Record>& record = record_it->second;
+  DCHECK(!record->pending_stop_request);
 
   // Note that it is possible that the app launched on the device, but we
   // haven't received the launch response yet. In this case we will treat it
@@ -226,7 +227,6 @@ void DialActivityManager::StopApp(
         GURL(activity.launch_info.app_launch_url.spec() + "/run");
   }
 
-  // TODO(https://crbug.com/816628): Add metrics to record stop success/error.
   auto fetcher =
       CreateFetcher(base::BindOnce(&DialActivityManager::OnStopSuccess,
                                    base::Unretained(this), route_id),

chrome/browser/media/router/providers/dial/dial_activity_manager.h

@@ -131,10 +131,17 @@ class DialActivityManager {
                  const CustomDialLaunchMessageBody& message,
                  LaunchAppCallback callback);
 
-  // Stops the app that is currently active on |route_id|.
-  // On success, the associated DialActivity and MediaRoute will be removed
-  // before |callback| is invoked. On failure, the DialActivity and MediaRoute
-  // will not be removed.
+  // Checks if there are existing conditions that would cause a stop app request
+  // to fail, such as |route_id| being invalid or there already being a pending
+  // stop request. If so, returns the error message and error code. Returns
+  // nullopt and RouteRequestResult::OK otherwise.
+  std::pair<base::Optional<std::string>, RouteRequestResult::ResultCode>
+  CanStopApp(const MediaRoute::Id& route_id) const;
+
+  // Stops the app that is currently active on |route_id|. Assumes that
+  // |route_id| has already been verified with CanStopApp(). On success, the
+  // associated DialActivity and MediaRoute will be removed before |callback| is
+  // invoked. On failure, the DialActivity and MediaRoute will not be removed.
   void StopApp(const MediaRoute::Id& route_id,
                mojom::MediaRouteProvider::TerminateRouteCallback callback);
 

chrome/browser/media/router/providers/dial/dial_activity_manager_unittest.cc

@@ -187,13 +187,15 @@ TEST_F(DialActivityManagerTest, StopApp) {
       GURL(sink_.dial_data().app_url.spec() + "/YouTube/app_instance");
   TestLaunchApp(*activity, base::nullopt, app_instance_url);
 
+  auto can_stop = manager_.CanStopApp(activity->route.media_route_id());
+  EXPECT_EQ(can_stop.second, RouteRequestResult::OK);
   manager_.SetExpectedRequest(app_instance_url, "DELETE", base::nullopt);
   StopApp(activity->route.media_route_id());
+  testing::Mock::VerifyAndClearExpectations(this);
 
-  // Pending stop request.
-  EXPECT_CALL(*this, OnStopAppResult(_, Not(RouteRequestResult::OK)));
-  EXPECT_CALL(manager_, OnFetcherCreated()).Times(0);
-  StopApp(activity->route.media_route_id());
+  // // The result should not be OK because there is a pending request.
+  can_stop = manager_.CanStopApp(activity->route.media_route_id());
+  EXPECT_NE(can_stop.second, RouteRequestResult::OK);
 
   loader_factory_.AddResponse(app_instance_url, network::ResourceResponseHead(),
                               "", network::URLLoaderCompletionStatus());

chrome/browser/media/router/providers/dial/dial_media_route_provider.cc

@@ -345,14 +345,21 @@ void DialMediaRouteProvider::DoTerminateRoute(const DialActivity& activity,
                                               TerminateRouteCallback callback) {
   const MediaRoute::Id& route_id = activity.route.media_route_id();
   DVLOG(2) << "Terminating route " << route_id;
-  std::vector<mojom::RouteMessagePtr> messages;
-  messages.emplace_back(internal_message_util_.CreateReceiverActionStopMessage(
-      activity.launch_info, sink));
-  message_sender_->SendMessages(route_id, std::move(messages));
-  activity_manager_->StopApp(
-      route_id,
-      base::BindOnce(&DialMediaRouteProvider::HandleStopAppResult,
-                     base::Unretained(this), route_id, std::move(callback)));
+  std::pair<base::Optional<std::string>, RouteRequestResult::ResultCode>
+      can_stop_app = activity_manager_->CanStopApp(route_id);
+  if (can_stop_app.second == RouteRequestResult::OK) {
+    std::vector<mojom::RouteMessagePtr> messages;
+    messages.emplace_back(
+        internal_message_util_.CreateReceiverActionStopMessage(
+            activity.launch_info, sink));
+    message_sender_->SendMessages(route_id, std::move(messages));
+    activity_manager_->StopApp(
+        route_id,
+        base::BindOnce(&DialMediaRouteProvider::HandleStopAppResult,
+                       base::Unretained(this), route_id, std::move(callback)));
+  } else {
+    std::move(callback).Run(can_stop_app.first, can_stop_app.second);
+  }
 }
 
 void DialMediaRouteProvider::HandleStopAppResult(