Commit d4bcb119 authored by maxmorin's avatar maxmorin Committed by Commit bot

Fix double close in MojoAudioOutputStream.

Currently, the sync socket in MojoAudioOutputStream is closed by both
Mojo and the SyncSocket destructor. This causes problems if a handle
happens to be reused.

BUG=709394
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.android:android_optional_gpu_tests_rel;master.tryserver.chromium.linux:linux_optional_gpu_tests_rel;master.tryserver.chromium.mac:mac_optional_gpu_tests_rel;master.tryserver.chromium.win:win_optional_gpu_tests_rel

Review-Url: https://codereview.chromium.org/2809673002
Cr-Commit-Position: refs/heads/master@{#464374}
parent 322a0254
......@@ -93,6 +93,9 @@ class BASE_EXPORT SyncSocket {
// processes.
Handle handle() const { return handle_; }
// Extracts and takes ownership of the contained handle.
Handle Release();
protected:
Handle handle_;
......
......@@ -75,6 +75,12 @@ size_t SyncSocket::Peek() {
return 0;
}
SyncSocket::Handle SyncSocket::Release() {
Handle r = handle_;
handle_ = kInvalidHandle;
return r;
}
CancelableSyncSocket::CancelableSyncSocket() {
}
......
......@@ -207,6 +207,12 @@ size_t SyncSocket::Peek() {
return number_chars;
}
SyncSocket::Handle SyncSocket::Release() {
Handle r = handle_;
handle_ = kInvalidHandle;
return r;
}
CancelableSyncSocket::CancelableSyncSocket() {}
CancelableSyncSocket::CancelableSyncSocket(Handle handle)
: SyncSocket(handle) {
......
......@@ -293,6 +293,12 @@ size_t SyncSocket::Peek() {
return available;
}
SyncSocket::Handle SyncSocket::Release() {
Handle r = handle_;
handle_ = kInvalidHandle;
return r;
}
CancelableSyncSocket::CancelableSyncSocket()
: shutdown_event_(base::WaitableEvent::ResetPolicy::MANUAL,
base::WaitableEvent::InitialState::NOT_SIGNALED),
......
......@@ -171,7 +171,7 @@ void AudioOutputDelegateImpl::OnSetVolume(double volume) {
void AudioOutputDelegateImpl::SendCreatedNotification() {
DCHECK_CURRENTLY_ON(BrowserThread::IO);
subscriber_->OnStreamCreated(stream_id_, reader_->shared_memory(),
reader_->foreign_socket());
reader_->TakeForeignSocket());
}
void AudioOutputDelegateImpl::UpdatePlayingState(bool playing) {
......
......@@ -14,6 +14,7 @@
#include "base/memory/ptr_util.h"
#include "base/message_loop/message_loop.h"
#include "base/run_loop.h"
#include "base/sync_socket.h"
#include "content/browser/audio_manager_thread.h"
#include "content/browser/media/capture/audio_mirroring_manager.h"
#include "content/public/browser/browser_thread.h"
......@@ -71,10 +72,16 @@ class MockObserver : public content::MediaObserver {
class MockEventHandler : public media::AudioOutputDelegate::EventHandler {
public:
MOCK_METHOD3(OnStreamCreated,
void(int stream_id,
base::SharedMemory* shared_memory,
base::CancelableSyncSocket* socket));
void OnStreamCreated(int stream_id,
base::SharedMemory* shared_memory,
std::unique_ptr<base::CancelableSyncSocket> socket) {
EXPECT_EQ(stream_id, kStreamId);
EXPECT_NE(shared_memory, nullptr);
EXPECT_NE(socket.get(), nullptr);
GotOnStreamCreated();
}
MOCK_METHOD0(GotOnStreamCreated, void());
MOCK_METHOD1(OnStreamError, void(int stream_id));
};
......@@ -114,8 +121,7 @@ class AudioOutputDelegateTest : public testing::Test {
void CreateTest(base::Closure done) {
EXPECT_CALL(media_observer_,
OnCreatingAudioStream(kRenderProcessId, kRenderFrameId));
EXPECT_CALL(event_handler_,
OnStreamCreated(kStreamId, NotNull(), NotNull()));
EXPECT_CALL(event_handler_, GotOnStreamCreated());
EXPECT_CALL(mirroring_manager_,
AddDiverter(kRenderProcessId, kRenderFrameId, NotNull()));
......@@ -139,8 +145,7 @@ class AudioOutputDelegateTest : public testing::Test {
void PlayTest(base::Closure done) {
EXPECT_CALL(media_observer_,
OnCreatingAudioStream(kRenderProcessId, kRenderFrameId));
EXPECT_CALL(event_handler_,
OnStreamCreated(kStreamId, NotNull(), NotNull()));
EXPECT_CALL(event_handler_, GotOnStreamCreated());
EXPECT_CALL(mirroring_manager_,
AddDiverter(kRenderProcessId, kRenderFrameId, NotNull()));
......@@ -166,8 +171,7 @@ class AudioOutputDelegateTest : public testing::Test {
void PauseTest(base::Closure done) {
EXPECT_CALL(media_observer_,
OnCreatingAudioStream(kRenderProcessId, kRenderFrameId));
EXPECT_CALL(event_handler_,
OnStreamCreated(kStreamId, NotNull(), NotNull()));
EXPECT_CALL(event_handler_, GotOnStreamCreated());
EXPECT_CALL(mirroring_manager_,
AddDiverter(kRenderProcessId, kRenderFrameId, NotNull()));
......@@ -193,8 +197,7 @@ class AudioOutputDelegateTest : public testing::Test {
void PlayPausePlayTest(base::Closure done) {
EXPECT_CALL(media_observer_,
OnCreatingAudioStream(kRenderProcessId, kRenderFrameId));
EXPECT_CALL(event_handler_,
OnStreamCreated(kStreamId, NotNull(), NotNull()));
EXPECT_CALL(event_handler_, GotOnStreamCreated());
EXPECT_CALL(mirroring_manager_,
AddDiverter(kRenderProcessId, kRenderFrameId, NotNull()));
......@@ -222,8 +225,7 @@ class AudioOutputDelegateTest : public testing::Test {
void PlayPlayTest(base::Closure done) {
EXPECT_CALL(media_observer_,
OnCreatingAudioStream(kRenderProcessId, kRenderFrameId));
EXPECT_CALL(event_handler_,
OnStreamCreated(kStreamId, NotNull(), NotNull()));
EXPECT_CALL(event_handler_, GotOnStreamCreated());
EXPECT_CALL(mirroring_manager_,
AddDiverter(kRenderProcessId, kRenderFrameId, NotNull()));
......@@ -250,8 +252,7 @@ class AudioOutputDelegateTest : public testing::Test {
void CreateDivertTest(base::Closure done) {
EXPECT_CALL(media_observer_,
OnCreatingAudioStream(kRenderProcessId, kRenderFrameId));
EXPECT_CALL(event_handler_,
OnStreamCreated(kStreamId, NotNull(), NotNull()));
EXPECT_CALL(event_handler_, GotOnStreamCreated());
EXPECT_CALL(mirroring_manager_,
AddDiverter(kRenderProcessId, kRenderFrameId, NotNull()));
......@@ -278,8 +279,7 @@ class AudioOutputDelegateTest : public testing::Test {
void CreateDivertPauseTest(base::Closure done) {
EXPECT_CALL(media_observer_,
OnCreatingAudioStream(kRenderProcessId, kRenderFrameId));
EXPECT_CALL(event_handler_,
OnStreamCreated(kStreamId, NotNull(), NotNull()));
EXPECT_CALL(event_handler_, GotOnStreamCreated());
EXPECT_CALL(mirroring_manager_,
AddDiverter(kRenderProcessId, kRenderFrameId, NotNull()));
......@@ -309,8 +309,7 @@ class AudioOutputDelegateTest : public testing::Test {
void PlayDivertTest(base::Closure done) {
EXPECT_CALL(media_observer_,
OnCreatingAudioStream(kRenderProcessId, kRenderFrameId));
EXPECT_CALL(event_handler_,
OnStreamCreated(kStreamId, NotNull(), NotNull()));
EXPECT_CALL(event_handler_, GotOnStreamCreated());
EXPECT_CALL(mirroring_manager_,
AddDiverter(kRenderProcessId, kRenderFrameId, NotNull()));
......@@ -338,8 +337,7 @@ class AudioOutputDelegateTest : public testing::Test {
void ErrorTest(base::Closure done) {
EXPECT_CALL(media_observer_,
OnCreatingAudioStream(kRenderProcessId, kRenderFrameId));
EXPECT_CALL(event_handler_,
OnStreamCreated(kStreamId, NotNull(), NotNull()));
EXPECT_CALL(event_handler_, GotOnStreamCreated());
EXPECT_CALL(event_handler_, OnStreamError(kStreamId));
EXPECT_CALL(mirroring_manager_,
AddDiverter(kRenderProcessId, kRenderFrameId, NotNull()));
......@@ -386,8 +384,7 @@ class AudioOutputDelegateTest : public testing::Test {
void PlayAndDestroyTest(base::Closure done) {
EXPECT_CALL(media_observer_,
OnCreatingAudioStream(kRenderProcessId, kRenderFrameId));
EXPECT_CALL(event_handler_,
OnStreamCreated(kStreamId, NotNull(), NotNull()));
EXPECT_CALL(event_handler_, GotOnStreamCreated());
EXPECT_CALL(mirroring_manager_,
AddDiverter(kRenderProcessId, kRenderFrameId, NotNull()));
EXPECT_CALL(mirroring_manager_, RemoveDiverter(NotNull()));
......@@ -412,8 +409,7 @@ class AudioOutputDelegateTest : public testing::Test {
void ErrorAndDestroyTest(base::Closure done) {
EXPECT_CALL(media_observer_,
OnCreatingAudioStream(kRenderProcessId, kRenderFrameId));
EXPECT_CALL(event_handler_,
OnStreamCreated(kStreamId, NotNull(), NotNull()));
EXPECT_CALL(event_handler_, GotOnStreamCreated());
EXPECT_CALL(mirroring_manager_,
AddDiverter(kRenderProcessId, kRenderFrameId, NotNull()));
EXPECT_CALL(mirroring_manager_, RemoveDiverter(NotNull()));
......
......@@ -107,7 +107,7 @@ void AudioRendererHost::OnDestruct() const {
void AudioRendererHost::OnStreamCreated(
int stream_id,
base::SharedMemory* shared_memory,
base::CancelableSyncSocket* foreign_socket) {
std::unique_ptr<base::CancelableSyncSocket> foreign_socket) {
DCHECK_CURRENTLY_ON(BrowserThread::IO);
if (!PeerHandle()) {
......
......@@ -91,9 +91,10 @@ class CONTENT_EXPORT AudioRendererHost
bool OnMessageReceived(const IPC::Message& message) override;
// AudioOutputDelegate::EventHandler implementation
void OnStreamCreated(int stream_id,
base::SharedMemory* shared_memory,
base::CancelableSyncSocket* foreign_socket) override;
void OnStreamCreated(
int stream_id,
base::SharedMemory* shared_memory,
std::unique_ptr<base::CancelableSyncSocket> foreign_socket) override;
void OnStreamError(int stream_id) override;
void OverrideDevicePermissionsForTesting(bool has_access);
......
......@@ -5,6 +5,7 @@
#include "content/browser/renderer_host/media/audio_sync_reader.h"
#include <algorithm>
#include <limits>
#include <string>
#include <utility>
......@@ -140,6 +141,12 @@ std::unique_ptr<AudioSyncReader> AudioSyncReader::Create(
std::move(foreign_socket)));
}
std::unique_ptr<base::CancelableSyncSocket>
AudioSyncReader::TakeForeignSocket() {
DCHECK(foreign_socket_);
return std::move(foreign_socket_);
}
// media::AudioOutputController::SyncReader implementations.
void AudioSyncReader::RequestMoreData(base::TimeDelta delay,
base::TimeTicks delay_timestamp,
......
......@@ -41,9 +41,8 @@ class AudioSyncReader : public media::AudioOutputController::SyncReader {
const media::AudioParameters& params);
base::SharedMemory* shared_memory() const { return shared_memory_.get(); }
base::CancelableSyncSocket* foreign_socket() const {
return foreign_socket_.get();
}
std::unique_ptr<base::CancelableSyncSocket> TakeForeignSocket();
// media::AudioOutputController::SyncReader implementations.
void RequestMoreData(base::TimeDelta delay,
......
......@@ -239,9 +239,11 @@ TEST(RenderFrameAudioOutputStreamFactoryTest, CreateStream) {
base::SharedMemory shared_memory;
ASSERT_TRUE(shared_memory.CreateAndMapAnonymous(100));
base::CancelableSyncSocket local, remote;
ASSERT_TRUE(base::CancelableSyncSocket::CreatePair(&local, &remote));
event_handler->OnStreamCreated(kStreamId, &shared_memory, &remote);
auto local = base::MakeUnique<base::CancelableSyncSocket>();
auto remote = base::MakeUnique<base::CancelableSyncSocket>();
ASSERT_TRUE(
base::CancelableSyncSocket::CreatePair(local.get(), remote.get()));
event_handler->OnStreamCreated(kStreamId, &shared_memory, std::move(remote));
base::RunLoop().RunUntilIdle();
// Make sure we got the callback from creating stream.
......
......@@ -32,9 +32,10 @@ class MEDIA_EXPORT AudioOutputDelegate {
// Called when construction is finished and the stream is ready for
// playout.
virtual void OnStreamCreated(int stream_id,
base::SharedMemory* shared_memory,
base::CancelableSyncSocket* socket) = 0;
virtual void OnStreamCreated(
int stream_id,
base::SharedMemory* shared_memory,
std::unique_ptr<base::CancelableSyncSocket> socket) = 0;
// Called if stream encounters an error and has become unusable.
virtual void OnStreamError(int stream_id) = 0;
......
......@@ -58,7 +58,7 @@ void MojoAudioOutputStream::SetVolume(double volume) {
void MojoAudioOutputStream::OnStreamCreated(
int stream_id,
base::SharedMemory* shared_memory,
base::CancelableSyncSocket* foreign_socket) {
std::unique_ptr<base::CancelableSyncSocket> foreign_socket) {
DCHECK(thread_checker_.CalledOnValidThread());
DCHECK(stream_created_callback_);
DCHECK(shared_memory);
......@@ -71,7 +71,7 @@ void MojoAudioOutputStream::OnStreamCreated(
mojo::ScopedSharedBufferHandle buffer_handle = mojo::WrapSharedMemoryHandle(
foreign_memory_handle, shared_memory->requested_size(), false);
mojo::ScopedHandle socket_handle =
mojo::WrapPlatformFile(foreign_socket->handle());
mojo::WrapPlatformFile(foreign_socket->Release());
DCHECK(buffer_handle.is_valid());
DCHECK(socket_handle.is_valid());
......
......@@ -47,9 +47,10 @@ class MEDIA_MOJO_EXPORT MojoAudioOutputStream
void SetVolume(double volume) override;
// AudioOutputDelegate::EventHandler implementation.
void OnStreamCreated(int stream_id,
base::SharedMemory* shared_memory,
base::CancelableSyncSocket* foreign_socket) override;
void OnStreamCreated(
int stream_id,
base::SharedMemory* shared_memory,
std::unique_ptr<base::CancelableSyncSocket> foreign_socket) override;
void OnStreamError(int stream_id) override;
// Closes connection to client and notifies owner.
......
......@@ -35,6 +35,26 @@ using testing::Test;
using AudioOutputStream = mojom::AudioOutputStream;
using AudioOutputStreamPtr = mojo::InterfacePtr<AudioOutputStream>;
class TestCancelableSyncSocket : public base::CancelableSyncSocket {
public:
TestCancelableSyncSocket() {}
void ExpectOwnershipTransfer() { expect_ownership_transfer_ = true; }
~TestCancelableSyncSocket() override {
// When the handle is sent over mojo, mojo takes ownership over it and
// closes it. We have to make sure we do not also retain the handle in the
// sync socket, as the sync socket closes the handle on destruction.
if (expect_ownership_transfer_)
EXPECT_EQ(handle(), kInvalidHandle);
}
private:
bool expect_ownership_transfer_ = false;
DISALLOW_COPY_AND_ASSIGN(TestCancelableSyncSocket);
};
class MockDelegate : NON_EXPORTED_BASE(public AudioOutputDelegate) {
public:
MockDelegate() {}
......@@ -111,7 +131,8 @@ class MockClient {
class MojoAudioOutputStreamTest : public Test {
public:
MojoAudioOutputStreamTest() {}
MojoAudioOutputStreamTest()
: foreign_socket_(base::MakeUnique<TestCancelableSyncSocket>()) {}
AudioOutputStreamPtr CreateAudioOutput() {
AudioOutputStreamPtr p;
......@@ -132,14 +153,15 @@ class MojoAudioOutputStreamTest : public Test {
mock_delegate_factory_.PrepareDelegateForCreation(
base::WrapUnique(delegate_));
EXPECT_TRUE(
base::CancelableSyncSocket::CreatePair(&local_, &foreign_socket_));
base::CancelableSyncSocket::CreatePair(&local_, foreign_socket_.get()));
EXPECT_TRUE(mem_.CreateAnonymous(kShmemSize));
EXPECT_CALL(mock_delegate_factory_, MockCreateDelegate(NotNull()))
.WillOnce(SaveArg<0>(&delegate_event_handler_));
}
base::MessageLoop loop_;
base::CancelableSyncSocket local_, foreign_socket_;
base::CancelableSyncSocket local_;
std::unique_ptr<TestCancelableSyncSocket> foreign_socket_;
base::SharedMemory mem_;
StrictMock<MockDelegate>* delegate_ = nullptr;
AudioOutputDelegate::EventHandler* delegate_event_handler_ = nullptr;
......@@ -178,7 +200,9 @@ TEST_F(MojoAudioOutputStreamTest, DestructWithCallPending_Safe) {
base::RunLoop().RunUntilIdle();
ASSERT_NE(nullptr, delegate_event_handler_);
delegate_event_handler_->OnStreamCreated(kStreamId, &mem_, &foreign_socket_);
foreign_socket_->ExpectOwnershipTransfer();
delegate_event_handler_->OnStreamCreated(kStreamId, &mem_,
std::move(foreign_socket_));
audio_output_ptr->Play();
impl_.reset();
base::RunLoop().RunUntilIdle();
......@@ -191,7 +215,9 @@ TEST_F(MojoAudioOutputStreamTest, Created_NotifiesClient) {
EXPECT_CALL(client_, GotNotification());
ASSERT_NE(nullptr, delegate_event_handler_);
delegate_event_handler_->OnStreamCreated(kStreamId, &mem_, &foreign_socket_);
foreign_socket_->ExpectOwnershipTransfer();
delegate_event_handler_->OnStreamCreated(kStreamId, &mem_,
std::move(foreign_socket_));
base::RunLoop().RunUntilIdle();
}
......@@ -231,7 +257,9 @@ TEST_F(MojoAudioOutputStreamTest, DelegateErrorAfterCreated_PropagatesError) {
base::RunLoop().RunUntilIdle();
ASSERT_NE(nullptr, delegate_event_handler_);
delegate_event_handler_->OnStreamCreated(kStreamId, &mem_, &foreign_socket_);
foreign_socket_->ExpectOwnershipTransfer();
delegate_event_handler_->OnStreamCreated(kStreamId, &mem_,
std::move(foreign_socket_));
delegate_event_handler_->OnStreamError(kStreamId);
base::RunLoop().RunUntilIdle();
......
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