race in Call/AudioSendStream on sending rtp packet and destruction |
|||||||
Issue descriptionThis is a clone of https://bugs.chromium.org/p/webrtc/issues/detail?id=8794, for the purpose of an M65 merge request. Merge type: Fix for concurrency regression in WebRTC metric reporting. Effect: Concurrent read + write on metric about to be reported. Prevalence: Caught by tsan on 1/1000 test runs. Daily metric reports on Chrome according to UMA (i.e. number of times where the error might occur at least once): 13,000,000. WebRTC fix + unittest CL: https://bugs.chromium.org/p/webrtc/issues/detail?id=5118 Fix verified: b/72296504#comment11 --- original bug description --- test where sending rtp packet is delayed and passed through pacer fails under tsan: WARNING: ThreadSanitizer: data race Read of size 8 at 0x7b580000e6c8 by thread T74: #0 webrtc::TimeInterval::Extend(webrtc::TimeInterval const&) webrtc/audio/time_interval.cc:40:38 #1 webrtc::internal::Call::DestroyAudioSendStream(webrtc::AudioSendStream*) webrtc/call/call.cc:654:28 ... #11 rtc::PlatformThread::StartThread(void*) webrtc/rtc_base/platform_thread.cc:146:40 Previous write of size 8 at 0x7b580000e6c8 by thread T44 #0 Extend webrtc/audio/time_interval.cc:32:23 #1 webrtc::TimeInterval::Extend() webrtc/audio/time_interval.cc:21 #2 webrtc::internal::AudioSendStream::TimedTransport::SendRtp(unsigned char const*, unsigned long, webrtc::PacketOptions const&) webrtc/audio/audio_send_stream.cc:74:18 #3 webrtc::voe::Channel::SendRtp(unsigned char const*, unsigned long, webrtc::PacketOptions const&) webrtc/audio/channel.cc:368:23 #4 non-virtual thunk to webrtc::voe::Channel::SendRtp(unsigned char const*, unsigned long, webrtc::PacketOptions const&) webrtc/audio/channel.cc #5 webrtc::RTPSender::SendPacketToNetwork(webrtc::RtpPacketToSend const&, webrtc::PacketOptions const&, webrtc::PacedPacketInfo const&) webrtc/modules/rtp_rtcp/source/rtp_sender.cc:647:30 #6 webrtc::RTPSender::PrepareAndSendPacket(std::unique_ptr<webrtc::RtpPacketToSend, std::default_delete<webrtc::RtpPacketToSend> >, bool, bool, webrtc::PacedPacketInfo const&) webrtc/modules/rtp_rtcp/source/rtp_sender.cc:785:8 #7 webrtc::RTPSender::TimeToSendPacket(unsigned int, unsigned short, long, bool, webrtc::PacedPacketInfo const&) webrtc/modules/rtp_rtcp/source/rtp_sender.cc:724:10 #8 webrtc::ModuleRtpRtcpImpl::TimeToSendPacket(unsigned int, unsigned short, long, bool, webrtc::PacedPacketInfo const&) webrtc/modules/rtp_rtcp/source/rtp_rtcp_impl.cc:454:23 #9 webrtc::PacketRouter::TimeToSendPacket(unsigned int, unsigned short, long, bool, webrtc::PacedPacketInfo const&) webrtc/modules/pacing/packet_router.cc:107:26 #10 webrtc::PacedSender::SendPacket(webrtc::PacketQueue::Packet const&, webrtc::PacedPacketInfo const&) webrtc/modules/pacing/paced_sender.cc:335:40 #11 webrtc::PacedSender::Process() webrtc/modules/pacing/paced_sender.cc:287:9 #12 webrtc::ProcessThreadImpl::Process() webrtc/modules/utility/source/process_thread_impl.cc:195:21 #13 webrtc::ProcessThreadImpl::Run(void*) webrtc/modules/utility/source/process_thread_impl.cc:169:48 #14 rtc::PlatformThread::Run() webrtc/rtc_base/platform_thread.cc:250:10 #15 rtc::PlatformThread::StartThread(void*) webrtc/rtc_base/platform_thread.cc:146:40
,
Feb 2 2018
bugdroid entry for CL: The following revision refers to this bug: https://webrtc.googlesource.com/src.git/+/06953bac6d61c4e24169c0ad4711648472bcab25 commit 06953bac6d61c4e24169c0ad4711648472bcab25 Author: Sam Zackrisson <saza@webrtc.org> Date: Thu Feb 01 16:49:39 2018 Move AudioSendStream lifetime reporting into destructor This avoids a data race in which the lifetime TimeInterval is accessed by the owning Call objects concurrently with SendRtp calls on the underlying Channel object. Bug: webrtc:8794 Change-Id: If53d5680095c0177656b659162457287cb8e45dd Reviewed-on: https://webrtc-review.googlesource.com/46525 Commit-Queue: Sam Zackrisson <saza@webrtc.org> Reviewed-by: Fredrik Solenberg <solenberg@webrtc.org> Cr-Commit-Position: refs/heads/master@{#21853} [modify] https://crrev.com/06953bac6d61c4e24169c0ad4711648472bcab25/audio/audio_send_stream.cc [modify] https://crrev.com/06953bac6d61c4e24169c0ad4711648472bcab25/audio/audio_send_stream.h [modify] https://crrev.com/06953bac6d61c4e24169c0ad4711648472bcab25/audio/audio_send_stream_unittest.cc [modify] https://crrev.com/06953bac6d61c4e24169c0ad4711648472bcab25/call/call.cc
,
Feb 5 2018
24-hour Canary: Released in 66.0.3338.0, on Feb 3. (Chrome git hash bdb350abcec69d2cb65778a7e26a7f1389a71604)
,
Feb 5 2018
Issue 808612 has been merged into this issue.
,
Feb 5 2018
Pls add appropriate OSs label. Thank you.
,
Feb 6 2018
Your change meets the bar and is auto-approved for M65. Please go ahead and merge the CL to branch 3325 manually. Please contact milestone owner if you have questions. Owners: cmasso@(Android), cmasso@(iOS), bhthompson@(ChromeOS), govind@(Desktop) For more details visit https://www.chromium.org/issue-tracking/autotriage - Your friendly Sheriffbot
,
Feb 6 2018
Blanket adding all OSs, since there's nothing platform-dependent about the regression.
,
Feb 6 2018
The following revision refers to this bug: https://webrtc.googlesource.com/src.git/+/3047145d4ac1665f11cc9af081d6e9cfd2d79c6f commit 3047145d4ac1665f11cc9af081d6e9cfd2d79c6f Author: Sam Zackrisson <saza@webrtc.org> Date: Tue Feb 06 16:04:11 2018 [MERGE TO M65] Move AudioSendStream lifetime reporting into destructor This avoids a data race in which the lifetime TimeInterval is accessed by the owning Call objects concurrently with SendRtp calls on the underlying Channel object. TBR=solenberg (cherry picked from commit 06953bac6d61c4e24169c0ad4711648472bcab25) Bug: chromium:808364 , webrtc:8794 Change-Id: If53d5680095c0177656b659162457287cb8e45dd Reviewed-on: https://webrtc-review.googlesource.com/46525 Commit-Queue: Sam Zackrisson <saza@webrtc.org> Reviewed-by: Fredrik Solenberg <solenberg@webrtc.org> Cr-Original-Commit-Position: refs/heads/master@{#21853} Reviewed-on: https://webrtc-review.googlesource.com/48521 Reviewed-by: Henrik Lundin <henrik.lundin@webrtc.org> Cr-Commit-Position: refs/branch-heads/65@{#10} Cr-Branched-From: 3ac67a736bb200ecf7c116a88b2f8d5c542973c8-refs/heads/master@{#21637} [modify] https://crrev.com/3047145d4ac1665f11cc9af081d6e9cfd2d79c6f/audio/audio_send_stream.cc [modify] https://crrev.com/3047145d4ac1665f11cc9af081d6e9cfd2d79c6f/audio/audio_send_stream.h [modify] https://crrev.com/3047145d4ac1665f11cc9af081d6e9cfd2d79c6f/audio/audio_send_stream_unittest.cc [modify] https://crrev.com/3047145d4ac1665f11cc9af081d6e9cfd2d79c6f/call/call.cc
,
Feb 6 2018
|
|||||||
►
Sign in to add a comment |
|||||||
Comment 1 by saza@chromium.org
, Feb 2 2018