Commit e793e1bc authored by Ken Rockot's avatar Ken Rockot Committed by Commit Bot

Fix AudioFocusDelegateDefault behavior

This was revealed by
https://chromium-review.googlesource.com/c/chromium/src/+/1145692
which makes subtle changes to Mojo message dispatch timing and can
thus expose incorrect assumptions in various bits of code.

In this case, there is a race between AbandonAudioFocus and
receiving a reply from a previous call to RequestAudioFocus on
the audio_focus_ptr_. The callback which handles the latter reply
DCHECKs that request_client_ptr_ is bound, but it's possible for
AbandonAudioFocus to have been called before that reply is
dispatched.

The above Mojo CL in question causes some unit tests to flake by
hitting this DCHECK occasionally.

This CL corrects the issue by also resetting audio_focus_ptr_ in
AbandonAudioFocus, which ensures that any pending replies on that
interface will *not* be dispatched.

This also fixes a bug in AudioFocusManagerMetricsHelper (also
revealed by this CL) where it was retaining a *reference* to an
unowned std::string that could be deleted before the helper. This
is because the string was ultimately owned by a binding endpooint,
but the helper is owned by a StackRow which can outlive any given
binding endpoint. Because the name is effectively only useful at
helper construction time, this simply changes the helper to retain
a copy instead of a reference.

TBR=mlamouri@chromium.org

Bug: 895693
Change-Id: I958b783d005cb85dc9dc9f7371df100883d774cb
Reviewed-on: https://chromium-review.googlesource.com/c/1286202
Commit-Queue: Ken Rockot <rockot@google.com>
Reviewed-by: default avatarBecca Hughes <beccahughes@chromium.org>
Cr-Commit-Position: refs/heads/master@{#601362}
parent 0e4149ac
...@@ -112,6 +112,7 @@ void AudioFocusDelegateDefault::AbandonAudioFocus() { ...@@ -112,6 +112,7 @@ void AudioFocusDelegateDefault::AbandonAudioFocus() {
request_client_ptr_->AbandonAudioFocus(); request_client_ptr_->AbandonAudioFocus();
request_client_ptr_.reset(); request_client_ptr_.reset();
audio_focus_ptr_.reset();
} }
base::Optional<media_session::mojom::AudioFocusType> base::Optional<media_session::mojom::AudioFocusType>
......
...@@ -65,7 +65,7 @@ class AudioFocusManagerMetricsHelper { ...@@ -65,7 +65,7 @@ class AudioFocusManagerMetricsHelper {
bool ShouldRecordMetrics() const; bool ShouldRecordMetrics() const;
const std::string& source_name_; std::string source_name_;
base::HistogramBase* const request_source_histogram_; base::HistogramBase* const request_source_histogram_;
base::HistogramBase* const focus_type_histogram_; base::HistogramBase* const focus_type_histogram_;
......
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