Use TraceWrapperV8Reference in ReadableStreamBytesConsumer

ReadableStreamBytesConsumer::reader_ has been a weak reference. This CL
makes it a TraceWrapperV8Reference as it is easier to understand.

Bug: 872462
Change-Id: I669daa87f854a4eab89810b7bf030fc4796d696b
Reviewed-on: https://chromium-review.googlesource.com/1249281Reviewed-by: Kentaro Hara <haraken@chromium.org>
Reviewed-by: Kouhei Ueno <kouhei@chromium.org>
Reviewed-by: Adam Rice <ricea@chromium.org>
Commit-Queue: Yutaka Hirano <yhirano@chromium.org>
Cr-Commit-Position: refs/heads/master@{#595392}

third_party/blink/renderer/bindings/core/v8/v8_wasm_response_extensions.cc

@@ -13,6 +13,7 @@
 #include "third_party/blink/renderer/core/fetch/fetch_data_loader.h"
 #include "third_party/blink/renderer/platform/bindings/exception_state.h"
 #include "third_party/blink/renderer/platform/bindings/script_state.h"
+#include "third_party/blink/renderer/platform/bindings/trace_wrapper_member.h"
 #include "third_party/blink/renderer/platform/bindings/v8_per_isolate_data.h"
 #include "third_party/blink/renderer/platform/heap/handle.h"
 
@@ -108,7 +109,7 @@ class FetchDataLoaderForWasmStreaming final : public FetchDataLoader,
       streaming_->Abort(v8::Local<v8::Value>());
     }
   }
-  Member<BytesConsumer> consumer_;
+  TraceWrapperMember<BytesConsumer> consumer_;
   Member<FetchDataLoader::Client> client_;
   std::shared_ptr<v8::WasmStreaming> streaming_;
   const Member<ScriptState> script_state_;
@@ -205,7 +206,7 @@ class FetchDataLoaderAsWasmModule final : public FetchDataLoader,
       builder_.Abort(v8::Local<v8::Value>());
     }
   }
-  Member<BytesConsumer> consumer_;
+  TraceWrapperMember<BytesConsumer> consumer_;
   Member<FetchDataLoader::Client> client_;
   v8::WasmModuleObjectBuilderStreaming builder_;
   const Member<ScriptState> script_state_;

third_party/blink/renderer/core/fetch/blob_bytes_consumer.h

@@ -9,6 +9,7 @@
 #include "base/memory/scoped_refptr.h"
 #include "third_party/blink/renderer/core/core_export.h"
 #include "third_party/blink/renderer/core/fetch/bytes_consumer.h"
+#include "third_party/blink/renderer/platform/bindings/trace_wrapper_member.h"
 #include "third_party/blink/renderer/platform/heap/handle.h"
 
 namespace blink {
@@ -42,7 +43,7 @@ class CORE_EXPORT BlobBytesConsumer final : public BytesConsumer {
  private:
   Member<ExecutionContext> execution_context_;
   scoped_refptr<BlobDataHandle> blob_data_handle_;
-  Member<BytesConsumer> nested_consumer_;
+  TraceWrapperMember<BytesConsumer> nested_consumer_;
   Member<BytesConsumer::Client> client_;
 };
 

third_party/blink/renderer/core/fetch/body_stream_buffer.cc

@@ -143,8 +143,9 @@ BodyStreamBuffer::BodyStreamBuffer(ScriptState* script_state,
       script_state_(script_state),
       signal_(nullptr),
       made_from_readable_stream_(true) {
-  // TODO(ricea): Perhaps this is not needed since the caller must have a strong
-  // reference to |stream| anyway?
+  // This is needed because sometimes a BodyStreamBuffer can be detached from
+  // the owner object such as Request. We rely on the wrapper and
+  // HasPendingActivity in such a case.
   RetainWrapperDuringConstruction(this, script_state);
   DCHECK(ReadableStreamOperations::IsReadableStreamForDCheck(script_state,
                                                              stream));

third_party/blink/renderer/core/fetch/body_stream_buffer.h

@@ -101,9 +101,9 @@ class CORE_EXPORT BodyStreamBuffer final : public UnderlyingSourceBase,
 
   Member<ScriptState> script_state_;
   TraceWrapperV8Reference<v8::Object> stream_;
-  Member<BytesConsumer> consumer_;
+  TraceWrapperMember<BytesConsumer> consumer_;
   // We need this member to keep it alive while loading.
-  Member<FetchDataLoader> loader_;
+  TraceWrapperMember<FetchDataLoader> loader_;
   // We need this to ensure that we detect that abort has been signalled
   // correctly.
   Member<AbortSignal> signal_;

third_party/blink/renderer/core/fetch/bytes_consumer.cc

@@ -10,6 +10,7 @@
 #include "third_party/blink/public/platform/task_type.h"
 #include "third_party/blink/renderer/core/execution_context/execution_context.h"
 #include "third_party/blink/renderer/core/fetch/blob_bytes_consumer.h"
+#include "third_party/blink/renderer/platform/bindings/trace_wrapper_member.h"
 #include "third_party/blink/renderer/platform/blob/blob_data.h"
 #include "third_party/blink/renderer/platform/heap/persistent.h"
 #include "third_party/blink/renderer/platform/wtf/functional.h"
@@ -301,7 +302,7 @@ class TeeHelper final : public GarbageCollectedFinalized<TeeHelper>,
     destination2_->Notify();
   }
 
-  Member<BytesConsumer> src_;
+  TraceWrapperMember<BytesConsumer> src_;
   Member<Destination> destination1_;
   Member<Destination> destination2_;
 };

third_party/blink/renderer/core/fetch/bytes_consumer.h

@@ -24,6 +24,8 @@ class ExecutionContext;
 // BytesConsumer has four states: waiting, readable, closed and errored. Once
 // the state becomes closed or errored, it will never change. |readable| means
 // that the BytesConsumer is ready to read non-empty bytes synchronously.
+// A BytesConsumer should be retained by TraceWrapperMember, not Member, as
+// a subclass has a reference to a v8::Value.
 class CORE_EXPORT BytesConsumer
     : public GarbageCollectedFinalized<BytesConsumer> {
  public:

third_party/blink/renderer/core/fetch/fetch_data_loader.cc

@@ -106,7 +106,7 @@ class FetchDataLoaderAsBlobHandle final : public FetchDataLoader,
   }
 
  private:
-  Member<BytesConsumer> consumer_;
+  TraceWrapperMember<BytesConsumer> consumer_;
   Member<FetchDataLoader::Client> client_;
 
   String mime_type_;
@@ -179,7 +179,7 @@ class FetchDataLoaderAsArrayBuffer final : public FetchDataLoader,
   }
 
  private:
-  Member<BytesConsumer> consumer_;
+  TraceWrapperMember<BytesConsumer> consumer_;
   Member<FetchDataLoader::Client> client_;
 
   std::unique_ptr<ArrayBufferBuilder> raw_data_;
@@ -235,7 +235,7 @@ class FetchDataLoaderAsFailure final : public FetchDataLoader,
   }
 
  private:
-  Member<BytesConsumer> consumer_;
+  TraceWrapperMember<BytesConsumer> consumer_;
   Member<FetchDataLoader::Client> client_;
 };
 
@@ -405,7 +405,7 @@ class FetchDataLoaderAsFormData final : public FetchDataLoader,
     std::unique_ptr<TextResourceDecoder> string_decoder_;
   };
 
-  Member<BytesConsumer> consumer_;
+  TraceWrapperMember<BytesConsumer> consumer_;
   Member<FetchDataLoader::Client> client_;
   Member<FormData> form_data_;
   Member<MultipartParser> multipart_parser_;
@@ -473,7 +473,7 @@ class FetchDataLoaderAsString final : public FetchDataLoader,
   }
 
  private:
-  Member<BytesConsumer> consumer_;
+  TraceWrapperMember<BytesConsumer> consumer_;
   Member<FetchDataLoader::Client> client_;
 
   std::unique_ptr<TextResourceDecoder> decoder_;
@@ -576,7 +576,7 @@ class FetchDataLoaderAsDataPipe final : public FetchDataLoader,
     out_data_pipe_.reset();
   }
 
-  Member<BytesConsumer> consumer_;
+  TraceWrapperMember<BytesConsumer> consumer_;
   Member<FetchDataLoader::Client> client_;
 
   mojo::ScopedDataPipeProducerHandle out_data_pipe_;

third_party/blink/renderer/core/fetch/fetch_manager.cc

@@ -192,7 +192,7 @@ class SRIBytesConsumer final : public BytesConsumer {
   }
 
  private:
-  Member<BytesConsumer> underlying_;
+  TraceWrapperMember<BytesConsumer> underlying_;
   Member<Client> client_;
   bool is_cancelled_ = false;
 };

third_party/blink/renderer/core/fetch/form_data_bytes_consumer.cc

@@ -468,7 +468,7 @@ class ComplexFormDataBytesConsumer final : public BytesConsumer {
 
  private:
   scoped_refptr<EncodedFormData> form_data_;
-  Member<BytesConsumer> blob_bytes_consumer_;
+  TraceWrapperMember<BytesConsumer> blob_bytes_consumer_;
 };
 
 }  // namespace

third_party/blink/renderer/core/fetch/form_data_bytes_consumer.h

@@ -8,6 +8,7 @@
 #include "base/memory/scoped_refptr.h"
 #include "third_party/blink/renderer/core/core_export.h"
 #include "third_party/blink/renderer/core/fetch/bytes_consumer.h"
+#include "third_party/blink/renderer/platform/bindings/trace_wrapper_member.h"
 #include "third_party/blink/renderer/platform/heap/handle.h"
 #include "third_party/blink/renderer/platform/wtf/forward.h"
 
@@ -71,7 +72,7 @@ class FormDataBytesConsumer final : public BytesConsumer {
                                     scoped_refptr<EncodedFormData>,
                                     BytesConsumer* consumer_for_testing);
 
-  const Member<BytesConsumer> impl_;
+  const TraceWrapperMember<BytesConsumer> impl_;
 };
 
 }  // namespace blink

third_party/blink/renderer/core/fetch/readable_stream_bytes_consumer.cc

@@ -98,7 +98,6 @@ ReadableStreamBytesConsumer::ReadableStreamBytesConsumer(
     ScriptValue stream_reader)
     : reader_(script_state->GetIsolate(), stream_reader.V8Value()),
       script_state_(script_state) {
-  reader_.SetPhantom();
 }
 
 ReadableStreamBytesConsumer::~ReadableStreamBytesConsumer() {}
@@ -172,6 +171,7 @@ BytesConsumer::Error ReadableStreamBytesConsumer::GetError() const {
 }
 
 void ReadableStreamBytesConsumer::Trace(blink::Visitor* visitor) {
+  visitor->Trace(reader_);
   visitor->Trace(client_);
   visitor->Trace(pending_buffer_);
   visitor->Trace(script_state_);

third_party/blink/renderer/core/fetch/readable_stream_bytes_consumer.h

@@ -11,6 +11,7 @@
 #include "third_party/blink/renderer/core/core_export.h"
 #include "third_party/blink/renderer/core/fetch/bytes_consumer.h"
 #include "third_party/blink/renderer/core/typed_arrays/dom_typed_array.h"
+#include "third_party/blink/renderer/platform/bindings/trace_wrapper_v8_reference.h"
 #include "third_party/blink/renderer/platform/heap/handle.h"
 #include "third_party/blink/renderer/platform/wtf/forward.h"
 
@@ -22,10 +23,6 @@ class ScriptState;
 // implemented with V8 Extras.
 // The stream will be immediately locked by the consumer and will never be
 // released.
-//
-// The ReadableStreamReader handle held in a ReadableStreamDataConsumerHandle
-// is weak. A user must guarantee that the ReadableStreamReader object is kept
-// alive appropriately.
 class CORE_EXPORT ReadableStreamBytesConsumer final : public BytesConsumer {
   USING_PRE_FINALIZER(ReadableStreamBytesConsumer, Dispose);
 
@@ -55,11 +52,7 @@ class CORE_EXPORT ReadableStreamBytesConsumer final : public BytesConsumer {
   void OnRejected();
   void Notify();
 
-  // |m_reader| is a weak persistent. It should be kept alive by someone
-  // outside of ReadableStreamBytesConsumer.
-  // Holding a ScopedPersistent here is safe in terms of cross-world wrapper
-  // leakage because we read only Uint8Array chunks from the reader.
-  ScopedPersistent<v8::Value> reader_;
+  TraceWrapperV8Reference<v8::Value> reader_;
   Member<ScriptState> script_state_;
   Member<BytesConsumer::Client> client_;
   Member<DOMUint8Array> pending_buffer_;