1548539 - Stopping blocking bfcache on unconnected peerconnections

Status: NEW

(Core :: WebRTC, defect, P3)

Comment 1

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

We never really anticipated that people would create PeerConnections (such as for network IP discovery) and never connect them. This appears to be blocking bfcache on quite a few sites in the wild, including major sites (though many of them may actually be ads).

There might be issues if a PeerConnection is in the progress of being connected.

Note that we'd still block on GetUserMedia use.

There may be some edge cases we can live with - if so let's enumerate the issues with rehydrating a bfcached page with an unconnected peerconnection

Comment 2

[Byron Campen [:bwc]]

That depends on what you mean by "unconnected", I guess.

If by "unconnected" we mean we have not gathered any ICE candidates (ie; neither sRD nor sLD have been called, nor have we pre-gathered any ICE candidates), then we should be ok I think, because there's not any transport state yet.

If we bfcache a PeerConnection with ICE candidates, then things get complicated. ICE candidates are actively maintained (eg; keepalives, TURN allocation/permission refreshes), and if they are not they will become non-functional. I would argue that if we cache a PeerConnection with candidates, we at least need to terminate ICE gathering (if it is in progress) and set the ICE state to failed.

If we bfcache a PeerConnection with active media streams, things get weirder. We won't want media to continue flowing (this includes send and recv tracks, as well as SCTP), of course, and while the transport shutdown will at least stop the flow of network traffic, we still need to consider the other internal resources devoted to these things. We almost certainly don't want the various webrtc.org threads hanging around, for instance. So, at the very least we would need to tell every transceiver to stop sending and receiving temporarily (we'd need to verify whether this does, in fact, free up all of the webrtc.org resources). We would also probably want to find a way to free up resources in the SCTP stack. There are a lot of fiddly bits here.

If we wanted to just use a big hammer, we could say that if we bfcache a PC, we close it first. I don't know if that would break things, however.

Comment 3

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

Obviously no connected peerconnections.... We could Close it, but that might confuse things if you navigate away from a call, then hit back. The current implicit reload lets it re-init everything. So blocking those from bfcache seems a win.

How about for unconnected ones terminating ICE?

Comment 4

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

Sites can know when they come out of bfcache (or go in) with pagehide/show, so they can adapt to it if they want. And Chrome is working to add bfcache (and are planning to break compat with sites that depend on things like unload firing when you navigate away; they'll fire pagehide/show like we do). So the Chrome people may have opinions on bfcache and webrtc; I have contacts with the bfcache people at Chrome (indirectly)

Comment 5

[Byron Campen [:bwc]]

We need a hard definition of "connected" here, or we need to use a different term. I suggest we draw the line when a PeerConnection sets an answer. Before that, we can terminate gathering. After that, we can block it from bfcache.

Comment 6

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

Agreed; that was what I meant by "connected"

Comment 7

[Denis Palmeiro [:denispal]]

Marking this as p2 for now so it doesn't prioritize equally as all the other p1 bugs we have. Once we have telemetry data showing how impactful bfcache would be in the wild, we can probably change the priority.