From b009f6cd634004b69e6c1377927efff3a4bb3618 Mon Sep 17 00:00:00 2001 From: Andreas Pehrson Date: Tue, 5 Nov 2019 20:04:15 +0000 Subject: [PATCH] Bug 1212237 - Be explicit about principals for received tracks. r=bwc This patch fixes two things: 1) A potential threading issue in setting and reading the PrincipalHandle for MediaPipelineReceiveVideo. 2) Plumbs the PrincipalHandle down to the receiving MediaPipelines right from the start. It hasn't been necessary in the past as initially the principal handed to a track's constructor is trusted, but it's good form and will help clarify the situation for the next patch which switches the initial principal from always-private to mostly-non-private. In most cases this principal does no longer get updated after the track's been created, so it helps to have a PrincipalHandle that matches the track's principal. Differential Revision: https://phabricator.services.mozilla.com/D48946 --HG-- extra : moz-landing-system : lando --- .../gtest/mediapipeline_unittest.cpp | 2 +- .../src/mediapipeline/MediaPipeline.cpp | 97 +++++++++++-------- .../src/mediapipeline/MediaPipeline.h | 10 +- .../src/peerconnection/PeerConnectionImpl.cpp | 45 ++++----- .../src/peerconnection/PeerConnectionImpl.h | 2 +- .../peerconnection/PeerConnectionMedia.cpp | 10 +- .../src/peerconnection/PeerConnectionMedia.h | 1 + .../src/peerconnection/TransceiverImpl.cpp | 23 +++-- .../src/peerconnection/TransceiverImpl.h | 7 +- 9 files changed, 113 insertions(+), 84 deletions(-) diff --git a/media/webrtc/signaling/gtest/mediapipeline_unittest.cpp b/media/webrtc/signaling/gtest/mediapipeline_unittest.cpp index fcb41857fccf..c14414ce42a2 100644 --- a/media/webrtc/signaling/gtest/mediapipeline_unittest.cpp +++ b/media/webrtc/signaling/gtest/mediapipeline_unittest.cpp @@ -345,7 +345,7 @@ class TestAgentReceive : public TestAgent { audio_pipeline_ = new mozilla::MediaPipelineReceiveAudio( test_pc, transport_, nullptr, test_utils->sts_target(), static_cast(audio_conduit_.get()), - nullptr); + nullptr, PRINCIPAL_HANDLE_NONE); audio_pipeline_->Start(); diff --git a/media/webrtc/signaling/src/mediapipeline/MediaPipeline.cpp b/media/webrtc/signaling/src/mediapipeline/MediaPipeline.cpp index 14988ddc37ff..b9833aedf8dc 100644 --- a/media/webrtc/signaling/src/mediapipeline/MediaPipeline.cpp +++ b/media/webrtc/signaling/src/mediapipeline/MediaPipeline.cpp @@ -1168,7 +1168,6 @@ class GenericReceiveListener : public MediaTrackListener { &static_cast(aTrack->GetSource()))), mSource(mTrackSource->mStream), mIsAudio(aTrack->AsAudioStreamTrack()), - mPrincipalHandle(PRINCIPAL_HANDLE_NONE), mListening(false), mMaybeTrackNeedsUnmute(true) { MOZ_DIAGNOSTIC_ASSERT(NS_IsMainThread()); @@ -1229,38 +1228,10 @@ class GenericReceiveListener : public MediaTrackListener { &RemoteTrackSource::ForceEnded)); } - // Must be called on the main thread - void SetPrincipalHandle_m(const PrincipalHandle& aPrincipalHandle) { - class Message : public ControlMessage { - public: - Message(GenericReceiveListener* aListener, - const PrincipalHandle& aPrincipalHandle) - : ControlMessage(nullptr), - mListener(aListener), - mPrincipalHandle(aPrincipalHandle) {} - - void Run() override { - mListener->SetPrincipalHandle_mtg(mPrincipalHandle); - } - - const RefPtr mListener; - PrincipalHandle mPrincipalHandle; - }; - - mSource->GraphImpl()->AppendMessage( - MakeUnique(this, aPrincipalHandle)); - } - - // Must be called on the MediaTrackGraph thread - void SetPrincipalHandle_mtg(const PrincipalHandle& aPrincipalHandle) { - mPrincipalHandle = aPrincipalHandle; - } - protected: const nsMainThreadPtrHandle mTrackSource; const RefPtr mSource; const bool mIsAudio; - PrincipalHandle mPrincipalHandle; bool mListening; Atomic mMaybeTrackNeedsUnmute; }; @@ -1278,7 +1249,8 @@ class MediaPipelineReceiveAudio::PipelineListener : public GenericReceiveListener { public: PipelineListener(dom::MediaStreamTrack* aTrack, - const RefPtr& aConduit) + const RefPtr& aConduit, + const PrincipalHandle& aPrincipalHandle) : GenericReceiveListener(aTrack), mConduit(aConduit), // AudioSession conduit only supports 16, 32, 44.1 and 48kHz @@ -1292,7 +1264,8 @@ class MediaPipelineReceiveAudio::PipelineListener mTaskQueue( new TaskQueue(GetMediaThreadPool(MediaThreadType::WEBRTC_DECODER), "AudioPipelineListener")), - mPlayedTicks(0) { + mPlayedTicks(0), + mPrincipalHandle(aPrincipalHandle) { mSource->SetAppendDataSourceRate(mRate); mSource->AddListener(this); } @@ -1303,6 +1276,33 @@ class MediaPipelineReceiveAudio::PipelineListener NotifyPullImpl(aDesiredTime); } + // Must be called on the main thread + void SetPrincipalHandle_m(const PrincipalHandle& aPrincipalHandle) { + class Message : public ControlMessage { + public: + Message(PipelineListener* aListener, + const PrincipalHandle& aPrincipalHandle) + : ControlMessage(nullptr), + mListener(aListener), + mPrincipalHandle(aPrincipalHandle) {} + + void Run() override { + mListener->SetPrincipalHandle_mtg(mPrincipalHandle); + } + + const RefPtr mListener; + const PrincipalHandle mPrincipalHandle; + }; + + mSource->GraphImpl()->AppendMessage( + MakeUnique(this, aPrincipalHandle)); + } + + // Must be called on the MediaTrackGraph thread + void SetPrincipalHandle_mtg(const PrincipalHandle& aPrincipalHandle) { + mPrincipalHandle = aPrincipalHandle; + } + private: ~PipelineListener() { NS_ReleaseOnMainThreadSystemGroup("MediaPipeline::mConduit", @@ -1399,15 +1399,19 @@ class MediaPipelineReceiveAudio::PipelineListener // Number of frames of data that has been added to the SourceMediaTrack in // the graph's rate. TrackTicks mPlayedTicks; + PrincipalHandle mPrincipalHandle; }; MediaPipelineReceiveAudio::MediaPipelineReceiveAudio( const std::string& aPc, MediaTransportHandler* aTransportHandler, nsCOMPtr aMainThread, nsCOMPtr aStsThread, - RefPtr aConduit, dom::MediaStreamTrack* aTrack) + RefPtr aConduit, dom::MediaStreamTrack* aTrack, + const PrincipalHandle& aPrincipalHandle) : MediaPipelineReceive(aPc, aTransportHandler, aMainThread, aStsThread, aConduit), - mListener(aTrack ? new PipelineListener(aTrack, mConduit) : nullptr) { + mListener(aTrack + ? new PipelineListener(aTrack, mConduit, aPrincipalHandle) + : nullptr) { mDescription = mPc + "| Receive audio"; } @@ -1448,15 +1452,28 @@ void MediaPipelineReceiveAudio::OnRtpPacketReceived() { class MediaPipelineReceiveVideo::PipelineListener : public GenericReceiveListener { public: - explicit PipelineListener(dom::MediaStreamTrack* aTrack) + PipelineListener(dom::MediaStreamTrack* aTrack, + const PrincipalHandle& aPrincipalHandle) : GenericReceiveListener(aTrack), mImageContainer( - LayerManager::CreateImageContainer(ImageContainer::ASYNCHRONOUS)) { + LayerManager::CreateImageContainer(ImageContainer::ASYNCHRONOUS)), + mMutex("MediaPipelineReceiveVideo::PipelineListener::mMutex"), + mPrincipalHandle(aPrincipalHandle) { mSource->AddListener(this); } + void SetPrincipalHandle_m(const PrincipalHandle& aPrincipalHandle) { + MutexAutoLock lock(mMutex); + mPrincipalHandle = aPrincipalHandle; + } + void RenderVideoFrame(const webrtc::VideoFrameBuffer& aBuffer, uint32_t aTimeStamp, int64_t aRenderTime) { + PrincipalHandle principal; + { + MutexAutoLock lock(mMutex); + principal = mPrincipalHandle; + } RefPtr image; if (aBuffer.type() == webrtc::VideoFrameBuffer::Type::kNative) { // We assume that only native handles are used with the @@ -1501,12 +1518,14 @@ class MediaPipelineReceiveVideo::PipelineListener VideoSegment segment; auto size = image->GetSize(); - segment.AppendFrame(image.forget(), size, mPrincipalHandle); + segment.AppendFrame(image.forget(), size, principal); mSource->AppendData(&segment); } private: RefPtr mImageContainer; + Mutex mMutex; // Protects the below members. + PrincipalHandle mPrincipalHandle; }; class MediaPipelineReceiveVideo::PipelineRenderer @@ -1531,11 +1550,13 @@ class MediaPipelineReceiveVideo::PipelineRenderer MediaPipelineReceiveVideo::MediaPipelineReceiveVideo( const std::string& aPc, MediaTransportHandler* aTransportHandler, nsCOMPtr aMainThread, nsCOMPtr aStsThread, - RefPtr aConduit, dom::MediaStreamTrack* aTrack) + RefPtr aConduit, dom::MediaStreamTrack* aTrack, + const PrincipalHandle& aPrincipalHandle) : MediaPipelineReceive(aPc, aTransportHandler, aMainThread, aStsThread, aConduit), mRenderer(new PipelineRenderer(this)), - mListener(aTrack ? new PipelineListener(aTrack) : nullptr) { + mListener(aTrack ? new PipelineListener(aTrack, aPrincipalHandle) + : nullptr) { mDescription = mPc + "| Receive video"; aConduit->AttachRenderer(mRenderer); } diff --git a/media/webrtc/signaling/src/mediapipeline/MediaPipeline.h b/media/webrtc/signaling/src/mediapipeline/MediaPipeline.h index adb236540a65..656535ad03d5 100644 --- a/media/webrtc/signaling/src/mediapipeline/MediaPipeline.h +++ b/media/webrtc/signaling/src/mediapipeline/MediaPipeline.h @@ -348,7 +348,8 @@ class MediaPipelineReceiveAudio : public MediaPipelineReceive { nsCOMPtr aMainThread, nsCOMPtr aStsThread, RefPtr aConduit, - dom::MediaStreamTrack* aTrack); + dom::MediaStreamTrack* aTrack, + const PrincipalHandle& aPrincipalHandle); void DetachMedia() override; @@ -365,7 +366,7 @@ class MediaPipelineReceiveAudio : public MediaPipelineReceive { // Separate class to allow ref counting class PipelineListener; - RefPtr mListener; + const RefPtr mListener; }; // A specialization of pipeline for reading from the network and @@ -377,7 +378,8 @@ class MediaPipelineReceiveVideo : public MediaPipelineReceive { nsCOMPtr aMainThread, nsCOMPtr aStsThread, RefPtr aConduit, - dom::MediaStreamTrack* aTrack); + dom::MediaStreamTrack* aTrack, + const PrincipalHandle& aPrincipalHandle); // Called on the main thread. void DetachMedia() override; @@ -399,7 +401,7 @@ class MediaPipelineReceiveVideo : public MediaPipelineReceive { class PipelineListener; const RefPtr mRenderer; - RefPtr mListener; + const RefPtr mListener; }; } // namespace mozilla diff --git a/media/webrtc/signaling/src/peerconnection/PeerConnectionImpl.cpp b/media/webrtc/signaling/src/peerconnection/PeerConnectionImpl.cpp index 598b1e6db6b1..1b04268140c4 100644 --- a/media/webrtc/signaling/src/peerconnection/PeerConnectionImpl.cpp +++ b/media/webrtc/signaling/src/peerconnection/PeerConnectionImpl.cpp @@ -1001,13 +1001,29 @@ already_AddRefed PeerConnectionImpl::CreateTransceiverImpl( aSendTrack->AddPrincipalChangeObserver(this); } + // Set the principal used for the receive tracks. This makes the track + // data (audio/video samples) accessible to the receiving page. We're + // only certain that privacy has been requested if we're connected. + RefPtr principal; + Document* doc = GetWindow()->GetExtantDoc(); + MOZ_ASSERT(doc); + const bool privacyRequested = mPrivacyRequested.valueOr(true); + if (privacyRequested) { + // We're either certain that we need isolation for the tracks, OR + // we're not sure and we can fix the track and pipeline in AlpnNegotiated. + principal = + NullPrincipal::CreateWithInheritedAttributes(doc->NodePrincipal()); + } else { + principal = doc->NodePrincipal(); + } + OwningNonNull receiveTrack = - CreateReceiveTrack(aJsepTransceiver->GetMediaType()); + CreateReceiveTrack(aJsepTransceiver->GetMediaType(), principal); RefPtr transceiverImpl; - - aRv = mMedia->AddTransceiver(aJsepTransceiver, *receiveTrack, aSendTrack, - &transceiverImpl); + aRv = + mMedia->AddTransceiver(aJsepTransceiver, *receiveTrack, aSendTrack, + MakePrincipalHandle(principal), &transceiverImpl); return transceiverImpl.forget(); } @@ -1829,24 +1845,9 @@ static int GetDTMFToneCode(uint16_t c) { } OwningNonNull PeerConnectionImpl::CreateReceiveTrack( - SdpMediaSection::MediaType type) { + SdpMediaSection::MediaType type, nsIPrincipal* aPrincipal) { bool audio = (type == SdpMediaSection::MediaType::kAudio); - // Set the principal used for creating the tracks. This makes the track - // data (audio/video samples) accessible to the receiving page. We're - // only certain that privacy hasn't been requested if we're connected. - nsCOMPtr principal; - Document* doc = GetWindow()->GetExtantDoc(); - MOZ_ASSERT(doc); - if (mPrivacyRequested.isSome() && !*mPrivacyRequested) { - principal = doc->NodePrincipal(); - } else { - // we're either certain that we need isolation for the tracks, OR - // we're not sure and we can fix the track in SetDtlsConnected - principal = - NullPrincipal::CreateWithInheritedAttributes(doc->NodePrincipal()); - } - MediaTrackGraph* graph = MediaTrackGraph::GetInstance( audio ? MediaTrackGraph::AUDIO_THREAD_DRIVER : MediaTrackGraph::SYSTEM_THREAD_DRIVER, @@ -1857,13 +1858,13 @@ OwningNonNull PeerConnectionImpl::CreateReceiveTrack( if (audio) { RefPtr source = graph->CreateSourceTrack(MediaSegment::AUDIO); - trackSource = new RemoteTrackSource(source, principal, + trackSource = new RemoteTrackSource(source, aPrincipal, NS_ConvertASCIItoUTF16("remote audio")); track = new AudioStreamTrack(GetWindow(), source, trackSource); } else { RefPtr source = graph->CreateSourceTrack(MediaSegment::VIDEO); - trackSource = new RemoteTrackSource(source, principal, + trackSource = new RemoteTrackSource(source, aPrincipal, NS_ConvertASCIItoUTF16("remote video")); track = new VideoStreamTrack(GetWindow(), source, trackSource); } diff --git a/media/webrtc/signaling/src/peerconnection/PeerConnectionImpl.h b/media/webrtc/signaling/src/peerconnection/PeerConnectionImpl.h index a2a05c6bc054..0570525e5d1b 100644 --- a/media/webrtc/signaling/src/peerconnection/PeerConnectionImpl.h +++ b/media/webrtc/signaling/src/peerconnection/PeerConnectionImpl.h @@ -310,7 +310,7 @@ class PeerConnectionImpl final ErrorResult& rv); OwningNonNull CreateReceiveTrack( - SdpMediaSection::MediaType type); + SdpMediaSection::MediaType type, nsIPrincipal* aPrincipal); bool CheckNegotiationNeeded(ErrorResult& rv); diff --git a/media/webrtc/signaling/src/peerconnection/PeerConnectionMedia.cpp b/media/webrtc/signaling/src/peerconnection/PeerConnectionMedia.cpp index 11fef1ef6840..c4964afe4bfa 100644 --- a/media/webrtc/signaling/src/peerconnection/PeerConnectionMedia.cpp +++ b/media/webrtc/signaling/src/peerconnection/PeerConnectionMedia.cpp @@ -581,16 +581,16 @@ void PeerConnectionMedia::ShutdownMediaTransport_s() { nsresult PeerConnectionMedia::AddTransceiver( JsepTransceiver* aJsepTransceiver, dom::MediaStreamTrack& aReceiveTrack, - dom::MediaStreamTrack* aSendTrack, + dom::MediaStreamTrack* aSendTrack, const PrincipalHandle& aPrincipalHandle, RefPtr* aTransceiverImpl) { if (!mCall) { mCall = WebRtcCallWrapper::Create(); } - RefPtr transceiver = - new TransceiverImpl(mParent->GetHandle(), mTransportHandler, - aJsepTransceiver, mMainThread.get(), mSTSThread.get(), - &aReceiveTrack, aSendTrack, mCall.get()); + RefPtr transceiver = new TransceiverImpl( + mParent->GetHandle(), mTransportHandler, aJsepTransceiver, + mMainThread.get(), mSTSThread.get(), &aReceiveTrack, aSendTrack, + mCall.get(), aPrincipalHandle); if (!transceiver->IsValid()) { return NS_ERROR_FAILURE; diff --git a/media/webrtc/signaling/src/peerconnection/PeerConnectionMedia.h b/media/webrtc/signaling/src/peerconnection/PeerConnectionMedia.h index 48dba58b225a..6d0d774b3f31 100644 --- a/media/webrtc/signaling/src/peerconnection/PeerConnectionMedia.h +++ b/media/webrtc/signaling/src/peerconnection/PeerConnectionMedia.h @@ -82,6 +82,7 @@ class PeerConnectionMedia : public sigslot::has_slots<> { nsresult AddTransceiver(JsepTransceiver* aJsepTransceiver, dom::MediaStreamTrack& aReceiveTrack, dom::MediaStreamTrack* aSendTrack, + const PrincipalHandle& aPrincipalHandle, RefPtr* aTransceiverImpl); void GetTransmitPipelinesMatching( diff --git a/media/webrtc/signaling/src/peerconnection/TransceiverImpl.cpp b/media/webrtc/signaling/src/peerconnection/TransceiverImpl.cpp index 0fc41632048c..258791468270 100644 --- a/media/webrtc/signaling/src/peerconnection/TransceiverImpl.cpp +++ b/media/webrtc/signaling/src/peerconnection/TransceiverImpl.cpp @@ -35,7 +35,8 @@ TransceiverImpl::TransceiverImpl( const std::string& aPCHandle, MediaTransportHandler* aTransportHandler, JsepTransceiver* aJsepTransceiver, nsIEventTarget* aMainThread, nsIEventTarget* aStsThread, dom::MediaStreamTrack* aReceiveTrack, - dom::MediaStreamTrack* aSendTrack, WebRtcCallWrapper* aCallWrapper) + dom::MediaStreamTrack* aSendTrack, WebRtcCallWrapper* aCallWrapper, + const PrincipalHandle& aPrincipalHandle) : mPCHandle(aPCHandle), mTransportHandler(aTransportHandler), mJsepTransceiver(aJsepTransceiver), @@ -47,9 +48,9 @@ TransceiverImpl::TransceiverImpl( mSendTrack(aSendTrack), mCallWrapper(aCallWrapper) { if (IsVideo()) { - InitVideo(); + InitVideo(aPrincipalHandle); } else { - InitAudio(); + InitAudio(aPrincipalHandle); } if (!IsValid()) { @@ -69,7 +70,7 @@ TransceiverImpl::~TransceiverImpl() = default; NS_IMPL_ISUPPORTS0(TransceiverImpl) -void TransceiverImpl::InitAudio() { +void TransceiverImpl::InitAudio(const PrincipalHandle& aPrincipalHandle) { mConduit = AudioSessionConduit::Create(mCallWrapper, mStsThread); if (!mConduit) { @@ -82,10 +83,11 @@ void TransceiverImpl::InitAudio() { mReceivePipeline = new MediaPipelineReceiveAudio( mPCHandle, mTransportHandler, mMainThread.get(), mStsThread.get(), - static_cast(mConduit.get()), mReceiveTrack); + static_cast(mConduit.get()), mReceiveTrack, + aPrincipalHandle); } -void TransceiverImpl::InitVideo() { +void TransceiverImpl::InitVideo(const PrincipalHandle& aPrincipalHandle) { mConduit = VideoSessionConduit::Create(mCallWrapper, mStsThread); if (!mConduit) { @@ -98,7 +100,8 @@ void TransceiverImpl::InitVideo() { mReceivePipeline = new MediaPipelineReceiveVideo( mPCHandle, mTransportHandler, mMainThread.get(), mStsThread.get(), - static_cast(mConduit.get()), mReceiveTrack); + static_cast(mConduit.get()), mReceiveTrack, + aPrincipalHandle); } nsresult TransceiverImpl::UpdateSinkIdentity( @@ -256,9 +259,9 @@ nsresult TransceiverImpl::UpdatePrincipal(nsIPrincipal* aPrincipal) { return NS_OK; } - // This blasts away the existing principal. - // We only do this when we become certain that all tracks are safe to make - // accessible to the script principal. + // This updates the existing principal. If we're setting a principal that does + // not subsume the old principal, there will be a delay while the principal + // propagates in media content, before the track has the principal aPrincipal. static_cast(mReceiveTrack->GetSource()) .SetPrincipal(aPrincipal); diff --git a/media/webrtc/signaling/src/peerconnection/TransceiverImpl.h b/media/webrtc/signaling/src/peerconnection/TransceiverImpl.h index 4d1dd2fc36c5..79bf64b372fb 100644 --- a/media/webrtc/signaling/src/peerconnection/TransceiverImpl.h +++ b/media/webrtc/signaling/src/peerconnection/TransceiverImpl.h @@ -56,7 +56,8 @@ class TransceiverImpl : public nsISupports { nsIEventTarget* aMainThread, nsIEventTarget* aStsThread, dom::MediaStreamTrack* aReceiveTrack, dom::MediaStreamTrack* aSendTrack, - WebRtcCallWrapper* aCallWrapper); + WebRtcCallWrapper* aCallWrapper, + const PrincipalHandle& aPrincipalHandle); bool IsValid() const { return !!mConduit; } @@ -133,8 +134,8 @@ class TransceiverImpl : public nsISupports { private: virtual ~TransceiverImpl(); - void InitAudio(); - void InitVideo(); + void InitAudio(const PrincipalHandle& aPrincipalHandle); + void InitVideo(const PrincipalHandle& aPrincipalHandle); nsresult UpdateAudioConduit(); nsresult UpdateVideoConduit(); nsresult ConfigureVideoCodecMode(VideoSessionConduit& aConduit);