Commit 07b2a51b authored by Takumi Fujimoto's avatar Takumi Fujimoto Committed by Commit Bot

When a DIAL stop request fails, query the app status

Most of the failures to stop a DIAL session is because the session no
longer exists. So we query for the app's current status, and if it's not
running, we remove the corresponding route.

Bug: 1141714, b/158693455
Change-Id: I71e9770d1188861c37bce996fedf30461a0f9bd6
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2492918
Commit-Queue: Takumi Fujimoto <takumif@chromium.org>
Reviewed-by: default avatarmark a. foltz <mfoltz@chromium.org>
Reviewed-by: default avatarDaniel Cheng <dcheng@chromium.org>
Cr-Commit-Position: refs/heads/master@{#827458}
parent 8bbf58d6
......@@ -6,6 +6,7 @@
#include "base/bind.h"
#include "base/strings/string_split.h"
#include "chrome/browser/media/router/discovery/dial/dial_app_discovery_service.h"
#include "chrome/browser/media/router/providers/dial/dial_internal_message_util.h"
#include "components/media_router/common/media_source.h"
#include "net/base/url_util.h"
......@@ -105,16 +106,19 @@ std::unique_ptr<DialActivity> DialActivity::From(
/* is_local */ true, /* for_display */ true);
route.set_presentation_id(presentation_id);
route.set_off_the_record(off_the_record);
return std::make_unique<DialActivity>(launch_info, route);
return std::make_unique<DialActivity>(launch_info, route, sink);
}
DialActivity::DialActivity(const DialLaunchInfo& launch_info,
const MediaRoute& route)
: launch_info(launch_info), route(route) {}
const MediaRoute& route,
const MediaSinkInternal& sink)
: launch_info(launch_info), route(route), sink(sink) {}
DialActivity::~DialActivity() = default;
DialActivityManager::DialActivityManager() = default;
DialActivityManager::DialActivityManager(
DialAppDiscoveryService* app_discovery_service)
: app_discovery_service_(app_discovery_service) {}
DialActivityManager::~DialActivityManager() = default;
......@@ -303,11 +307,38 @@ void DialActivityManager::OnStopError(const MediaRoute::Id& route_id,
if (record_it == records_.end())
return;
// Move the callback out of the record since we are erasing the record.
// The vast majority of failures to stop a DIAL session is due to the session
// no longer existing on the receiver device. So we make another request to
// the receiver to determine if the session is already terminated.
app_discovery_service_->FetchDialAppInfo(
record_it->second->activity.sink,
record_it->second->activity.launch_info.app_name,
base::BindOnce(&DialActivityManager::OnInfoFetchedAfterStopError,
base::Unretained(this), route_id, message));
}
void DialActivityManager::OnInfoFetchedAfterStopError(
const MediaRoute::Id& route_id,
const std::string& message,
const MediaSink::Id& sink_id,
const std::string& app_name,
DialAppInfoResult result) {
auto record_it = records_.find(route_id);
if (record_it == records_.end())
return;
auto& record = record_it->second;
auto cb = std::move(record->pending_stop_request->callback);
record->pending_stop_request.reset();
std::move(cb).Run(message, RouteRequestResult::UNKNOWN_ERROR);
if (result.app_info && result.app_info->state != DialAppState::kRunning) {
// The app is no longer running, so we remove the record and the MediaRoute
// associated with it.
records_.erase(record_it);
std::move(cb).Run(message, RouteRequestResult::ROUTE_ALREADY_TERMINATED);
} else {
// The app might still be running, so we don't remove the record.
record->pending_stop_request.reset();
std::move(cb).Run(message, RouteRequestResult::UNKNOWN_ERROR);
}
}
DialActivityManager::Record::Record(const DialActivity& activity)
......
......@@ -10,6 +10,7 @@
#include "base/containers/flat_map.h"
#include "base/sequence_checker.h"
#include "chrome/browser/media/router/discovery/dial/dial_app_discovery_service.h"
#include "chrome/browser/media/router/discovery/dial/dial_url_fetcher.h"
#include "components/media_router/common/discovery/media_sink_internal.h"
#include "components/media_router/common/media_route.h"
......@@ -55,17 +56,14 @@ struct DialActivity {
const MediaSource::Id& source_id,
bool off_the_record);
DialActivity(const DialLaunchInfo& launch_info, const MediaRoute& route);
DialActivity(const DialLaunchInfo& launch_info,
const MediaRoute& route,
const MediaSinkInternal& sink);
~DialActivity();
DialLaunchInfo launch_info;
// TODO(https://crbug.com/816628): The MediaRoute itself does not contain
// sufficient information to tell the current state of the activity (launching
// vs. launched). Because of this, the route is rendered in the Media Router
// UI the same way for both states. Consider introducing a state property in
// MediaRoute so that the UI can render them differently.
MediaRoute route;
MediaSinkInternal sink;
};
template <typename CallbackType>
......@@ -102,7 +100,7 @@ class DialActivityManager {
public:
using LaunchAppCallback = base::OnceCallback<void(bool)>;
DialActivityManager();
explicit DialActivityManager(DialAppDiscoveryService* app_discovery_service);
virtual ~DialActivityManager();
// Adds |activity| to the manager. This call is valid only if there is no
......@@ -185,8 +183,16 @@ class DialActivityManager {
int response_code,
const std::string& message);
void OnInfoFetchedAfterStopError(const MediaRoute::Id& route_id,
const std::string& message,
const MediaSink::Id& sink_id,
const std::string& app_name,
DialAppInfoResult result);
base::flat_map<MediaRoute::Id, std::unique_ptr<Record>> records_;
DialAppDiscoveryService* const app_discovery_service_;
SEQUENCE_CHECKER(sequence_checker_);
DISALLOW_COPY_AND_ASSIGN(DialActivityManager);
};
......
......@@ -6,6 +6,7 @@
#include "base/bind.h"
#include "base/test/task_environment.h"
#include "chrome/browser/media/router/discovery/dial/dial_app_discovery_service.h"
#include "chrome/browser/media/router/providers/dial/dial_internal_message_util.h"
#include "chrome/browser/media/router/test/provider_test_helpers.h"
#include "net/http/http_status_code.h"
......@@ -48,7 +49,8 @@ TEST(DialActivityTest, From) {
class DialActivityManagerTest : public testing::Test {
public:
DialActivityManagerTest() : manager_(&loader_factory_) {}
DialActivityManagerTest()
: manager_(&app_discovery_service_, &loader_factory_) {}
~DialActivityManagerTest() override = default;
void TestLaunchApp(const DialActivity& activity,
......@@ -108,6 +110,27 @@ class DialActivityManagerTest : public testing::Test {
void(const base::Optional<std::string>&,
RouteRequestResult::ResultCode));
std::unique_ptr<DialActivity> FailToStopApp() {
auto activity = DialActivity::From(presentation_id_, sink_, source_id_,
/*off_the_record*/ false);
CHECK(activity);
manager_.AddActivity(*activity);
TestLaunchApp(*activity, base::nullopt, base::nullopt);
GURL app_instance_url =
GURL(activity->launch_info.app_launch_url.spec() + "/run");
manager_.SetExpectedRequest(app_instance_url, "DELETE", base::nullopt);
StopApp(activity->route.media_route_id());
loader_factory_.AddResponse(
app_instance_url, network::mojom::URLResponseHead::New(), "",
network::URLLoaderCompletionStatus(net::HTTP_SERVICE_UNAVAILABLE));
EXPECT_CALL(*this, OnStopAppResult(_, Not(RouteRequestResult::OK)));
base::RunLoop().RunUntilIdle();
return activity;
}
protected:
base::test::TaskEnvironment environment_;
std::string presentation_id_ = "presentationId";
......@@ -115,6 +138,7 @@ class DialActivityManagerTest : public testing::Test {
MediaSource::Id source_id_ =
"cast-dial:YouTube?clientId=152127444812943594&dialPostData=foo";
network::TestURLLoaderFactory loader_factory_;
MockDialAppDiscoveryService app_discovery_service_;
TestDialActivityManager manager_;
DISALLOW_COPY_AND_ASSIGN(DialActivityManagerTest);
};
......@@ -231,26 +255,28 @@ TEST_F(DialActivityManagerTest, StopAppUseFallbackURL) {
}
TEST_F(DialActivityManagerTest, StopAppFails) {
auto activity = DialActivity::From(presentation_id_, sink_, source_id_,
/*off_the_record*/ false);
ASSERT_TRUE(activity);
manager_.AddActivity(*activity);
TestLaunchApp(*activity, base::nullopt, base::nullopt);
GURL app_instance_url =
GURL(activity->launch_info.app_launch_url.spec() + "/run");
manager_.SetExpectedRequest(app_instance_url, "DELETE", base::nullopt);
StopApp(activity->route.media_route_id());
loader_factory_.AddResponse(
app_instance_url, network::mojom::URLResponseHead::New(), "",
network::URLLoaderCompletionStatus(net::HTTP_SERVICE_UNAVAILABLE));
EXPECT_CALL(*this, OnStopAppResult(_, Not(RouteRequestResult::OK)));
base::RunLoop().RunUntilIdle();
auto activity = FailToStopApp();
app_discovery_service_.PassCallback().Run(
sink_.sink().id(), "YouTube",
DialAppInfoResult(
CreateParsedDialAppInfoPtr("YouTube", DialAppState::kRunning),
DialAppInfoResultCode::kOk));
EXPECT_TRUE(manager_.GetActivity(activity->route.media_route_id()));
EXPECT_FALSE(manager_.GetRoutes().empty());
}
TEST_F(DialActivityManagerTest, TryToStopAppThatIsAlreadyStopped) {
auto activity = FailToStopApp();
app_discovery_service_.PassCallback().Run(
sink_.sink().id(), "YouTube",
DialAppInfoResult(
CreateParsedDialAppInfoPtr("YouTube", DialAppState::kStopped),
DialAppInfoResultCode::kOk));
// |manager_|, upon learning that the app's state is already |kStopped|,
// should remove the route.
EXPECT_TRUE(manager_.GetRoutes().empty());
}
} // namespace media_router
......@@ -67,7 +67,8 @@ void DialMediaRouteProvider::Init(
// |activity_manager_| might have already been set in tests.
if (!activity_manager_)
activity_manager_ = std::make_unique<DialActivityManager>();
activity_manager_ = std::make_unique<DialActivityManager>(
media_sink_service_->app_discovery_service());
message_sender_ =
std::make_unique<BufferedMessageSender>(media_router_.get());
......@@ -435,27 +436,43 @@ void DialMediaRouteProvider::HandleStopAppResult(
TerminateRouteCallback callback,
const base::Optional<std::string>& message,
RouteRequestResult::ResultCode result_code) {
if (result_code == RouteRequestResult::OK) {
media_router_->OnPresentationConnectionStateChanged(
route_id, blink::mojom::PresentationConnectionState::TERMINATED);
NotifyAllOnRoutesUpdated();
DialMediaRouteProviderMetrics::RecordTerminateRouteResult(
DialTerminateRouteResult::kSuccess);
logger_->LogInfo(mojom::LogCategory::kRoute, kLoggerComponent,
"Successfully terminated route.", "",
MediaRoute::GetMediaSourceIdFromMediaRouteId(route_id),
MediaRoute::GetPresentationIdFromMediaRouteId(route_id));
} else {
DialMediaRouteProviderMetrics::RecordTerminateRouteResult(
DialTerminateRouteResult::kStopAppFailed);
logger_->LogError(
mojom::LogCategory::kRoute, kLoggerComponent,
base::StringPrintf(
"Failed to terminate route. %s RouteRequestResult: %d",
message.value_or("").c_str(), static_cast<int>(result_code)),
"", MediaRoute::GetMediaSourceIdFromMediaRouteId(route_id),
MediaRoute::GetPresentationIdFromMediaRouteId(route_id));
switch (result_code) {
case RouteRequestResult::OK:
DialMediaRouteProviderMetrics::RecordTerminateRouteResult(
DialTerminateRouteResult::kSuccess);
logger_->LogInfo(mojom::LogCategory::kRoute, kLoggerComponent,
"Successfully terminated route.", "",
MediaRoute::GetMediaSourceIdFromMediaRouteId(route_id),
MediaRoute::GetPresentationIdFromMediaRouteId(route_id));
break;
case RouteRequestResult::ROUTE_ALREADY_TERMINATED:
DialMediaRouteProviderMetrics::RecordTerminateRouteResult(
DialTerminateRouteResult::kRouteAlreadyTerminated);
logger_->LogInfo(mojom::LogCategory::kRoute, kLoggerComponent,
"Tried to stop a session that no longer exists.", "",
MediaRoute::GetMediaSourceIdFromMediaRouteId(route_id),
MediaRoute::GetPresentationIdFromMediaRouteId(route_id));
break;
default:
// In this case, the MediaRoute still exists. but we disconnect the
// controller with the OnPresentationConnectionStateChanged() call
// below. This results in a local MediaRoute that is not associated with a
// PresentationConnection.
DialMediaRouteProviderMetrics::RecordTerminateRouteResult(
DialTerminateRouteResult::kStopAppFailed);
logger_->LogError(
mojom::LogCategory::kRoute, kLoggerComponent,
base::StringPrintf(
"Failed to terminate route. %s RouteRequestResult: %d",
message.value_or("").c_str(), static_cast<int>(result_code)),
"", MediaRoute::GetMediaSourceIdFromMediaRouteId(route_id),
MediaRoute::GetPresentationIdFromMediaRouteId(route_id));
}
// We set the PresentationConnection state to "terminated" per the API spec:
// https://w3c.github.io/presentation-api/#terminating-a-presentation-in-a-controlling-browsing-context
media_router_->OnPresentationConnectionStateChanged(
route_id, blink::mojom::PresentationConnectionState::TERMINATED);
NotifyAllOnRoutesUpdated();
std::move(callback).Run(message, result_code);
}
......
......@@ -16,7 +16,9 @@ static constexpr char kHistogramDialParseMessageResult[] =
// Note on enums defined in this file:
// These values are persisted to logs. Entries should not be renumbered and
// numeric values should never be reused.
// numeric values should never be reused. They must also be kept in sync with
// tools/metrics/histograms/enums.xml.
enum class DialCreateRouteResult {
kSuccess = 0,
kSinkNotFound = 1,
......@@ -32,6 +34,7 @@ enum class DialTerminateRouteResult {
kRouteNotFound = 1,
kSinkNotFound = 2,
kStopAppFailed = 3,
kRouteAlreadyTerminated = 4,
kCount
};
......
......@@ -98,8 +98,8 @@ class DialMediaRouteProviderTest : public ::testing::Test {
&mock_sink_service_, "hash-token",
base::SequencedTaskRunnerHandle::Get());
auto activity_manager =
std::make_unique<TestDialActivityManager>(&loader_factory_);
auto activity_manager = std::make_unique<TestDialActivityManager>(
mock_sink_service_.app_discovery_service(), &loader_factory_);
activity_manager_ = activity_manager.get();
provider_->SetActivityManagerForTest(std::move(activity_manager));
......@@ -244,8 +244,8 @@ class DialMediaRouteProviderTest : public ::testing::Test {
// the app if |doLaunch| is |true|.
auto* activity = activity_manager_->GetActivity(route_id);
ASSERT_TRUE(activity);
const GURL& app_launch_url = activity->launch_info.app_launch_url;
activity_manager_->SetExpectedRequest(app_launch_url, "POST",
app_launch_url_ = activity->launch_info.app_launch_url;
activity_manager_->SetExpectedRequest(app_launch_url_, "POST",
"pairingCode=foo");
provider_->SendRouteMessage(
route_id, base::StringPrintf(kCustomDialLaunchMessage,
......@@ -253,11 +253,11 @@ class DialMediaRouteProviderTest : public ::testing::Test {
base::RunLoop().RunUntilIdle();
// Simulate a successful launch response.
app_instance_url_ = GURL(app_launch_url.spec() + "/run");
app_instance_url_ = GURL(app_launch_url_.spec() + "/run");
auto response_head = network::mojom::URLResponseHead::New();
response_head->headers = base::MakeRefCounted<net::HttpResponseHeaders>("");
response_head->headers->AddHeader("LOCATION", app_instance_url_.spec());
loader_factory_.AddResponse(app_launch_url, std::move(response_head), "",
loader_factory_.AddResponse(app_launch_url_, std::move(response_head), "",
network::URLLoaderCompletionStatus());
std::vector<MediaRoute> routes;
EXPECT_CALL(mock_router_, OnRoutesUpdated(_, Not(IsEmpty()), _, IsEmpty()))
......@@ -353,16 +353,28 @@ class DialMediaRouteProviderTest : public ::testing::Test {
loader_factory_.AddResponse(
app_instance_url_, network::mojom::URLResponseHead::New(), "",
network::URLLoaderCompletionStatus(net::HTTP_SERVICE_UNAVAILABLE));
loader_factory_.AddResponse(
app_launch_url_, network::mojom::URLResponseHead::New(), "",
network::URLLoaderCompletionStatus(net::HTTP_SERVICE_UNAVAILABLE));
EXPECT_CALL(*this,
OnTerminateRoute(_, testing::Ne(RouteRequestResult::OK)));
EXPECT_CALL(mock_router_, OnRouteMessagesReceived(_, _));
EXPECT_CALL(mock_router_, OnPresentationConnectionStateChanged(_, _))
.Times(0);
EXPECT_CALL(mock_router_, OnRoutesUpdated(_, _, _, _)).Times(0);
EXPECT_CALL(mock_router_, OnRoutesUpdated(_, _, _, _)).Times(1);
EXPECT_CALL(*mock_sink_service_.app_discovery_service(),
DoFetchDialAppInfo(_, _));
provider_->TerminateRoute(
route_id, base::BindOnce(&DialMediaRouteProviderTest::OnTerminateRoute,
base::Unretained(this)));
base::RunLoop().RunUntilIdle();
// The DialActivityManager requests to confirm the state of the app, so we
// tell it that the app is still running.
mock_sink_service_.app_discovery_service()->PassCallback().Run(
sink_.sink().id(), "YouTube",
DialAppInfoResult(
CreateParsedDialAppInfoPtr("YouTube", DialAppState::kRunning),
DialAppInfoResultCode::kOk));
base::RunLoop().RunUntilIdle();
}
MOCK_METHOD2(OnTerminateRoute,
......@@ -387,6 +399,7 @@ class DialMediaRouteProviderTest : public ::testing::Test {
RouteRequestResult::ResultCode expected_result_code_ = RouteRequestResult::OK;
std::unique_ptr<MediaRoute> route_;
int custom_dial_launch_seq_number_ = -1;
GURL app_launch_url_;
GURL app_instance_url_;
DISALLOW_COPY_AND_ASSIGN(DialMediaRouteProviderTest);
......
......@@ -7,6 +7,7 @@
#include "base/bind.h"
#include "base/json/json_reader.h"
#include "base/strings/stringprintf.h"
#include "chrome/browser/media/router/discovery/dial/dial_app_discovery_service.h"
#include "components/media_router/common/media_source.h"
#include "content/public/browser/browser_task_traits.h"
#include "content/public/browser/browser_thread.h"
......@@ -78,8 +79,9 @@ void TestDialURLFetcher::StartDownload() {
}
TestDialActivityManager::TestDialActivityManager(
DialAppDiscoveryService* app_discovery_service,
network::TestURLLoaderFactory* factory)
: DialActivityManager(), factory_(factory) {}
: DialActivityManager(app_discovery_service), factory_(factory) {}
TestDialActivityManager::~TestDialActivityManager() = default;
std::unique_ptr<DialURLFetcher> TestDialActivityManager::CreateFetcher(
......
......@@ -11,6 +11,7 @@
#include "base/macros.h"
#include "base/strings/string_piece.h"
#include "base/test/values_test_util.h"
#include "chrome/browser/media/router/discovery/dial/dial_app_discovery_service.h"
#include "chrome/browser/media/router/discovery/dial/dial_media_sink_service.h"
#include "chrome/browser/media/router/discovery/dial/dial_url_fetcher.h"
#include "chrome/browser/media/router/discovery/mdns/cast_media_sink_service.h"
......@@ -113,7 +114,8 @@ class TestDialURLFetcher : public DialURLFetcher {
class TestDialActivityManager : public DialActivityManager {
public:
explicit TestDialActivityManager(network::TestURLLoaderFactory* factory);
TestDialActivityManager(DialAppDiscoveryService* app_discovery_service,
network::TestURLLoaderFactory* factory);
~TestDialActivityManager() override;
std::unique_ptr<DialURLFetcher> CreateFetcher(
......
......@@ -212,7 +212,8 @@ enum RouteRequestResultCode {
NO_SUPPORTED_PROVIDER,
CANCELLED,
ROUTE_ALREADY_EXISTS,
DESKTOP_PICKER_FAILED
DESKTOP_PICKER_FAILED,
ROUTE_ALREADY_TERMINATED
// New values must be added here.
};
......
......@@ -440,6 +440,9 @@ struct EnumTraits<media_router::mojom::RouteRequestResultCode,
case media_router::RouteRequestResult::DESKTOP_PICKER_FAILED:
return media_router::mojom::RouteRequestResultCode::
DESKTOP_PICKER_FAILED;
case media_router::RouteRequestResult::ROUTE_ALREADY_TERMINATED:
return media_router::mojom::RouteRequestResultCode::
ROUTE_ALREADY_TERMINATED;
default:
NOTREACHED() << "Unknown RouteRequestResultCode "
<< static_cast<int>(code);
......@@ -483,6 +486,10 @@ struct EnumTraits<media_router::mojom::RouteRequestResultCode,
case media_router::mojom::RouteRequestResultCode::DESKTOP_PICKER_FAILED:
*output = media_router::RouteRequestResult::DESKTOP_PICKER_FAILED;
return true;
case media_router::mojom::RouteRequestResultCode::
ROUTE_ALREADY_TERMINATED:
*output = media_router::RouteRequestResult::ROUTE_ALREADY_TERMINATED;
return true;
}
return false;
}
......
......@@ -47,9 +47,10 @@ class RouteRequestResult {
CANCELLED = 8,
ROUTE_ALREADY_EXISTS = 9,
DESKTOP_PICKER_FAILED = 10,
ROUTE_ALREADY_TERMINATED = 11,
// New values must be added here.
TOTAL_COUNT = 11 // The total number of values.
TOTAL_COUNT = 12 // The total number of values.
};
static std::unique_ptr<RouteRequestResult> FromSuccess(
......
......@@ -48371,6 +48371,7 @@ Called by update_use_counter_css.py.-->
<int value="8" label="Cancelled"/>
<int value="9" label="RouteAlreadyExists"/>
<int value="10" label="DesktopPickerFailed"/>
<int value="11" label="RouteAlreadyTerminated"/>
</enum>
<enum name="MediaRouteProviderVersion">
......@@ -48508,6 +48509,7 @@ Called by update_use_counter_css.py.-->
<int value="1" label="Route not found"/>
<int value="2" label="Sink not found"/>
<int value="3" label="Stop app failed"/>
<int value="4" label="App already terminated"/>
</enum>
<enum name="MediaRouterIconState">
Markdown is supported
0%
or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment