Closed Bug 1319489 Opened 7 years ago Closed 7 years ago

port mediapipeline_unittest to xul gtest

Categories

(Core :: WebRTC: Audio/Video, defect, P1)

defect

Tracking

()

RESOLVED FIXED
Tracking Status
firefox53 --- affected

People

(Reporter: dminor, Assigned: dminor)

References

Details

Attachments

(7 files, 1 obsolete file)

      No description provided.
Status: NEW → ASSIGNED
Rank: 17
Due to the way these both depend upon fake media streams, it isn't practical to port them separately like I was able to do for the other signaling unit tests.
Summary: port mediapipeline_unittest to xul gtest → port mediapipeline_unittest and signaling_unittests to xul gtest
No longer blocks: 1316611
Since we have temporarily stopped building mediapipeline_unittest and signaling_unittests, it is now practical to port these one at a time.
Summary: port mediapipeline_unittest and signaling_unittests to xul gtest → port mediapipeline_unittest to xul gtest
Depends on: 848189
No longer depends on: 848189
Comment on attachment 8825872 [details]
Bug 1319489 - Remove USE_FAKE_MEDIA_STREAMS;

https://reviewboard.mozilla.org/r/103932/#review104646
Attachment #8825872 - Flags: review?(rjesup) → review+
Comment on attachment 8825871 [details]
Bug 1319489 - Stop building signalingtest libraries;

https://reviewboard.mozilla.org/r/103930/#review104648
Attachment #8825871 - Flags: review?(rjesup) → review+
Comment on attachment 8825875 [details]
Bug 1319489 - move mediapipeline_unittest to xul-gtest;

https://reviewboard.mozilla.org/r/103938/#review104650

::: media/webrtc/signaling/gtest/mediapipeline_unittest.cpp:505
(Diff revision 1)
> -    mozilla::SyncRunnable::DispatchToThread(
> -      test_utils->sts_target(),
> -      WrapRunnable(&p2_, &TestAgent::CreatePipelines_s, aIsRtcpMux));
> +    NS_DispatchToMainThread(
> +      WrapRunnable(&p2_, &TestAgent::CreatePipelines_s, aIsRtcpMux),
> +      NS_DISPATCH_SYNC);

This looks wrong... CreatePipelines_s runs on STS thread, not MainThread
Attachment #8825875 - Flags: review?(rjesup) → review-
Comment on attachment 8825874 [details]
Bug 1319489 - Make it possible for "fake" media streams to inherit from real streams;

https://reviewboard.mozilla.org/r/103936/#review104654

::: media/webrtc/signaling/src/mediapipeline/MediaPipeline.cpp:1704
(Diff revision 1)
> +  size_t graph_rate = 16000;
> +  if (graph) {
> +    graph_rate = graph->GraphRate();
> +  }

nit: (minor, personal preference)
I'd tend to avoid always double-assigning it:
size_t rate;
if (graph) {
  rate = graph->foo();
} else {
  // When running tests, graph may be null. In that case use a default.
  rate = some_define_for_default_rate;
}

Default rate for the graph is usually 44100 or 48000 now, but the tests may assume 16000!
Attachment #8825874 - Flags: review?(rjesup) → review+
(In reply to Randell Jesup [:jesup] from comment #12)
> Comment on attachment 8825875 [details]
> Bug 1319489 - move mediapipeline_unittest to xul-gtest;
> 
> https://reviewboard.mozilla.org/r/103938/#review104650
> 
> ::: media/webrtc/signaling/gtest/mediapipeline_unittest.cpp:505
> (Diff revision 1)
> > -    mozilla::SyncRunnable::DispatchToThread(
> > -      test_utils->sts_target(),
> > -      WrapRunnable(&p2_, &TestAgent::CreatePipelines_s, aIsRtcpMux));
> > +    NS_DispatchToMainThread(
> > +      WrapRunnable(&p2_, &TestAgent::CreatePipelines_s, aIsRtcpMux),
> > +      NS_DISPATCH_SYNC);
> 
> This looks wrong... CreatePipelines_s runs on STS thread, not MainThread

If I try to run this on sts_thread, I hit this assertion: 

# Fatal error in /home/dminor/src/firefox-asan/media/webrtc/trunk/webrtc/modules/audio_coding/acm2/codec_manager.cc, line 76
# Check failed: thread_checker_.CalledOnValidThread()

The thread ownership is established a few lines up in the CodecManager constructor [1]. This constructor is called as part of creating the AudioConduit, which has an assertion here [2] that means that it can only be called on the main thread.

The test as originally written creates p1 on the main thread and p2 on the sts thread, so it doesn't seem to be incorrect to initialize this on the main thread. It appears that CodecManager is new code from the branch 49 update, so it is not surprising that the test behaviour has changed here.

Unless I'm missing something, I don't think it's possible for me to move this to the sts_thread. Please let me know if I am missing something :)

[1] https://hg.mozilla.org/mozilla-central/file/tip/media/webrtc/trunk/webrtc/modules/audio_coding/acm2/codec_manager.cc#l70
[2] http://searchfox.org/mozilla-central/rev/0aed9484bd3e97206fd1949ee4a4992ef300a81f/media/webrtc/signaling/src/media-conduit/AudioConduit.cpp#51
(In reply to Dan Minor [:dminor] from comment #14)

> The thread ownership is established a few lines up in the CodecManager
> constructor [1]. This constructor is called as part of creating the
> AudioConduit, which has an assertion here [2] that means that it can only be
> called on the main thread.
> 
> The test as originally written creates p1 on the main thread and p2 on the
> sts thread, so it doesn't seem to be incorrect to initialize this on the
> main thread. It appears that CodecManager is new code from the branch 49
> update, so it is not surprising that the test behaviour has changed here.

The threading restrictions got tighter when we landed the 43 update; they used to use critsects in a lot of places, and they removed a bunch and replaced them with thread DCHECKs.  Lots of stuff now needs to always run on the thread it's first used on
Comment on attachment 8825873 [details]
Bug 1319489 - Remove MOZILLA_EXTERNAL_LINKAGE;

https://reviewboard.mozilla.org/r/103934/#review106726

::: media/webrtc/signaling/src/peerconnection/PeerConnectionImpl.h:648
(Diff revision 1)
>      // Thread assertions are disabled in the C++ unit tests because those
>      // make API calls off the main thread.
>      // This affects the standalone version of WebRTC since it is also used
>      // for an alternate build of the unit tests.
>      // TODO(ekr@rtfm.com): Fix the unit tests so they don't do that.

Probsbly this comment can go too
Attachment #8825873 - Flags: review?(rjesup) → review+
Comment on attachment 8825875 [details]
Bug 1319489 - move mediapipeline_unittest to xul-gtest;

https://reviewboard.mozilla.org/r/103938/#review106730

::: media/webrtc/signaling/gtest/mediapipeline_unittest.cpp:505
(Diff revision 2)
> -    mozilla::SyncRunnable::DispatchToThread(
> -      test_utils->sts_target(),
> -      WrapRunnable(&p2_, &TestAgent::CreatePipelines_s, aIsRtcpMux));
> +    NS_DispatchToMainThread(
> +      WrapRunnable(&p2_, &TestAgent::CreatePipelines_s, aIsRtcpMux),
> +      NS_DISPATCH_SYNC);
>  

Still need to resolve the review- on the CreatePipelines_s issue
Attachment #8825875 - Flags: review?(rjesup) → review-
(In reply to Randell Jesup [:jesup] from comment #19)
> Comment on attachment 8825875 [details]
> Bug 1319489 - move mediapipeline_unittest to xul-gtest;
> 
> https://reviewboard.mozilla.org/r/103938/#review106730
> 
> ::: media/webrtc/signaling/gtest/mediapipeline_unittest.cpp:505
> (Diff revision 2)
> > -    mozilla::SyncRunnable::DispatchToThread(
> > -      test_utils->sts_target(),
> > -      WrapRunnable(&p2_, &TestAgent::CreatePipelines_s, aIsRtcpMux));
> > +    NS_DispatchToMainThread(
> > +      WrapRunnable(&p2_, &TestAgent::CreatePipelines_s, aIsRtcpMux),
> > +      NS_DISPATCH_SYNC);
> >  
> 
> Still need to resolve the review- on the CreatePipelines_s issue

It is not possible to create these on the sts_thread. CodecManager is first created as part of creating the AudioSessionConduit, which must be done on the main thread (there is an assertion to ensure this.) Running CreatePipeline_s from sts_thread attempts to access the CodecManager from a different thread that it was created on, which triggers an assertion.
(In reply to Dan Minor [:dminor] from comment #20)
> > Still need to resolve the review- on the CreatePipelines_s issue
> 
> It is not possible to create these on the sts_thread. CodecManager is first
> created as part of creating the AudioSessionConduit, which must be done on
> the main thread (there is an assertion to ensure this.) Running
> CreatePipeline_s from sts_thread attempts to access the CodecManager from a
> different thread that it was created on, which triggers an assertion.

That says to me that we're invoking some other function inbetween on MainThread (which locks in the DCHECK test to that thread), which in normal usage is on STS thread.

In the tests, we don't want to run majorly different threading models than the code we run in the browser.

A stack backtrace of the assertion (and maybe the creation) would help. Also, breakpoint the assertion point and try in the browser (or run in rr and backtrack to where it locks in the thread for comparison).
Here are some backtraces for creation in the unit test and when doing an appear.in call to another Firefox. On appear.in, there are two separate CodecManager instances, one off main thread for the microphone, and one on main thread. The main thread backtrace matches the backtrace for the unit test case.

I've also attached a backtrace for when we hit the assertion in the unit test if we try to run CreatePipelines_s on the sts_thread.

creation (unittest):

#0  webrtc::acm2::CodecManager::CodecManager (
    this=0x625000098a90)
    at /home/dminor/src/firefox-asan/media/webrtc/trunk/webrtc/modules/audio_coding/acm2/codec_manager.cc:70
#1  0x00007fffdf5040f1 in webrtc::acm2::AudioCodingModuleImpl::AudioCodingModuleImpl (this=0x625000098900, config=...)
    at /home/dminor/src/firefox-asan/media/webrtc/trunk/webrtc/modules/audio_coding/acm2/audio_coding_module_impl.cc:104
#2  0x00007fffdf503418 in webrtc::AudioCodingModule::Create (
    config=...)
    at /home/dminor/src/firefox-asan/media/webrtc/trunk/webrtc/modules/audio_coding/acm2/audio_coding_module.cc:38
#3  0x00007fffdf5a8af7 in webrtc::voe::Channel::Channel (
    this=0x626000003100, channelId=20804, 
    instanceId=<optimized out>, event_log=0x6260000050b8, 
    config=...)
    at /home/dminor/src/firefox-asan/media/webrtc/trunk/webrtc/voice_engine/channel.cc:917
#4  0x00007fffdf5c1ee9 in webrtc::voe::Channel::CreateChannel (
    channelId=0, instanceId=1, event_log=0x6020000d8e10, 
    config=..., channel=<optimized out>)
    at /home/dminor/src/firefox-asan/media/webrtc/trunk/webrtc/voice_engine/channel.cc:736
#5  webrtc::voe::ChannelManager::CreateChannelInternal (
    this=0x6250000a2918, config=...)
    at /home/dminor/src/firefox-asan/media/webrtc/trunk/webrtc/voice_engine/channel_manager.cc:65
#6  0x00007fffdf5e188a in webrtc::voe::ChannelManager::CreateChannel (this=0x1)
    at /home/dminor/src/firefox-asan/media/webrtc/trunk/webrtc/voice_engine/channel_manager.cc:56
#7  webrtc::VoEBaseImpl::CreateChannel (this=<optimized out>)
    at /home/dminor/src/firefox-asan/media/webrtc/trunk/webrtc/voice_engine/voe_base_impl.cc:396
#8  0x00007fffe02dc767 in mozilla::WebrtcAudioConduit::Init (
    this=<optimized out>)
    at /home/dminor/src/firefox-asan/media/webrtc/signaling/src/media-conduit/AudioConduit.cpp:342
#9  0x00007fffe02dc132 in mozilla::AudioSessionConduit::Create
    ()
    at /home/dminor/src/firefox-asan/media/webrtc/signaling/src/media-conduit/AudioConduit.cpp:52
#10 0x00007fffe1e0da2b in (anonymous namespace)::TestAgent::TestAgent (this=0x613000004f10)
    at /home/dminor/src/firefox-asan/media/webrtc/signaling/gtest/mediapipeline_unittest.cpp:250
#11 0x00007fffe1e0d578 in (anonymous namespace)::TestAgentSend::TestAgentSend (this=<optimized out>)
    at /home/dminor/src/firefox-asan/media/webrtc/signaling/gtest/mediapipeline_unittest.cpp:351
#12 (anonymous namespace)::MediaPipelineTest::MediaPipelineTest
    (this=<optimized out>)
    at /home/dminor/src/firefox-asan/media/webrtc/signaling/gtest/mediapipeline_unittest.cpp:446
#13 (anonymous namespace)::MediaPipelineTest_TestAudioSendNoMux_Test::MediaPipelineTest_TestAudioSendNoMux_Test (
    this=<optimized out>)
    at /home/dminor/src/firefox-asan/media/webrtc/signaling/gtest/mediapipeline_unittest.cpp:766

assertion (unittest):

#0  0x00007ffff6b79428 in __GI_raise (sig=sig@entry=6)
    at ../sysdeps/unix/sysv/linux/raise.c:54
#1  0x00007ffff6b7b02a in __GI_abort () at abort.c:89
#2  0x00007fffdf513920 in rtc::FatalMessage::~FatalMessage (
    this=0x7fffcea304e0)
    at /home/dminor/src/firefox-asan/media/webrtc/trunk/webrtc/base/checks.cc:104
#3  0x00007fffdf4fca54 in webrtc::acm2::CodecManager::RegisterEncoder (this=0x625000098a90, send_codec=...)
    at /home/dminor/src/firefox-asan/media/webrtc/trunk/webrtc/modules/audio_coding/acm2/codec_manager.cc:76
#4  0x00007fffdf5061f0 in webrtc::acm2::AudioCodingModuleImpl::RegisterSendCodec (this=0x625000098900, send_codec=...)
    at /home/dminor/src/firefox-asan/media/webrtc/trunk/webrtc/modules/audio_coding/acm2/audio_coding_module_impl.cc:201
#5  0x00007fffdf5acd68 in webrtc::voe::Channel::SetSendCodec (
    this=0x626000003100, codec=...)
    at /home/dminor/src/firefox-asan/media/webrtc/trunk/webrtc/voice_engine/channel.cc:1407
#6  0x00007fffdf5e5a67 in webrtc::VoECodecImpl::SetSendCodec (
    this=<optimized out>, channel=-828176384, codec=...)
    at /home/dminor/src/firefox-asan/media/webrtc/trunk/webrtc/voice_engine/voe_codec_impl.cc:105
#7  0x00007fffe02df99f in mozilla::WebrtcAudioConduit::ConfigureSendMediaCodec (this=<optimized out>, 
    codecConfig=0x613000004f18)
    at /home/dminor/src/firefox-asan/media/webrtc/signaling/src/media-conduit/AudioConduit.cpp:424
#8  0x00007fffe1e0dd6a in (anonymous namespace)::TestAgentSend::CreatePipelines_s (this=0x613000004f10, 
    aIsRtcpMux=<error reading variable: access outside bounds of object referenced via synthetic pointer>)
    at /home/dminor/src/firefox-asan/media/webrtc/signaling/gtest/mediapipeline_unittest.cpp:355


creation (appear.in, not on main thread):

#0  webrtc::acm2::CodecManager::CodecManager (
    this=0x6250015eca90)
    at /home/dminor/src/firefox-asan/media/webrtc/trunk/webrtc/modules/audio_coding/acm2/codec_manager.cc:70
#1  0x00007fffe0af22c1 in webrtc::acm2::AudioCodingModuleImpl::AudioCodingModuleImpl (this=0x6250015ec900, config=...)
    at /home/dminor/src/firefox-asan/media/webrtc/trunk/webrtc/modules/audio_coding/acm2/audio_coding_module_impl.cc:104
#2  0x00007fffe0af15e8 in webrtc::AudioCodingModule::Create (
    config=...)
    at /home/dminor/src/firefox-asan/media/webrtc/trunk/webrtc/modules/audio_coding/acm2/audio_coding_module.cc:38
#3  0x00007fffe0b96cc7 in webrtc::voe::Channel::Channel (
    this=0x62600015f100, channelId=1446212, 
    instanceId=<optimized out>, event_log=0x6260001610b8, 
    config=...)
    at /home/dminor/src/firefox-asan/media/webrtc/trunk/webrtc/voice_engine/channel.cc:917
#4  0x00007fffe0bb00b9 in webrtc::voe::Channel::CreateChannel (
    channelId=0, instanceId=1, event_log=0x60200037a970, 
    config=..., channel=<optimized out>)
    at /home/dminor/src/firefox-asan/media/webrtc/trunk/webrtc/voice_engine/channel.cc:736
#5  webrtc::voe::ChannelManager::CreateChannelInternal (
    this=0x6250015f6918, config=...)
    at /home/dminor/src/firefox-asan/media/webrtc/trunk/webrtc/voice_engine/channel_manager.cc:65
#6  0x00007fffe0bcfa5a in webrtc::voe::ChannelManager::CreateChannel (this=0x1)
    at /home/dminor/src/firefox-asan/media/webrtc/trunk/webrtc/voice_engine/channel_manager.cc:56
#7  webrtc::VoEBaseImpl::CreateChannel (this=<optimized out>)
    at /home/dminor/src/firefox-asan/media/webrtc/trunk/webrtc/voice_engine/voe_base_impl.cc:396
#8  0x00007fffdeaf811b in mozilla::MediaEngineWebRTCMicrophoneSource::AllocChannel (this=0x6160004d7980)
    at /home/dminor/src/firefox-asan/dom/media/webrtc/MediaEngineWebRTCAudio.cpp:736
#9  0x00007fffdeaf6e0b in mozilla::MediaEngineWebRTCMicrophoneSource::UpdateSingleSource (this=0x6160004d7980, 
    aHandle=0x619000849e80, aNetConstraints=..., aPrefs=..., 
    aDeviceId=..., aOutBadConstraint=<optimized out>)
    at /home/dminor/src/firefox-asan/dom/media/webrtc/MediaEngineWebRTCAudio.cpp:306
#10 0x00007fffdeadf71d in mozilla::MediaEngineSource::ReevaluateAllocation (this=0x6160004d7980, aHandle=<optimized out>, 
    aConstraintsUpdate=0x0, aPrefs=..., aDeviceId=..., 
    aOutBadConstraint=0x7fff9a697920)
    at /home/dminor/src/firefox-asan/dom/media/webrtc/MediaEngine.h:427
#11 0x00007fffdeaded46 in mozilla::MediaEngineSource::Allocate
    (this=<optimized out>, aConstraints=..., aPrefs=..., 
    aDeviceId=..., aOrigin=..., aOutHandle=<optimized out>, 
    aOutBadConstraint=0x1a)
    at /home/dminor/src/firefox-asan/dom/media/webrtc/MediaEngine.h:336
#12 0x00007fffde623115 in mozilla::MediaDevice::Allocate (
    this=<optimized out>, aConstraints=..., aPrefs=..., 
    aOrigin=..., aOutBadConstraint=<optimized out>)
    at /home/dminor/src/firefox-asan/dom/media/MediaManager.cpp:879
#13 0x00007fffde6ed49d in mozilla::GetUserMediaTask::Run (
    this=<optimized out>)
    at /home/dminor/src/firefox-asan/dom/media/MediaManager.cpp:1438
#14 0x00007fffd943e085 in nsThread::ProcessNextEvent (
    this=<optimized out>, aMayWait=<optimized out>, 
    aResult=<optimized out>)
    at /home/dminor/src/firefox-asan/xpcom/threads/nsThread.cpp:1240


creation (2, appear.in, must be on main thread)

#0  webrtc::acm2::CodecManager::CodecManager (
    this=0x62500196ba90)
    at /home/dminor/src/firefox-asan/media/webrtc/trunk/webrtc/modules/audio_coding/acm2/codec_manager.cc:70
#1  0x00007fffe0af22c1 in webrtc::acm2::AudioCodingModuleImpl::AudioCodingModuleImpl (this=0x62500196b900, config=...)
    at /home/dminor/src/firefox-asan/media/webrtc/trunk/webrtc/modules/audio_coding/acm2/audio_coding_module_impl.cc:104
#2  0x00007fffe0af15e8 in webrtc::AudioCodingModule::Create (
    config=...)
    at /home/dminor/src/firefox-asan/media/webrtc/trunk/webrtc/modules/audio_coding/acm2/audio_coding_module.cc:38
#3  0x00007fffe0b96cc7 in webrtc::voe::Channel::Channel (
    this=0x6260002f4100, channelId=3105092, 
    instanceId=<optimized out>, event_log=0x6260002f60b8, 
    config=...)
    at /home/dminor/src/firefox-asan/media/webrtc/trunk/webrtc/voice_engine/channel.cc:917
#4  0x00007fffe0bb00b9 in webrtc::voe::Channel::CreateChannel (
    channelId=0, instanceId=2, event_log=0x602000170390, 
    config=..., channel=<optimized out>)
    at /home/dminor/src/firefox-asan/media/webrtc/trunk/webrtc/voice_engine/channel.cc:736
#5  webrtc::voe::ChannelManager::CreateChannelInternal (
    this=0x625001975918, config=...)
    at /home/dminor/src/firefox-asan/media/webrtc/trunk/webrtc/voice_engine/channel_manager.cc:65
#6  0x00007fffe0bcfa5a in webrtc::voe::ChannelManager::CreateChannel (this=0x1)
    at /home/dminor/src/firefox-asan/media/webrtc/trunk/webrtc/voice_engine/channel_manager.cc:56
#7  webrtc::VoEBaseImpl::CreateChannel (this=<optimized out>)
    at /home/dminor/src/firefox-asan/media/webrtc/trunk/webrtc/voice_engine/voe_base_impl.cc:396
#8  0x00007fffe18ca937 in mozilla::WebrtcAudioConduit::Init (
    this=<optimized out>)
    at /home/dminor/src/firefox-asan/media/webrtc/signaling/src/media-conduit/AudioConduit.cpp:342
#9  0x00007fffe18ca302 in mozilla::AudioSessionConduit::Create
    ()
    at /home/dminor/src/firefox-asan/media/webrtc/signaling/src/media-conduit/AudioConduit.cpp:52
#10 0x00007fffe191667d in mozilla::MediaPipelineFactory::GetOrCreateAudioConduit (this=0x7fffffff3200, aTrackPair=..., 
    aTrack=..., aConduitp=<optimized out>)
    at /home/dminor/src/firefox-asan/media/webrtc/signaling/src/peerconnection/MediaPipelineFactory.cpp:682
#11 0x00007fffe1913fa9 in mozilla::MediaPipelineFactory::CreateOrUpdateMediaPipeline (this=0x7fffffff3200, aTrackPair=..., 
    aTrack=...)
    at /home/dminor/src/firefox-asan/media/webrtc/signaling/src/peerconnection/MediaPipelineFactory.cpp:439
#12 0x00007fffe1973cd7 in mozilla::PeerConnectionMedia::UpdateMediaPipelines (this=<optimized out>, session=...)
    at /home/dminor/src/firefox-asan/media/webrtc/signaling/src/peerconnection/PeerConnectionMedia.cpp:569
#13 0x00007fffe194c938 in mozilla::PeerConnectionImpl::SetSignalingState_m (this=0x616000066f80, 
    aSignalingState=<optimized out>, 
    rollback=<error reading variable: access outside bounds of object referenced via synthetic pointer>)
    at /home/dminor/src/firefox-asan/media/webrtc/signaling/src/peerconnection/PeerConnectionImpl.cpp:3023
#14 0x00007fffe193dba7 in mozilla::PeerConnectionImpl::SetLocalDescription (this=<optimized out>, aAction=1, 
    aSDP=0x61d0006d9c88 "v=0\r\no=mozilla...THIS_IS_SDPARTA-53.0a1 6704299056158655396 0 IN IP4 0.0.0.0\r\ns=-\r\nt=0 0\r\na=fingerprint:sha-256 65:1C:B3:C4:E3:42:D7:69:02:9F:8A:52:01:4A:36:F5:58:03:4A:85:5E:31:E3:BF:A2:EC:70:FB:A1:F"...)
    at /home/dminor/src/firefox-asan/media/webrtc/signaling/src/
peerconnection/PeerConnectionImpl.cpp:1735
#15 0x00007fffdc7c0837 in mozilla::PeerConnectionImpl::SetLocalDescription (this=0x616000066f80, aAction=1, aSDP=..., rv=...)
    at /home/dminor/src/firefox-asan/media/webrtc/signaling/src/peerconnection/PeerConnectionImpl.h:356
#16 0x00007fffdc775ebe in mozilla::dom::PeerConnectionImplBinding::setLocalDescription (cx=0x62d00006e400, obj=..., 
    self=<optimized out>, args=...)
    at /home/dminor/src/firefox-asan/objdir-ff-asan/dom/bindings/PeerConnectionImplBinding.cpp:189
#17 0x00007fffddc43487 in mozilla::dom::GenericBindingMethod (
    cx=0x7fffe749d744 <webrtc::acm2::ACMCodecDB::database_+1156>, argc=<optimized out>, vp=<optimized out>)
    at /home/dminor/src/firefox-asan/dom/bindings/BindingUtils.cpp:2914
#18 0x00007fffe30702e5 in js::CallJSNative (
    native=<optimized out>, args=..., cx=<optimized out>)
    at /home/dminor/src/firefox-asan/js/src/jscntxtinlines.h:239
#19 js::InternalCallOrConstruct (cx=<optimized out>, args=..., 
    construct=js::NO_CONSTRUCT)
    at /home/dminor/src/firefox-asan/js/src/vm/Interpreter.cpp:457
#20 0x00007fffe3050bb3 in js::CallFromStack (
    cx=0x62d00006e400, args=...)
    at /home/dminor/src/firefox-asan/js/src/vm/Interpreter.cpp:508
#21 Interpret (cx=0x62d00006e400, state=...)
    at /home/dminor/src/firefox-asan/js/src/vm/Interpreter.cpp:2931
#22 0x00007fffe3034d42 in js::RunScript (cx=<optimized out>, 
    state=...)
    at /home/dminor/src/firefox-asan/js/src/vm/Interpreter.cpp:403
#23 0x00007fffe307034b in js::InternalCallOrConstruct (
TestAgentReceive::CreatePipelines_s for the receive side creates a MediaPipelineReceiveAudio and then calls Init() on it. MediaPipelineReceiveAudio::Init() has an assertion that is is running on the main thread [1]. Similarly on the transmit side, MediaPipelineTransmit::Init() calls MediaPipeline::Init() which also has an assertion that it is running on the main thread [2].

In both cases, after the assert that it is on the main thread, MediaPipeline::Init() then does a dispatch to the sts_thread to run Init_s() over there.

Unless these assertions are somehow disabled and left in to confuse people, I don't see how we could call CreatePipelines_s on the sts_thread.

[1] http://searchfox.org/mozilla-central/source/media/webrtc/signaling/src/mediapipeline/MediaPipeline.cpp#2102

[2] http://searchfox.org/mozilla-central/rev/30fcf167af036aeddf322de44a2fadd370acfd2f/media/webrtc/signaling/src/mediapipeline/MediaPipeline.cpp#640
:jesup, please let me know if this makes sense or if I have missed something.
Flags: needinfo?(rjesup)
I'm going to rebase and land the r+ patches from this bug so we can get rid of MOZILLA_EXTERNAL_LINKAGE and USE_FAKE_MEDIA_STREAMS sooner rather than later.
Keywords: leave-open
Pushed by dminor@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/4ebe288d09f5
Stop building signalingtest libraries; r=jesup
https://hg.mozilla.org/integration/mozilla-inbound/rev/062be1b8915f
Remove USE_FAKE_MEDIA_STREAMS; r=jesup
https://hg.mozilla.org/integration/mozilla-inbound/rev/f03319359207
Remove MOZILLA_EXTERNAL_LINKAGE; r=jesup
https://hg.mozilla.org/integration/mozilla-inbound/rev/ee41fa1c5b0e
Make it possible for "fake" media streams to inherit from real streams; r=jesup
This has bitrotted due to changes to DOMMediaStream which will additional work if we're going to continue with the approach of subclassing "fake" versions of classes in order to make these tests run.
Flags: needinfo?(rjesup)
Full try run: https://treeherder.mozilla.org/#/jobs?repo=try&revision=43d61ab5b887e5069682c9f5c64ef1dcd0b82871

Try run with test fixes: https://treeherder.mozilla.org/#/jobs?repo=try&revision=48da70cac7aaa57496f83b0a6e3a237f7c203503

I've left this disabled on Windows. It never ran there and seems unstable.

Please double check jesup's concerns with threading. My reading of the code is that things end up being dispatched to the correct thread and so the test is fine as is, but it's entirely possible I missed something :)

Sorry this isn't in MozReview - it's not happy with reviews where part of the patch series have been landed.
Attachment #8877653 - Flags: review?(drno)
Comment on attachment 8877653 [details] [diff] [review]
Run mediapipeline_unittest as a xul gtest.

Review of attachment 8877653 [details] [diff] [review]:
-----------------------------------------------------------------

Just wanted to publish this comment.

Going to comment on the threading topic in a separate comment, and want to make review decision based on that.

::: dom/media/DOMMediaStream.cpp
@@ +408,4 @@
>      mPlaybackTrackListener(MakeAndAddRef<PlaybackTrackListener>(this)),
>      mTracksCreated(false), mNotifiedOfMediaStreamGraphShutdown(false),
>      mActive(false), mSetInactiveOnFinish(false),
> +    mAbstractMainThread(aWindow ? aWindow->GetDocGroup()->AbstractMainThreadFor(TaskCategory::Other) : nullptr)

I guess the intention here is to use the nullptr only for tests. But it seems to me there is quite a bit of code which is not prepared to handle a nullptr properly.
I think we should either double check with an expert on this, or put in safety checks everywhere |mAbstractMainThread| gets used.
It looks like the original code used to run both calls for CreatePipelines_s() on the STS thread: http://searchfox.org/mozilla-central/rev/1d652361f7aa8335780c8b954f49f63cbd3dec86/media/webrtc/signaling/test/mediapipeline_unittest.cpp#405

So my guess is that the test now asserts, because it uses only one CodecManager, which then asserts that it always runs on the same thread. That makes me wonder what happens if you revert not the CreatePipelines_s() call for p1_ back to the STS thread?

Another question I have regarding the stack traces from the appear.in call in comment #22 is: do you remember if that was a call where you called yourself just in one browser?
It looks to me like one CodecManager gets instantiated through gUM, and the second through setLocalDescription. Do we really run two CodecManager instance in every normal call (where the second PeerConnection is in another browser)?

If the later is the case I guess it would actually be more helpful to see if we can make these tests also behave this way and instantiated two separate CodecManager instances on different threads. As Randell said we should run this as close as possible to how we do it in Firefox.
Flags: needinfo?(dminor)
This moves the calls to ConfigureSendMediaCodec and ConfigureRecvMediaCodec to the main thread. I've verified by adding assertions and making a appear.in call that these calls also occur on the main thread when making an actual call.

I've also moved the calls to Init() to the main thread. As mentioned in comment 23, there are assertions in the code which require these to be run on the main thread.

With these changes, it's possible to run CreatePipelines_s on the sts thread.

I double checked and the two CodecManager instances do appear when making calls from two separate computers, one from a gUM thread and the other from webrtc. I think simulating a gUM thread is beyond the scope of this bug. The best approach would be to plumb through the fake audio source we use for mochitests into this unit test. I'm willing to file a follow on if that would be worthwhile doing in the future.
Attachment #8877653 - Attachment is obsolete: true
Attachment #8877653 - Flags: review?(drno)
Flags: needinfo?(dminor)
Attachment #8880048 - Flags: review?(drno)
Attachment #8880046 - Flags: review?(rjesup) → review+
Comment on attachment 8880048 [details] [diff] [review]
Run mediapipeline_unittest as a xul gtest.

Review of attachment 8880048 [details] [diff] [review]:
-----------------------------------------------------------------

LGTM
Attachment #8880048 - Flags: review?(drno) → review+
Pushed by dminor@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/5ae582f8ac68
Allow DOMMediaStream constructor to accept a nullptr for aWindow; r=jesup
https://hg.mozilla.org/integration/mozilla-inbound/rev/638029b68a85
move mediapipeline_unittest to xul-gtest; r=drno
Depends on: 1377093
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
Keywords: leave-open
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.