Cleanly shut down SIPCC on system shutdown

RESOLVED FIXED in mozilla20

Status

()

Core
WebRTC: Signaling
P1
normal
RESOLVED FIXED
6 years ago
6 years ago

People

(Reporter: ekr, Assigned: jesup)

Tracking

Trunk
mozilla20
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [WebRTC][blocking-webrtc+][qa-])

Attachments

(2 attachments, 2 obsolete attachments)

(Reporter)

Description

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

Updated

6 years ago
Whiteboard: [WebRTC][blocking-webrtc-]

Updated

6 years ago
Priority: -- → P1
(Assignee)

Comment 1

6 years ago
Created attachment 696228 [details] [diff] [review]
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
(Assignee)

Updated

6 years ago
Attachment #696228 - Flags: review?(tterribe)
(Assignee)

Comment 3

6 years ago
Created attachment 696229 [details] [diff] [review]
Enable all PeerConnection mochitests by default

Note: applies on top of the gum mochitest patch
(Assignee)

Updated

6 years ago
Attachment #696229 - Flags: review?(hskupin)
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!
Attachment #696229 - Flags: review?(hskupin) → review+
(Assignee)

Comment 5

6 years ago
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.
(Reporter)

Comment 6

6 years ago
(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 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.
Attachment #696228 - Flags: review?(tterribe) → review-
(Assignee)

Comment 8

6 years ago
Created attachment 696325 [details] [diff] [review]
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
(Assignee)

Updated

6 years ago
Attachment #696228 - Attachment is obsolete: true
Assignee: nobody → rjesup
Whiteboard: [WebRTC][blocking-webrtc-] → [WebRTC][blocking-webrtc+]
(Assignee)

Comment 9

6 years ago
Created attachment 696330 [details] [diff] [review]
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
(Assignee)

Updated

6 years ago
Attachment #696325 - Attachment is obsolete: true
(Assignee)

Updated

6 years ago
Attachment #696330 - Flags: review?(tterribe)
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.
Attachment #696330 - Flags: review?(tterribe) → review+
(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.
Status: NEW → ASSIGNED
(Assignee)

Comment 12

6 years ago
See the try push I did; one of the M2 failures
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.
(Assignee)

Comment 14

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

Includes making the pointer a member var and the macro change
OS: Mac OS X → All
Hardware: x86 → All
Target Milestone: --- → mozilla20
(Assignee)

Comment 15

6 years ago
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)
https://hg.mozilla.org/mozilla-central/rev/7c7c7d20f1c8
Status: ASSIGNED → RESOLVED
Last Resolved: 6 years ago
Resolution: --- → FIXED

Updated

6 years ago
Whiteboard: [WebRTC][blocking-webrtc+] → [WebRTC][blocking-webrtc+][qa-]
You need to log in before you can comment on or make changes to this bug.