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()
 {
+    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
 {
     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))
+        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);
 }
 
 } // namespace WebCore