[Fetch API] Fix a memory leak with a Response constructed with a ReadableStream

third_party/WebKit/Source/modules/fetch/Response.cpp

 // Copyright 2014 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 "modules/fetch/Response.h"
 
 #include "bindings/core/v8/Dictionary.h"
 #include "bindings/core/v8/ExceptionState.h"
 #include "bindings/core/v8/ReadableStreamOperations.h"
 #include "bindings/core/v8/ScriptState.h"
 #include "bindings/core/v8/V8ArrayBuffer.h"
 #include "bindings/core/v8/V8ArrayBufferView.h"
 #include "bindings/core/v8/V8Binding.h"
 #include "bindings/core/v8/V8Blob.h"
 #include "bindings/core/v8/V8FormData.h"
+#include "bindings/core/v8/V8HiddenValue.h"
 #include "core/dom/DOMArrayBuffer.h"
 #include "core/dom/DOMArrayBufferView.h"
 #include "core/fileapi/Blob.h"
 #include "core/html/FormData.h"
 #include "modules/fetch/BodyStreamBuffer.h"
+#include "modules/fetch/DataConsumerHandleUtil.h"
 #include "modules/fetch/FetchBlobDataConsumerHandle.h"
 #include "modules/fetch/FetchFormDataConsumerHandle.h"
 #include "modules/fetch/ReadableStreamDataConsumerHandle.h"
 #include "modules/fetch/ResponseInit.h"
 #include "platform/RuntimeEnabledFeatures.h"
 #include "platform/network/EncodedFormData.h"
 #include "platform/network/HTTPHeaderMap.h"
 #include "public/platform/modules/serviceworker/WebServiceWorkerResponse.h"
 #include "wtf/RefPtr.h"
 
@@ skipping 73 matching lines @@
 }
 
 Response* Response::create(ScriptState* scriptState, ExceptionState& exceptionState)
 {
     return create(scriptState->executionContext(), nullptr, String(), ResponseInit(), exceptionState);
 }
 
 Response* Response::create(ScriptState* scriptState, ScriptValue bodyValue, const Dictionary& init, ExceptionState& exceptionState)
 {
     v8::Local<v8::Value> body = bodyValue.v8Value();
+    ScriptValue reader;
     v8::Isolate* isolate = scriptState->isolate();
     ExecutionContext* executionContext = scriptState->executionContext();
 
     OwnPtr<FetchDataConsumerHandle> bodyHandle;
     String contentType;
     if (bodyValue.isUndefined() || bodyValue.isNull()) {
         // Note: The IDL processor cannot handle this situation. See
         // https://crbug.com/335871.
     } else if (V8Blob::hasInstance(body, isolate)) {
         Blob* blob = V8Blob::toImpl(body.As<v8::Object>());
         bodyHandle = FetchBlobDataConsumerHandle::create(executionContext, blob->blobDataHandle());
         contentType = blob->type();
     } else if (V8ArrayBuffer::hasInstance(body, isolate)) {
         bodyHandle = FetchFormDataConsumerHandle::create(V8ArrayBuffer::toImpl(body.As<v8::Object>()));
     } else if (V8ArrayBufferView::hasInstance(body, isolate)) {
         bodyHandle = FetchFormDataConsumerHandle::create(V8ArrayBufferView::toImpl(body.As<v8::Object>()));
     } else if (V8FormData::hasInstance(body, isolate)) {
         RefPtr<EncodedFormData> formData = V8FormData::toImpl(body.As<v8::Object>())->encodeMultiPartFormData();
         // Here we handle formData->boundary() as a C-style string. See
         // FormDataEncoder::generateUniqueBoundaryString.
         contentType = AtomicString("multipart/form-data; boundary=", AtomicString::ConstructFromLiteral) + formData->boundary().data();
         bodyHandle = FetchFormDataConsumerHandle::create(executionContext, formData.release());
     } else if (RuntimeEnabledFeatures::responseConstructedWithReadableStreamEnabled() && ReadableStreamOperations::isReadableStream(scriptState, bodyValue)) {
         bodyHandle = ReadableStreamDataConsumerHandle::create(scriptState, bodyValue);
+        reader = ReadableStreamOperations::getReader(scriptState, bodyValue, exceptionState);
+        if (exceptionState.hadException()) {
+            reader = ScriptValue();

[haraken, 2016/01/30 15:35:53]

If getReader throws an exception, it would be better to return an empty reader
from the getReader; i.e., remove this line.

[yhirano, 2016/02/02 10:59:28]

At this moment I would like to not throw an exception here, and make the stream
errored. A user can detect the error when he / she tries to read data from the
stream (i.e. new Response(locked_stream).text() rejects). This is a fetch &
streams spec related issue rather than a binding issue, I think.

+            bodyHandle = createFetchDataConsumerHandleFromWebHandle(createUnexpectedErrorDataConsumerHandle());
+            exceptionState.clearException();

[haraken, 2016/01/30 15:35:53]

Is it okay to clear the exception here? Then the caller site won't be able to
detect that Response::create has failed at creating a reader.

[yhirano, 2016/02/02 10:59:28]

Ditto

+        } else {
+            bodyHandle = ReadableStreamDataConsumerHandle::create(scriptState, reader);
+        }
     } else {
         String string = toUSVString(isolate, body, exceptionState);
         if (exceptionState.hadException())
             return nullptr;
         bodyHandle = FetchFormDataConsumerHandle::create(string);
         contentType = "text/plain;charset=UTF-8";
     }
     // TODO(yhirano): Add the URLSearchParams case.
-    return create(executionContext, bodyHandle.release(), contentType, ResponseInit(init, exceptionState), exceptionState);
+    Response* response = create(executionContext, bodyHandle.release(), contentType, ResponseInit(init, exceptionState), exceptionState);
+    if (!exceptionState.hadException() && !reader.isEmpty()) {
+        // Add a hidden reference so that the weak persistent in the
+        // ReadableStreamDataConsumerHandle will be valid as long as the
+        // Response is valid.
+        v8::Local<v8::Value> wrapper = toV8(response, scriptState->context()->Global(), scriptState->isolate());

[haraken, 2016/01/30 15:35:53]

Can we use toV8(response, scriptState) ?

[haraken, 2016/01/30 15:35:53]

Shall we enter ScriptState::Scope before calling toV8?

[yhirano, 2016/02/02 10:59:28]

Done.

[yhirano, 2016/02/02 10:59:28]

We are in the correct scope as this is called from the binding code with
[CallWith=ScriptState].

+        if (wrapper.IsEmpty()) {
+            exceptionState.throwTypeError("Cannot create a Response wrapper");
+            return nullptr;
+        }
+        ASSERT(wrapper->IsObject());
+        V8HiddenValue::setHiddenValue(scriptState, wrapper.As<v8::Object>(), V8HiddenValue::readableStreamReaderInResponse(scriptState->isolate()), reader.v8Value());
+    }
+    return response;
 }
 
 Response* Response::create(ExecutionContext* context, PassOwnPtr<FetchDataConsumerHandle> bodyHandle, const String& contentType, const ResponseInit& init, ExceptionState& exceptionState)
 {
     unsigned short status = init.status;
 
     // "1. If |init|'s status member is not in the range 200 to 599, inclusive, throw a
     // RangeError."
     if (status < 200 || 599 < status) {
         exceptionState.throwRangeError(ExceptionMessages::indexOutsideRange<unsigned>("status", status, 200, ExceptionMessages::InclusiveBound, 599, ExceptionMessages::InclusiveBound));
@@ skipping 239 matching lines @@
 }
 
 DEFINE_TRACE(Response)
 {
     Body::trace(visitor);
     visitor->trace(m_response);
     visitor->trace(m_headers);
 }
 
 } // namespace blink