New issue
Advanced search Search tips
Note: Color blocks (like or ) mean that a user may not be available. Tooltip shows the reason.

Issue 808364 link

Starred by 1 user

Issue metadata

Status: Verified
Owner:
Last visit 19 days ago
Closed: Feb 2018
Cc:
Components:
EstimatedDays: ----
NextAction: ----
OS: Linux , Android , Windows , iOS , Chrome , Mac , Fuchsia
Pri: 1
Type: Bug-Regression



Sign in to add a comment

race in Call/AudioSendStream on sending rtp packet and destruction

Project Member Reported by saza@chromium.org, Feb 2 2018

Issue description

This 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
 
 

Comment 1 by saza@chromium.org, Feb 2 2018

Labels: -Merge-Request-65

Comment 2 by saza@chromium.org, Feb 2 2018

Status: Verified (was: Started)
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

Comment 3 by saza@chromium.org, Feb 5 2018

Labels: Merge-Request-65
24-hour Canary:

Released in 66.0.3338.0, on Feb 3.
(Chrome git hash bdb350abcec69d2cb65778a7e26a7f1389a71604)

Comment 4 by saza@chromium.org, Feb 5 2018

 Issue 808612  has been merged into this issue.
Pls add appropriate OSs label. Thank you.
Project Member

Comment 6 by sheriffbot@chromium.org, Feb 6 2018

Labels: -Merge-Request-65 Hotlist-Merge-Approved Merge-Approved-65
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

Comment 7 by saza@chromium.org, Feb 6 2018

Labels: OS-Android OS-Chrome OS-Fuchsia OS-iOS OS-Linux OS-Mac OS-Windows
Blanket adding all OSs, since there's nothing platform-dependent about the regression.
Project Member

Comment 8 by bugdroid1@chromium.org, Feb 6 2018

Labels: merge-merged-65
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

Comment 9 by saza@chromium.org, Feb 6 2018

Labels: -Merge-Approved-65

Sign in to add a comment