Closed Bug 807103 Opened 8 years ago Closed 8 years ago

PeerConnectionImpl shutdown incomplete after fixing leaked reference

Categories

(Core :: WebRTC, defect, P1)

defect

Tracking

()

RESOLVED FIXED
mozilla19
Tracking Status
firefox17 --- unaffected
firefox18 --- wontfix

People

(Reporter: jesup, Assigned: jesup)

References

Details

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

Attachments

(1 file)

I fixed a leaked (sort of) reference to PeerConnectionImpl, by setting _pc to null in the observer when the inner window is destroyed in PeerConnection.js.

Now I see the DataChannelConnection getting destroyed!  However, I also see that after we've done PeerConnectionImpl::ShutdownMedia(), the Audio conduit is still running (my test only has fake audio and datachannels).

-1287444672[7f16b3261150]: PeerConnectionImpl ShutdownMedia Media shut down
-1851791616[7f169467b240]: WebrtcAudioSessionConduit SendRTCPPacket : channel 0
-1851791616[7f169467b240]: WebrtcAudioSessionConduit SendRTCPPacket Sent RTCP Packet 

The RTCP sends continue ad infinitum.

I'm also unable to create a new call after this.

I'll upload my working patch, which also fixes a DataChannelConnection issue once shutdowns can happen (turn off SO_LINGER)
You'll need the patch from bug 807109 as well
Depends on: 807109
Ok, so we're destroying transmit pipelines (somehow), but not receive pipelines.

I don't see anything doing EndTrack on these tracks, or any other cleanup.  lsm_close_rx is effectively turned off for our use.  I suspect something is assuming it will get GC'd when the refcount goes to 0, but it's not because of a bug somewhere.

See also the suggestion in bug 807238; we may be able to EndTrack when disconnecting the streams, then when they get ended and we're notified we call Detach(), and let the receive audio pipeline die.  However, the lifetime and references to these tracks/pipelines etc is unclear enough to me to be at all sure.
The lifetime of the pipelines is intended to be tied to the pipeline arrays in the PC (since that's the only thing that is holding a reference to them). The PC can call DetachMediaStream() to disconnect it from the media stream and DetachTransport() to detach it from the transport.

Probably the Pipeline should be calling DetachMediaStream() on destruction the same way it calls DetachTransport(). At that point either an explicit close by the PC or just GC should do the right thing. A second issue is that DetachTransport needs to call EndTrack(), I guess.
Whiteboard: [WebRTC], [blocking-webrtc+]
Priority: -- → P1
Attachment #676777 - Flags: review?(anant)
Attachment #676777 - Flags: review?(anant) → review+
Jesup: do you want to use this bug for the DetachMediaStream stuff in c3, or shall I file a new bug.
(In reply to Eric Rescorla from comment #6)
> Jesup: do you want to use this bug for the DetachMediaStream stuff in c3, or
> shall I file a new bug.

New Bug.  That's just a problem exposed by fixing the leak
https://hg.mozilla.org/mozilla-central/rev/2b628536b1d3
Assignee: nobody → rjesup
Status: NEW → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Whiteboard: [WebRTC], [blocking-webrtc+] → [WebRTC], [blocking-webrtc+] [qa-]
You need to log in before you can comment on or make changes to this bug.