825514 - Crash in shutdown of PCCtx/SIPCC in C++ unit tests

Status: RESOLVED FIXED

(Core :: WebRTC: Signaling, defect, P1)

Description

[Eric Rescorla (:ekr)]

(gdb) bt
#0  linked_ptr<CSF::CallControlManager>::operator-> (this=0x40) at linked_ptr.h:108
#1  0x000000010009d7d6 in sipcc::PeerConnectionCtx::Cleanup (this=0x0) at PeerConnectionCtx.cpp:186
#2  0x000000010009d741 in sipcc::PeerConnectionCtx::Destroy () at PeerConnectionCtx.cpp:140
#3  0x000000010009e2e9 in mozilla::PeerConnectionCtxShutdown::Observe (this=0x10da04dc0, aSubject=0x10c32a5e8, aTopic=0x1057484bb "xpcom-shutdown", aData=0x0) at PeerConnectionCtx.cpp:63
#4  0x0000000104165c99 in nsObserverList::NotifyObservers (this=0x10c35a8d8, aSubject=0x10c32a5e8, aTopic=0x1057484bb "xpcom-shutdown", someData=0x0) at /Users/ekr/dev/mozilla-inbound/xpcom/ds/nsObserverList.cpp:99
#5  0x0000000104168135 in nsObserverService::NotifyObservers (this=0x10da01cf0, aSubject=0x10c32a5e8, aTopic=0x1057484bb "xpcom-shutdown", someData=0x0) at /Users/ekr/dev/mozilla-inbound/xpcom/ds/nsObserverService.cpp:161
#6  0x0000000104142d27 in mozilla::ShutdownXPCOM (servMgr=0x0) at /Users/ekr/dev/mozilla-inbound/xpcom/build/nsXPComInit.cpp:552
#7  0x0000000104142b05 in NS_ShutdownXPCOM_P (servMgr=0x0) at /Users/ekr/dev/mozilla-inbound/xpcom/build/nsXPComInit.cpp:513
#8  0x0000000101b17bb5 in NS_ShutdownXPCOM (svcMgr=0x0) at /Users/ekr/dev/mozilla-inbound/xpcom/stub/nsXPComStub.cpp:134
#9  0x0000000100016c28 in ScopedXPCOM::~ScopedXPCOM (this=0x10c3238a0) at TestHarness.h:143
#10 0x00000001000168b5 in ScopedXPCOM::~ScopedXPCOM (this=0x10c3238a0) at TestHarness.h:121
#11 0x00000001000167f2 in MtransportTestUtils::~MtransportTestUtils (this=0x10c3238a0) at mtransport_test_utils.h:43
#12 0x000000010000e735 in MtransportTestUtils::~MtransportTestUtils (this=0x10c3238a0) at mtransport_test_utils.h:41
#13 0x000000010000927d in main (argc=1, argv=0x7fff5fbff630) at /Users/ekr/dev/mozilla-inbound/media/webrtc/signaling/test/signaling_unittests.cpp:1657
(gdb) info locals
observerService = {
  mRawPtr = 0x0
}
rv = 1606414928
kungFuDeathGrip = {
  mRawPtr = 0x7fff5fbff2a8
}
(gdb)

Comment 1

[Eric Rescorla (:ekr)]

Here is what happens on my linux box:

gdb) bt
#0  0x00000000005ca6b4 in testing::internal::String::c_str (this=0x40)
    at /usr/include/c++/4.6/bits/stl_iterator.h:953
#1  0x00000000006386ab in sipcc::PeerConnectionCtx::Cleanup (this=0x0)
    at /home/ekr/dev/mozilla-inbound/media/webrtc/signaling/src/peerconnection/PeerConnectionCtx.cpp:186
#2  0x0000000000638428 in sipcc::PeerConnectionCtx::Destroy ()
    at /home/ekr/dev/mozilla-inbound/media/webrtc/signaling/src/peerconnection/PeerConnectionCtx.cpp:140
#3  0x0000000000637e17 in mozilla::PeerConnectionCtxShutdown::Observe (
    this=0x7fffe9840420, aSubject=0x7fffeae6a338, 
    aTopic=0x7ffff69d313c "xpcom-shutdown", aData=0x0)
    at /home/ekr/dev/mozilla-inbound/media/webrtc/signaling/src/peerconnection/PeerConnectionCtx.cpp:63
#4  0x00007ffff5240969 in nsObserverList::NotifyObservers (
    this=0x7fffe98864a8, aSubject=0x7fffeae6a338, 
    aTopic=0x7ffff69d313c "xpcom-shutdown", someData=0x0)
    at /home/ekr/dev/mozilla-inbound/xpcom/ds/nsObserverList.cpp:99
#5  0x00007ffff524242e in nsObserverService::NotifyObservers (
    this=0x7fffeaec8150, aSubject=0x7fffeae6a338, 
    aTopic=0x7ffff69d313c "xpcom-shutdown", someData=0x0)
    at /home/ekr/dev/mozilla-inbound/xpcom/ds/nsObserverService.cpp:161
#6  0x00007ffff522959a in mozilla::ShutdownXPCOM (servMgr=0x0)
    at /home/ekr/dev/mozilla-inbound/xpcom/build/nsXPComInit.cpp:553
#7  0x00007ffff5229386 in NS_ShutdownXPCOM_P (servMgr=0x0)
    at /home/ekr/dev/mozilla-inbound/xpcom/build/nsXPComInit.cpp:513
#8  0x00007ffff7fe0841 in NS_ShutdownXPCOM (svcMgr=0x0)
    at /home/ekr/dev/mozilla-inbound/xpcom/stub/nsXPComStub.cpp:134
#9  0x00000000005cd0c4 in ScopedXPCOM::~ScopedXPCOM (this=0x7fffeae18290, 
    __in_chrg=<optimized out>)
    at ../../../../dist/include/testing/TestHarness.h:143
#10 0x00000000005cdadf in MtransportTestUtils::~MtransportTestUtils (
    this=0x7fffeae18290, __in_chrg=<optimized out>)
    at /home/ekr/dev/mozilla-inbound/media/mtransport/test/mtransport_test_utils.h:41
#11 0x00000000005da771 in main (argc=1, argv=0x7fffffffe598)
    at /home/ekr/dev/mozilla-inbound/media/webrtc/signaling/test/signaling_unittests.cpp:1657
(gdb)

Comment 2

[Randell Jesup [:jesup] (needinfo me)]

I think this is partially an artifact of the test setup:

class SignalingEnvironment : public ::testing::Environment {
 public:
  void TearDown() {
    sipcc::PeerConnectionImpl::Shutdown();
  }
};

It's also due to the PeerConnectionCtx shutdown observer not expecting someone to shut it down on their own.  I'll add a check of gInstance to make sure it hasn't been shut down before.

Comment 3

[Eric Rescorla (:ekr)]

Yes. I suggest also removing PCImpl::Shutdown() entirely, since there's no legit reason to be calling it.

Comment 4

[Randell Jesup [:jesup] (needinfo me)]

Attachment: Make sure someone didn't destroy WebRTC signaling before Destroying

Comment 5

[Eric Rescorla (:ekr)]

Comment on attachment 696629 [details] [diff] [review]
Make sure someone didn't destroy WebRTC signaling before Destroying

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

I agree that this is will solve the problem. I would also like to remove the offending PCImpl::Shutdown() API point.

Do you want to do this here or in a separate patch?

Comment 6

[Randell Jesup [:jesup] (needinfo me)]

Attachment: Add safety check to PeerConnectionCtx shutdown

Comment 7

[Randell Jesup [:jesup] (needinfo me)]

Comment on attachment 696692 [details] [diff] [review]
Add safety check to PeerConnectionCtx shutdown

remove shutdown, also fix typo in original patch

Comment 8

[Eric Rescorla (:ekr)]

Comment on attachment 696692 [details] [diff] [review]
Add safety check to PeerConnectionCtx shutdown

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

lgtm

Comment 9

[Randell Jesup [:jesup] (needinfo me)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/f6e45f550d30

Comment 10

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/f6e45f550d30