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

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

 // Copyright 2015 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 "config.h"
 #include "modules/fetch/ReadableStreamDataConsumerHandle.h"
 
 #include "bindings/core/v8/ExceptionState.h"
 #include "bindings/core/v8/ReadableStreamOperations.h"
+#include "bindings/core/v8/ScopedPersistent.h"
 #include "bindings/core/v8/ScriptFunction.h"
 #include "bindings/core/v8/ScriptState.h"
 #include "bindings/core/v8/ScriptValue.h"
 #include "bindings/core/v8/V8BindingMacros.h"
 #include "bindings/core/v8/V8IteratorResultValue.h"
 #include "bindings/core/v8/V8RecursionScope.h"
 #include "bindings/core/v8/V8Uint8Array.h"
 #include "core/dom/DOMTypedArray.h"
 #include "public/platform/Platform.h"
 #include "public/platform/WebTaskRunner.h"
 #include "public/platform/WebThread.h"
 #include "public/platform/WebTraceLocation.h"
 #include "wtf/Assertions.h"
 #include "wtf/Functional.h"
 #include "wtf/RefCounted.h"
-#include "wtf/WeakPtr.h"
 #include <algorithm>
 #include <string.h>
 #include <v8.h>
 
 namespace blink {
 
 using Result = WebDataConsumerHandle::Result;
 using Flags = WebDataConsumerHandle::Flags;
 
 // This context is not yet thread-safe.
 class ReadableStreamDataConsumerHandle::ReadingContext final : public RefCounted<ReadingContext> {
     WTF_MAKE_NONCOPYABLE(ReadingContext);
 public:
     class OnFulfilled final : public ScriptFunction {
     public:
-        static v8::Local<v8::Function> createFunction(ScriptState* scriptState, WeakPtr<ReadingContext> context)
+        static v8::Local<v8::Function> createFunction(ScriptState* scriptState, PassRefPtr<ReadingContext> context)
         {
             return (new OnFulfilled(scriptState, context))->bindToV8Function();
         }
 
         ScriptValue call(ScriptValue v) override
         {
-            RefPtr<ReadingContext> readingContext(m_readingContext.get());
+            RefPtr<ReadingContext> readingContext(m_readingContext);
             if (!readingContext)
                 return v;
             bool done;
             v8::Local<v8::Value> item = v.v8Value();
             ASSERT(item->IsObject());
             v8::Local<v8::Value> value = v8CallOrCrash(v8UnpackIteratorResult(v.scriptState(), item.As<v8::Object>(), &done));
             if (done) {
                 readingContext->onReadDone();
                 return v;
             }
             if (!V8Uint8Array::hasInstance(value, v.isolate())) {
                 readingContext->onRejected();
                 return ScriptValue();
             }
             readingContext->onRead(V8Uint8Array::toImpl(value.As<v8::Object>()));
             return v;
         }
 
     private:
-        OnFulfilled(ScriptState* scriptState, WeakPtr<ReadingContext> context)
+        OnFulfilled(ScriptState* scriptState, PassRefPtr<ReadingContext> context)
             : ScriptFunction(scriptState), m_readingContext(context) {}
 
-        WeakPtr<ReadingContext> m_readingContext;
+        RefPtr<ReadingContext> m_readingContext;
     };
 
     class OnRejected final : public ScriptFunction {
     public:
-        static v8::Local<v8::Function> createFunction(ScriptState* scriptState, WeakPtr<ReadingContext> context)
+        static v8::Local<v8::Function> createFunction(ScriptState* scriptState, PassRefPtr<ReadingContext> context)
         {
             return (new OnRejected(scriptState, context))->bindToV8Function();
         }
 
         ScriptValue call(ScriptValue v) override
         {
-            RefPtr<ReadingContext> readingContext(m_readingContext.get());
+            RefPtr<ReadingContext> readingContext(m_readingContext);
             if (!readingContext)
                 return v;
             readingContext->onRejected();
             return v;
         }
 
     private:
-        OnRejected(ScriptState* scriptState, WeakPtr<ReadingContext> context)
+        OnRejected(ScriptState* scriptState, PassRefPtr<ReadingContext> context)
             : ScriptFunction(scriptState), m_readingContext(context) {}
 
-        WeakPtr<ReadingContext> m_readingContext;
+        RefPtr<ReadingContext> m_readingContext;
     };
 
     class ReaderImpl final : public FetchDataConsumerHandle::Reader {
     public:
         ReaderImpl(PassRefPtr<ReadingContext> context, Client* client)
             : m_readingContext(context)
         {
             m_readingContext->attachReader(client);
         }
         ~ReaderImpl() override
@@ skipping 21 matching lines @@
 
         Result endRead(size_t readSize) override
         {
             return m_readingContext->endRead(readSize);
         }
 
     private:
         RefPtr<ReadingContext> m_readingContext;
     };
 
-    static PassRefPtr<ReadingContext> create(ScriptState* scriptState, v8::Local<v8::Value> stream)
+    static PassRefPtr<ReadingContext> create(ScriptState* scriptState, v8::Local<v8::Object> stream)

[tyoshino (SeeGerritForStatus), 2016/02/12 07:24:21]

streamReader

[tyoshino (SeeGerritForStatus), 2016/02/12 07:25:05]

Sorry. It's already fixed in the latest ps. Please ignore.

[yhirano, 2016/02/12 07:33:54]

Done.

     {
         return adoptRef(new ReadingContext(scriptState, stream));
     }
 
     void attachReader(WebDataConsumerHandle::Client* client)
     {
         m_client = client;
         notifyLater();
     }
 
@@ skipping 10 matching lines @@
             return WebDataConsumerHandle::UnexpectedError;
         if (m_isDone)
             return WebDataConsumerHandle::Done;
 
         if (m_pendingBuffer) {
             ASSERT(m_pendingOffset < m_pendingBuffer->length());
             *buffer = m_pendingBuffer->data() + m_pendingOffset;
             *available = m_pendingBuffer->length() - m_pendingOffset;
             return WebDataConsumerHandle::Ok;
         }
-        ASSERT(!m_reader.isEmpty());
         m_isInRecursion = true;
         if (!m_isReading) {
             m_isReading = true;
-            ScriptState::Scope scope(m_reader.scriptState());
-            V8RecursionScope recursionScope(m_reader.isolate());
-            ReadableStreamOperations::read(m_reader.scriptState(), m_reader.v8Value()).then(
-                OnFulfilled::createFunction(m_reader.scriptState(), m_weakPtrFactory.createWeakPtr()),
-                OnRejected::createFunction(m_reader.scriptState(), m_weakPtrFactory.createWeakPtr()));
+            ScriptState::Scope scope(m_scriptState);
+            v8::Local<v8::Object> reader = m_reader.newLocal(m_scriptState->isolate());
+            if (reader.IsEmpty()) {
+                // The reader was collected.
+                m_hasError = true;
+                m_isReading = false;
+                return WebDataConsumerHandle::UnexpectedError;
+            }
+            V8RecursionScope recursionScope(m_scriptState->isolate());
+            ReadableStreamOperations::read(m_scriptState, reader).then(
+                OnFulfilled::createFunction(m_scriptState, this),
+                OnRejected::createFunction(m_scriptState, this));
             // Note: Microtasks may run here.
         }
         m_isInRecursion = false;
         return WebDataConsumerHandle::ShouldWait;
     }
 
     Result endRead(size_t readSize)
     {
         ASSERT(m_pendingBuffer);
         ASSERT(m_pendingOffset + readSize <= m_pendingBuffer->length());
@@ skipping 47 matching lines @@
         m_client->didGetReadable();
     }
 
     void notifyLater()
     {
         ASSERT(m_client);
         Platform::current()->currentThread()->taskRunner()->postTask(BLINK_FROM_HERE, bind(&ReadingContext::notify, PassRefPtr<ReadingContext>(this)));
     }
 
 private:
-    ReadingContext(ScriptState* scriptState, v8::Local<v8::Value> stream)
-        : m_client(nullptr)
-        , m_weakPtrFactory(this)
+    ReadingContext(ScriptState* scriptState, v8::Local<v8::Object> streamReader)
+        : m_reader(scriptState->isolate(), streamReader)
+        , m_scriptState(scriptState)
+        , m_client(nullptr)
         , m_pendingOffset(0)
         , m_isReading(false)
         , m_isDone(false)
         , m_hasError(false)
         , m_isInRecursion(false)
     {
-        if (!ReadableStreamOperations::isLocked(scriptState, stream)) {
-            // Here the stream implementation must not throw an exception.
-            NonThrowableExceptionState es;
-            m_reader = ReadableStreamOperations::getReader(scriptState, stream, es);
-        }
-        m_hasError = m_reader.isEmpty();
+        m_reader.setWeak(this, &ReadingContext::onCollected);
     }
 
-    // This ScriptValue is leaky because it stores a strong reference to a
-    // JavaScript object.
-    // TODO(yhirano): Fix it.
-    //
-    // Holding a ScriptValue here is safe in terms of cross-world wrapper
+    void onCollected()
+    {
+        m_reader.clear();
+        if (m_isDone || m_hasError)
+            return;
+        m_hasError = true;
+        if (m_client)
+            notifyLater();
+    }
+
+    static void onCollected(const v8::WeakCallbackInfo<ReadableStreamDataConsumerHandle::ReadingContext>& data)
+    {
+        data.GetParameter()->onCollected();

[haraken, 2016/01/06 05:49:06]

ReadableStreamDataConsumerHandle.cpp uses a bunch of V8 APIs. We want to limit
direct V8 API usage only to bindings/, so consider moving (a part of)
ReadableStreamDataConsumerHandle.cpp to bindings/. You don't need to address it
in this CL though.

[yhirano, 2016/01/26 05:09:32]

Is this point affected by the policy change currently discussed?

==

Let me confirm:

1. Regarding OnFulfilled::call, I can move the code to
   ReadableStreamOperations.{cpp,h}.
2. Regarding ScopedPersistent, All we need is weak ScriptValue and there are
   several solutions.
 a. I can implement makeWeak on ScriptValue and use it in this file.
 b. I can implement a custom wrapper class for this specific use-case, but I
    don't like it.
 c. I have a plan to make this class on-heap. IIRC in the future an on-heap
    object can hold a ScriptValue, right? Then we will not need
    weak-related code.

Are there other part you want to move to bindings? If so, I think I need to move
the entire file to bindings/.

[haraken, 2016/01/26 06:09:06]

Yes, I think our new policy is going to allow V8 APIs in core/. I'll start the
thread in blink-dev@ today.

+    }
+
+    // |m_reader| is a weak persistent. It should be kept alive by someone
+    // outside of ReadableStreamDataConsumerHandle.
+    // Holding a ScopedPersistent here is safe in terms of cross-world wrapper
     // leakage because we read only Uint8Array chunks from the reader.
-    ScriptValue m_reader;
+    ScopedPersistent<v8::Object> m_reader;

[haraken, 2016/01/06 05:49:06]

Instead of holding a weak reference, how about holding a pointer to the Response
object and retrieve the V8 object from the Response object's wrapper?

[yhirano, 2016/01/26 05:09:32]

I would keep that a Response is constructed from a WebDataConsumerHandle. That
means we cannot have a Response here because it doesn't exist when constructed
(and after that we can call only WebDataConsumerHandle methods).

+    ScriptState* m_scriptState;

[haraken, 2016/01/06 05:49:06]

This must be RefPtr<ScriptState>.

[yhirano, 2016/01/26 05:09:32]

Done.

     WebDataConsumerHandle::Client* m_client;
     RefPtr<DOMUint8Array> m_pendingBuffer;
-    WeakPtrFactory<ReadingContext> m_weakPtrFactory;
     size_t m_pendingOffset;
     bool m_isReading;
     bool m_isDone;
     bool m_hasError;
     bool m_isInRecursion;
 };
 
-ReadableStreamDataConsumerHandle::ReadableStreamDataConsumerHandle(ScriptState* scriptState, v8::Local<v8::Value> stream)
-    : m_readingContext(ReadingContext::create(scriptState, stream))
+ReadableStreamDataConsumerHandle::ReadableStreamDataConsumerHandle(ScriptState* scriptState, v8::Local<v8::Object> streamReader)
+    : m_readingContext(ReadingContext::create(scriptState, streamReader))
 {
 }
 ReadableStreamDataConsumerHandle::~ReadableStreamDataConsumerHandle() = default;
 
 FetchDataConsumerHandle::Reader* ReadableStreamDataConsumerHandle::obtainReaderInternal(Client* client)
 {
     return new ReadingContext::ReaderImpl(m_readingContext, client);
 }
 
 } // namespace blink