1805320 - Connected outgoing RTCPeerConnection gets garbage collected

Status: NEW

(Core :: WebRTC, defect, P2)

Description

[Jan-Ivar Bruaroey [:jib] (needinfo? me)]

This one was a little easier to repro than bug 1805317.

STRs:

1. Open https://jsfiddle.net/jib1/caoqbmpr/ (allow camera if prompted)
2. move around

Expected result:

• Video keeps playing ▶️ and video frames keep updating

Actual result:

• After 5-7 seconds the video says it's still playing ▶️ but has visibly frozen, and the camera light has gone out

In this case, it appears to be the outgoing RTCPeerConnection (the pc2 helper) and its getUserMedia stream that have been garbage collected (confirmed in about:webrtc and by the camera light going away).

Comment 1

[BugBot [:suhaib / :marco/ :calixte]]

The severity field is not set for this bug.
:mjf, could you have a look please?

For more information, please visit auto_nag documentation.

Comment 2

[Michael Froman [:mjf]]

I've run mozregression and it has identified 00c45094026a8d589e8d2d6127bcc3fc16966436 as the problem. That commit seems unlikely, but is right around the time that Bug 1769802 landed. Byron, can you take a look?

Comment 3

[Byron Campen [:bwc]]

That bug did have lifecycle modifications for sure. Let me look into it.

Comment 4

[Byron Campen [:bwc]]

So, pc2 is being garbage collected because nothing is holding a reference to it. I do not think there's anything in the spec that says an RTCPeerConnection will not be garbage collected as long as it has an active network connection. Are we sure this is a bug?

Comment 5

[Byron Campen [:bwc]]

Ok, webrtc-pc says:

" An RTCPeerConnection object MUST not be garbage collected as long as any event can cause an event handler to be triggered on the object. When the object's [[IsClosed]] internal slot is true, no such event handler can be triggered and it is therefore safe to garbage collect the object.

All RTCDataChannel and MediaStreamTrack objects that are connected to an RTCPeerConnection have a strong reference to the RTCPeerConnection object."

This implies that one could create a pc, register a single event handler (ice state changes, for instance), let go of all references to the pc, and the pc would hang around forever. Is this really the intent? Seems very strange to me.

Comment 6

[Byron Campen [:bwc]]

At any rate, that fiddle does not register any event handlers on pc2, nor does it hold onto a reference to the MediaStreamTracks passed to pc2.addTrack, so even if we followed the spec to the letter, pc2 would still be garbage-collected.

Comment 7

[Karl Tomlinson (:karlt)]

(In reply to Byron Campen [:bwc] from comment #4)

So, pc2 is being garbage collected because nothing is holding a reference to it. I do not think there's anything in the spec that says an RTCPeerConnection will not be garbage collected as long as it has an active network connection. Are we sure this is a bug?

If the spec indicates that frames should update, then they should update whether or not GC occurs and whether or not JS has a reference. The video can only appear frozen after GC if it appears frozen before GC. If GC were to cause observable effects then it would be collecting something that is not garbage.

(In reply to Byron Campen [:bwc] from comment #6)

At any rate, that fiddle does not register any event handlers on pc2, nor does it hold onto a reference to the MediaStreamTracks passed to pc2.addTrack, so even if we followed the spec to the letter, pc2 would still be garbage-collected.

The spec is clarifying some cases where GC must not occur. It is not intending to specify the precise criteria for when GC may occur. GC may occur whenever it would not be observable, and so the spec does not need to describe GC. There is similar discussion at https://groups.google.com/g/mozilla.dev.platform/c/EtjfqRd9FI0/m/0U-e8L64DgAJ and subsequent posts.

(In reply to Byron Campen [:bwc] from comment #5)

This implies that one could create a pc, register a single event handler (ice state changes, for instance), let go of all references to the pc, and the pc would hang around forever. Is this really the intent? Seems very strange to me.

If content has registered for events on a pc object, then it should receive those events. The browser will usually need to keep the object alive to dispatch the events because the events contain references to the target object.

Comment 8

[Byron Campen [:bwc]]

(In reply to Karl Tomlinson (:karlt) from comment #7)

(In reply to Byron Campen [:bwc] from comment #4)

So, pc2 is being garbage collected because nothing is holding a reference to it. I do not think there's anything in the spec that says an RTCPeerConnection will not be garbage collected as long as it has an active network connection. Are we sure this is a bug?

If the spec indicates that frames should update, then they should update whether or not GC occurs and whether or not JS has a reference. The video can only appear frozen after GC if it appears frozen before GC. If GC were to cause observable effects then it would be collecting something that is not garbage.

pc2 is only connected to the video element through an RTP packet flow; pc is the thing that is actually involved in rendering frames.

(In reply to Byron Campen [:bwc] from comment #6)

At any rate, that fiddle does not register any event handlers on pc2, nor does it hold onto a reference to the MediaStreamTracks passed to pc2.addTrack, so even if we followed the spec to the letter, pc2 would still be garbage-collected.

The spec is clarifying some cases where GC must not occur. It is not intending to specify the precise criteria for when GC may occur. GC may occur whenever it would not be observable, and so the spec does not need to describe GC. There is similar discussion at https://groups.google.com/g/mozilla.dev.platform/c/EtjfqRd9FI0/m/0U-e8L64DgAJ and subsequent posts.

Well, it is observable by the other pc, in that the packets stop flowing (and an RTCP BYE should happen too).

(In reply to Byron Campen [:bwc] from comment #5)

This implies that one could create a pc, register a single event handler (ice state changes, for instance), let go of all references to the pc, and the pc would hang around forever. Is this really the intent? Seems very strange to me.

If content has registered for events on a pc object, then it should receive those events. The browser will usually need to keep the object alive to dispatch the events because the events contain references to the target object.

Hmm. If this is a common pattern, I guess that is just how it is. I see that WebSocket has similar (if more detailed) rules, although the event handlers for a WebSocket are essential for its usefulness, which is not exactly true for RTCPeerConnection.

Comment 9

[Jan-Ivar Bruaroey [:jib] (needinfo? me)]

I agree with Karl in comment 7 that there's a higher design principle that garbage collection not be observable in the web platform.

If the webrtc-pc spec doesn't accomplish this we should fix the spec.

Well, it is observable by the other pc, in that the packets stop flowing (and an RTCP BYE should happen too).

Comment 0 shows it's trivial for a web page to use two peer connections in a local loop to detect GC, so I think we need to fix this.

Comment 10

[Jan-Ivar Bruaroey [:jib] (needinfo? me)]

I've confirmed the regression range from comment 2:

16:05.09 INFO: Last good revision: 584f5d45998b362cf4f5b1f211ee62fe625911ce
16:05.09 INFO: First bad revision: 52f3bf10579032db4db89612e72ba16754508b31
16:05.09 INFO: Pushlog:
https://hg.mozilla.org/integration/autoland/pushloghtml?fromchange=584f5d45998b362cf4f5b1f211ee62fe625911ce&tochange=52f3bf10579032db4db89612e72ba16754508b31

Comment 11

[BugBot [:suhaib / :marco/ :calixte]]

Set release status flags based on info from the regressing bug 1769802

Comment 12

[BugBot [:suhaib / :marco/ :calixte]]

Set release status flags based on info from the regressing bug 1769802