[Streams API] ReadableStream reader should not be disposable when having pending...
authoryouenn.fablet@crf.canon.fr <youenn.fablet@crf.canon.fr@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Tue, 12 May 2015 07:15:37 +0000 (07:15 +0000)
committeryouenn.fablet@crf.canon.fr <youenn.fablet@crf.canon.fr@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Tue, 12 May 2015 07:15:37 +0000 (07:15 +0000)
https://bugs.webkit.org/show_bug.cgi?id=144869

Reviewed by Darin Adler.

Source/WebCore:

Made error promise callback to take a ref to the reader so that the reader is not disposed as long as the promise callback is not resolved.

Covered by tests added to streams/readable-stream-gc.html.

* Modules/streams/ReadableStreamReader.cpp:
(WebCore::ReadableStreamReader::ReadableStreamReader): Moved initialize() call outside constructor as it can ref/unref.
(WebCore::ReadableStreamReader::releaseStreamAndClean): Added protector.
* Modules/streams/ReadableStreamReader.h:
* bindings/js/JSReadableStreamReaderCustom.cpp:
(WebCore::JSReadableStreamReader::closed): Lambda for error now takes a ref to the reader.
* bindings/js/ReadableJSStream.cpp:
(WebCore::ReadableJSStream::Reader::create): Calling initialize() after adoptRef().

LayoutTests:

* streams/readable-stream-gc.html:
* streams/readable-stream-gc-expected.txt:

git-svn-id: https://svn.webkit.org/repository/webkit/trunk@184159 268f45cc-cd09-0410-ab3c-d52691b4dbfc

LayoutTests/ChangeLog
LayoutTests/streams/readable-stream-gc-expected.txt
LayoutTests/streams/readable-stream-gc.html
Source/WebCore/ChangeLog
Source/WebCore/Modules/streams/ReadableStreamReader.cpp
Source/WebCore/Modules/streams/ReadableStreamReader.h
Source/WebCore/bindings/js/JSReadableStreamReaderCustom.cpp
Source/WebCore/bindings/js/ReadableJSStream.cpp

index d4ea6e7..a78d1e0 100644 (file)
@@ -1,3 +1,13 @@
+2015-05-12  Xabier Rodriguez Calvar  <calvaris@igalia.com> and Youenn Fablet <youenn.fablet@crf.canon.fr>
+
+        [Streams API] ReadableStream reader should not be disposable when having pending promises
+        https://bugs.webkit.org/show_bug.cgi?id=144869
+
+        Reviewed by Darin Adler.
+
+        * streams/readable-stream-gc.html:
+        * streams/readable-stream-gc-expected.txt:
+
 2015-05-11  Zalan Bujtas  <zalan@apple.com>
 
         Text is misplaced when custom font does not have space glyph.
index e6791aa..06c2840 100644 (file)
@@ -1,3 +1,5 @@
 
 PASS Readable stream controller methods should continue working properly when scripts are loosing reference to the readable stream 
+PASS Readable stream closed promise should resolve even if stream and reader JS references are lost 
+PASS Readable stream closed promise should reject even if stream and reader JS references are lost 
 
index f957684..e892772 100644 (file)
@@ -33,4 +33,36 @@ t1.step(function() {
     }));
 });
 
+t2 = async_test('Readable stream closed promise should resolve even if stream and reader JS references are lost');
+t2.step(function() {
+    var controller;
+    new ReadableStream({
+        start: function(c) {
+            controller = c;
+        }
+    }).getReader().closed.then(t2.step_func(function() {
+        t2.done();
+    }));
+
+    garbageCollectAndDo(t2.step_func(function() {
+        controller.close();
+    }));
+});
+
+t3 = async_test('Readable stream closed promise should reject even if stream and reader JS references are lost');
+t3.step(function() {
+    var controller;
+    new ReadableStream({
+        start: function(c) {
+            controller = c;
+        }
+    }).getReader().closed.catch(t3.step_func(function() {
+        t3.done();
+    }));
+
+    garbageCollectAndDo(t3.step_func(function() {
+        controller.error();
+    }));
+});
+
 </script>
index 17a9118..44b89a4 100644 (file)
@@ -1,3 +1,23 @@
+2015-05-12  Xabier Rodriguez Calvar  <calvaris@igalia.com> and Youenn Fablet <youenn.fablet@crf.canon.fr>
+
+        [Streams API] ReadableStream reader should not be disposable when having pending promises
+        https://bugs.webkit.org/show_bug.cgi?id=144869
+
+        Reviewed by Darin Adler.
+
+        Made error promise callback to take a ref to the reader so that the reader is not disposed as long as the promise callback is not resolved.
+
+        Covered by tests added to streams/readable-stream-gc.html.
+
+        * Modules/streams/ReadableStreamReader.cpp:
+        (WebCore::ReadableStreamReader::ReadableStreamReader): Moved initialize() call outside constructor as it can ref/unref.
+        (WebCore::ReadableStreamReader::releaseStreamAndClean): Added protector.
+        * Modules/streams/ReadableStreamReader.h:
+        * bindings/js/JSReadableStreamReaderCustom.cpp:
+        (WebCore::JSReadableStreamReader::closed): Lambda for error now takes a ref to the reader.
+        * bindings/js/ReadableJSStream.cpp:
+        (WebCore::ReadableJSStream::Reader::create): Calling initialize() after adoptRef().
+
 2015-05-11  Zan Dobersek  <zdobersek@igalia.com>
 
         Clean up redundant resources in case of failure in GLContextEGL context creation methods
index 22cb087..8992fef 100644 (file)
@@ -47,7 +47,6 @@ ReadableStreamReader::ReadableStreamReader(ReadableStream& stream)
     readableStreamReaderCounter.increment();
 #endif
     suspendIfNeeded();
-    initialize();
 }
 
 ReadableStreamReader::~ReadableStreamReader()
@@ -77,6 +76,9 @@ void ReadableStreamReader::initialize()
 
 void ReadableStreamReader::releaseStreamAndClean()
 {
+    // Releasing callbacks may trigger unrefing of the reader.
+    Ref<ReadableStreamReader> protect(*this);
+
     ASSERT(m_stream);
     m_stream->release();
     m_stream = nullptr;
index 7191020..38f7b59 100644 (file)
@@ -53,6 +53,7 @@ public:
         Errored
     };
 
+    void initialize();
     virtual ~ReadableStreamReader();
 
     ReadableStream* stream() { return m_stream.get(); }
@@ -74,7 +75,6 @@ private:
     const char* activeDOMObjectName() const override;
     bool canSuspendForPageCache() const override;
 
-    void initialize();
     void releaseStreamAndClean();
 
     RefPtr<ReadableStream> m_stream;
index d31d885..899f08e 100644 (file)
@@ -71,11 +71,13 @@ JSValue JSReadableStreamReader::closed(ExecState* exec) const
 {
     JSPromiseDeferred* promiseDeferred = getOrCreatePromiseDeferredFromObject(exec, this, globalObject(), closedPromiseSlotName());
     DeferredWrapper wrapper(exec, globalObject(), promiseDeferred);
+
+    RefPtr<ReadableStreamReader> reader = &impl();
     auto successCallback = [wrapper]() mutable {
         wrapper.resolve(jsUndefined());
     };
-    auto failureCallback = [this, wrapper]() mutable {
-        wrapper.reject(impl().error());
+    auto failureCallback = [wrapper, reader]() mutable {
+        wrapper.reject(reader->error());
     };
 
     impl().closed(WTF::move(successCallback), WTF::move(failureCallback));
index 0a91300..772727a 100644 (file)
@@ -179,7 +179,9 @@ void ReadableJSStream::storeError(JSC::ExecState& exec)
 
 Ref<ReadableJSStream::Reader> ReadableJSStream::Reader::create(ReadableJSStream& stream)
 {
-    return adoptRef(*new Reader(stream));
+    Ref<Reader> reader = adoptRef(*new Reader(stream));
+    reader->initialize();
+    return reader;
 }
 
 ReadableJSStream::Reader::Reader(ReadableJSStream& readableStream)