Dangling pointer bug fix for RTCQuicStream.

If a finish is read out by JavaScript and then write() is called with
finish set to true, the underlying stream objects are destroyed
(proxy & host). This is fine, but in the case when the finish is written
with a large amount of data, it might be buffered by the QUIC library.
In this case the QuicStreamHost object is destroyed, while the
P2PQuicStream stays alive writing the data to the remote endpoint. This
violates the lifetime requirements of the P2PQuicStream's Delegate
object (P2PQuicStreamHost), and fires Delegate::OnWriteDataConsumed to a
dangling pointer.

Bug: 927098
Change-Id: I211ff7c0514c2f58f60066a64b6c5051e11ce9ac
Reviewed-on: https://chromium-review.googlesource.com/c/1447023
Commit-Queue: Seth Hampson <shampson@chromium.org>
Reviewed-by: Steve Anton <steveanton@chromium.org>
Cr-Original-Commit-Position: refs/heads/master@{#627781}(cherry picked from commit 3eacc778ece91687df08454c508180eae8232d17)
Reviewed-on: https://chromium-review.googlesource.com/c/1449324
Reviewed-by: Seth Hampson <shampson@chromium.org>
Cr-Commit-Position: refs/branch-heads/3683@{#104}
Cr-Branched-From: e51029943e0a38dd794b73caaf6373d5496ae783-refs/heads/master@{#625896}
diff --git a/third_party/blink/renderer/modules/peerconnection/adapters/p2p_quic_stream.h b/third_party/blink/renderer/modules/peerconnection/adapters/p2p_quic_stream.h
index a6d923ed..c3ce1df 100644
--- a/third_party/blink/renderer/modules/peerconnection/adapters/p2p_quic_stream.h
+++ b/third_party/blink/renderer/modules/peerconnection/adapters/p2p_quic_stream.h
@@ -77,7 +77,8 @@
   // for reading and writing and deleted by the quic::QuicSession.
   virtual void WriteData(Vector<uint8_t> data, bool fin) = 0;
 
-  // Sets the delegate object, which must outlive the P2PQuicStream.
+  // Sets the delegate object. In the case that the Delegate does not outlive
+  // the P2PQuicStream object, it must unset itself before destructing.
   virtual void SetDelegate(Delegate* delegate) = 0;
 };
 
diff --git a/third_party/blink/renderer/modules/peerconnection/adapters/p2p_quic_stream_impl.cc b/third_party/blink/renderer/modules/peerconnection/adapters/p2p_quic_stream_impl.cc
index 567214b7..b53479f 100644
--- a/third_party/blink/renderer/modules/peerconnection/adapters/p2p_quic_stream_impl.cc
+++ b/third_party/blink/renderer/modules/peerconnection/adapters/p2p_quic_stream_impl.cc
@@ -4,6 +4,7 @@
 
 #include "third_party/blink/renderer/modules/peerconnection/adapters/p2p_quic_stream_impl.h"
 
+#include <utility>
 #include "net/third_party/quic/core/quic_error_codes.h"
 #include "third_party/blink/renderer/platform/wtf/std_lib_extras.h"
 
@@ -34,13 +35,14 @@
 P2PQuicStreamImpl::~P2PQuicStreamImpl() {}
 
 void P2PQuicStreamImpl::OnDataAvailable() {
-  DCHECK(delegate_);
   if (!sequencer()->HasBytesToRead() && sequencer()->IsClosed()) {
     // We have consumed all data from the sequencer up to the FIN bit. This can
     // only occur by receiving an empty STREAM frame with the FIN bit set.
     quic::QuicStream::OnFinRead();
-    delegate_->OnDataReceived({}, /*fin=*/true);
     consumed_fin_ = true;
+    if (delegate_) {
+      delegate_->OnDataReceived({}, /*fin=*/true);
+    }
   }
 
   uint32_t delegate_read_buffer_available =
@@ -87,17 +89,20 @@
     quic::QuicStream::OnFinRead();
     consumed_fin_ = fin;
   }
-  delegate_->OnDataReceived(std::move(data), fin);
+  if (delegate_) {
+    delegate_->OnDataReceived(std::move(data), fin);
+  }
 }
 
 void P2PQuicStreamImpl::OnStreamDataConsumed(size_t bytes_consumed) {
-  DCHECK(delegate_);
   // We should never consume more than has been written.
   DCHECK_GE(write_buffered_amount_, bytes_consumed);
   QuicStream::OnStreamDataConsumed(bytes_consumed);
   if (bytes_consumed > 0) {
     write_buffered_amount_ -= bytes_consumed;
-    delegate_->OnWriteDataConsumed(SafeCast<uint32_t>(bytes_consumed));
+    if (delegate_) {
+      delegate_->OnWriteDataConsumed(SafeCast<uint32_t>(bytes_consumed));
+    }
   }
 }
 
@@ -134,12 +139,13 @@
 }
 
 void P2PQuicStreamImpl::OnStreamReset(const quic::QuicRstStreamFrame& frame) {
-  DCHECK(delegate_);
   // Calling this on the QuicStream will ensure that the stream is closed
   // for reading and writing and we send a RST frame to the remote side if
   // we have not already.
   quic::QuicStream::OnStreamReset(frame);
-  delegate_->OnRemoteReset();
+  if (delegate_) {
+    delegate_->OnRemoteReset();
+  }
 }
 
 void P2PQuicStreamImpl::OnClose() {
diff --git a/third_party/blink/renderer/modules/peerconnection/adapters/p2p_quic_stream_impl.h b/third_party/blink/renderer/modules/peerconnection/adapters/p2p_quic_stream_impl.h
index 733ff5a..2af3a5f 100644
--- a/third_party/blink/renderer/modules/peerconnection/adapters/p2p_quic_stream_impl.h
+++ b/third_party/blink/renderer/modules/peerconnection/adapters/p2p_quic_stream_impl.h
@@ -67,7 +67,7 @@
  private:
   using quic::QuicStream::Reset;
 
-  // Outlives the P2PQuicStreamImpl.
+  // Must either outlive the P2PQuicStream or unset itself upon destruction.
   Delegate* delegate_;
 
   // The read buffer size of the delegate. The |delegate_read_buffered_amount_|
diff --git a/third_party/blink/renderer/modules/peerconnection/adapters/p2p_quic_stream_unittest.cc b/third_party/blink/renderer/modules/peerconnection/adapters/p2p_quic_stream_unittest.cc
index 0a36ce34..d3c03c7 100644
--- a/third_party/blink/renderer/modules/peerconnection/adapters/p2p_quic_stream_unittest.cc
+++ b/third_party/blink/renderer/modules/peerconnection/adapters/p2p_quic_stream_unittest.cc
@@ -439,4 +439,64 @@
   // Just the last data received ("ta") is held in the delegate's read buffer.
   EXPECT_EQ(2u, stream_->DelegateReadBufferedAmountForTesting());
 }
+
+// Tests that after a delegate unsets itself, it will no longer receive the
+// OnWriteDataConsumed callback.
+TEST_F(P2PQuicStreamTest, UnsetDelegateDoesNotFireOnWriteDataConsumed) {
+  InitializeStream();
+  stream_->SetDelegate(nullptr);
+  // Mock out the QuicSession to get the QuicStream::OnStreamDataConsumed
+  // callback to fire.
+  EXPECT_CALL(session_,
+              WritevData(stream_, kStreamId,
+                         /*write_length=*/base::size(kSomeData), _, _))
+      .WillOnce(Invoke([](quic::QuicStream* stream, quic::QuicStreamId id,
+                          size_t write_length, quic::QuicStreamOffset offset,
+                          quic::StreamSendingState state) {
+        return quic::QuicConsumedData(
+            write_length, state != quic::StreamSendingState::NO_FIN);
+      }));
+
+  EXPECT_CALL(delegate_, OnWriteDataConsumed(_)).Times(0);
+
+  stream_->WriteData(VectorFromArray(kSomeData), /*fin=*/false);
+}
+
+// Tests that after a delegate unsets itself, it will no longer receive the
+// OnRemoteReset callback.
+TEST_F(P2PQuicStreamTest, UnsetDelegateDoesNotFireOnRemoteReset) {
+  InitializeStream();
+  stream_->SetDelegate(nullptr);
+  EXPECT_CALL(delegate_, OnRemoteReset()).Times(0);
+
+  quic::QuicRstStreamFrame rst_frame(quic::kInvalidControlFrameId, kStreamId,
+                                     quic::QUIC_STREAM_CANCELLED, 0);
+  stream_->OnStreamReset(rst_frame);
+}
+
+// Tests that after a delegate unsets itself, it will no longer receive the
+// OnDataReceived callback when receiving a stream frame with data and no FIN
+// bit.
+TEST_F(P2PQuicStreamTest, UnsetDelegateDoesNotFireOnDataReceivedWithData) {
+  InitializeStream();
+  stream_->SetDelegate(nullptr);
+
+  EXPECT_CALL(delegate_, OnDataReceived(_, _)).Times(0);
+
+  quic::QuicStreamFrame stream_frame(stream_->id(), /*fin=*/false, 0,
+                                     StringPieceFromArray(kSomeData));
+  stream_->OnStreamFrame(stream_frame);
+}
+
+// Tests that after a delegate unsets itself, it will no longer receive the
+// OnDataReceived callback when receiving a stream frame with the FIN bit.
+TEST_F(P2PQuicStreamTest, UnsetDelegateDoesNotFireOnDataReceivedWithFin) {
+  InitializeStream();
+  stream_->SetDelegate(nullptr);
+
+  EXPECT_CALL(delegate_, OnDataReceived(_, _)).Times(0);
+
+  quic::QuicStreamFrame stream_frame(stream_->id(), /*fin=*/true, 0, {});
+  stream_->OnStreamFrame(stream_frame);
+}
 }  // namespace blink
diff --git a/third_party/blink/renderer/modules/peerconnection/adapters/quic_stream_host.cc b/third_party/blink/renderer/modules/peerconnection/adapters/quic_stream_host.cc
index a0937ef..70debe1 100644
--- a/third_party/blink/renderer/modules/peerconnection/adapters/quic_stream_host.cc
+++ b/third_party/blink/renderer/modules/peerconnection/adapters/quic_stream_host.cc
@@ -99,6 +99,7 @@
 void QuicStreamHost::Delete() {
   DCHECK_CALLED_ON_VALID_THREAD(thread_checker_);
   DCHECK(transport_host_);
+  p2p_stream_->SetDelegate(nullptr);
   // OnRemoveStream will delete |this|.
   transport_host_->OnRemoveStream(this);
 }
diff --git a/third_party/blink/renderer/modules/peerconnection/rtc_quic_stream.cc b/third_party/blink/renderer/modules/peerconnection/rtc_quic_stream.cc
index 6acfef6..ac4de7c 100644
--- a/third_party/blink/renderer/modules/peerconnection/rtc_quic_stream.cc
+++ b/third_party/blink/renderer/modules/peerconnection/rtc_quic_stream.cc
@@ -193,9 +193,6 @@
                               data_vector.size(), 1, 24000000, 50);
   proxy_->WriteData(std::move(data_vector), finish);
   if (finish) {
-    // TODO(shampson): This can cause a crash. If the P2PQuicStream
-    // has buffered data with a FIN, it will try calling
-    // host->OnWriteDataConsumed, after the QuicStreamHost has been deleted.
     wrote_fin_ = true;
     if (!read_fin_) {
       DCHECK_EQ(state_, RTCQuicStreamState::kOpen);
diff --git a/third_party/blink/renderer/modules/peerconnection/rtc_quic_stream_test.cc b/third_party/blink/renderer/modules/peerconnection/rtc_quic_stream_test.cc
index 3ab9b0e..8ae23b7 100644
--- a/third_party/blink/renderer/modules/peerconnection/rtc_quic_stream_test.cc
+++ b/third_party/blink/renderer/modules/peerconnection/rtc_quic_stream_test.cc
@@ -171,6 +171,8 @@
   RunUntilIdle();
 
   ASSERT_TRUE(stream_delegate);
+  EXPECT_CALL(*p2p_quic_stream.get(), SetDelegate(nullptr));
+
   stream_delegate->OnRemoteReset();
 
   RunUntilIdle();
@@ -205,6 +207,8 @@
   RunUntilIdle();
 
   ASSERT_TRUE(stream_delegate);
+  EXPECT_CALL(*p2p_quic_stream.get(), SetDelegate(nullptr));
+
   stream_delegate->OnRemoteReset();
   quic_stream->reset();
   EXPECT_EQ("closed", quic_stream->state());
@@ -328,6 +332,8 @@
   RunUntilIdle();
 
   ASSERT_TRUE(stream_delegate);
+  EXPECT_CALL(*p2p_quic_stream.get(), SetDelegate(nullptr));
+
   stream_delegate->OnRemoteReset();
 
   RunUntilIdle();
@@ -358,6 +364,8 @@
   RunUntilIdle();
 
   ASSERT_TRUE(stream_delegate);
+  EXPECT_CALL(*p2p_quic_stream.get(), SetDelegate(nullptr));
+
   stream_delegate->OnDataReceived({}, /*fin=*/true);
 
   RunUntilIdle();
@@ -384,6 +392,8 @@
   RunUntilIdle();
 
   ASSERT_TRUE(stream_delegate);
+  EXPECT_CALL(*p2p_quic_stream.get(), SetDelegate(nullptr));
+
   stream_delegate->OnRemoteReset();
 
   RunUntilIdle();
@@ -575,6 +585,8 @@
   RunUntilIdle();
 
   ASSERT_TRUE(stream_delegate);
+  EXPECT_CALL(*p2p_quic_stream.get(), SetDelegate(nullptr));
+
   stream_delegate->OnRemoteReset();
 
   RunUntilIdle();
@@ -878,6 +890,8 @@
 
   Persistent<RTCQuicStream> stream =
       CreateQuicStream(scope, p2p_quic_stream.get());
+  EXPECT_CALL(*p2p_quic_stream.get(), SetDelegate(nullptr));
+
   stream->reset();
 
   NotShared<DOMUint8Array> read_buffer(DOMUint8Array::Create(2));
@@ -1136,6 +1150,8 @@
 
   ASSERT_TRUE(stream_delegate);
   EXPECT_EQ("open", stream->state());
+  EXPECT_CALL(*p2p_quic_stream.get(), SetDelegate(nullptr));
+
   stream_delegate->OnRemoteReset();
 
   RunUntilIdle();
@@ -1167,6 +1183,7 @@
   EXPECT_TRUE(stream->readInto(read_buffer, ASSERT_NO_EXCEPTION)->finished());
 
   EXPECT_EQ("closing", stream->state());
+  EXPECT_CALL(*p2p_quic_stream.get(), SetDelegate(nullptr));
 
   stream->write(CreateWriteParametersWithoutData(/*finish=*/true),
                 ASSERT_NO_EXCEPTION);
@@ -1195,6 +1212,8 @@
   RunUntilIdle();
 
   ASSERT_TRUE(stream_delegate);
+  EXPECT_CALL(*p2p_quic_stream.get(), SetDelegate(nullptr));
+
   stream_delegate->OnDataReceived({}, /*fin=*/true);
 
   RunUntilIdle();