Handle failure of constructor wrapper retention

The call to PostTask() which is used to make objects created from
bindings code traceable before the constructor returns can fail if the
runner is in the process of shutting down. Correctly handle failure.

In the case of TextEncoderStream and TextDecoderStream, failure is
handled by making the constructor throw. In the case of
BodyStreamBuffer, the stream is marked as broken.

Also consolidate the three copies of the
RetainWrapperUntilV8WrapperGetReturnedToV8() method into a single
RetainWrapperDuringConstruction() function.

BUG=882599

Change-Id: I056b85cc980785c0df3a946a272937c9928f31ee
Reviewed-on: https://chromium-review.googlesource.com/1235359Reviewed-by: Yutaka Hirano <yhirano@chromium.org>
Reviewed-by: Joshua Bell <jsbell@chromium.org>
Commit-Queue: Adam Rice <ricea@chromium.org>
Cr-Commit-Position: refs/heads/master@{#593043}

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

@@ -11,6 +11,7 @@
 #include "third_party/blink/renderer/core/fetch/readable_stream_bytes_consumer.h"
 #include "third_party/blink/renderer/core/streams/readable_stream_default_controller_wrapper.h"
 #include "third_party/blink/renderer/core/streams/readable_stream_operations.h"
+#include "third_party/blink/renderer/core/streams/retain_wrapper_during_construction.h"
 #include "third_party/blink/renderer/core/typed_arrays/dom_array_buffer.h"
 #include "third_party/blink/renderer/core/typed_arrays/dom_typed_array.h"
 #include "third_party/blink/renderer/platform/bindings/exception_code.h"
@@ -99,7 +100,8 @@ BodyStreamBuffer::BodyStreamBuffer(ScriptState* script_state,
       consumer_(consumer),
       signal_(signal),
       made_from_readable_stream_(false) {
-  RetainWrapperUntilV8WrapperGetReturnedToV8(script_state);
+  if (!RetainWrapperDuringConstruction(this, script_state))
+    stream_broken_ = true;
 
   {
     // Leaving an exception pending will cause Blink to crash in the bindings
@@ -141,7 +143,9 @@ BodyStreamBuffer::BodyStreamBuffer(ScriptState* script_state,
       script_state_(script_state),
       signal_(nullptr),
       made_from_readable_stream_(true) {
-  RetainWrapperUntilV8WrapperGetReturnedToV8(script_state);
+  // TODO(ricea): Perhaps this is not needed since the caller must have a strong
+  // reference to |stream| anyway?
+  RetainWrapperDuringConstruction(this, script_state);
   DCHECK(ReadableStreamOperations::IsReadableStreamForDCheck(script_state,
                                                              stream));
 
@@ -529,20 +533,6 @@ base::Optional<bool> BodyStreamBuffer::BooleanStreamOperation(
   return result;
 }
 
-void BodyStreamBuffer::RetainWrapperUntilV8WrapperGetReturnedToV8(
-    ScriptState* script_state) {
-  bool post_task_succeeded =
-      ExecutionContext::From(script_state)
-          ->GetTaskRunner(TaskType::kInternalDefault)
-          ->PostTask(FROM_HERE,
-                     WTF::Bind(Noop, ScriptValue(script_state,
-                                                 ToV8(this, script_state))));
-  // Temporary CHECK to find out if and how often this PostTask fails.
-  // TODO(ricea): Set stream_broken_ to false if PostTask fails instead of
-  // crashing.
-  CHECK(post_task_succeeded);
-}
-
 BytesConsumer* BodyStreamBuffer::ReleaseHandle(
     ExceptionState& exception_state) {
   DCHECK(!IsStreamLockedForDCheck());

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

@@ -80,14 +80,6 @@ class CORE_EXPORT BodyStreamBuffer final : public UnderlyingSourceBase,
  private:
   class LoaderClient;
 
-  // We need to keep the wrapper alive in order to make
-  // |Stream()| alive. We can create a wrapper in the constructor, but there is
-  // a chance that GC happens after construction happens before the wrapper is
-  // connected to the value returned to the user in the JS world. This function
-  // posts a task with a ScriptPromise containing the wrapper to avoid that.
-  // TODO(yhirano): Remove this once the unified GC is available.
-  void RetainWrapperUntilV8WrapperGetReturnedToV8(ScriptState*);
-
   BytesConsumer* ReleaseHandle(ExceptionState&);
   void Abort();
   void Close();
@@ -107,8 +99,6 @@ class CORE_EXPORT BodyStreamBuffer final : public UnderlyingSourceBase,
                                         ExceptionState&),
       ExceptionState& exception_state);
 
-  static void Noop(ScriptValue) {}
-
   Member<ScriptState> script_state_;
   TraceWrapperV8Reference<v8::Object> stream_;
   Member<BytesConsumer> consumer_;

third_party/blink/renderer/core/streams/BUILD.gn

@@ -9,6 +9,8 @@ blink_core_sources("streams") {
     "readable_stream_default_controller_wrapper.h",
     "readable_stream_operations.cc",
     "readable_stream_operations.h",
+    "retain_wrapper_during_construction.cc",
+    "retain_wrapper_during_construction.h",
     "transform_stream.cc",
     "transform_stream.h",
     "transform_stream_default_controller.cc",

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

+// Copyright 2018 The Chromium Authors. All rights reserved.
+// Use of this source code is governed by a BSD-style license that can be
+// found in the LICENSE file.
+
+#include "third_party/blink/renderer/core/streams/retain_wrapper_during_construction.h"
+
+#include <utility>
+
+#include "base/sequenced_task_runner.h"
+#include "third_party/blink/public/platform/task_type.h"
+#include "third_party/blink/renderer/bindings/core/v8/script_value.h"
+#include "third_party/blink/renderer/core/execution_context/execution_context.h"
+#include "third_party/blink/renderer/platform/bindings/script_state.h"
+#include "third_party/blink/renderer/platform/bindings/script_wrappable.h"
+#include "third_party/blink/renderer/platform/bindings/to_v8.h"
+#include "third_party/blink/renderer/platform/wtf/functional.h"
+
+namespace blink {
+
+// This function is defined here, rather than in platform/bindings, because it
+// needs to use ScriptValue and ExecutionContext.
+bool RetainWrapperDuringConstruction(ScriptWrappable* script_wrapper,
+                                     ScriptState* script_state) {
+  const auto noop = [](ScriptValue) -> void {};
+  auto task_runner = ExecutionContext::From(script_state)
+                         ->GetTaskRunner(TaskType::kInternalDefault);
+  ScriptValue v8_wrapper(script_state, ToV8(script_wrapper, script_state));
+  bool post_task_succeeded =
+      task_runner->PostTask(FROM_HERE, WTF::Bind(noop, std::move(v8_wrapper)));
+  return post_task_succeeded;
+}
+
+}  // namespace blink

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

+// Copyright 2018 The Chromium Authors. All rights reserved.
+// Use of this source code is governed by a BSD-style license that can be
+// found in the LICENSE file.
+
+#ifndef THIRD_PARTY_BLINK_RENDERER_CORE_STREAMS_RETAIN_WRAPPER_DURING_CONSTRUCTION_H_
+#define THIRD_PARTY_BLINK_RENDERER_CORE_STREAMS_RETAIN_WRAPPER_DURING_CONSTRUCTION_H_
+
+#include "third_party/blink/renderer/core/core_export.h"
+
+namespace blink {
+
+class ScriptState;
+class ScriptWrappable;
+
+// Objects which
+// a) directly or indirectly own V8 objects held via TraceWrapperV8References,
+//    and
+// b) are constructed via the bindings
+// need to call this function at the start of the constructor to ensure that a
+// wrapper object exists for V8 to trace through. This ensures that the
+// TraceWrapperV8Reference will not be collected even if a GC takes place during
+// construction.
+//
+// This function posts a task containing a strong reference. The reference
+// remains until the task is executed. This is slightly longer than necessary,
+// but it shouldn't matter in practice.
+//
+// This function returns false if it fails. Usually this means the task runner
+// is in the process of being destroyed. The caller must ensure that the
+// reference is not used in this case.
+//
+// TODO(yhirano): Remove this once the unified GC is available.
+bool CORE_EXPORT RetainWrapperDuringConstruction(ScriptWrappable*,
+                                                 ScriptState*);
+
+}  // namespace blink
+
+#endif  // THIRD_PARTY_BLINK_RENDERER_CORE_STREAMS_RETAIN_WRAPPER_DURING_CONSTRUCTION_H_

third_party/blink/renderer/modules/encoding/text_decoder_stream.cc

@@ -7,10 +7,9 @@
 #include <memory>
 #include <utility>
 
-#include "third_party/blink/public/platform/task_type.h"
 #include "third_party/blink/renderer/bindings/core/v8/array_buffer_or_array_buffer_view.h"
 #include "third_party/blink/renderer/bindings/core/v8/v8_binding_for_core.h"
-#include "third_party/blink/renderer/core/execution_context/execution_context.h"
+#include "third_party/blink/renderer/core/streams/retain_wrapper_during_construction.h"
 #include "third_party/blink/renderer/core/streams/transform_stream_default_controller.h"
 #include "third_party/blink/renderer/core/streams/transform_stream_transformer.h"
 #include "third_party/blink/renderer/core/typed_arrays/dom_typed_array.h"
@@ -19,7 +18,6 @@
 #include "third_party/blink/renderer/platform/bindings/exception_messages.h"
 #include "third_party/blink/renderer/platform/bindings/exception_state.h"
 #include "third_party/blink/renderer/platform/bindings/to_v8.h"
-#include "third_party/blink/renderer/platform/wtf/functional.h"
 #include "third_party/blink/renderer/platform/wtf/string_extras.h"
 #include "third_party/blink/renderer/platform/wtf/text/text_codec.h"
 #include "third_party/blink/renderer/platform/wtf/text/text_encoding.h"
@@ -184,18 +182,6 @@ void TextDecoderStream::Trace(Visitor* visitor) {
   ScriptWrappable::Trace(visitor);
 }
 
-// static
-void TextDecoderStream::Noop(ScriptValue) {}
-
-void TextDecoderStream::RetainWrapperUntilV8WrapperGetReturnedToV8(
-    ScriptState* script_state) {
-  ExecutionContext::From(script_state)
-      ->GetTaskRunner(TaskType::kInternalDefault)
-      ->PostTask(
-          FROM_HERE,
-          WTF::Bind(Noop, ScriptValue(script_state, ToV8(this, script_state))));
-}
-
 TextDecoderStream::TextDecoderStream(ScriptState* script_state,
                                      const WTF::TextEncoding& encoding,
                                      const TextDecoderOptions& options,
@@ -204,7 +190,11 @@ TextDecoderStream::TextDecoderStream(ScriptState* script_state,
       encoding_(encoding),
       fatal_(options.fatal()),
       ignore_bom_(options.ignoreBOM()) {
-  RetainWrapperUntilV8WrapperGetReturnedToV8(script_state);
+  if (!RetainWrapperDuringConstruction(this, script_state)) {
+    exception_state.ThrowDOMException(DOMExceptionCode::kInvalidStateError,
+                                      "Cannot queue task to retain wrapper");
+    return;
+  }
   transform_->Init(new Transformer(script_state, encoding, fatal_, ignore_bom_),
                    script_state, exception_state);
 }

third_party/blink/renderer/modules/encoding/text_decoder_stream.h

@@ -45,21 +45,12 @@ class TextDecoderStream final : public ScriptWrappable {
 
  private:
   class Transformer;
-  static void Noop(ScriptValue);
 
   TextDecoderStream(ScriptState*,
                     const WTF::TextEncoding&,
                     const TextDecoderOptions&,
                     ExceptionState&);
 
-  // We need to keep the wrapper alive in order to make |transform_| alive. We
-  // can create a wrapper in the constructor, but there is a chance that GC
-  // happens after construction happens before the wrapper is connected to the
-  // value returned to the user in the JS world. This function posts a task with
-  // a ScriptPromise containing the wrapper to avoid that.
-  // TODO(ricea): Remove this once the unified GC is available.
-  void RetainWrapperUntilV8WrapperGetReturnedToV8(ScriptState*);
-
   const TraceWrapperMember<TransformStream> transform_;
   const WTF::TextEncoding encoding_;
   const bool fatal_;

third_party/blink/renderer/modules/encoding/text_encoder_stream.cc

@@ -12,16 +12,14 @@
 
 #include "base/optional.h"
 #include "base/stl_util.h"
-#include "third_party/blink/public/platform/task_type.h"
 #include "third_party/blink/renderer/bindings/core/v8/v8_string_resource.h"
-#include "third_party/blink/renderer/core/execution_context/execution_context.h"
+#include "third_party/blink/renderer/core/streams/retain_wrapper_during_construction.h"
 #include "third_party/blink/renderer/core/streams/transform_stream_default_controller.h"
 #include "third_party/blink/renderer/core/streams/transform_stream_transformer.h"
 #include "third_party/blink/renderer/core/typed_arrays/dom_typed_array.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/to_v8.h"
-#include "third_party/blink/renderer/platform/wtf/functional.h"
 #include "third_party/blink/renderer/platform/wtf/text/cstring.h"
 #include "third_party/blink/renderer/platform/wtf/text/text_codec.h"
 #include "third_party/blink/renderer/platform/wtf/text/text_encoding.h"
@@ -187,22 +185,14 @@ void TextEncoderStream::Trace(Visitor* visitor) {
   ScriptWrappable::Trace(visitor);
 }
 
-// static
-void TextEncoderStream::Noop(ScriptValue) {}
-
-void TextEncoderStream::RetainWrapperUntilV8WrapperGetReturnedToV8(
-    ScriptState* script_state) {
-  ExecutionContext::From(script_state)
-      ->GetTaskRunner(TaskType::kInternalDefault)
-      ->PostTask(
-          FROM_HERE,
-          WTF::Bind(Noop, ScriptValue(script_state, ToV8(this, script_state))));
-}
-
 TextEncoderStream::TextEncoderStream(ScriptState* script_state,
                                      ExceptionState& exception_state)
     : transform_(new TransformStream()) {
-  RetainWrapperUntilV8WrapperGetReturnedToV8(script_state);
+  if (!RetainWrapperDuringConstruction(this, script_state)) {
+    exception_state.ThrowDOMException(DOMExceptionCode::kInvalidStateError,
+                                      "Cannot queue task to retain wrapper");
+    return;
+  }
   transform_->Init(new Transformer(script_state), script_state,
                    exception_state);
 }

third_party/blink/renderer/modules/encoding/text_encoder_stream.h

@@ -38,19 +38,9 @@ class TextEncoderStream final : public ScriptWrappable {
 
  private:
   class Transformer;
-  static void Noop(ScriptValue);
 
   TextEncoderStream(ScriptState*, ExceptionState&);
 
-  // We need to keep the wrapper alive in order to make
-  // |transform_| alive. We can create a wrapper in the constructor, but there
-  // is a chance that GC happens after construction happens before the wrapper
-  // is connected to the value returned to the user in the JS world. This
-  // function posts a task with a ScriptPromise containing the wrapper to avoid
-  // that.
-  // TODO(ricea): Remove this once the unified GC is available.
-  void RetainWrapperUntilV8WrapperGetReturnedToV8(ScriptState*);
-
   const TraceWrapperMember<TransformStream> transform_;
 
   DISALLOW_COPY_AND_ASSIGN(TextEncoderStream);