Onstate handler is allowed to close a PeerConnection.

Bug: chromium:1068084
Change-Id: Icd3f70b6784ac22ef4e3bc1c99233f51145a917f
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2146542
Commit-Queue: Harald Alvestrand <hta@chromium.org>
Reviewed-by: Guido Urdaneta <guidou@chromium.org>
Cr-Commit-Position: refs/heads/master@{#759242}

third_party/blink/renderer/modules/peerconnection/mock_rtc_peer_connection_handler_client.h

@@ -64,7 +64,7 @@ class MockRTCPeerConnectionHandlerClient
   MOCK_METHOD1(DidAddRemoteDataChannel,
                void(scoped_refptr<webrtc::DataChannelInterface>));
   MOCK_METHOD1(DidNoteInterestingUsage, void(int));
-  MOCK_METHOD0(ReleasePeerConnectionHandler, void());
+  MOCK_METHOD0(UnregisterPeerConnectionHandler, void());
 
   // Move-only arguments do not play nicely with MOCK, the workaround is to
   // EXPECT_CALL with these instead.

third_party/blink/renderer/modules/peerconnection/mock_rtc_peer_connection_handler_platform.cc

@@ -397,6 +397,7 @@ MockRTCPeerConnectionHandlerPlatform::CreateDataChannel(
 }
 
 void MockRTCPeerConnectionHandlerPlatform::Stop() {}
+void MockRTCPeerConnectionHandlerPlatform::StopAndUnregister() {}
 
 webrtc::PeerConnectionInterface*
 MockRTCPeerConnectionHandlerPlatform::NativePeerConnection() {

third_party/blink/renderer/modules/peerconnection/mock_rtc_peer_connection_handler_platform.h

@@ -29,6 +29,8 @@ class MockRTCPeerConnectionHandlerPlatform
   bool Initialize(const webrtc::PeerConnectionInterface::RTCConfiguration&,
                   const MediaConstraints&,
                   WebLocalFrame*) override;
+  void Stop() override;
+  void StopAndUnregister() override;
 
   Vector<std::unique_ptr<RTCRtpTransceiverPlatform>> CreateOffer(
       RTCSessionDescriptionRequest*,
@@ -74,7 +76,6 @@ class MockRTCPeerConnectionHandlerPlatform
   scoped_refptr<webrtc::DataChannelInterface> CreateDataChannel(
       const String& label,
       const webrtc::DataChannelInit&) override;
-  void Stop() override;
   webrtc::PeerConnectionInterface* NativePeerConnection() override;
   void RunSynchronousOnceClosureOnSignalingThread(
       CrossThreadOnceClosure closure,

third_party/blink/renderer/modules/peerconnection/rtc_peer_connection.cc

@@ -829,9 +829,11 @@ RTCPeerConnection::~RTCPeerConnection() {
 }
 
 void RTCPeerConnection::Dispose() {
-  // Promptly clears a raw reference from content/ to an on-heap object
+  // Promptly clears the handler's pointer to |this|
   // so that content/ doesn't access it in a lazy sweeping phase.
-  peer_handler_.reset();
+  if (peer_handler_) {
+    peer_handler_->StopAndUnregister();
+  }
 }
 
 ScriptPromise RTCPeerConnection::createOffer(ScriptState* script_state,
@@ -3189,7 +3191,7 @@ void RTCPeerConnection::DidNoteInterestingUsage(int usage_pattern) {
       .Record(document->UkmRecorder());
 }
 
-void RTCPeerConnection::ReleasePeerConnectionHandler() {
+void RTCPeerConnection::UnregisterPeerConnectionHandler() {
   if (stopped_)
     return;
 
@@ -3197,7 +3199,7 @@ void RTCPeerConnection::ReleasePeerConnectionHandler() {
   ice_connection_state_ = webrtc::PeerConnectionInterface::kIceConnectionClosed;
   signaling_state_ = webrtc::PeerConnectionInterface::SignalingState::kClosed;
 
-  peer_handler_.reset();
+  peer_handler_->StopAndUnregister();
   dispatch_scheduled_events_task_handle_.Cancel();
   feature_handle_for_scheduler_.reset();
 }
@@ -3217,7 +3219,7 @@ ExecutionContext* RTCPeerConnection::GetExecutionContext() const {
 }
 
 void RTCPeerConnection::ContextDestroyed() {
-  ReleasePeerConnectionHandler();
+  UnregisterPeerConnectionHandler();
 }
 
 void RTCPeerConnection::ChangeSignalingState(

third_party/blink/renderer/modules/peerconnection/rtc_peer_connection.h

@@ -355,7 +355,7 @@ class MODULES_EXPORT RTCPeerConnection final
   void DidAddRemoteDataChannel(
       scoped_refptr<webrtc::DataChannelInterface> channel) override;
   void DidNoteInterestingUsage(int usage_pattern) override;
-  void ReleasePeerConnectionHandler() override;
+  void UnregisterPeerConnectionHandler() override;
   void ClosePeerConnection() override;
 
   // EventTarget

third_party/blink/renderer/modules/peerconnection/rtc_peer_connection_handler.cc

@@ -1063,6 +1063,7 @@ RTCPeerConnectionHandler::RTCPeerConnectionHandler(
     : initialize_called_(false),
       client_(client),
       is_closed_(false),
+      is_unregistered_(false),
       dependency_factory_(dependency_factory),
       track_adapter_map_(
           base::MakeRefCounted<blink::WebRtcMediaStreamTrackAdapterMap>(
@@ -1079,6 +1080,12 @@ RTCPeerConnectionHandler::RTCPeerConnectionHandler(
 }
 
 RTCPeerConnectionHandler::~RTCPeerConnectionHandler() {
+  if (!is_unregistered_) {
+    StopAndUnregister();
+  }
+}
+
+void RTCPeerConnectionHandler::StopAndUnregister() {
   DCHECK(task_runner_->RunsTasksInCurrentSequence());
 
   Stop();
@@ -1089,6 +1096,10 @@ RTCPeerConnectionHandler::~RTCPeerConnectionHandler() {
 
   UMA_HISTOGRAM_COUNTS_10000("WebRTC.NumDataChannelsPerPeerConnection",
                              num_data_channels_created_);
+  // Clear the pointer to client_ so that it does not interfere with
+  // garbage collection.
+  client_ = nullptr;
+  is_unregistered_ = true;
 }
 
 bool RTCPeerConnectionHandler::Initialize(
@@ -2190,6 +2201,10 @@ void RTCPeerConnectionHandler::OnSignalingChange(
       peer_connection_tracker_->TrackSignalingStateChange(this, stable_state);
     if (!is_closed_)
       client_->DidChangeSignalingState(stable_state);
+    // The callback may have closed the PC. If so, do not continue.
+    if (is_closed_ || !client_) {
+      return;
+    }
   }
   previous_signaling_state_ = new_state;
   if (peer_connection_tracker_)

third_party/blink/renderer/modules/peerconnection/rtc_peer_connection_handler.h

@@ -108,6 +108,9 @@ class MODULES_EXPORT RTCPeerConnectionHandler
                   const MediaConstraints& options,
                   WebLocalFrame* web_frame) override;
 
+  void Stop() override;
+  void StopAndUnregister() override;
+
   Vector<std::unique_ptr<RTCRtpTransceiverPlatform>> CreateOffer(
       RTCSessionDescriptionRequest* request,
       const MediaConstraints& options) override;
@@ -162,7 +165,6 @@ class MODULES_EXPORT RTCPeerConnectionHandler
   scoped_refptr<webrtc::DataChannelInterface> CreateDataChannel(
       const String& label,
       const webrtc::DataChannelInit& init) override;
-  void Stop() override;
   webrtc::PeerConnectionInterface* NativePeerConnection() override;
   void RunSynchronousOnceClosureOnSignalingThread(
       CrossThreadOnceClosure closure,
@@ -349,14 +351,18 @@ class MODULES_EXPORT RTCPeerConnectionHandler
   // first call fails.
   bool initialize_called_;
 
-  // |client_| is a weak pointer to the blink object (blink::RTCPeerConnection)
+  // |client_| is a raw pointer to the blink object (blink::RTCPeerConnection)
   // that owns this object.
-  // It is valid for the lifetime of this object.
-  RTCPeerConnectionHandlerClient* const client_;
+  // It is valid for the lifetime of this object, but is cleared when
+  // StopAndUnregister() is called, in order to make sure it doesn't
+  // interfere with garbage collection of the owner object.
+  RTCPeerConnectionHandlerClient* client_;
   // True if this PeerConnection has been closed.
   // After the PeerConnection has been closed, this object may no longer
   // forward callbacks to blink.
   bool is_closed_;
+  // True if StopAndUnregister has been called.
+  bool is_unregistered_;
 
   // Transition from kHaveLocalOffer to kHaveRemoteOffer indicates implicit
   // rollback in which case we need to also make visiting of kStable observable.

third_party/blink/renderer/platform/peerconnection/rtc_peer_connection_handler_client.h

@@ -85,7 +85,7 @@ class PLATFORM_EXPORT RTCPeerConnectionHandlerClient {
   virtual void DidAddRemoteDataChannel(
       scoped_refptr<webrtc::DataChannelInterface>) = 0;
   virtual void DidNoteInterestingUsage(int usage_pattern) = 0;
-  virtual void ReleasePeerConnectionHandler() = 0;
+  virtual void UnregisterPeerConnectionHandler() = 0;
   virtual void ClosePeerConnection();
 };
 

third_party/blink/renderer/platform/peerconnection/rtc_peer_connection_handler_platform.h

@@ -84,6 +84,14 @@ class PLATFORM_EXPORT RTCPeerConnectionHandlerPlatform {
       const MediaConstraints&,
       WebLocalFrame*) = 0;
 
+  virtual void Stop() = 0;
+  // This function should be called when the object is taken out of service.
+  // There might be functions that need to return through the object, so it
+  // cannot be deleted yet, but no new operations should be allowed.
+  // All references to the object except the owning reference are deleted
+  // by this function.
+  virtual void StopAndUnregister() = 0;
+
   // Unified Plan: The list of transceivers after the createOffer() call.
   // Because of offerToReceive[Audio/Video] it is possible for createOffer() to
   // create new transceivers or update the direction of existing transceivers.
@@ -144,7 +152,6 @@ class PLATFORM_EXPORT RTCPeerConnectionHandlerPlatform {
   // In Unified Plan: Returns OK() with the updated transceiver state.
   virtual webrtc::RTCErrorOr<std::unique_ptr<RTCRtpTransceiverPlatform>>
   RemoveTrack(RTCRtpSenderPlatform*) = 0;
-  virtual void Stop() = 0;
 
   // Returns a pointer to the underlying native PeerConnection object.
   virtual webrtc::PeerConnectionInterface* NativePeerConnection() = 0;

third_party/blink/web_tests/fast/peerconnection/resources/statechange-iframe-destroy-child.html

+<html>
+<script>
+'use strict;'
+
+let cnt = 0;
+
+function causeIssue() {
+  youConnection = new RTCPeerConnection({});
+
+  youConnection.onsignalingstatechange = ev => {
+    if(cnt==1) {
+      parent.trigger();
+    }
+    cnt++;
+  };
+  var offerOptions = {
+    offerToReceiveVideo: 1
+  };
+
+  youConnection.createOffer(offerOptions)
+    .then(function(offer){
+      youConnection.setLocalDescription(offer);
+      // Cause an implicit rollback.
+      youConnection.setRemoteDescription(offer);
+    });
+}
+
+causeIssue();
+</script>
+</html>

third_party/blink/web_tests/fast/peerconnection/statechange-iframe-destroy-parent.html

+<html>
+  <script src="../../resources/testharness.js"></script>
+  <script src="../../resources/testharnessreport.js"></script>
+  <iframe id="ifr">
+  </iframe>
+<script>
+
+let triggered = null;
+
+function trigger()
+{
+  ifr.remove();
+  triggered();
+}
+
+promise_test(t => {
+  ifr.src = "resources/statechange-iframe-destroy-child.html";
+  return new Promise(resolve => {
+    triggered = resolve;
+  });
+}, 'Remove iframe from a statechange callback invoked from iframe');
+
+</script>
+</html>