[Fetch] Use wrapper tracing to express references between wrappers

BodyStreamBuffer has a ReadableStream instance, but because it's
implemented in V8Extra we cannot holds it as a member (i.e., as a
ScriptValue). Instead, we attach the value to the JS wrapper of the
C++ object. This required us to maintain chains of wrappers,
FetchEvent -> Request -> BodyStreamBuffer for example, manually.

This CL replaces that mechanism with wrapper tracing.

Bug: None
Change-Id: I889ef0d0442d62ad50826a4b487ec234f041a982
Reviewed-on: https://chromium-review.googlesource.com/1124270Reviewed-by: Kentaro Hara <haraken@chromium.org>
Reviewed-by: Adam Rice <ricea@chromium.org>
Reviewed-by: Matt Falkenhagen <falken@chromium.org>
Reviewed-by: Yuki Shiino <yukishiino@chromium.org>
Commit-Queue: Yutaka Hirano <yhirano@chromium.org>
Cr-Commit-Position: refs/heads/master@{#573533}

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

@@ -16,11 +16,11 @@
 #include "third_party/blink/renderer/platform/bindings/exception_code.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/v8_private_property.h"
 #include "third_party/blink/renderer/platform/bindings/v8_throw_exception.h"
 #include "third_party/blink/renderer/platform/blob/blob_data.h"
 #include "third_party/blink/renderer/platform/network/encoded_form_data.h"
 #include "third_party/blink/renderer/platform/wtf/assertions.h"
+#include "third_party/blink/renderer/platform/wtf/functional.h"
 
 namespace blink {
 
@@ -99,10 +99,7 @@ BodyStreamBuffer::BodyStreamBuffer(ScriptState* script_state,
       consumer_(consumer),
       signal_(signal),
       made_from_readable_stream_(false) {
-  v8::Local<v8::Value> body_value = ToV8(this, script_state);
-  DCHECK(!body_value.IsEmpty());
-  DCHECK(body_value->IsObject());
-  v8::Local<v8::Object> body = body_value.As<v8::Object>();
+  RetainWrapperUntilV8WrapperGetReturnedToV8(script_state);
 
   {
     // Leaving an exception pending will cause Blink to crash in the bindings
@@ -115,8 +112,8 @@ BodyStreamBuffer::BodyStreamBuffer(ScriptState* script_state,
           ReadableStreamOperations::CreateReadableStream(script_state, this,
                                                          strategy);
       if (!readable_stream.IsEmpty()) {
-        V8PrivateProperty::GetInternalBodyStream(script_state->GetIsolate())
-            .Set(body, readable_stream.V8Value());
+        stream_.Set(script_state->GetIsolate(),
+                    readable_stream.V8Value().As<v8::Object>());
       } else {
         stream_broken_ = true;
       }
@@ -144,16 +141,11 @@ BodyStreamBuffer::BodyStreamBuffer(ScriptState* script_state,
       script_state_(script_state),
       signal_(nullptr),
       made_from_readable_stream_(true) {
+  RetainWrapperUntilV8WrapperGetReturnedToV8(script_state);
   DCHECK(ReadableStreamOperations::IsReadableStreamForDCheck(script_state,
                                                              stream));
 
-  v8::Local<v8::Value> body_value = ToV8(this, script_state);
-  DCHECK(!body_value.IsEmpty());
-  DCHECK(body_value->IsObject());
-  v8::Local<v8::Object> body = body_value.As<v8::Object>();
-
-  V8PrivateProperty::GetInternalBodyStream(script_state->GetIsolate())
-      .Set(body, stream.V8Value());
+  stream_.Set(script_state->GetIsolate(), stream.V8Value().As<v8::Object>());
 }
 
 ScriptValue BodyStreamBuffer::Stream() {
@@ -161,15 +153,8 @@ ScriptValue BodyStreamBuffer::Stream() {
   // even if |stream_broken_| is true, so that expected JavaScript attribute
   // behaviour is not changed. User code is still permitted to access the
   // stream even when it has thrown an exception.
-  ScriptState::Scope scope(script_state_.get());
-  v8::Local<v8::Value> body_value = ToV8(this, script_state_.get());
-  DCHECK(!body_value.IsEmpty());
-  DCHECK(body_value->IsObject());
-  v8::Local<v8::Object> body = body_value.As<v8::Object>();
-  return ScriptValue(
-      script_state_.get(),
-      V8PrivateProperty::GetInternalBodyStream(script_state_->GetIsolate())
-          .GetOrUndefined(body));
+  return ScriptValue(script_state_.get(),
+                     stream_.NewLocal(script_state_->GetIsolate()));
 }
 
 scoped_refptr<BlobDataHandle> BodyStreamBuffer::DrainAsBlobDataHandle(
@@ -531,6 +516,15 @@ base::Optional<bool> BodyStreamBuffer::BooleanStreamOperation(
   return result;
 }
 
+void BodyStreamBuffer::RetainWrapperUntilV8WrapperGetReturnedToV8(
+    ScriptState* script_state) {
+  ExecutionContext::From(script_state)
+      ->GetTaskRunner(TaskType::kInternalDefault)
+      ->PostTask(
+          FROM_HERE,
+          WTF::Bind(Noop, ScriptValue(script_state, ToV8(this, script_state))));
+}
+
 BytesConsumer* BodyStreamBuffer::ReleaseHandle(
     ExceptionState& exception_state) {
   DCHECK(!IsStreamLockedForDCheck());

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

@@ -16,6 +16,7 @@
 #include "third_party/blink/renderer/core/fetch/bytes_consumer.h"
 #include "third_party/blink/renderer/core/fetch/fetch_data_loader.h"
 #include "third_party/blink/renderer/core/streams/underlying_source_base.h"
+#include "third_party/blink/renderer/platform/bindings/trace_wrapper_v8_reference.h"
 #include "third_party/blink/renderer/platform/heap/handle.h"
 
 namespace blink {
@@ -75,6 +76,7 @@ class CORE_EXPORT BodyStreamBuffer final : public UnderlyingSourceBase,
   bool IsAborted();
 
   void Trace(blink::Visitor* visitor) override {
+    visitor->Trace(stream_);
     visitor->Trace(consumer_);
     visitor->Trace(loader_);
     visitor->Trace(signal_);
@@ -84,6 +86,14 @@ 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();
@@ -103,7 +113,10 @@ class CORE_EXPORT BodyStreamBuffer final : public UnderlyingSourceBase,
                                         ExceptionState&),
       ExceptionState& exception_state);
 
+  static void Noop(ScriptValue) {}
+
   scoped_refptr<ScriptState> script_state_;
+  TraceWrapperV8Reference<v8::Object> stream_;
   Member<BytesConsumer> consumer_;
   // We need this member to keep it alive while loading.
   Member<FetchDataLoader> loader_;

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

@@ -9,7 +9,6 @@
 #include "third_party/blink/public/platform/web_url_request.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/core/fetch/body_stream_buffer.h"
 #include "third_party/blink/renderer/core/fetch/bytes_consumer.h"
 #include "third_party/blink/renderer/core/fetch/fetch_header_list.h"
 #include "third_party/blink/renderer/core/fetch/form_data_bytes_consumer.h"

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

@@ -12,6 +12,7 @@
 #include "third_party/blink/public/platform/modules/fetch/fetch_api_request.mojom-shared.h"
 #include "third_party/blink/public/platform/modules/serviceworker/web_service_worker_request.h"
 #include "third_party/blink/public/platform/web_url_request.h"
+#include "third_party/blink/renderer/core/fetch/body_stream_buffer.h"
 #include "third_party/blink/renderer/platform/heap/handle.h"
 #include "third_party/blink/renderer/platform/weborigin/kurl.h"
 #include "third_party/blink/renderer/platform/weborigin/referrer.h"
@@ -21,7 +22,6 @@
 
 namespace blink {
 
-class BodyStreamBuffer;
 class ExceptionState;
 class FetchHeaderList;
 class SecurityOrigin;
@@ -143,7 +143,7 @@ class FetchRequestData final
   // FIXME: Support m_useURLCredentialsFlag;
   // FIXME: Support m_redirectCount;
   Tainting response_tainting_;
-  Member<BodyStreamBuffer> buffer_;
+  TraceWrapperMember<BodyStreamBuffer> buffer_;
   String mime_type_;
   String integrity_;
   bool keepalive_;

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

@@ -5,7 +5,6 @@
 #include "third_party/blink/renderer/core/fetch/fetch_response_data.h"
 
 #include "third_party/blink/public/platform/modules/serviceworker/web_service_worker_response.h"
-#include "third_party/blink/renderer/core/fetch/body_stream_buffer.h"
 #include "third_party/blink/renderer/core/fetch/fetch_header_list.h"
 #include "third_party/blink/renderer/core/typed_arrays/dom_array_buffer.h"
 #include "third_party/blink/renderer/platform/bindings/exception_state.h"

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

@@ -15,6 +15,7 @@
 #include "third_party/blink/public/platform/modules/serviceworker/web_service_worker_request.h"
 #include "third_party/blink/public/platform/web_cors.h"
 #include "third_party/blink/renderer/core/core_export.h"
+#include "third_party/blink/renderer/core/fetch/body_stream_buffer.h"
 #include "third_party/blink/renderer/platform/heap/handle.h"
 #include "third_party/blink/renderer/platform/weborigin/kurl.h"
 #include "third_party/blink/renderer/platform/wtf/text/atomic_string.h"
@@ -23,7 +24,6 @@
 
 namespace blink {
 
-class BodyStreamBuffer;
 class ExceptionState;
 class FetchHeaderList;
 class ScriptState;
@@ -116,8 +116,8 @@ class CORE_EXPORT FetchResponseData final
   unsigned short status_;
   AtomicString status_message_;
   Member<FetchHeaderList> header_list_;
-  Member<FetchResponseData> internal_response_;
-  Member<BodyStreamBuffer> buffer_;
+  TraceWrapperMember<FetchResponseData> internal_response_;
+  TraceWrapperMember<BodyStreamBuffer> buffer_;
   String mime_type_;
   Time response_time_;
   String cache_storage_cache_name_;

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

@@ -29,7 +29,6 @@
 #include "third_party/blink/renderer/core/loader/threadable_loader.h"
 #include "third_party/blink/renderer/core/url/url_search_params.h"
 #include "third_party/blink/renderer/platform/bindings/exception_state.h"
-#include "third_party/blink/renderer/platform/bindings/v8_private_property.h"
 #include "third_party/blink/renderer/platform/blob/blob_data.h"
 #include "third_party/blink/renderer/platform/loader/cors/cors.h"
 #include "third_party/blink/renderer/platform/loader/fetch/fetch_utils.h"
@@ -548,10 +547,8 @@ Request* Request::CreateRequestWithRequestOrString(
   }
 
   // "Set |r|'s request's body to |temporaryBody|.
-  if (temporary_body) {
+  if (temporary_body)
     r->request_->SetBuffer(temporary_body);
-    r->RefreshBody(script_state);
-  }
 
   // "Set |r|'s MIME type to the result of extracting a MIME type from |r|'s
   // request's header list."
@@ -566,7 +563,6 @@ Request* Request::CreateRequestWithRequestOrString(
     // "Set |input|'s request's body to a new body whose stream is
     // |dummyStream|."
     input_request->request_->SetBuffer(dummy_stream);
-    input_request->RefreshBody(script_state);
     // "Let |reader| be the result of getting reader from |dummyStream|."
     // "Read all bytes from |dummyStream| with |reader|."
     input_request->BodyBuffer()->CloseAndLockAndDisturb(exception_state);
@@ -653,7 +649,6 @@ Request::Request(ScriptState* script_state,
       request_(request),
       headers_(headers),
       signal_(signal) {
-  RefreshBody(script_state);
 }
 
 Request::Request(ScriptState* script_state, FetchRequestData* request)
@@ -865,7 +860,6 @@ Request* Request::clone(ScriptState* script_state,
   FetchRequestData* request = request_->Clone(script_state, exception_state);
   if (exception_state.HadException())
     return nullptr;
-  RefreshBody(script_state);
   Headers* headers = Headers::Create(request->HeaderList());
   headers->SetGuard(headers_->GetGuard());
   auto* signal = new AbortSignal(ExecutionContext::From(script_state));
@@ -879,7 +873,6 @@ FetchRequestData* Request::PassRequestData(ScriptState* script_state,
   FetchRequestData* data = request_->Pass(script_state, exception_state);
   if (exception_state.HadException())
     return nullptr;
-  RefreshBody(script_state);
   // |data|'s buffer('s js wrapper) has no retainer, but it's OK because
   // the only caller is the fetch function and it uses the body buffer
   // immediately.
@@ -931,21 +924,6 @@ String Request::ContentType() const {
   return result;
 }
 
-void Request::RefreshBody(ScriptState* script_state) {
-  v8::Local<v8::Value> request = ToV8(this, script_state);
-  if (request.IsEmpty()) {
-    // |toV8| can return an empty handle when the worker is terminating.
-    // We don't want the renderer to crash in such cases.
-    // TODO(yhirano): Delete this block after the graceful shutdown
-    // mechanism is introduced.
-    return;
-  }
-  DCHECK(request->IsObject());
-  v8::Local<v8::Value> body_buffer = ToV8(this->BodyBuffer(), script_state);
-  V8PrivateProperty::GetInternalBodyBuffer(script_state->GetIsolate())
-      .Set(request.As<v8::Object>(), body_buffer);
-}
-
 void Request::Trace(blink::Visitor* visitor) {
   Body::Trace(visitor);
   visitor->Trace(request_);

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

@@ -101,9 +101,8 @@ class CORE_EXPORT Request final : public Body {
 
   String ContentType() const override;
   String MimeType() const override;
-  void RefreshBody(ScriptState*);
 
-  const Member<FetchRequestData> request_;
+  const TraceWrapperMember<FetchRequestData> request_;
   const Member<Headers> headers_;
   const Member<AbortSignal> signal_;
   DISALLOW_COPY_AND_ASSIGN(Request);

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

@@ -33,7 +33,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/script_state.h"
-#include "third_party/blink/renderer/platform/bindings/v8_private_property.h"
 #include "third_party/blink/renderer/platform/loader/cors/cors.h"
 #include "third_party/blink/renderer/platform/loader/fetch/fetch_utils.h"
 #include "third_party/blink/renderer/platform/network/encoded_form_data.h"
@@ -325,7 +324,6 @@ Response* Response::Create(ScriptState* script_state,
       return nullptr;
     }
     r->response_->ReplaceBodyStreamBuffer(body);
-    r->RefreshBody(script_state);
     if (!content_type.IsEmpty() &&
         !r->response_->HeaderList()->Has("Content-Type"))
       r->response_->HeaderList()->Append("Content-Type", content_type);
@@ -465,7 +463,6 @@ Response* Response::clone(ScriptState* script_state,
   FetchResponseData* response = response_->Clone(script_state, exception_state);
   if (exception_state.HadException())
     return nullptr;
-  RefreshBody(script_state);
   Headers* headers = Headers::Create(response->HeaderList());
   headers->SetGuard(headers_->GetGuard());
   return new Response(GetExecutionContext(), response, headers);
@@ -501,9 +498,7 @@ Response::Response(ExecutionContext* context, FetchResponseData* response)
 Response::Response(ExecutionContext* context,
                    FetchResponseData* response,
                    Headers* headers)
-    : Body(context), response_(response), headers_(headers) {
-  InstallBody();
-}
+    : Body(context), response_(response), headers_(headers) {}
 
 bool Response::HasBody() const {
   return response_->InternalBuffer();
@@ -539,27 +534,6 @@ const Vector<KURL>& Response::InternalURLList() const {
   return response_->InternalURLList();
 }
 
-void Response::InstallBody() {
-  if (!InternalBodyBuffer())
-    return;
-  RefreshBody(InternalBodyBuffer()->GetScriptState());
-}
-
-void Response::RefreshBody(ScriptState* script_state) {
-  v8::Local<v8::Value> body_buffer = ToV8(InternalBodyBuffer(), script_state);
-  v8::Local<v8::Value> response = ToV8(this, script_state);
-  if (response.IsEmpty()) {
-    // |toV8| can return an empty handle when the worker is terminating.
-    // We don't want the renderer to crash in such cases.
-    // TODO(yhirano): Delete this block after the graceful shutdown
-    // mechanism is introduced.
-    return;
-  }
-  DCHECK(response->IsObject());
-  V8PrivateProperty::GetInternalBodyBuffer(script_state->GetIsolate())
-      .Set(response.As<v8::Object>(), body_buffer);
-}
-
 void Response::Trace(blink::Visitor* visitor) {
   Body::Trace(visitor);
   visitor->Trace(response_);

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

@@ -113,9 +113,6 @@ class CORE_EXPORT Response final : public Body {
   Response(ExecutionContext*, FetchResponseData*);
   Response(ExecutionContext*, FetchResponseData*, Headers*);
 
-  void InstallBody();
-  void RefreshBody(ScriptState*);
-
   const Member<FetchResponseData> response_;
   const Member<Headers> headers_;
   DISALLOW_COPY_AND_ASSIGN(Response);

third_party/blink/renderer/modules/serviceworkers/fetch_event.cc

@@ -19,7 +19,6 @@
 #include "third_party/blink/renderer/modules/serviceworkers/service_worker_error.h"
 #include "third_party/blink/renderer/modules/serviceworkers/service_worker_global_scope.h"
 #include "third_party/blink/renderer/platform/bindings/script_state.h"
-#include "third_party/blink/renderer/platform/bindings/v8_private_property.h"
 #include "third_party/blink/renderer/platform/loader/fetch/resource_timing_info.h"
 #include "third_party/blink/renderer/platform/network/network_utils.h"
 
@@ -100,27 +99,7 @@ FetchEvent::FetchEvent(ScriptState* script_state,
 
   client_id_ = initializer.clientId();
   is_reload_ = initializer.isReload();
-  if (initializer.hasRequest()) {
-    ScriptState::Scope scope(script_state);
-    request_ = initializer.request();
-    v8::Local<v8::Value> request = ToV8(request_, script_state);
-    v8::Local<v8::Value> event = ToV8(this, script_state);
-    if (event.IsEmpty()) {
-      // |toV8| can return an empty handle when the worker is terminating.
-      // We don't want the renderer to crash in such cases.
-      // TODO(yhirano): Replace this branch with an assertion when the
-      // graceful shutdown mechanism is introduced.
-      return;
-    }
-    DCHECK(event->IsObject());
-    // Sets a hidden value in order to teach V8 the dependency from
-    // the event to the request.
-    V8PrivateProperty::GetFetchEventRequest(script_state->GetIsolate())
-        .Set(event.As<v8::Object>(), request);
-    // From the same reason as above, setHiddenValue can return false.
-    // TODO(yhirano): Add an assertion that it returns true once the
-    // graceful shutdown mechanism is introduced.
-  }
+  request_ = initializer.request();
 }
 
 FetchEvent::~FetchEvent() = default;

third_party/blink/renderer/modules/serviceworkers/fetch_event.h

@@ -93,7 +93,7 @@ class MODULES_EXPORT FetchEvent final
 
  private:
   Member<FetchRespondWithObserver> observer_;
-  Member<Request> request_;
+  TraceWrapperMember<Request> request_;
   Member<PreloadResponseProperty> preload_response_property_;
   std::unique_ptr<WebURLResponse> preload_response_;
   String client_id_;

third_party/blink/renderer/platform/bindings/v8_private_property.h

@@ -20,8 +20,7 @@ namespace blink {
 class ScriptWrappable;
 
 // TODO(peria): Remove properties just to keep V8 objects alive.
-// e.g. InternalBody.Buffer, InternalBody.Stream, IDBCursor.Request,
-// FetchEvent.Request.
+// e.g. IDBCursor.Request.
 // Apply |X| for each pair of (InterfaceName, PrivateKeyName).
 #define V8_PRIVATE_PROPERTY_FOR_EACH(X)               \
   X(CustomElement, Document)                          \
@@ -35,11 +34,8 @@ class ScriptWrappable;
   X(CustomElementLifecycle, DetachedCallback)         \
   X(DOMException, Error)                              \
   X(ErrorEvent, Error)                                \
-  X(FetchEvent, Request)                              \
   X(Global, Event)                                    \
   X(IDBCursor, Request)                               \
-  X(InternalBody, Buffer)                             \
-  X(InternalBody, Stream)                             \
   X(IntersectionObserver, Callback)                   \
   X(MessageChannel, Port1)                            \
   X(MessageChannel, Port2)                            \