Closed
Bug 820011
Opened 12 years ago
Closed 11 years ago
Cleanly shut down SIPCC on system shutdown
Categories
(Core :: WebRTC: Signaling, defect, P1)
Core
WebRTC: Signaling
Tracking
()
RESOLVED
FIXED
mozilla20
People
(Reporter: ekr, Assigned: jesup)
Details
(Whiteboard: [WebRTC][blocking-webrtc+][qa-])
Attachments
(2 files, 2 obsolete files)
1.30 KB,
patch
|
whimboo
:
review+
|
Details | Diff | Splinter Review |
3.50 KB,
patch
|
derf
:
review+
|
Details | Diff | Splinter Review |
Per agreement with Jesup, we need to shut down SIPCC on XPCOM shutdown.
Updated•12 years ago
|
Whiteboard: [WebRTC][blocking-webrtc-]
Updated•12 years ago
|
Priority: -- → P1
Assignee | ||
Comment 1•11 years ago
|
||
with this patch, a full gum+peerconnection mochitest run has NO LEAKS - though I've only run it once so far
Assignee | ||
Comment 2•11 years ago
|
||
https://tbpl.mozilla.org/?tree=Try&rev=ef36c408a561
Assignee | ||
Updated•11 years ago
|
Attachment #696228 -
Flags: review?(tterribe)
Assignee | ||
Comment 3•11 years ago
|
||
Note: applies on top of the gum mochitest patch
Assignee | ||
Updated•11 years ago
|
Attachment #696229 -
Flags: review?(hskupin)
Comment 4•11 years ago
|
||
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•11 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•11 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 7•11 years ago
|
||
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•11 years ago
|
||
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•11 years ago
|
Attachment #696228 -
Attachment is obsolete: true
Updated•11 years ago
|
Assignee: nobody → rjesup
Whiteboard: [WebRTC][blocking-webrtc-] → [WebRTC][blocking-webrtc+]
Assignee | ||
Comment 9•11 years ago
|
||
moved into PeerConnectionCtx.cpp, init global from Ctx init to avoid any chance of a leak per review
Assignee | ||
Updated•11 years ago
|
Attachment #696325 -
Attachment is obsolete: true
Assignee | ||
Updated•11 years ago
|
Attachment #696330 -
Flags: review?(tterribe)
Comment 10•11 years ago
|
||
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+
Comment 11•11 years ago
|
||
(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•11 years ago
|
||
See the try push I did; one of the M2 failures
Comment 13•11 years ago
|
||
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•11 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•11 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)
Comment 16•11 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/7c7c7d20f1c8
Status: ASSIGNED → RESOLVED
Closed: 11 years ago
Resolution: --- → FIXED
Updated•11 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.
Description
•