From 2515e12a809c08ece9e5b39b6a2f9b51e61e0f8a Mon Sep 17 00:00:00 2001
From: Bryant Chandler <bryantchandler@chromium.org>
Date: Thu, 30 Mar 2023 16:45:16 +0000
Subject: [PATCH] [fuchsia] Close async completer on destruction of server.
FIDL completers on two-way methods trigger a CHECK error if
`Reply` or `Close` isn't called before they are destroyed.
This CL adds a call to `Close` for the `WatchInfoChangeCompleter`
that is being held by `MediaPlayerImpl` to cover that case.
Bug: 1429266
Change-Id: I35dd8f3b9565614b64cc4765ddb0dfb91f7b12ab
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/4384573
Reviewed-by: David Dorwin <ddorwin@chromium.org>
Commit-Queue: Bryant Chandler <bryantchandler@chromium.org>
Cr-Commit-Position: refs/heads/main@{#1124255}
---
.../webengine/browser/media_player_impl.cc | 6 +++++-
.../browser/media_player_impl_unittest.cc | 19 +++++++++++++++++--
2 files changed, 22 insertions(+), 3 deletions(-)
diff --git a/fuchsia_web/webengine/browser/media_player_impl.cc b/fuchsia_web/webengine/browser/media_player_impl.cc
index bb5cd0de09c4e..425354a7f3ae3 100644
--- a/fuchsia_web/webengine/browser/media_player_impl.cc
+++ b/fuchsia_web/webengine/browser/media_player_impl.cc
@@ -103,7 +103,11 @@ MediaPlayerImpl::MediaPlayerImpl(
pending_info_delta_.local(true);
}
-MediaPlayerImpl::~MediaPlayerImpl() = default;
+MediaPlayerImpl::~MediaPlayerImpl() {
+ if (pending_info_change_callback_) {
+ pending_info_change_callback_->Close(ZX_ERR_PEER_CLOSED);
+ }
+}
void MediaPlayerImpl::WatchInfoChange(
MediaPlayerImpl::WatchInfoChangeCompleter::Sync& completer) {
diff --git a/fuchsia_web/webengine/browser/media_player_impl_unittest.cc b/fuchsia_web/webengine/browser/media_player_impl_unittest.cc
index 58d9f7bc22a70..f01e24adba77a 100644
--- a/fuchsia_web/webengine/browser/media_player_impl_unittest.cc
+++ b/fuchsia_web/webengine/browser/media_player_impl_unittest.cc
@@ -104,7 +104,7 @@ class MediaPlayerImplTest : public testing::Test {
std::unique_ptr<MediaPlayerImpl> player_impl_;
};
-// Verify that the |on_disconnect| closure is invoked if the client disconnects.
+// Verify that the `on_disconnect` closure is invoked if the client disconnects.
TEST_F(MediaPlayerImplTest, OnDisconnectCalledOnDisconnect) {
base::RunLoop run_loop;
player_impl_ = std::make_unique<MediaPlayerImpl>(
@@ -113,7 +113,7 @@ TEST_F(MediaPlayerImplTest, OnDisconnectCalledOnDisconnect) {
run_loop.Run();
}
-// Verify that the |on_disconnect| closure is invoked if the client calls the
+// Verify that the `on_disconnect` closure is invoked if the client calls the
// WatchInfoChange() API incorrectly.
TEST_F(MediaPlayerImplTest, ClientDisconnectedOnBadApiUsage) {
base::RunLoop on_disconnected_loop;
@@ -145,6 +145,21 @@ TEST_F(MediaPlayerImplTest, ClientDisconnectedOnBadApiUsage) {
player_error_loop.Run();
}
+// Verify that the completer is Closed on destruction of `MediaPlayerImpl`.
+// Otherwise it will trigger a CHECK for failing to reply.
+TEST_F(MediaPlayerImplTest, WatchInfoChangeAsyncCompleterClosedOnDestruction) {
+ player_impl_ = std::make_unique<MediaPlayerImpl>(
+ &fake_session_, std::move(player_server_end_),
+ MakeExpectedNotRunClosure(FROM_HERE));
+ player_->WatchInfoChange().Then([](auto result) {});
+ // The first call always replies immediately, so call a second call to hold a
+ // pending completer.
+ player_->WatchInfoChange().Then([](auto result) {});
+
+ // Pump the message loop to process the WatchInfoChange() call.
+ base::RunLoop().RunUntilIdle();
+}
+
// Verify that the first WatchInfoChange() registers the observer.
TEST_F(MediaPlayerImplTest, WatchInfoChangeRegistersObserver) {
player_impl_ = std::make_unique<MediaPlayerImpl>(