Oilpan: Use weak processing to stop a closed RTCPeerConnection

Source/modules/mediastream/RTCDataChannel.cpp

Index: Source/modules/mediastream/RTCDataChannel.cpp
diff --git a/Source/modules/mediastream/RTCDataChannel.cpp b/Source/modules/mediastream/RTCDataChannel.cpp
index 9a0ec1b246913bb4ac02b0e4d0f6f5a6f4b241b1..90d3acf9f231e1dafcae31171436079ee30ac834 100644
--- a/Source/modules/mediastream/RTCDataChannel.cpp
+++ b/Source/modules/mediastream/RTCDataChannel.cpp
@@ -51,29 +51,32 @@ static void throwNoBlobSupportException(ExceptionState& exceptionState)
     exceptionState.throwDOMException(NotSupportedError, "Blob support not implemented yet");
 }
 
-PassRefPtrWillBeRawPtr<RTCDataChannel> RTCDataChannel::create(ExecutionContext* context, PassOwnPtr<blink::WebRTCDataChannelHandler> handler)
+PassRefPtrWillBeRawPtr<RTCDataChannel> RTCDataChannel::create(ExecutionContext* context, RTCPeerConnection* connection, PassOwnPtr<blink::WebRTCDataChannelHandler> handler)
 {
     ASSERT(handler);
-    return adoptRefWillBeRefCountedGarbageCollected(new RTCDataChannel(context, handler));
+    return adoptRefWillBeRefCountedGarbageCollected(new RTCDataChannel(context, connection, handler));
 }
 
-PassRefPtrWillBeRawPtr<RTCDataChannel> RTCDataChannel::create(ExecutionContext* context, blink::WebRTCPeerConnectionHandler* peerConnectionHandler, const String& label, const blink::WebRTCDataChannelInit& init, ExceptionState& exceptionState)
+PassRefPtrWillBeRawPtr<RTCDataChannel> RTCDataChannel::create(ExecutionContext* context, RTCPeerConnection* connection, blink::WebRTCPeerConnectionHandler* peerConnectionHandler, const String& label, const blink::WebRTCDataChannelInit& init, ExceptionState& exceptionState)
 {
     OwnPtr<blink::WebRTCDataChannelHandler> handler = adoptPtr(peerConnectionHandler->createDataChannel(label, init));
     if (!handler) {
         exceptionState.throwDOMException(NotSupportedError, "RTCDataChannel is not supported");
         return nullptr;
     }
-    return adoptRefWillBeRefCountedGarbageCollected(new RTCDataChannel(context, handler.release()));
+    return adoptRefWillBeRefCountedGarbageCollected(new RTCDataChannel(context, connection, handler.release()));
 }
 
-RTCDataChannel::RTCDataChannel(ExecutionContext* context, PassOwnPtr<blink::WebRTCDataChannelHandler> handler)
+RTCDataChannel::RTCDataChannel(ExecutionContext* context, RTCPeerConnection* connection, PassOwnPtr<blink::WebRTCDataChannelHandler> handler)
     : m_executionContext(context)
     , m_handler(handler)
     , m_stopped(false)
     , m_readyState(ReadyStateConnecting)
     , m_binaryType(BinaryTypeArrayBuffer)
     , m_scheduledEventTimer(this, &RTCDataChannel::scheduledEventTimerFired)
+#if ENABLE(OILPAN)
+    , m_connection(connection)
+#endif
 {
     ScriptWrappable::init(this);
     m_handler->setClient(this);
@@ -81,6 +84,15 @@ RTCDataChannel::RTCDataChannel(ExecutionContext* context, PassOwnPtr<blink::WebR
 
 RTCDataChannel::~RTCDataChannel()
 {
+#if ENABLE(OILPAN)
+    // If the peer connection and the data channel die in the same
+    // GC cycle stop has not been called and we need to notify the
+    // client that the channel is gone.
+    if (!m_stopped)
+        m_handler->setClient(0);
+#else
+    ASSERT(m_stopped);

[Henrik Grunell, 2014/06/18 11:49:13]

I'm not sure why we would assert this?

[Mads Ager (chromium), 2014/06/18 12:35:13]

Is it not the case that an RTCDataChannel needs to call m_handler->setClient(0)
before it dies? That is done via stop when the owning RTCPeerConnection dies and
this assert verifies that it has been done and adds documentation that without
oilpan that is the expectation. The reason why I suggested adding it is that it
gives context for the oilpan code above. That we need to call setClient(0) if we
are dying in the same GC round as the RTCPeerConnection.

That said, I'm perfectly fine with removing the ASSERT if it is wrong or if you
do not find it helpful. :)

[Henrik Grunell, 2014/06/18 18:21:56]

I'm by no means an expert on the data channels itself (or blink for that
matter). From what I can see, hasActivityPending isn't overridden so it could be
deleted after all refs are released and that could happen without stop being
called. And |m_handler| would be deleted when the data channel is deleted
avoiding calls into the deleted object. Does this sound correct?

+#endif
 }
 
 String RTCDataChannel::label() const
@@ -313,9 +325,22 @@ void RTCDataChannel::scheduledEventTimerFired(Timer<RTCDataChannel>*)
     events.clear();
 }
 
+#if ENABLE(OILPAN)
+void RTCDataChannel::clearWeakMembers(Visitor* visitor)
+{
+    if (visitor->isAlive(m_connection))
+        return;
+    stop();
+    m_connection = nullptr;
+}
+#endif
+
 void RTCDataChannel::trace(Visitor* visitor)
 {
+#if ENABLE(OILPAN)
     visitor->trace(m_scheduledEvents);
+    visitor->registerWeakMembers<RTCDataChannel, &RTCDataChannel::clearWeakMembers>(this);
+#endif
     EventTargetWithInlineData::trace(visitor);
 }