XMLHttpRequest: Retain response text using wrapper tracing

Remove ScriptString which is a scoped object keeping a strong handle.
Using wrapper tracing implies that the retaining path for DevTools
shows up properly.

Bug: chromium:810194
Change-Id: Ia5126fdca8d270a9f5700af335735210e4062d61
Reviewed-on: https://chromium-review.googlesource.com/911969
Commit-Queue: Michael Lippautz <mlippautz@chromium.org>
Reviewed-by: Kentaro Hara <haraken@chromium.org>
Cr-Commit-Position: refs/heads/master@{#536993}

third_party/WebKit/Source/bindings/bindings.gni

@@ -92,8 +92,6 @@ bindings_core_v8_files =
                     "core/v8/ScriptStreamer.h",
                     "core/v8/ScriptStreamerThread.cpp",
                     "core/v8/ScriptStreamerThread.h",
-                    "core/v8/ScriptString.cpp",
-                    "core/v8/ScriptString.h",
                     "core/v8/ScriptValue.cpp",
                     "core/v8/ScriptValue.h",
                     "core/v8/SourceLocation.cpp",

third_party/WebKit/Source/bindings/core/v8/ScriptString.cpp

-/*
- * Copyright (C) 2013 Google Inc. All rights reserved.
- *
- * Redistribution and use in source and binary forms, with or without
- * modification, are permitted provided that the following conditions are
- * met:
- *
- *     * Redistributions of source code must retain the above copyright
- * notice, this list of conditions and the following disclaimer.
- *     * Redistributions in binary form must reproduce the above
- * copyright notice, this list of conditions and the following disclaimer
- * in the documentation and/or other materials provided with the
- * distribution.
- *     * Neither the name of Google Inc. nor the names of its
- * contributors may be used to endorse or promote products derived from
- * this software without specific prior written permission.
- *
- * THIS SOFTWARE IS PROVIDED BY THE COPYRIGHT HOLDERS AND CONTRIBUTORS
- * "AS IS" AND ANY EXPRESS OR IMPLIED WARRANTIES, INCLUDING, BUT NOT
- * LIMITED TO, THE IMPLIED WARRANTIES OF MERCHANTABILITY AND FITNESS FOR
- * A PARTICULAR PURPOSE ARE DISCLAIMED. IN NO EVENT SHALL THE COPYRIGHT
- * OWNER OR CONTRIBUTORS BE LIABLE FOR ANY DIRECT, INDIRECT, INCIDENTAL,
- * SPECIAL, EXEMPLARY, OR CONSEQUENTIAL DAMAGES (INCLUDING, BUT NOT
- * LIMITED TO, PROCUREMENT OF SUBSTITUTE GOODS OR SERVICES; LOSS OF USE,
- * DATA, OR PROFITS; OR BUSINESS INTERRUPTION) HOWEVER CAUSED AND ON ANY
- * THEORY OF LIABILITY, WHETHER IN CONTRACT, STRICT LIABILITY, OR TORT
- * (INCLUDING NEGLIGENCE OR OTHERWISE) ARISING IN ANY WAY OUT OF THE USE
- * OF THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE.
- */
-
-#include "bindings/core/v8/ScriptString.h"
-
-#include "bindings/core/v8/V8BindingForCore.h"
-
-namespace blink {
-
-ScriptString::ScriptString() : isolate_(nullptr) {}
-
-ScriptString::ScriptString(v8::Isolate* isolate, v8::Local<v8::String> string)
-    : isolate_(isolate),
-      string_(SharedPersistent<v8::String>::Create(string, isolate_)) {}
-
-ScriptString& ScriptString::operator=(const ScriptString& string) {
-  if (this != &string) {
-    isolate_ = string.isolate_;
-    string_ = string.string_;
-  }
-  return *this;
-}
-
-v8::Local<v8::String> ScriptString::V8Value() {
-  if (IsEmpty())
-    return v8::Local<v8::String>();
-  return string_->NewLocal(GetIsolate());
-}
-
-ScriptString ScriptString::ConcatenateWith(const String& string) {
-  v8::Isolate* non_null_isolate = GetIsolate();
-  v8::HandleScope handle_scope(non_null_isolate);
-  v8::Local<v8::String> target_string = V8String(non_null_isolate, string);
-  if (IsEmpty())
-    return ScriptString(non_null_isolate, target_string);
-  return ScriptString(non_null_isolate,
-                      v8::String::Concat(V8Value(), target_string));
-}
-
-String ScriptString::FlattenToString() {
-  if (IsEmpty())
-    return String();
-  v8::HandleScope handle_scope(GetIsolate());
-  return V8StringToWebCoreString<String>(V8Value(), kExternalize);
-}
-
-}  // namespace blink

third_party/WebKit/Source/bindings/core/v8/ScriptString.h

-/*
- * Copyright (C) 2013 Google Inc. All rights reserved.
- *
- * Redistribution and use in source and binary forms, with or without
- * modification, are permitted provided that the following conditions are
- * met:
- *
- *     * Redistributions of source code must retain the above copyright
- * notice, this list of conditions and the following disclaimer.
- *     * Redistributions in binary form must reproduce the above
- * copyright notice, this list of conditions and the following disclaimer
- * in the documentation and/or other materials provided with the
- * distribution.
- *     * Neither the name of Google Inc. nor the names of its
- * contributors may be used to endorse or promote products derived from
- * this software without specific prior written permission.
- *
- * THIS SOFTWARE IS PROVIDED BY THE COPYRIGHT HOLDERS AND CONTRIBUTORS
- * "AS IS" AND ANY EXPRESS OR IMPLIED WARRANTIES, INCLUDING, BUT NOT
- * LIMITED TO, THE IMPLIED WARRANTIES OF MERCHANTABILITY AND FITNESS FOR
- * A PARTICULAR PURPOSE ARE DISCLAIMED. IN NO EVENT SHALL THE COPYRIGHT
- * OWNER OR CONTRIBUTORS BE LIABLE FOR ANY DIRECT, INDIRECT, INCIDENTAL,
- * SPECIAL, EXEMPLARY, OR CONSEQUENTIAL DAMAGES (INCLUDING, BUT NOT
- * LIMITED TO, PROCUREMENT OF SUBSTITUTE GOODS OR SERVICES; LOSS OF USE,
- * DATA, OR PROFITS; OR BUSINESS INTERRUPTION) HOWEVER CAUSED AND ON ANY
- * THEORY OF LIABILITY, WHETHER IN CONTRACT, STRICT LIABILITY, OR TORT
- * (INCLUDING NEGLIGENCE OR OTHERWISE) ARISING IN ANY WAY OUT OF THE USE
- * OF THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE.
- */
-
-#ifndef ScriptString_h
-#define ScriptString_h
-
-#include "base/memory/scoped_refptr.h"
-#include "platform/bindings/SharedPersistent.h"
-#include "platform/wtf/Allocator.h"
-#include "platform/wtf/text/WTFString.h"
-#include "v8/include/v8.h"
-
-namespace blink {
-
-class ScriptString final {
-  DISALLOW_NEW();
-
- public:
-  ScriptString();
-  ScriptString(v8::Isolate*, v8::Local<v8::String>);
-  ScriptString& operator=(const ScriptString&);
-
-  v8::Isolate* GetIsolate() {
-    if (!isolate_)
-      isolate_ = v8::Isolate::GetCurrent();
-    return isolate_;
-  }
-  bool IsEmpty() const { return !string_ || string_->IsEmpty(); }
-  void Clear() { string_ = nullptr; }
-  v8::Local<v8::String> V8Value();
-  ScriptString ConcatenateWith(const String&);
-  String FlattenToString();
-
- private:
-  v8::Isolate* isolate_;
-  scoped_refptr<SharedPersistent<v8::String>> string_;
-};
-
-}  // namespace blink
-
-#endif  // ScriptString_h

third_party/WebKit/Source/bindings/core/v8/custom/V8XMLHttpRequestCustom.cpp

@@ -53,12 +53,12 @@ void V8XMLHttpRequest::responseTextAttributeGetterCustom(
   ExceptionState exception_state(info.GetIsolate(),
                                  ExceptionState::kGetterContext,
                                  "XMLHttpRequest", "responseText");
-  ScriptString text = xml_http_request->responseText(exception_state);
+  v8::Local<v8::String> text = xml_http_request->responseText(exception_state);
   if (text.IsEmpty()) {
     V8SetReturnValueString(info, g_empty_string, info.GetIsolate());
     return;
   }
-  V8SetReturnValue(info, text.V8Value());
+  V8SetReturnValue(info, text);
 }
 
 void V8XMLHttpRequest::responseAttributeGetterCustom(
@@ -77,7 +77,8 @@ void V8XMLHttpRequest::responseAttributeGetterCustom(
     case XMLHttpRequest::kResponseTypeJSON: {
       v8::Isolate* isolate = info.GetIsolate();
 
-      ScriptString json_source = xml_http_request->ResponseJSONSource();
+      v8::Local<v8::String> json_source =
+          xml_http_request->ResponseJSONSource();
       if (json_source.IsEmpty()) {
         V8SetReturnValue(info, v8::Null(isolate));
         return;
@@ -87,7 +88,7 @@ void V8XMLHttpRequest::responseAttributeGetterCustom(
       // spec says. https://xhr.spec.whatwg.org/#response-body
       v8::Local<v8::Value> json =
           FromJSONString(isolate, isolate->GetCurrentContext(),
-                         ToCoreString(json_source.V8Value()), exception_state);
+                         ToCoreString(json_source), exception_state);
       if (exception_state.HadException()) {
         exception_state.ClearException();
         V8SetReturnValue(info, v8::Null(isolate));

third_party/WebKit/Source/core/inspector/InspectorNetworkAgent.h

@@ -31,7 +31,6 @@
 #ifndef InspectorNetworkAgent_h
 #define InspectorNetworkAgent_h
 
-#include "bindings/core/v8/ScriptString.h"
 #include "core/CoreExport.h"
 #include "core/inspector/InspectedFrames.h"
 #include "core/inspector/InspectorBaseAgent.h"

third_party/WebKit/Source/core/xmlhttprequest/XMLHttpRequest.cpp

@@ -340,7 +340,8 @@ XMLHttpRequest::State XMLHttpRequest::readyState() const {
   return state_;
 }
 
-ScriptString XMLHttpRequest::responseText(ExceptionState& exception_state) {
+v8::Local<v8::String> XMLHttpRequest::responseText(
+    ExceptionState& exception_state) {
   if (response_type_code_ != kResponseTypeDefault &&
       response_type_code_ != kResponseTypeText) {
     exception_state.ThrowDOMException(kInvalidStateError,
@@ -348,19 +349,19 @@ ScriptString XMLHttpRequest::responseText(ExceptionState& exception_state) {
                                       "object's 'responseType' is '' or 'text' "
                                       "(was '" +
                                           responseType() + "').");
-    return ScriptString();
+    return v8::Local<v8::String>();
   }
   if (error_ || (state_ != kLoading && state_ != kDone))
-    return ScriptString();
-  return response_text_;
+    return v8::Local<v8::String>();
+  return response_text_.V8Value(isolate_);
 }
 
-ScriptString XMLHttpRequest::ResponseJSONSource() {
+v8::Local<v8::String> XMLHttpRequest::ResponseJSONSource() {
   DCHECK_EQ(response_type_code_, kResponseTypeJSON);
 
   if (error_ || state_ != kDone)
-    return ScriptString();
-  return response_text_;
+    return v8::Local<v8::String>();
+  return response_text_.V8Value(isolate_);
 }
 
 void XMLHttpRequest::InitResponseDocument() {
@@ -408,7 +409,7 @@ Document* XMLHttpRequest::responseXML(ExceptionState& exception_state) {
     if (!response_document_)
       return nullptr;
 
-    response_document_->SetContent(response_text_.FlattenToString());
+    response_document_->SetContent(response_text_.Flatten(isolate_));
     if (!response_document_->WellFormed())
       response_document_ = nullptr;
 
@@ -1686,7 +1687,7 @@ void XMLHttpRequest::DidFinishLoadingInternal() {
   if (decoder_) {
     auto text = decoder_->Flush();
     if (!text.IsEmpty() && !response_text_overflow_) {
-      response_text_ = response_text_.ConcatenateWith(text);
+      response_text_.Concat(isolate_, text);
       response_text_overflow_ = response_text_.IsEmpty();
     }
   }
@@ -1892,7 +1893,7 @@ void XMLHttpRequest::DidReceiveData(const char* data, unsigned len) {
 
     auto text = decoder_->Decode(data, len);
     if (!text.IsEmpty() && !response_text_overflow_) {
-      response_text_ = response_text_.ConcatenateWith(text);
+      response_text_.Concat(isolate_, text);
       response_text_overflow_ = response_text_.IsEmpty();
     }
   } else if (response_type_code_ == kResponseTypeArrayBuffer ||
@@ -2022,6 +2023,7 @@ void XMLHttpRequest::TraceWrappers(
   visitor->TraceWrappers(response_blob_);
   visitor->TraceWrappers(response_document_);
   visitor->TraceWrappers(response_array_buffer_);
+  visitor->TraceWrappers(response_text_);
   XMLHttpRequestEventTarget::TraceWrappers(visitor);
 }
 

third_party/WebKit/Source/core/xmlhttprequest/XMLHttpRequest.h

@@ -26,7 +26,6 @@
 #include <memory>
 #include "base/memory/scoped_refptr.h"
 #include "bindings/core/v8/ActiveScriptWrappable.h"
-#include "bindings/core/v8/ScriptString.h"
 #include "core/dom/DocumentParserClient.h"
 #include "core/dom/ExceptionCode.h"
 #include "core/dom/PausableObject.h"
@@ -35,6 +34,7 @@
 #include "core/xmlhttprequest/XMLHttpRequestProgressEventThrottle.h"
 #include "platform/bindings/ScriptWrappable.h"
 #include "platform/bindings/TraceWrapperMember.h"
+#include "platform/bindings/TraceWrapperV8String.h"
 #include "platform/heap/Handle.h"
 #include "platform/loader/fetch/ResourceResponse.h"
 #include "platform/network/EncodedFormData.h"
@@ -144,8 +144,8 @@ class XMLHttpRequest final : public XMLHttpRequestEventTarget,
   void overrideMimeType(const AtomicString& override, ExceptionState&);
   String getAllResponseHeaders() const;
   const AtomicString& getResponseHeader(const AtomicString&) const;
-  ScriptString responseText(ExceptionState&);
-  ScriptString ResponseJSONSource();
+  v8::Local<v8::String> responseText(ExceptionState&);
+  v8::Local<v8::String> ResponseJSONSource();
   Document* responseXML(ExceptionState&);
   Blob* ResponseBlob();
   DOMArrayBuffer* ResponseArrayBuffer();
@@ -293,7 +293,7 @@ class XMLHttpRequest final : public XMLHttpRequestEventTarget,
   // Report the memory usage associated with this object to V8 so that V8 can
   // schedule GC accordingly.  This function should be called whenever the
   // internal memory usage changes except for the following members.
-  // - response_text_ of type ScriptString
+  // - response_text_ of type TraceWrapperV8String
   //   ScriptString internally creates and holds a v8::String, so V8 is aware of
   //   its memory usage.
   // - response_array_buffer_ of type DOMArrayBuffer
@@ -320,7 +320,9 @@ class XMLHttpRequest final : public XMLHttpRequestEventTarget,
 
   std::unique_ptr<TextResourceDecoder> decoder_;
 
-  ScriptString response_text_;
+  // Avoid using a flat WTF::String here and rather use a traced v8::String
+  // which internally builds a string rope.
+  TraceWrapperV8String response_text_;
   TraceWrapperMember<Document> response_document_;
   Member<DocumentParser> response_document_parser_;
 

third_party/WebKit/Source/platform/BUILD.gn

@@ -574,6 +574,8 @@ jumbo_component("platform") {
     "bindings/TraceWrapperBase.h",
     "bindings/TraceWrapperMember.h",
     "bindings/TraceWrapperV8Reference.h",
+    "bindings/TraceWrapperV8String.cpp",
+    "bindings/TraceWrapperV8String.h",
     "bindings/V0CustomElementBinding.cpp",
     "bindings/V0CustomElementBinding.h",
     "bindings/V8Binding.cpp",

third_party/WebKit/Source/platform/bindings/TraceWrapperBase.h

@@ -5,6 +5,7 @@
 #ifndef TraceWrapperBase_h
 #define TraceWrapperBase_h
 
+#include "platform/PlatformExport.h"
 #include "platform/wtf/Noncopyable.h"
 
 namespace blink {

third_party/WebKit/Source/platform/bindings/TraceWrapperV8String.cpp

+// 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 "platform/bindings/TraceWrapperV8String.h"
+#include "platform/bindings/V8Binding.h"
+
+namespace blink {
+
+void TraceWrapperV8String::Concat(v8::Isolate* isolate, const String& string) {
+  DCHECK(isolate);
+  v8::HandleScope handle_scope(isolate);
+  v8::Local<v8::String> target_string =
+      (string_.IsEmpty()) ? V8String(isolate, string)
+                          : v8::String::Concat(string_.NewLocal(isolate),
+                                               V8String(isolate, string));
+  string_.Set(isolate, target_string);
+}
+
+String TraceWrapperV8String::Flatten(v8::Isolate* isolate) const {
+  if (IsEmpty())
+    return String();
+  DCHECK(isolate);
+  v8::HandleScope handle_scope(isolate);
+  return V8StringToWebCoreString<String>(string_.NewLocal(isolate),
+                                         kExternalize);
+}
+
+}  // namespace blink

third_party/WebKit/Source/platform/bindings/TraceWrapperV8String.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 TraceWrapperV8String_h
+#define TraceWrapperV8String_h
+
+#include "platform/bindings/TraceWrapperBase.h"
+#include "platform/bindings/TraceWrapperV8Reference.h"
+#include "platform/wtf/text/WTFString.h"
+#include "v8/include/v8.h"
+
+namespace blink {
+
+// Small shim around TraceWrapperReference<v8::String> with a few
+// utility methods. Internally, v8::String is represented as string
+// rope.
+class PLATFORM_EXPORT TraceWrapperV8String final : public TraceWrapperBase {
+  DISALLOW_COPY_AND_ASSIGN(TraceWrapperV8String);
+  DISALLOW_NEW();
+
+ public:
+  TraceWrapperV8String() = default;
+
+  bool IsEmpty() const { return string_.IsEmpty(); }
+  void Clear() { string_.Clear(); }
+
+  v8::Local<v8::String> V8Value(v8::Isolate* isolate) {
+    return string_.NewLocal(isolate);
+  }
+
+  void Concat(v8::Isolate*, const String&);
+  String Flatten(v8::Isolate*) const;
+
+  void TraceWrappers(const ScriptWrappableVisitor* visitor) const override {
+    visitor->TraceWrappers(string_);
+  }
+
+ private:
+  TraceWrapperV8Reference<v8::String> string_;
+};
+
+}  // namespace blink
+
+#endif  // TraceWrapperV8String_h