820011 - Cleanly shut down SIPCC on system shutdown

Status: RESOLVED FIXED

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

Description

[Eric Rescorla (:ekr)]

Per agreement with Jesup, we need to shut down SIPCC on XPCOM shutdown.

Comment 1

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

Attachment: Shut down webrtc signaling service on XPCOM shutdown

with this patch, a full gum+peerconnection mochitest run has NO LEAKS - though I've only run it once so far

Comment 2

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

https://tbpl.mozilla.org/?tree=Try&rev=ef36c408a561

Comment 3

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

Attachment: Enable all PeerConnection mochitests by default

Note: applies on top of the gum mochitest patch

Comment 4

[Henrik Skupin [:whimboo][⌚️UTC+2]]

Comment on attachment 696229 [details] [diff] [review]
Enable all PeerConnection mochitests by default

We have two aborts due to bug 823056 but if it fixes everything for you locally I'm absolutely happy to see that landed!

Comment 5

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

I didn't see any bug 823056's.  I did see these:

One is  a new assertion in MediaPipeline.cpp:129 (!description.empty())
One is an assertion at MediaPipeline.cpp:232 (!rtcpsend_srtp && !rtcp_recv_srtp) - I think this has been seen.

We also had Assertion failure: ok, at ../../../xpcom/build/mozPoisonWriteMac.cpp:89 twice (I retriggered) on OSX 10.6 debug
For good measure, I retriggered once more.

Comment 6

[Eric Rescorla (:ekr)]

(In reply to Randell Jesup [:jesup] from comment #5)
> I didn't see any bug 823056's.  I did see these:
> 
> One is  a new assertion in MediaPipeline.cpp:129 (!description.empty())

We have definitely seen this before. ABR and I are trying to figure
out why it's happening.


> One is an assertion at MediaPipeline.cpp:232 (!rtcpsend_srtp &&
> !rtcp_recv_srtp) - I think this has been seen.

I don't recall ever seeing this before. CAn I get a stack trace?


> We also had Assertion failure: ok, at
> ../../../xpcom/build/mozPoisonWriteMac.cpp:89 twice (I retriggered) on OSX
> 10.6 debug
> For good measure, I retriggered once more.

Comment 7

[Timothy B. Terriberry (:derf)]

Comment on attachment 696228 [details] [diff] [review]
Shut down webrtc signaling service on XPCOM shutdown

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

r- due to the first issue.

::: media/webrtc/signaling/src/peerconnection/PeerConnectionImpl.cpp
@@ +63,5 @@
>  static const int MEDIA_STREAM_MUTE = 0x80;
>  
> +namespace mozilla {
> +class PeerConnectionShutdown;
> +nsRefPtr<PeerConnectionShutdown> gPeerConnectionShutdown;

I think this should just be a member variable in PeerConnectionCtx. Otherwise it's possible to create a PeerConnectionCtx, but never create any PCs, which would cause it to leak the singleton. If for some reason you think that's a bad idea, this needs to be a StaticRefPtr, to avoid the static initializer.

@@ +97,5 @@
> +
> +  NS_IMETHODIMP Observe(nsISupports* aSubject, const char* aTopic,
> +                        const PRUnichar* aData) {
> +    if (strcmp(aTopic, NS_XPCOM_SHUTDOWN_OBSERVER_ID) == 0) {
> +      //      LOG(("Shutting down PeerConnectionCtx"));

Either actually log this or remove the comment.

@@ +108,5 @@
> +
> +      nsresult rv = observerService->RemoveObserver(this,
> +                                                    NS_XPCOM_SHUTDOWN_OBSERVER_ID);
> +      MOZ_ASSERT(rv == NS_OK);
> +      (void) rv;

You can use MOZ_ALWAYS_TRUE here to do this a little more cleanly.

@@ +110,5 @@
> +                                                    NS_XPCOM_SHUTDOWN_OBSERVER_ID);
> +      MOZ_ASSERT(rv == NS_OK);
> +      (void) rv;
> +
> +      nsRefPtr<PeerConnectionShutdown> kungFuDeathGrip(this);

I'm not sure what problem this is solving. However, I will note that there _is_ a problem in the shutdown observer for MediaManager (not part of this bug). Please file a follow-up bug to track the issues I raised about that on IRC.

Comment 8

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

Attachment: Shut down webrtc signaling service on XPCOM shutdown

I didn't move the gPeerConnectionShutdown into PeerConnectionCtx's object, since I need an XPCOM observer and PeerConnectionCtx isn't an XPCOM object

Comment 9

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

Attachment: Shut down webrtc signaling service on XPCOM shutdown

moved into PeerConnectionCtx.cpp, init global from Ctx init to avoid any chance of a leak per review

Comment 10

[Timothy B. Terriberry (:derf)]

Comment on attachment 696330 [details] [diff] [review]
Shut down webrtc signaling service on XPCOM shutdown

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

r=me, but see comments.

::: media/webrtc/signaling/src/peerconnection/PeerConnectionCtx.cpp
@@ +47,5 @@
> +      nsresult rv = observerService->AddObserver(this,
> +                                                 NS_XPCOM_SHUTDOWN_OBSERVER_ID,
> +                                                 false);
> +      MOZ_ASSERT(rv == NS_OK);
> +      (void) rv;

This can also be simplified with MOZ_ALWAYS_TRUE().

@@ +121,5 @@
>        return res;
>  
>      gInstance = ctx;
> +
> +    if (!gPeerConnectionCtxShutdown) {

This should probably just be a member variable of PeerConnectionCtx, instead of static. This helps ensure the listener and the object it destroys have the same lifetime.

Comment 11

[Henrik Skupin [:whimboo][⌚️UTC+2]]

(In reply to Eric Rescorla (:ekr) from comment #6)
> (In reply to Randell Jesup [:jesup] from comment #5)
> > One is  a new assertion in MediaPipeline.cpp:129 (!description.empty())
> 
> We have definitely seen this before. ABR and I are trying to figure
> out why it's happening.

This one I have filed yesterday as bug 824851.

> > One is an assertion at MediaPipeline.cpp:232 (!rtcpsend_srtp &&
> > !rtcp_recv_srtp) - I think this has been seen.

Do you have a link to those results? I would like to have a look at.

Comment 12

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

See the try push I did; one of the M2 failures

Comment 13

[Henrik Skupin [:whimboo][⌚️UTC+2]]

For reference:
https://tbpl.mozilla.org/php/getParsedLog.php?id=18312934&tree=Try

Seems to be Windows only. I haven't seen this before either.

Comment 14

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

https://hg.mozilla.org/integration/mozilla-inbound/rev/7c7c7d20f1c8

Includes making the pointer a member var and the macro change

Comment 15

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

Note: we'll probably move the mochitest enabling to the main mochitest bug, since we're not ready to turn them on yet (though they won't leak now)

Comment 16

[Graeme McCutcheon [:graememcc]]

https://hg.mozilla.org/mozilla-central/rev/7c7c7d20f1c8