812886 - Stop using NSS and sockets on net-teardown or when going offline

Status: VERIFIED FIXED

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

Description

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

+++ This bug was initially created as a clone of Bug #799419 +++

(In reply to Eric Rescorla from comment #17)
> Comment on attachment 682857 [details] [diff] [review]
> Watch network (tear)down events and kill PeerConnections; ensure init of NSS
> 
> Review of attachment 682857 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> I would suggest solving this bug with a patch like the one I attached and
> then file a separate bug to address the shutdown problem.
 

> ::: media/webrtc/signaling/src/peerconnection/PeerConnectionImpl.cpp
> @@ +964,5 @@
> >    // Note that no media calls may be made after this point
> >    // because we have removed the pointer.
> >    // Forget the reference so that we can transfer it to
> >    // SelfDestruct().
> >    RUN_ON_THREAD(mThread, WrapRunnable(mMedia.forget().get(),
> 
> I believe that using NS_DISPATCH_SYNC here will result in a regression on
> bug 811183, which resulted from synchronously closing.

Well, we're not being called *from* the GC here, so the recursive-gc issue isn't in play, so I think this is ok.  We only call Close() on active PeerConnections that are still linked to their parent InnerWindow.

Comment 1

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

Attachment: Watch network (tear)down events and kill PeerConnections

Comment 2

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

Comment on attachment 682895 [details] [diff] [review]
Watch network (tear)down events and kill PeerConnections

Or any other DOM peer I assume could do it

Comment 3

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

To test, easy ways are to go to the landing page and load the local loopback test, then shut down (quit/close) or select Work Offline.  (There's still *something* not shutting down all the way in that case, but the PeerConnection closes as intended.)

Comment 4

[Eric Rescorla (:ekr)]

Comment on attachment 682895 [details] [diff] [review]
Watch network (tear)down events and kill PeerConnections

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

I'm not an expert on this, but lgtm.

Comment 5

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 682895 [details] [diff] [review]
Watch network (tear)down events and kill PeerConnections


>+    } else if (topic == "profile-change-net-teardown" ||
>+               topic == "network:offline-about-to-go-offline") {
>+      // delete all peerconnections on shutdown - synchronously!
This could use some better comment, or is
network:offline-about-to-go-offline really only about shutdown?


>+++ b/dom/media/bridge/IPeerConnection.idl
update uuid

>+PeerConnectionImpl::Close(bool isSynchronous) // don't use aSynchronous, too confusing
aIsSynchronous 

>+PeerConnectionImpl::ShutdownMedia(bool isSynchronous)
Ditto


>+  // For the isSynchronous case, we *know* the PeerConnection is
>+  // still alive, and are shutting it down on network teardown/etc, so
>+  // recursive GC isn't an issue.
I wonder if we could assert that is the case here.

Comment 6

[Eric Rescorla (:ekr)]

(In reply to Olli Pettay [:smaug] from comment #5)
> >+  // For the isSynchronous case, we *know* the PeerConnection is
> >+  // still alive, and are shutting it down on network teardown/etc, so
> >+  // recursive GC isn't an issue.
> I wonder if we could assert that is the case here.

FWIW, recursive gc itself causes an assert. But of course early detection would
be nice.

Comment 7

[Brian Smith (:briansmith, :bsmith, use NEEDINFO?)]

Comment on attachment 682895 [details] [diff] [review]
Watch network (tear)down events and kill PeerConnections

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

ekr's review is a lot more meaningful than mine here. I agree with the approach, but I don't understand the structure of the PeerConnection code enough to really review the patch in detail.

Comment 8

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

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

Comment 9

[Brian Smith (:briansmith, :bsmith, use NEEDINFO?)]

Randell, I forgot one important thing: before net-restore and after net-teardown (and analogous events for offline mode), we must also refuse to create new PeerConnections.

Comment 10

[Ed Morley [:emorley]]

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

Comment 11

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

Attachment: disable peerconnection creation while offline

Comment 12

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

Comment on attachment 690142 [details] [diff] [review]
disable peerconnection creation while offline

Finish this bug off by refusing to create PeerConnections in an offline state.

Testing this would involve going offline, waiting for notification of offline state (see this patch for how), then trying to new an RTCPeerConnection.  It should throw an error.  Then go online, wait for notification, and try again - it should succeed.

Comment 13

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 690142 [details] [diff] [review]
disable peerconnection creation while offline

I hope there is a bug open to add localizable-errors.
Also, what does the spec say about this case?

Comment 14

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

https://hg.mozilla.org/integration/mozilla-inbound/rev/92c1455aca7e

Comment 15

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/92c1455aca7e

Comment 16

[Jason Smith [:jsmith]]

I'm seeing the exception fire, so that verifies. There's some other edge cases I'm noticed with weak networks depending on where the problem occurs, so I'll file additional bugs for specific cases.