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..af20afa5714ea2741889d1f89a3ceda295349beb 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,7 @@ RTCDataChannel::RTCDataChannel(ExecutionContext* context, PassOwnPtr<blink::WebR
 
 RTCDataChannel::~RTCDataChannel()
 {
+    m_handler->setClient(0);

[Mads Ager (chromium), 2014/06/18 09:09:26]

Do we need to guard this:

if (!m_stopped)
    m_handler->setClient(0);

?

Not sure if it matters to get multiple setClient(0) calls?

In any case we should probably make the difference between non-oilpan and oilpan
clear here. How about:

#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);
#endif

[haraken, 2014/06/18 09:17:08]

Looks nicer.

[keishi, 2014/06/18 10:48:00]

Done.

 }
 
 String RTCDataChannel::label() const
@@ -313,9 +317,21 @@ void RTCDataChannel::scheduledEventTimerFired(Timer<RTCDataChannel>*)
     events.clear();
 }
 
+#if ENABLE(OILPAN)
+void RTCDataChannel::clearWeakMembers(Visitor* visitor)
+{
+    if (!visitor->isAlive(m_connection))
+        stop();
+}
+#endif
+
 void RTCDataChannel::trace(Visitor* visitor)
 {
     visitor->trace(m_scheduledEvents);
+#if ENABLE(OILPAN)

[Mads Ager (chromium), 2014/06/18 09:09:26]

Do you need this #if? If you do I would just put the entire body inside of the
#if since non of this code will be run non-oilpan anyway.

[keishi, 2014/06/18 10:48:00]

Done. We need it because m_connection is oilpan only

+    visitor->trace(m_connection);

[haraken, 2014/06/18 09:17:08]

You can remove this trace, and explicitly do 'm_connection = nullptr' in
clearWeakMembers().

[Mads Ager (chromium), 2014/06/18 09:32:45]

Auch, good catch Haraken! Actually you *have* to remove this one. Otherwise
m_connection will have been set to 0 before you get into clearWeakMembers
(because weak cell callbacks are run before object weak callbacks).

[keishi, 2014/06/18 10:48:00]

Done.

+    visitor->registerWeakMembers<RTCDataChannel, &RTCDataChannel::clearWeakMembers>(this);
+#endif
     EventTargetWithInlineData::trace(visitor);
 }