Allow PeerConnection to be garbage collected after close().

Source/modules/mediastream/RTCPeerConnection.cpp

 /*
  * Copyright (C) 2012 Google Inc. All rights reserved.
  *
  * Redistribution and use in source and binary forms, with or without
  * modification, are permitted provided that the following conditions
  * are met:
  *
  * 1. Redistributions of source code must retain the above copyright
  *    notice, this list of conditions and the following disclaimer.
  * 2. Redistributions in binary form must reproduce the above copyright
@@ skipping 164 matching lines @@
     return peerConnection.release();
 }
 
 RTCPeerConnection::RTCPeerConnection(ExecutionContext* context, PassRefPtr<RTCConfiguration> configuration, blink::WebMediaConstraints constraints, ExceptionState& exceptionState)
     : ActiveDOMObject(context)
     , m_signalingState(SignalingStateStable)
     , m_iceGatheringState(ICEGatheringStateNew)
     , m_iceConnectionState(ICEConnectionStateNew)
     , m_dispatchScheduledEventRunner(this, &RTCPeerConnection::dispatchScheduledEvent)
     , m_stopped(false)
+    , m_closed(false)
 {
     ScriptWrappable::init(this);
     Document* document = toDocument(executionContext());
 
+    // If we fail, set |m_closed| and |m_stopped| to true, to avoid hitting the assert in the destructor.
+
     if (!document->frame()) {
+        m_closed = true;
+        m_stopped = true;
         exceptionState.throwDOMException(NotSupportedError, "PeerConnections may not be created in detached documents.");
         return;
     }
 
     m_peerHandler = adoptPtr(blink::Platform::current()->createRTCPeerConnectionHandler(this));
     if (!m_peerHandler) {
+        m_closed = true;
+        m_stopped = true;
         exceptionState.throwDOMException(NotSupportedError, "No PeerConnection handler can be created, perhaps WebRTC is disabled?");
         return;
     }
 
     document->frame()->loader().client()->dispatchWillStartUsingPeerConnectionHandler(m_peerHandler.get());
 
     if (!m_peerHandler->initialize(configuration, constraints)) {
+        m_closed = true;
+        m_stopped = true;
         exceptionState.throwDOMException(NotSupportedError, "Failed to initialize native PeerConnection.");
         return;
     }
 }
 
 RTCPeerConnection::~RTCPeerConnection()
 {
-    // This checks that stop() is called if necessary before the destructor.
-    // We are assuming that a wrapper is always created when RTCPeerConnection is created
-    ASSERT(m_dataChannels.isEmpty());
+    // This checks that close() or stop() is called before the destructor.
+    // We are assuming that a wrapper is always created when RTCPeerConnection is created.
+    ASSERT(m_closed || m_stopped);
+    stop();

[haraken, 2014/06/15 13:43:57]

This is not allowed in oilpan, because in oilpan destructors are not allowed to
touch other on-heap objects (since there is no guarantee about the order in
which on-heap objects are destructed).

keishi@: Do you have any idea on how to address this destruction issue?

[Henrik Grunell, 2014/06/16 07:48:59]

Help me understand this, RTCDataChannel is ref counted and RTCPeerConnection
holds a reference to it. Why is this not safe?
keishi@: Please comment on this.

[keishi, 2014/06/16 08:04:01]

I think we might need to use weak processing to call stop() before destruction.
The problem seems to be that the RTCDataChannel can outlive RTCPeerConnection by
holding a reference to it in JS and that could fire events after
RTCPeerConnection has been destroyed.

We could have a RTCDataChannelObserver to call RTCDataChannel::stop() before
destruction. (like DatabaseSync::TransactionObserver)
Or maybe RTCDataChannel could keep RTCPeerConnection as a weak member, check it
before dispatching events, and call RTCDataChannel::stop on itself when the
member becomes null. This might be better if it works.

[Henrik Grunell, 2014/06/16 08:34:38]

Thanks for the input. Actually, the right thing could be to stop the
DataChannels in close(). I'll look into that, then we don't have to deal with
this.

 }
 
 void RTCPeerConnection::createOffer(PassOwnPtr<RTCSessionDescriptionCallback> successCallback, PassOwnPtr<RTCErrorCallback> errorCallback, const Dictionary& mediaConstraints, ExceptionState& exceptionState)
 {
     if (throwExceptionIfSignalingStateClosed(m_signalingState, exceptionState))
         return;
 
     ASSERT(successCallback);
 
     blink::WebMediaConstraints constraints = MediaConstraintsImpl::create(mediaConstraints, exceptionState);
     if (exceptionState.hadException())
         return;
 
-    RefPtr<RTCSessionDescriptionRequest> request = RTCSessionDescriptionRequestImpl::create(executionContext(), successCallback, errorCallback);
+    RefPtr<RTCSessionDescriptionRequest> request = RTCSessionDescriptionRequestImpl::create(executionContext(), this, successCallback, errorCallback);
     m_peerHandler->createOffer(request.release(), constraints);
 }
 
 void RTCPeerConnection::createAnswer(PassOwnPtr<RTCSessionDescriptionCallback> successCallback, PassOwnPtr<RTCErrorCallback> errorCallback, const Dictionary& mediaConstraints, ExceptionState& exceptionState)
 {
     if (throwExceptionIfSignalingStateClosed(m_signalingState, exceptionState))
         return;
 
     ASSERT(successCallback);
 
     blink::WebMediaConstraints constraints = MediaConstraintsImpl::create(mediaConstraints, exceptionState);
     if (exceptionState.hadException())
         return;
 
-    RefPtr<RTCSessionDescriptionRequest> request = RTCSessionDescriptionRequestImpl::create(executionContext(), successCallback, errorCallback);
+    RefPtr<RTCSessionDescriptionRequest> request = RTCSessionDescriptionRequestImpl::create(executionContext(), this, successCallback, errorCallback);
     m_peerHandler->createAnswer(request.release(), constraints);
 }
 
 void RTCPeerConnection::setLocalDescription(PassRefPtrWillBeRawPtr<RTCSessionDescription> prpSessionDescription, PassOwnPtr<VoidCallback> successCallback, PassOwnPtr<RTCErrorCallback> errorCallback, ExceptionState& exceptionState)
 {
     if (throwExceptionIfSignalingStateClosed(m_signalingState, exceptionState))
         return;
 
     RefPtrWillBeRawPtr<RTCSessionDescription> sessionDescription = prpSessionDescription;
     if (!sessionDescription) {
         exceptionState.throwDOMException(TypeMismatchError, ExceptionMessages::argumentNullOrIncorrectType(1, "RTCSessionDescription"));
         return;
     }
 
-    RefPtr<RTCVoidRequest> request = RTCVoidRequestImpl::create(executionContext(), successCallback, errorCallback);
+    RefPtr<RTCVoidRequest> request = RTCVoidRequestImpl::create(executionContext(), this, successCallback, errorCallback);
     m_peerHandler->setLocalDescription(request.release(), sessionDescription->webSessionDescription());
 }
 
 PassRefPtrWillBeRawPtr<RTCSessionDescription> RTCPeerConnection::localDescription(ExceptionState& exceptionState)
 {
     blink::WebRTCSessionDescription webSessionDescription = m_peerHandler->localDescription();
     if (webSessionDescription.isNull())
         return nullptr;
 
     return RTCSessionDescription::create(webSessionDescription);
 }
 
 void RTCPeerConnection::setRemoteDescription(PassRefPtrWillBeRawPtr<RTCSessionDescription> prpSessionDescription, PassOwnPtr<VoidCallback> successCallback, PassOwnPtr<RTCErrorCallback> errorCallback, ExceptionState& exceptionState)
 {
     if (throwExceptionIfSignalingStateClosed(m_signalingState, exceptionState))
         return;
 
     RefPtrWillBeRawPtr<RTCSessionDescription> sessionDescription = prpSessionDescription;
     if (!sessionDescription) {
         exceptionState.throwDOMException(TypeMismatchError, ExceptionMessages::argumentNullOrIncorrectType(1, "RTCSessionDescription"));
         return;
     }
 
-    RefPtr<RTCVoidRequest> request = RTCVoidRequestImpl::create(executionContext(), successCallback, errorCallback);
+    RefPtr<RTCVoidRequest> request = RTCVoidRequestImpl::create(executionContext(), this, successCallback, errorCallback);
     m_peerHandler->setRemoteDescription(request.release(), sessionDescription->webSessionDescription());
 }
 
 PassRefPtrWillBeRawPtr<RTCSessionDescription> RTCPeerConnection::remoteDescription(ExceptionState& exceptionState)
 {
     blink::WebRTCSessionDescription webSessionDescription = m_peerHandler->remoteDescription();
     if (webSessionDescription.isNull())
         return nullptr;
 
     return RTCSessionDescription::create(webSessionDescription);
@@ skipping 37 matching lines @@
     if (throwExceptionIfSignalingStateClosed(m_signalingState, exceptionState))
         return;
 
     if (!iceCandidate) {
         exceptionState.throwDOMException(TypeMismatchError, ExceptionMessages::argumentNullOrIncorrectType(1, "RTCIceCandidate"));
         return;
     }
     ASSERT(successCallback);
     ASSERT(errorCallback);
 
-    RefPtr<RTCVoidRequest> request = RTCVoidRequestImpl::create(executionContext(), successCallback, errorCallback);
+    RefPtr<RTCVoidRequest> request = RTCVoidRequestImpl::create(executionContext(), this, successCallback, errorCallback);
 
     bool implemented = m_peerHandler->addICECandidate(request.release(), iceCandidate->webCandidate());
-    if (!implemented)
+    if (!implemented) {
         exceptionState.throwDOMException(NotSupportedError, "This method is not yet implemented.");
+    }
 }
 
 String RTCPeerConnection::signalingState() const
 {
     switch (m_signalingState) {
     case SignalingStateStable:
         return "stable";
     case SignalingStateHaveLocalOffer:
         return "have-local-offer";
     case SignalingStateHaveRemoteOffer:
@@ skipping 114 matching lines @@
     for (MediaStreamVector::iterator iter = m_remoteStreams.begin(); iter != m_remoteStreams.end(); ++iter) {
         if ((*iter)->id() == streamId)
             return iter->get();
     }
 
     return 0;
 }
 
 void RTCPeerConnection::getStats(PassOwnPtr<RTCStatsCallback> successCallback, PassRefPtr<MediaStreamTrack> selector)
 {
-    RefPtr<RTCStatsRequest> statsRequest = RTCStatsRequestImpl::create(executionContext(), successCallback, selector);
+    RefPtr<RTCStatsRequest> statsRequest = RTCStatsRequestImpl::create(executionContext(), this, successCallback, selector);
     // FIXME: Add passing selector as part of the statsRequest.
     m_peerHandler->getStats(statsRequest.release());
 }
 
 PassRefPtrWillBeRawPtr<RTCDataChannel> RTCPeerConnection::createDataChannel(String label, const Dictionary& options, ExceptionState& exceptionState)
 {
     if (throwExceptionIfSignalingStateClosed(m_signalingState, exceptionState))
         return nullptr;
 
     blink::WebRTCDataChannelInit init;
@@ skipping 50 matching lines @@
         return nullptr;
     return dtmfSender.release();
 }
 
 void RTCPeerConnection::close(ExceptionState& exceptionState)
 {
     if (throwExceptionIfSignalingStateClosed(m_signalingState, exceptionState))
         return;
 
     m_peerHandler->stop();
+    m_closed = true;
 
     changeIceConnectionState(ICEConnectionStateClosed);
     changeIceGatheringState(ICEGatheringStateComplete);
     changeSignalingState(SignalingStateClosed);
 }
 
 void RTCPeerConnection::negotiationNeeded()
 {
+    ASSERT(!m_closed);
     scheduleDispatchEvent(Event::create(EventTypeNames::negotiationneeded));
 }
 
 void RTCPeerConnection::didGenerateICECandidate(const blink::WebRTCICECandidate& webCandidate)
 {
+    ASSERT(!m_closed);
     ASSERT(executionContext()->isContextThread());
     if (webCandidate.isNull())
         scheduleDispatchEvent(RTCIceCandidateEvent::create(false, false, nullptr));
     else {
         RefPtrWillBeRawPtr<RTCIceCandidate> iceCandidate = RTCIceCandidate::create(webCandidate);
         scheduleDispatchEvent(RTCIceCandidateEvent::create(false, false, iceCandidate.release()));
     }
 }
 
 void RTCPeerConnection::didChangeSignalingState(SignalingState newState)
 {
+    ASSERT(!m_closed);
     ASSERT(executionContext()->isContextThread());
     changeSignalingState(newState);
 }
 
 void RTCPeerConnection::didChangeICEGatheringState(ICEGatheringState newState)
 {
+    ASSERT(!m_closed);

[keishi, 2014/06/16 08:06:35]

When I run your patch locally I hit this assertion in
fast/mediastream/RTCPeerConnection-state.html

[Henrik Grunell, 2014/06/16 08:34:38]

Ah, right, it's a problem in the mock in Chromium. I'll land the fix asap.
https://codereview.chromium.org/331143003/

     ASSERT(executionContext()->isContextThread());
     changeIceGatheringState(newState);
 }
 
 void RTCPeerConnection::didChangeICEConnectionState(ICEConnectionState newState)
 {
+    ASSERT(!m_closed);
     ASSERT(executionContext()->isContextThread());
     changeIceConnectionState(newState);
 }
 
 void RTCPeerConnection::didAddRemoteStream(const blink::WebMediaStream& remoteStream)
 {
+    ASSERT(!m_closed);
     ASSERT(executionContext()->isContextThread());
 
     if (m_signalingState == SignalingStateClosed)
         return;
 
     RefPtrWillBeRawPtr<MediaStream> stream = MediaStream::create(executionContext(), remoteStream);
     m_remoteStreams.append(stream);
 
     scheduleDispatchEvent(MediaStreamEvent::create(EventTypeNames::addstream, false, false, stream.release()));
 }
 
 void RTCPeerConnection::didRemoveRemoteStream(const blink::WebMediaStream& remoteStream)
 {
+    ASSERT(!m_closed);
     ASSERT(executionContext()->isContextThread());
 
     MediaStreamDescriptor* streamDescriptor = remoteStream;
     ASSERT(streamDescriptor->client());
 
     RefPtrWillBeRawPtr<MediaStream> stream = static_cast<MediaStream*>(streamDescriptor->client());
     stream->streamEnded();
 
     if (m_signalingState == SignalingStateClosed)
         return;
 
     size_t pos = m_remoteStreams.find(stream);
     ASSERT(pos != kNotFound);
     m_remoteStreams.remove(pos);
 
     scheduleDispatchEvent(MediaStreamEvent::create(EventTypeNames::removestream, false, false, stream.release()));
 }
 
 void RTCPeerConnection::didAddRemoteDataChannel(blink::WebRTCDataChannelHandler* handler)
 {
+    ASSERT(!m_closed);
     ASSERT(executionContext()->isContextThread());
 
     if (m_signalingState == SignalingStateClosed)
         return;
 
     RefPtrWillBeRawPtr<RTCDataChannel> channel = RTCDataChannel::create(executionContext(), adoptPtr(handler));
     m_dataChannels.append(channel);
 
     scheduleDispatchEvent(RTCDataChannelEvent::create(EventTypeNames::datachannel, false, false, channel.release()));
 }
@@ skipping 88 matching lines @@
 void RTCPeerConnection::trace(Visitor* visitor)
 {
     visitor->trace(m_localStreams);
     visitor->trace(m_remoteStreams);
     visitor->trace(m_dataChannels);
     visitor->trace(m_scheduledEvents);
     EventTargetWithInlineData::trace(visitor);
 }
 
 } // namespace WebCore