Fix native Cast MRP incognito profile shutdown crash

If a launch or stop session request is made for a channel ID when
there still is an outstanding request for the same ID, then the
subsequent requests had their callbacks dropped, which was problematic
especially if the callbacks were Mojo callbacks. This change ensures
that we call them.

Bug: 1148083, b/173533007
Change-Id: Ie99b76ea0716913f91faac97bccff41418e4b25b
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2545263Reviewed-by: John Williams <jrw@chromium.org>
Reviewed-by: mark a. foltz <mfoltz@chromium.org>
Commit-Queue: Takumi Fujimoto <takumif@chromium.org>
Cr-Commit-Position: refs/heads/master@{#828497}

components/cast_channel/cast_message_handler.cc

@@ -479,7 +479,7 @@ void CastMessageHandler::OnMessageSent(int result) {
   DVLOG_IF(2, result < 0) << "SendMessage failed with code: " << result;
 }
 
-CastMessageHandler::PendingRequests::PendingRequests() {}
+CastMessageHandler::PendingRequests::PendingRequests() = default;
 CastMessageHandler::PendingRequests::~PendingRequests() {
   for (auto& request : pending_app_availability_requests_) {
     std::move(request->callback)
@@ -523,8 +523,12 @@ bool CastMessageHandler::PendingRequests::AddAppAvailabilityRequest(
 bool CastMessageHandler::PendingRequests::AddLaunchRequest(
     std::unique_ptr<LaunchSessionRequest> request,
     base::TimeDelta timeout) {
-  if (pending_launch_session_request_)
+  if (pending_launch_session_request_) {
+    std::move(request->callback)
+        .Run(cast_channel::GetLaunchSessionResponseError(
+            "There already exists a launch request for the channel"));
     return false;
+  }
 
   int request_id = request->request_id;
   request->timeout_timer.Start(
@@ -538,8 +542,10 @@ bool CastMessageHandler::PendingRequests::AddLaunchRequest(
 
 bool CastMessageHandler::PendingRequests::AddStopRequest(
     std::unique_ptr<StopSessionRequest> request) {
-  if (pending_stop_session_request_)
+  if (pending_stop_session_request_) {
+    std::move(request->callback).Run(cast_channel::Result::kFailed);
     return false;
+  }
 
   int request_id = request->request_id;
   request->timeout_timer.Start(

components/cast_channel/cast_message_handler_unittest.cc

@@ -3,11 +3,13 @@
 // found in the LICENSE file.
 
 #include "components/cast_channel/cast_message_handler.h"
+#include <string>
 
 #include "base/bind.h"
 #include "base/callback_helpers.h"
 #include "base/json/json_reader.h"
 #include "base/run_loop.h"
+#include "base/strings/string_number_conversions.h"
 #include "base/strings/stringprintf.h"
 #include "base/test/mock_callback.h"
 #include "base/test/task_environment.h"
@@ -39,8 +41,9 @@ constexpr char kAppId2[] = "85CDB22F";
 constexpr char kTestUserAgentString[] =
     "Mozilla/5.0 (X11; Linux x86_64) AppleWebKit/537.36 (KHTML, like Gecko) "
     "Chrome/66.0.3331.0 Safari/537.36";
-constexpr char kSourceId[] = "sourceId";
-constexpr char kDestinationId[] = "destinationId";
+constexpr char kSessionId[] = "theSessionId";
+constexpr char kSourceId[] = "theSourceId";
+constexpr char kDestinationId[] = "theDestinationId";
 constexpr char kAppParams[] = R"(
 {
   "requiredFeatures" : ["STREAM_TRANSFER"],
@@ -105,7 +108,7 @@ class CastMessageHandlerTest : public testing::Test {
         .WillByDefault(testing::Return(&cast_socket_));
   }
 
-  ~CastMessageHandlerTest() override {}
+  ~CastMessageHandlerTest() override = default;
 
   void OnMessage(const CastMessage& message) {
     handler_.OnMessage(cast_socket_, message);
@@ -159,9 +162,9 @@ class CastMessageHandlerTest : public testing::Test {
       handler_.SendSetVolumeRequest(
           channel_id_,
           ParseJson(R"({"sessionId": "theSessionId", "type": "SET_VOLUME"})"),
-          "theSourceId", set_volume_callback_.Get());
+          kSourceId, set_volume_callback_.Get());
     }
-    handler_.StopSession(channel_id_, "theSessionId", "theSourceId",
+    handler_.StopSession(channel_id_, kSessionId, kSourceId,
                          stop_session_callback_.Get());
   }
 
@@ -186,6 +189,36 @@ class CastMessageHandlerTest : public testing::Test {
     EXPECT_EQ(Result::kOk, handler_.SendAppMessage(channel_id_, message));
   }
 
+  void HandlePendingLaunchSessionRequest(int request_id) {
+    handler_.HandleCastInternalMessage(channel_id_, kSourceId, kDestinationId,
+                                       "theNamespace",
+                                       ParseJsonLikeDataDecoder(R"(
+      {
+        "requestId": )" + base::NumberToString(request_id) + R"(,
+        "type": "RECEIVER_STATUS",
+        "status": {"foo": "bar"},
+      })"));
+  }
+
+  void HandlePendingGeneralRequest(int request_id) {
+    handler_.HandleCastInternalMessage(channel_id_, kSourceId, kDestinationId,
+                                       "theNamespace",
+                                       ParseJsonLikeDataDecoder(R"(
+      {
+        "requestId": )" + base::NumberToString(request_id) + R"(
+      })"));
+  }
+
+  void HandleAppAvailabilityRequest(int request_id) {
+    handler_.HandleCastInternalMessage(channel_id_, kSourceId, kDestinationId,
+                                       "theNamespace",
+                                       ParseJsonLikeDataDecoder(R"(
+      {
+        "requestId": )" + base::NumberToString(request_id) + R"(,
+        "availability": {")" + kAppId1 + R"(": "APP_AVAILABLE"},
+      })"));
+  }
+
  protected:
   content::BrowserTaskEnvironment task_environment_;
   std::unique_ptr<base::RunLoop> run_loop_;
@@ -477,7 +510,7 @@ TEST_F(CastMessageHandlerTest, SendMediaRequest) {
             "type": "PLAY",
           })";
           auto expected = CreateMediaRequest(ParseJson(expected_body), 1,
-                                             "theSourceId", "theDestinationId");
+                                             "theSourceId", kDestinationId);
           EXPECT_EQ(expected.namespace_(), message.namespace_());
           EXPECT_EQ(expected.source_id(), message.source_id());
           EXPECT_EQ(expected.destination_id(), message.destination_id());
@@ -493,7 +526,7 @@ TEST_F(CastMessageHandlerTest, SendMediaRequest) {
     "type": "PLAY",
   })";
   base::Optional<int> request_id = handler_.SendMediaRequest(
-      channel_id_, ParseJson(message_str), "theSourceId", "theDestinationId");
+      channel_id_, ParseJson(message_str), "theSourceId", kDestinationId);
   EXPECT_EQ(1, request_id);
 }
 
@@ -579,6 +612,7 @@ TEST_F(CastMessageHandlerTest, PendingRequestsDestructor) {
 }
 
 TEST_F(CastMessageHandlerTest, HandlePendingRequest) {
+  int next_request_id = 1;
   CreatePendingRequests();
 
   // Set up expanctions for pending request callbacks.
@@ -594,43 +628,18 @@ TEST_F(CastMessageHandlerTest, HandlePendingRequest) {
   EXPECT_CALL(set_volume_callback_, Run(Result::kOk)).Times(2);
   EXPECT_CALL(stop_session_callback_, Run(Result::kOk));
 
-  // Handle pending launch session request.
-  handler_.HandleCastInternalMessage(channel_id_, "theSourceId",
-                                     "theDestinationId", "theNamespace",
-                                     ParseJsonLikeDataDecoder(R"(
-      {
-        "requestId": 1,
-        "type": "RECEIVER_STATUS",
-        "status": {"foo": "bar"},
-      })"));
-
+  HandlePendingLaunchSessionRequest(next_request_id++);
   // Handle both pending get app availability requests.
-  handler_.HandleCastInternalMessage(
-      channel_id_, "theSourceId", "theDestinationId", "theNamespace",
-      ParseJsonLikeDataDecoder(base::StringPrintf(R"(
-      {
-        "requestId": 2,
-        "availability": {"%s": "APP_AVAILABLE"},
-      })",
-                                                  kAppId1)));
-
+  HandleAppAvailabilityRequest(next_request_id++);
   // Handle pending set volume request (1 of 2).
-  handler_.HandleCastInternalMessage(
-      channel_id_, "theSourceId", "theDestinationId", "theNamespace",
-      ParseJsonLikeDataDecoder(R"({"requestId": 3})"));
-
+  HandlePendingGeneralRequest(next_request_id++);
   // Skip request_id == 4, since it was used by the second get app availability
   // request.
-
+  next_request_id++;
   // Handle pending set volume request (2 of 2).
-  handler_.HandleCastInternalMessage(
-      channel_id_, "theSourceId", "theDestinationId", "theNamespace",
-      ParseJsonLikeDataDecoder(R"({"requestId": 5})"));
-
+  HandlePendingGeneralRequest(next_request_id++);
   // Handle pending stop session request.
-  handler_.HandleCastInternalMessage(
-      channel_id_, "theSourceId", "theDestinationId", "theNamespace",
-      ParseJsonLikeDataDecoder(R"({"requestId": 6})"));
+  HandlePendingGeneralRequest(next_request_id++);
 }
 
 // Check that set volume requests time out correctly.
@@ -648,4 +657,53 @@ TEST_F(CastMessageHandlerTest, SetVolumeTimedOut) {
   task_environment_.FastForwardBy(kRequestTimeout);
 }
 
+TEST_F(CastMessageHandlerTest, SendMultipleLaunchRequests) {
+  int next_request_id = 1;
+  base::MockCallback<LaunchSessionCallback> expect_success_callback;
+  base::MockCallback<LaunchSessionCallback> expect_failure_callback;
+
+  EXPECT_CALL(expect_success_callback, Run(_))
+      .WillOnce(WithArg<0>([](LaunchSessionResponse response) {
+        EXPECT_EQ(LaunchSessionResponse::Result::kOk, response.result);
+      }));
+  EXPECT_CALL(expect_failure_callback, Run(_))
+      .WillOnce(WithArg<0>([](LaunchSessionResponse response) {
+        EXPECT_EQ(LaunchSessionResponse::Result::kError, response.result);
+      }));
+  EXPECT_CALL(*transport_, SendMessage(_, _)).Times(AnyNumber());
+  handler_.LaunchSession(channel_id_, kAppId1, base::TimeDelta::Max(), {"WEB"},
+                         /* appParams */ base::nullopt,
+                         expect_success_callback.Get());
+  // When there already is a launch request queued, we expect subsequent
+  // requests to fail.
+  handler_.LaunchSession(channel_id_, kAppId1, base::TimeDelta::Max(), {"WEB"},
+                         /* appParams */ base::nullopt,
+                         expect_failure_callback.Get());
+  // This resolves the first launch request.
+  HandlePendingLaunchSessionRequest(next_request_id++);
+}
+
+TEST_F(CastMessageHandlerTest, SendMultipleStopRequests) {
+  int next_request_id = 1;
+  base::MockCallback<ResultCallback> expect_success_callback;
+  base::MockCallback<ResultCallback> expect_failure_callback;
+
+  EXPECT_CALL(*transport_, SendMessage(_, _)).Times(AnyNumber());
+  handler_.LaunchSession(channel_id_, kAppId1, base::TimeDelta::Max(), {"WEB"},
+                         /* appParams */ base::nullopt,
+                         launch_session_callback_.Get());
+  HandlePendingLaunchSessionRequest(next_request_id++);
+
+  EXPECT_CALL(expect_success_callback, Run(Result::kOk));
+  EXPECT_CALL(expect_failure_callback, Run(Result::kFailed));
+  handler_.StopSession(channel_id_, kSessionId, kSourceId,
+                       expect_success_callback.Get());
+  // When there already is a stop request queued, we expect subsequent requests
+  // to fail.
+  handler_.StopSession(channel_id_, kSessionId, kSourceId,
+                       expect_failure_callback.Get());
+  // This resolves the first stop request.
+  HandlePendingGeneralRequest(next_request_id++);
+}
+
 }  // namespace cast_channel