Oilpan: Use weak processing to stop a closed RTCPeerConnection

Source/modules/mediastream/RTCDataChannel.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
  *     notice, this list of conditions and the following disclaimer in the
@@ skipping 33 matching lines @@
 static void throwCouldNotSendDataException(ExceptionState& exceptionState)
 {
     exceptionState.throwDOMException(NetworkError, "Could not send data");
 }
 
 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);
 }
 
 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
 {
     return m_handler->label();
 }
 
 bool RTCDataChannel::reliable() const
 {
     return m_handler->isReliable();
@@ skipping 212 matching lines @@
     WillBeHeapVector<RefPtrWillBeMember<Event> > events;
     events.swap(m_scheduledEvents);
 
     WillBeHeapVector<RefPtrWillBeMember<Event> >::iterator it = events.begin();
     for (; it != events.end(); ++it)
         dispatchEvent((*it).release());
 
     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);
 }
 
 } // namespace WebCore