Assigning to test author. PTAL

Removing Sheriff label. If label is re-added due to additional flakes I plan to disable. 

Detected 3 new flakes for test/step "RTCRtpSenderTest.CopiedSenderSharesInternalStates". To see the actual flakes, please visit https://chromium-try-flakes.appspot.com/all_flake_occurrences?key=ahVzfmNocm9taXVtLXRyeS1mbGFrZXNyPAsSBUZsYWtlIjFSVENSdHBTZW5kZXJUZXN0LkNvcGllZFNlbmRlclNoYXJlc0ludGVybmFsU3RhdGVzDA. This message was posted automatically by the chromium-try-flakes app. Since flakiness is ongoing, the issue was moved back into Sheriff Bug Queue (unless already there).

Will proceed to disable the test since this is still flaking.

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/5c850ffbd63047b96cb4a1dd738ba09c4b9e9c4d

commit 5c850ffbd63047b96cb4a1dd738ba09c4b9e9c4d
Author: Karan Bhatia <karandeepb@chromium.org>
Date: Mon Mar 19 18:28:39 2018

Disable flaky RTCRtpSenderTest.CopiedSenderSharesInternalStates.

BUG= 812296 
TBR=sergeyu@chromium.org

Change-Id: I7992566c41b6ab650738d190a6980c8640b29487
Reviewed-on: https://chromium-review.googlesource.com/969096
Commit-Queue: Karan Bhatia <karandeepb@chromium.org>
Reviewed-by: Karan Bhatia <karandeepb@chromium.org>
Cr-Commit-Position: refs/heads/master@{#544073}
[modify] https://crrev.com/5c850ffbd63047b96cb4a1dd738ba09c4b9e9c4d/content/renderer/media/webrtc/rtc_rtp_sender_unittest.cc

I looked at this more recently but I was unable to figure out what was causing the flakes. I think it's test related, because these are tested in integrationtests and in practice without problems. I couldn't repro locally even if I ran the test a 1000 times. I don't have time to investigate further, this is all covered by other tests and running some of the tests on some platforms is good.

Actually I know what is causing it.

Stack trace:

logging::LogMessage::~LogMessage()
content::WebRtcMediaStreamAdapterMap::~WebRtcMediaStreamAdapterMap()
content::WebRtcMediaStreamAdapterMap::~WebRtcMediaStreamAdapterMap()
scoped_refptr<net::IOBuffer>::~scoped_refptr()
content::RTCRtpSender::RTCRtpSenderInternal::~RTCRtpSenderInternal()
content::RTCRtpSender::RTCRtpSenderInternal::~RTCRtpSenderInternal()
scoped_refptr<net::IOBuffer>::~scoped_refptr()
base::internal::BindState<void (content::RTCRtpSender::RTCRtpSenderInternal::*)(std::__ndk1::unique_ptr<content::WebRtcMediaStreamTrackAdapterMap::AdapterRef, std::__ndk1::default_delete<content::WebRtcMediaStreamTrackAdapterMap::AdapterRef> >, webrtc::MediaStreamTrackInterface*, base::OnceCallback<void (bool)>), scoped_refptr<content::RTCRtpSender::RTCRtpSenderInternal>, std::__ndk1::unique_ptr<content::WebRtcMediaStreamTrackAdapterMap::AdapterRef, std::__ndk1::default_delete<content::WebRtcMediaStreamTrackAdapterMap::AdapterRef> >, base::internal::UnretainedWrapper<webrtc::MediaStreamTrackInterface>, base::OnceCallback<void (bool)> >::~BindState()
base::internal::BindState<void (content::RTCRtpSender::RTCRtpSenderInternal::*)(std::__ndk1::unique_ptr<content::WebRtcMediaStreamTrackAdapterMap::AdapterRef, std::__ndk1::default_delete<content::WebRtcMediaStreamTrackAdapterMap::AdapterRef> >, webrtc::MediaStreamTrackInterface*, base::OnceCallback<void (bool)>), scoped_refptr<content::RTCRtpSender::RTCRtpSenderInternal>, std::__ndk1::unique_ptr<content::WebRtcMediaStreamTrackAdapterMap::AdapterRef, std::__ndk1::default_delete<content::WebRtcMediaStreamTrackAdapterMap::AdapterRef> >, base::internal::UnretainedWrapper<webrtc::MediaStreamTrackInterface>, base::OnceCallback<void (bool)> >::Destroy(base::internal::BindStateBase const*)
base::internal::CallbackBase::~CallbackBase()
base::debug::TaskAnnotator::RunTask(char const*, base::PendingTask*)
base::internal::IncomingTaskQueue::RunTask(base::PendingTask*)
base::MessageLoop::RunTask(base::PendingTask*)
base::MessageLoop::DeferOrRunPendingTask(base::PendingTask)
base::MessageLoop::DoWork()
base::MessagePumpDefault::Run(base::MessagePump::Delegate*)
base::MessageLoop::Run(bool)
base::RunLoop::Run()
base::Thread::Run(base::RunLoop*)
base::Thread::ThreadMain()
base::(anonymous namespace)::ThreadFunc(void*)

Both the test and the sender has a reference keeping the map alive.
The fact that the map is destroyed when the sender is destroyed means the sender was destroyed after the test was destroyed.

This should not happen, and the test waits for the sender to complete its operation before it shuts down and frees its reference. So why does it?
Here's the race: the base::BindOnce(&RTCRtpSender::RTCRtpSenderInternal::ReplaceTrackOnSignalingThread) keeps the sender alive, the callback from performing this operation signals "replace track completed", which unblocks the thread that the test is running on. In rare cases the test is able to shut down before the base::BindOnce() call has freed the reference that it holds to the sender as part of binding it.

When this race occurs, the sender is the last reference to the map, and the last sender reference is released on the signaling thread, which means the map is destroyed on the signaling thread. There's a DCHECK that is it destroyed on the main thread.

Knowing the cause, fixing this issue would require little resources, bumping up the priority to P-2 again. But I have higher priority items in my queue before I fix this.

Issue 800465 has been merged into this issue.

I propose to remove DCHECK that we are on the main thread here:
https://cs.chromium.org/chromium/src/content/renderer/media/webrtc/webrtc_media_stream_adapter_map.cc?type=cs&q=~WebRtcMediaStreamAdapterMap&sq=package:chromium&l=80
And add DCHECK(remote_stream_adapters_.empty()) - there is already a DCHECK for local_stream_adapters_.

It is the adapters that are required to be released on the main thread, not the map.

However it is still a problem if the RTCRtpSenderInternal is destroyed on another thread than the main thread, so we should DCHECK here:
https://cs.chromium.org/chromium/src/content/renderer/media/webrtc/rtc_rtp_sender.cc?type=cs&sq=package:chromium&l=145

We could make ~RTCRtpSenderInternal post its adapter refs to the main thread on destruction to ensure they are destroyed on the main thread (if not already on the main thread). But there would still be a race if the test is tearing down, it would tear down this thread?

The test should be updated as to not unblock the main/test thread until the internal reference has been released. This could be achieved with another post.

Latest M66 - 66.0.3359.67 is failing @ Unit Test: content_unittests 
[CRASH] RTCRtpSenderTest.ReplaceTrackCanFail:

Build:66.0.3359.67
https://uberchromegw.corp.google.com/i/official.android/builders/official-arm/builds/3046

Build link:
https://uberchromegw.corp.google.com/i/official.android/builders/test-official-arm/builds/2568

Unit Test: content_unittests link: 
https://uberchromegw.corp.google.com/i/official.android/builders/test-official-arm/builds/2568/steps/Unit%20Test%3A%20content_unittests/logs/stdio

Detailed Logs
C  107.406s Main  ********************************************************************************
C  107.412s Main  [CRASH] RTCRtpSenderTest.ReplaceTrackCanFail:
C  107.412s Main  [ RUN      ] RTCRtpSenderTest.ReplaceTrackCanFail
C  107.412s Main  [ERROR:resource_bundle_android.cc(52)] Failed to open pak file: assets/chrome_100_percent.pak
C  107.412s Main  [WARNING:resource_bundle_android.cc(124)] locale_file_path.empty() for locale 
C  107.412s Main  [ CRASHED      ]
C  107.412s Main  ********************************************************************************
C  107.412s Main  Summary
C  107.412s Main  ********************************************************************************
C  107.422s Main  [==========] 5267 tests ran.
C  107.422s Main  [  PASSED  ] 5266 tests.
C  107.422s Main  [  FAILED  ] 1 test, listed below:
C  107.422s Main  [  FAILED  ] RTCRtpSenderTest.ReplaceTrackCanFail (CRASHED)
C  107.422s Main  
C  107.422s Main  1 FAILED TEST