port mediapipeline_unittest to xul gtest

RESOLVED FIXED

Status

()

Core
WebRTC: Audio/Video
P1
normal
Rank:
17
RESOLVED FIXED
a year ago
5 months ago

People

(Reporter: dminor, Assigned: dminor)

Tracking

Trunk
Points:
---

Firefox Tracking Flags

(firefox53 affected)

Details

MozReview Requests

()

Submitter Diff Changes Open Issues Last Updated
Loading...
Error loading review requests:

Attachments

(7 attachments, 1 obsolete attachment)

Comment hidden (empty)
(Assignee)

Updated

a year ago
Status: NEW → ASSIGNED
Rank: 17
(Assignee)

Comment 1

a year ago
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
(Assignee)

Updated

a year ago
No longer blocks: 1316611
(Assignee)

Comment 2

11 months ago
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
(Assignee)

Updated

11 months ago
Depends on: 848189
(Assignee)

Updated

11 months ago
No longer depends on: 848189
(Assignee)

Comment 3

11 months ago
Try run here: https://treeherder.mozilla.org/#/jobs?repo=try&revision=6eb3886f06534a4897632105060908cc21d6dbe5
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
(Assignee)

Comment 9

11 months ago
Try run here (with all tests): https://treeherder.mozilla.org/#/jobs?repo=try&revision=e8233c8d61fcc1467c185fd190ec1c343054f7ea

Comment 10

11 months ago
mozreview-review
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 11

11 months ago
mozreview-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 12

11 months ago
mozreview-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 13

11 months ago
mozreview-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+
(Assignee)

Comment 14

11 months ago
(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
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)

Comment 17

11 months ago
(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 18

11 months ago
mozreview-review
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 19

11 months ago
mozreview-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-
(Assignee)

Comment 20

11 months ago
(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.

Comment 21

11 months ago
(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).
(Assignee)

Comment 22

11 months ago
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 (
(Assignee)

Comment 23

11 months ago
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
(Assignee)

Comment 24

11 months ago
:jesup, please let me know if this makes sense or if I have missed something.
Flags: needinfo?(rjesup)
(Assignee)

Comment 25

10 months ago
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

Comment 26

10 months ago
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

Comment 27

10 months ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/4ebe288d09f5
https://hg.mozilla.org/mozilla-central/rev/062be1b8915f
https://hg.mozilla.org/mozilla-central/rev/f03319359207
https://hg.mozilla.org/mozilla-central/rev/ee41fa1c5b0e
(Assignee)

Comment 28

6 months ago
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)
(Assignee)

Comment 29

6 months ago
Created attachment 8877653 [details] [diff] [review]
Run mediapipeline_unittest as a xul gtest.

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)
(Assignee)

Comment 32

6 months ago
Created attachment 8880046 [details] [diff] [review]
Allow DOMMediaStream constructor to accept a nullptr for aWindow
Attachment #8880046 - Flags: review?(rjesup)
(Assignee)

Comment 33

6 months ago
Created attachment 8880048 [details] [diff] [review]
Run mediapipeline_unittest as a xul gtest.

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)

Updated

6 months ago
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+

Comment 35

6 months ago
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

Comment 36

6 months ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/5ae582f8ac68
https://hg.mozilla.org/mozilla-central/rev/638029b68a85

Updated

6 months ago
Depends on: 1377093
(Assignee)

Updated

5 months ago
Status: ASSIGNED → RESOLVED
Last Resolved: 5 months ago
Keywords: leave-open
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.