Streams: Split construction from initialisation

Previously, initialisation of ReadableStreamNative, WritableStreamNative
and TransformStreamNative objects was done in the constructor. However,
this resulted in calling into JavaScript inside the constructor, which
could cause problems with garbage collection.

Move the code from inside the constructor to a separate InitInternal()
method which is called from Create(). The content of the new methods is
copied verbatim from the constructors so there should be no functional
differences.

Manually tested. There are no automated tests as I don't know how to
make them not be flaky.

BUG=1008470

Change-Id: I394cc60b0bac397dccbef18ca166a66528c22bd0
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1837326Reviewed-by: Yutaka Hirano <yhirano@chromium.org>
Commit-Queue: Adam Rice <ricea@chromium.org>
Cr-Commit-Position: refs/heads/master@{#704104}

third_party/blink/renderer/core/streams/readable_stream_native.cc

@@ -965,8 +965,9 @@ ReadableStreamNative* ReadableStreamNative::Create(
     ScriptValue underlying_source,
     ScriptValue strategy,
     ExceptionState& exception_state) {
-  auto* stream = MakeGarbageCollected<ReadableStreamNative>(
-      script_state, underlying_source, strategy, false, exception_state);
+  auto* stream = MakeGarbageCollected<ReadableStreamNative>();
+  stream->InitInternal(script_state, underlying_source, strategy, false,
+                       exception_state);
   if (exception_state.HadException()) {
     return nullptr;
   }
@@ -1001,7 +1002,8 @@ ReadableStreamNative* ReadableStreamNative::CreateWithCountQueueingStrategy(
   v8::Local<v8::Value> underlying_source_v8 =
       ToV8(underlying_source, script_state);
 
-  auto* stream = MakeGarbageCollected<ReadableStreamNative>(
+  auto* stream = MakeGarbageCollected<ReadableStreamNative>();
+  stream->InitInternal(
       script_state,
       ScriptValue(script_state->GetIsolate(), underlying_source_v8),
       ScriptValue(script_state->GetIsolate(), strategy_object), true,
@@ -1060,99 +1062,6 @@ ReadableStreamNative* ReadableStreamNative::Create(
 
 ReadableStreamNative::ReadableStreamNative() = default;
 
-ReadableStreamNative::ReadableStreamNative(ScriptState* script_state,
-                                           ScriptValue raw_underlying_source,
-                                           ScriptValue raw_strategy,
-                                           bool created_by_ua,
-                                           ExceptionState& exception_state) {
-  if (!created_by_ua) {
-    // TODO(ricea): Move this to IDL once blink::ReadableStreamOperations is
-    // no longer using the public constructor.
-    UseCounter::Count(ExecutionContext::From(script_state),
-                      WebFeature::kReadableStreamConstructor);
-  }
-
-  // https://streams.spec.whatwg.org/#rs-constructor
-  //  1. Perform ! InitializeReadableStream(this).
-  Initialize(this);
-
-  // The next part of this constructor corresponds to the object conversions
-  // that are implicit in the definition in the standard.
-  DCHECK(!raw_underlying_source.IsEmpty());
-  DCHECK(!raw_strategy.IsEmpty());
-
-  auto context = script_state->GetContext();
-  auto* isolate = script_state->GetIsolate();
-
-  v8::Local<v8::Object> underlying_source;
-  ScriptValueToObject(script_state, raw_underlying_source, &underlying_source,
-                      exception_state);
-  if (exception_state.HadException()) {
-    return;
-  }
-
-  // 2. Let size be ? GetV(strategy, "size").
-  // 3. Let highWaterMark be ? GetV(strategy, "highWaterMark").
-  StrategyUnpacker strategy_unpacker(script_state, raw_strategy,
-                                     exception_state);
-  if (exception_state.HadException()) {
-    return;
-  }
-
-  // 4. Let type be ? GetV(underlyingSource, "type").
-  v8::TryCatch try_catch(isolate);
-  v8::Local<v8::Value> type;
-  if (!underlying_source->Get(context, V8AtomicString(isolate, "type"))
-           .ToLocal(&type)) {
-    exception_state.RethrowV8Exception(try_catch.Exception());
-    return;
-  }
-
-  if (!type->IsUndefined()) {
-    // 5. Let typeString be ? ToString(type).
-    v8::Local<v8::String> type_string;
-    if (!type->ToString(context).ToLocal(&type_string)) {
-      exception_state.RethrowV8Exception(try_catch.Exception());
-      return;
-    }
-
-    // 6. If typeString is "bytes",
-    if (type_string == V8AtomicString(isolate, "bytes")) {
-      // TODO(ricea): Implement bytes type.
-      exception_state.ThrowRangeError("bytes type is not yet implemented");
-      return;
-    }
-
-    // 8. Otherwise, throw a RangeError exception.
-    exception_state.ThrowRangeError("Invalid type is specified");
-    return;
-  }
-
-  // 7. Otherwise, if type is undefined,
-  //   a. Let sizeAlgorithm be ? MakeSizeAlgorithmFromSizeFunction(size).
-  auto* size_algorithm =
-      strategy_unpacker.MakeSizeAlgorithm(script_state, exception_state);
-  if (exception_state.HadException()) {
-    return;
-  }
-  DCHECK(size_algorithm);
-
-  //   b. If highWaterMark is undefined, let highWaterMark be 1.
-  //   c. Set highWaterMark to ? ValidateAndNormalizeHighWaterMark(
-  //      highWaterMark).
-  double high_water_mark =
-      strategy_unpacker.GetHighWaterMark(script_state, 1, exception_state);
-  if (exception_state.HadException()) {
-    return;
-  }
-
-  // 4. Perform ? SetUpReadableStreamDefaultControllerFromUnderlyingSource
-  //  (this, underlyingSource, highWaterMark, sizeAlgorithm).
-  ReadableStreamDefaultController::SetUpFromUnderlyingSource(
-      script_state, this, underlying_source, high_water_mark, size_algorithm,
-      exception_state);
-}
-
 ReadableStreamNative::~ReadableStreamNative() = default;
 
 bool ReadableStreamNative::locked(ScriptState* script_state,
@@ -1296,6 +1205,102 @@ ScriptValue ReadableStreamNative::tee(ScriptState* script_state,
   return CallTeeAndReturnBranchArray(script_state, this, exception_state);
 }
 
+// Unlike in the standard, this is defined as a separate method from the
+// constructor. This prevents problems when garbage collection happens
+// re-entrantly during construction.
+void ReadableStreamNative::InitInternal(ScriptState* script_state,
+                                        ScriptValue raw_underlying_source,
+                                        ScriptValue raw_strategy,
+                                        bool created_by_ua,
+                                        ExceptionState& exception_state) {
+  if (!created_by_ua) {
+    // TODO(ricea): Move this to IDL once blink::ReadableStreamOperations is
+    // no longer using the public constructor.
+    UseCounter::Count(ExecutionContext::From(script_state),
+                      WebFeature::kReadableStreamConstructor);
+  }
+
+  // https://streams.spec.whatwg.org/#rs-constructor
+  //  1. Perform ! InitializeReadableStream(this).
+  Initialize(this);
+
+  // The next part of this constructor corresponds to the object conversions
+  // that are implicit in the definition in the standard.
+  DCHECK(!raw_underlying_source.IsEmpty());
+  DCHECK(!raw_strategy.IsEmpty());
+
+  auto context = script_state->GetContext();
+  auto* isolate = script_state->GetIsolate();
+
+  v8::Local<v8::Object> underlying_source;
+  ScriptValueToObject(script_state, raw_underlying_source, &underlying_source,
+                      exception_state);
+  if (exception_state.HadException()) {
+    return;
+  }
+
+  // 2. Let size be ? GetV(strategy, "size").
+  // 3. Let highWaterMark be ? GetV(strategy, "highWaterMark").
+  StrategyUnpacker strategy_unpacker(script_state, raw_strategy,
+                                     exception_state);
+  if (exception_state.HadException()) {
+    return;
+  }
+
+  // 4. Let type be ? GetV(underlyingSource, "type").
+  v8::TryCatch try_catch(isolate);
+  v8::Local<v8::Value> type;
+  if (!underlying_source->Get(context, V8AtomicString(isolate, "type"))
+           .ToLocal(&type)) {
+    exception_state.RethrowV8Exception(try_catch.Exception());
+    return;
+  }
+
+  if (!type->IsUndefined()) {
+    // 5. Let typeString be ? ToString(type).
+    v8::Local<v8::String> type_string;
+    if (!type->ToString(context).ToLocal(&type_string)) {
+      exception_state.RethrowV8Exception(try_catch.Exception());
+      return;
+    }
+
+    // 6. If typeString is "bytes",
+    if (type_string == V8AtomicString(isolate, "bytes")) {
+      // TODO(ricea): Implement bytes type.
+      exception_state.ThrowRangeError("bytes type is not yet implemented");
+      return;
+    }
+
+    // 8. Otherwise, throw a RangeError exception.
+    exception_state.ThrowRangeError("Invalid type is specified");
+    return;
+  }
+
+  // 7. Otherwise, if type is undefined,
+  //   a. Let sizeAlgorithm be ? MakeSizeAlgorithmFromSizeFunction(size).
+  auto* size_algorithm =
+      strategy_unpacker.MakeSizeAlgorithm(script_state, exception_state);
+  if (exception_state.HadException()) {
+    return;
+  }
+  DCHECK(size_algorithm);
+
+  //   b. If highWaterMark is undefined, let highWaterMark be 1.
+  //   c. Set highWaterMark to ? ValidateAndNormalizeHighWaterMark(
+  //      highWaterMark).
+  double high_water_mark =
+      strategy_unpacker.GetHighWaterMark(script_state, 1, exception_state);
+  if (exception_state.HadException()) {
+    return;
+  }
+
+  // 4. Perform ? SetUpReadableStreamDefaultControllerFromUnderlyingSource
+  //  (this, underlyingSource, highWaterMark, sizeAlgorithm).
+  ReadableStreamDefaultController::SetUpFromUnderlyingSource(
+      script_state, this, underlying_source, high_water_mark, size_algorithm,
+      exception_state);
+}
+
 //
 // Readable stream abstract operations
 //

third_party/blink/renderer/core/streams/readable_stream_native.h

@@ -67,13 +67,6 @@ class ReadableStreamNative : public ReadableStream {
 
   ReadableStreamNative();
 
-  // https://streams.spec.whatwg.org/#rs-constructor
-  ReadableStreamNative(ScriptState*,
-                       ScriptValue raw_underlying_source,
-                       ScriptValue raw_strategy,
-                       bool created_by_ua,
-                       ExceptionState&);
-
   ~ReadableStreamNative() override;
 
   // https://streams.spec.whatwg.org/#rs-constructor
@@ -208,6 +201,13 @@ class ReadableStreamNative : public ReadableStream {
   class ReadHandleImpl;
   class TeeEngine;
 
+  // https://streams.spec.whatwg.org/#rs-constructor
+  void InitInternal(ScriptState*,
+                    ScriptValue raw_underlying_source,
+                    ScriptValue raw_strategy,
+                    bool created_by_ua,
+                    ExceptionState&);
+
   // https://streams.spec.whatwg.org/#initialize-readable-stream
   static void Initialize(ReadableStreamNative*);
 

third_party/blink/renderer/core/streams/transform_stream_native.h

@@ -87,6 +87,14 @@ class CORE_EXPORT TransformStreamNative final
   class DefaultSourcePullAlgorithm;
   class DefaultSourceCancelAlgorithm;
 
+  // Performs the functions performed by the constructor in the standard.
+  // https://streams.spec.whatwg.org/#ts-constructor
+  void InitInternal(ScriptState*,
+                    ScriptValue raw_transformer,
+                    ScriptValue raw_writable_strategy,
+                    ScriptValue raw_readable_strategy,
+                    ExceptionState&);
+
   // https://streams.spec.whatwg.org/#initialize-transform-stream
   static void Initialize(ScriptState*,
                          TransformStreamNative*,

third_party/blink/renderer/core/streams/writable_stream.cc

@@ -35,13 +35,8 @@ WritableStream* WritableStream::Create(ScriptState* script_state,
                                        ScriptValue underlying_sink,
                                        ScriptValue strategy,
                                        ExceptionState& exception_state) {
-  WritableStream* stream = MakeGarbageCollected<WritableStreamNative>(
-      script_state, underlying_sink, strategy, exception_state);
-
-  if (exception_state.HadException())
-    return nullptr;
-
-  return stream;
+  return WritableStreamNative::Create(script_state, underlying_sink, strategy,
+                                      exception_state);
 }
 
 WritableStream* WritableStream::CreateWithCountQueueingStrategy(

third_party/blink/renderer/core/streams/writable_stream_native.cc

@@ -64,74 +64,21 @@ class WritableStreamNative::PendingAbortRequest final
   DISALLOW_COPY_AND_ASSIGN(PendingAbortRequest);
 };
 
-WritableStreamNative::WritableStreamNative() = default;
-
-WritableStreamNative::WritableStreamNative(ScriptState* script_state,
+WritableStreamNative* WritableStreamNative::Create(
+    ScriptState* script_state,
     ScriptValue raw_underlying_sink,
     ScriptValue raw_strategy,
     ExceptionState& exception_state) {
-  // The first parts of this constructor correspond to the object conversions
-  // that are implicit in the definition in the standard:
-  // https://streams.spec.whatwg.org/#ws-constructor
-  DCHECK(!raw_underlying_sink.IsEmpty());
-  DCHECK(!raw_strategy.IsEmpty());
-
-  auto context = script_state->GetContext();
-  auto* isolate = script_state->GetIsolate();
-
-  v8::Local<v8::Object> underlying_sink;
-  ScriptValueToObject(script_state, raw_underlying_sink, &underlying_sink,
-                      exception_state);
-  if (exception_state.HadException()) {
-    return;
-  }
-
-  // 2. Let size be ? GetV(strategy, "size").
-  // 3. Let highWaterMark be ? GetV(strategy, "highWaterMark").
-  StrategyUnpacker strategy_unpacker(script_state, raw_strategy,
+  auto* stream = MakeGarbageCollected<WritableStreamNative>();
+  stream->InitInternal(script_state, raw_underlying_sink, raw_strategy,
                        exception_state);
   if (exception_state.HadException()) {
-    return;
-  }
-
-  // 4. Let type be ? GetV(underlyingSink, "type").
-  v8::TryCatch try_catch(isolate);
-  v8::Local<v8::Value> type;
-  if (!underlying_sink->Get(context, V8AtomicString(isolate, "type"))
-           .ToLocal(&type)) {
-    exception_state.RethrowV8Exception(try_catch.Exception());
-    return;
-  }
-
-  // 5. If type is not undefined, throw a RangeError exception.
-  if (!type->IsUndefined()) {
-    exception_state.ThrowRangeError("Invalid type is specified");
-    return;
-  }
-
-  // 6. Let sizeAlgorithm be ? MakeSizeAlgorithmFromSizeFunction(size).
-  auto* size_algorithm =
-      strategy_unpacker.MakeSizeAlgorithm(script_state, exception_state);
-  if (exception_state.HadException()) {
-    return;
-  }
-  DCHECK(size_algorithm);
-
-  // 7. If highWaterMark is undefined, let highWaterMark be 1.
-  // 8. Set highWaterMark to ? ValidateAndNormalizeHighWaterMark(highWaterMark).
-  double high_water_mark =
-      strategy_unpacker.GetHighWaterMark(script_state, 1, exception_state);
-  if (exception_state.HadException()) {
-    return;
+    return nullptr;
   }
-
-  // 9. Perform ? SetUpWritableStreamDefaultControllerFromUnderlyingSink(this,
-  //    underlyingSink, highWaterMark, sizeAlgorithm).
-  WritableStreamDefaultController::SetUpFromUnderlyingSink(
-      script_state, this, underlying_sink, high_water_mark, size_algorithm,
-      exception_state);
+  return stream;
 }
 
+WritableStreamNative::WritableStreamNative() = default;
 WritableStreamNative::~WritableStreamNative() = default;
 
 bool WritableStreamNative::locked(ScriptState* script_state,
@@ -241,8 +188,9 @@ WritableStreamNative* WritableStreamNative::CreateWithCountQueueingStrategy(
   ExceptionState exception_state(script_state->GetIsolate(),
                                  ExceptionState::kConstructionContext,
                                  "WritableStream");
-  auto* stream = MakeGarbageCollected<WritableStreamNative>(
-      script_state, underlying_sink_value, strategy_value, exception_state);
+  auto* stream = MakeGarbageCollected<WritableStreamNative>();
+  stream->InitInternal(script_state, underlying_sink_value, strategy_value,
+                       exception_state);
   if (exception_state.HadException())
     return nullptr;
   return stream;
@@ -789,6 +737,74 @@ void WritableStreamNative::Trace(Visitor* visitor) {
   WritableStream::Trace(visitor);
 }
 
+// This is not implemented inside the constructor in C++, because calling into
+// JavaScript from the constructor can cause GC problems.
+void WritableStreamNative::InitInternal(ScriptState* script_state,
+                                        ScriptValue raw_underlying_sink,
+                                        ScriptValue raw_strategy,
+                                        ExceptionState& exception_state) {
+  // The first parts of this constructor implementation correspond to the object
+  // conversions that are implicit in the definition in the standard:
+  // https://streams.spec.whatwg.org/#ws-constructor
+  DCHECK(!raw_underlying_sink.IsEmpty());
+  DCHECK(!raw_strategy.IsEmpty());
+
+  auto context = script_state->GetContext();
+  auto* isolate = script_state->GetIsolate();
+
+  v8::Local<v8::Object> underlying_sink;
+  ScriptValueToObject(script_state, raw_underlying_sink, &underlying_sink,
+                      exception_state);
+  if (exception_state.HadException()) {
+    return;
+  }
+
+  // 2. Let size be ? GetV(strategy, "size").
+  // 3. Let highWaterMark be ? GetV(strategy, "highWaterMark").
+  StrategyUnpacker strategy_unpacker(script_state, raw_strategy,
+                                     exception_state);
+  if (exception_state.HadException()) {
+    return;
+  }
+
+  // 4. Let type be ? GetV(underlyingSink, "type").
+  v8::TryCatch try_catch(isolate);
+  v8::Local<v8::Value> type;
+  if (!underlying_sink->Get(context, V8AtomicString(isolate, "type"))
+           .ToLocal(&type)) {
+    exception_state.RethrowV8Exception(try_catch.Exception());
+    return;
+  }
+
+  // 5. If type is not undefined, throw a RangeError exception.
+  if (!type->IsUndefined()) {
+    exception_state.ThrowRangeError("Invalid type is specified");
+    return;
+  }
+
+  // 6. Let sizeAlgorithm be ? MakeSizeAlgorithmFromSizeFunction(size).
+  auto* size_algorithm =
+      strategy_unpacker.MakeSizeAlgorithm(script_state, exception_state);
+  if (exception_state.HadException()) {
+    return;
+  }
+  DCHECK(size_algorithm);
+
+  // 7. If highWaterMark is undefined, let highWaterMark be 1.
+  // 8. Set highWaterMark to ? ValidateAndNormalizeHighWaterMark(highWaterMark).
+  double high_water_mark =
+      strategy_unpacker.GetHighWaterMark(script_state, 1, exception_state);
+  if (exception_state.HadException()) {
+    return;
+  }
+
+  // 9. Perform ? SetUpWritableStreamDefaultControllerFromUnderlyingSink(this,
+  //    underlyingSink, highWaterMark, sizeAlgorithm).
+  WritableStreamDefaultController::SetUpFromUnderlyingSink(
+      script_state, this, underlying_sink, high_water_mark, size_algorithm,
+      exception_state);
+}
+
 bool WritableStreamNative::HasOperationMarkedInFlight(
     const WritableStreamNative* stream) {
   // https://streams.spec.whatwg.org/#writable-stream-has-operation-marked-in-flight

third_party/blink/renderer/core/streams/writable_stream_native.h

@@ -33,6 +33,12 @@ class CORE_EXPORT WritableStreamNative : public WritableStream {
     kErrored,
   };
 
+  // https://streams.spec.whatwg.org/#ws-constructor
+  static WritableStreamNative* Create(ScriptState*,
+                                      ScriptValue raw_underlying_sink,
+                                      ScriptValue raw_strategy,
+                                      ExceptionState&);
+
   // https://streams.spec.whatwg.org/#create-writable-stream
   // Unlike in the standard, |high_water_mark| and |size_algorithm| are
   // required parameters.
@@ -50,15 +56,8 @@ class CORE_EXPORT WritableStreamNative : public WritableStream {
       UnderlyingSinkBase*,
       size_t high_water_mark);
 
-  // Used by Create().
+  // Called by Create().
   WritableStreamNative();
-
-  // Used when creating a stream from JavaScript.
-  // https://streams.spec.whatwg.org/#ws-constructor
-  WritableStreamNative(ScriptState*,
-                       ScriptValue raw_underlying_sink,
-                       ScriptValue raw_strategy,
-                       ExceptionState&);
   ~WritableStreamNative() override;
 
   // IDL defined functions
@@ -201,6 +200,13 @@ class CORE_EXPORT WritableStreamNative : public WritableStream {
 
   class PendingAbortRequest;
 
+  // Used when creating a stream from JavaScript. Called from Create().
+  // https://streams.spec.whatwg.org/#ws-constructor
+  void InitInternal(ScriptState*,
+                    ScriptValue raw_underlying_sink,
+                    ScriptValue raw_strategy,
+                    ExceptionState&);
+
   // https://streams.spec.whatwg.org/#writable-stream-has-operation-marked-in-flight
   static bool HasOperationMarkedInFlight(const WritableStreamNative*);