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

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

Index: third_party/WebKit/Source/modules/fetch/ReadableStreamDataConsumerHandle.cpp
diff --git a/third_party/WebKit/Source/modules/fetch/ReadableStreamDataConsumerHandle.cpp b/third_party/WebKit/Source/modules/fetch/ReadableStreamDataConsumerHandle.cpp
index 449b0e6872a5a0a57e469eda5d442aeeb27a18a8..e0ba6db7b6e9896f0bed67ee2c06ec5a0a047a00 100644
--- a/third_party/WebKit/Source/modules/fetch/ReadableStreamDataConsumerHandle.cpp
+++ b/third_party/WebKit/Source/modules/fetch/ReadableStreamDataConsumerHandle.cpp
@@ -6,6 +6,7 @@
 
 #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"
@@ -21,7 +22,6 @@
 #include "wtf/Assertions.h"
 #include "wtf/Functional.h"
 #include "wtf/RefCounted.h"
-#include "wtf/WeakPtr.h"
 #include <algorithm>
 #include <string.h>
 #include <v8.h>
@@ -37,14 +37,14 @@ class ReadableStreamDataConsumerHandle::ReadingContext final : public RefCounted
 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;
@@ -64,22 +64,22 @@ public:
         }
 
     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();
@@ -87,10 +87,10 @@ public:
         }
 
     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 {
@@ -150,15 +150,21 @@ public:
             *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).then(
-                OnFulfilled::createFunction(m_reader.scriptState(), m_weakPtrFactory.createWeakPtr()),
-                OnRejected::createFunction(m_reader.scriptState(), m_weakPtrFactory.createWeakPtr()));
+            ScriptState::Scope scope(m_scriptState.get());
+            ScriptValue reader(m_scriptState.get(), 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.get(), reader).then(

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

Would you create a helper function in V8ScriptRunner for this one?

I want to encapsulate all the complexity about V8RecursionScope, Microtask etc
into the helper functions in V8ScriptRunner.

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

Do you have an idea how to encapsulate these calls without loss of generality?

[haraken, 2016/02/02 12:23:06]

Maybe can we introduce V8CallPromiseScope to V8ScriptRunner.h so that we can
write this as:

  V8CallPromiseScope scope(m_scriptState);
  ReadableStreamOperations::read(...).then();

All V8 calls that may run MicroTasks should be unified into V8ScriptRunner.h.

[yhirano, 2016/02/05 14:52:04]

As we talked at https://codereview.chromium.org/1167343002/#msg45, I was wrong
and we don't need V8RecursionScope here.

+                OnFulfilled::createFunction(m_scriptState.get(), this),
+                OnRejected::createFunction(m_scriptState.get(), this));
             // Note: Microtasks may run here.
         }
         m_isInRecursion = false;
@@ -226,33 +232,42 @@ public:
     }
 
 private:
-    ReadingContext(ScriptState* scriptState, ScriptValue stream)
-        : m_client(nullptr)
-        , m_weakPtrFactory(this)
+    ReadingContext(ScriptState* scriptState, ScriptValue streamReader)
+        : m_reader(scriptState->isolate(), streamReader.v8Value())
+        , 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);
+    }
+
+    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();
     }
 
-    // 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
+    // |m_reader| is a weak persistent. It should be kept alive by someone
+    // outside of ReadableStreamDataConsumerHandle.

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

Just to confirm: Are you assuming that ReadableStreamDataConsumerHandle can be
kept alive even after the Reader's wrapper is collected, right? Then it makes
sense to use a weak persistent here.

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

A typical object graph:

Response wrapper -> Response -> BodyStreamBuffer ->
ReadableStreamDataConsumerHandle -> |m_reader|.

The JS object referenced by |m_reader| can reference the Response wrapper  and
hence I want to make the last reference link weak.

+    // 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::Value> m_reader;
+    RefPtr<ScriptState> m_scriptState;
     WebDataConsumerHandle::Client* m_client;
     RefPtr<DOMUint8Array> m_pendingBuffer;
-    WeakPtrFactory<ReadingContext> m_weakPtrFactory;
     size_t m_pendingOffset;
     bool m_isReading;
     bool m_isDone;
@@ -260,8 +275,8 @@ private:
     bool m_isInRecursion;
 };
 
-ReadableStreamDataConsumerHandle::ReadableStreamDataConsumerHandle(ScriptState* scriptState, ScriptValue stream)
-    : m_readingContext(ReadingContext::create(scriptState, stream))
+ReadableStreamDataConsumerHandle::ReadableStreamDataConsumerHandle(ScriptState* scriptState, ScriptValue streamReader)
+    : m_readingContext(ReadingContext::create(scriptState, streamReader))
 {
 }
 ReadableStreamDataConsumerHandle::~ReadableStreamDataConsumerHandle() = default;