streams/readable-stream.html is very flaky
authoryouenn.fablet@crf.canon.fr <youenn.fablet@crf.canon.fr@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Tue, 5 May 2015 13:23:52 +0000 (13:23 +0000)
committeryouenn.fablet@crf.canon.fr <youenn.fablet@crf.canon.fr@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Tue, 5 May 2015 13:23:52 +0000 (13:23 +0000)
https://bugs.webkit.org/show_bug.cgi?id=144455

Reviewed by Darin Adler.

Source/WebCore:

Changed the link between readadable stream and controller.
Controller ref()/deref() now increments/decrements its stream ref counter.
This ensures that even if JS scripts do not keep track of the readable stream,
the readable stream will not be disposed as long as the JS script has access to its controller.

Test: streams/readable-stream-gc.html

* Modules/streams/ReadableStreamController.h:
(WebCore::ReadableStreamController::ReadableStreamController):
(WebCore::ReadableStreamController::ref):
(WebCore::ReadableStreamController::deref):
(WebCore::ReadableStreamController::create): Deleted.
(WebCore::ReadableStreamController::stream): Deleted.
* bindings/js/JSReadableStreamControllerCustom.cpp:
(WebCore::JSReadableStreamController::close):
(WebCore::JSReadableStreamController::enqueue):
(WebCore::JSReadableStreamController::error):
* bindings/js/ReadableStreamJSSource.cpp:
(WebCore::ReadableStreamJSSource::~ReadableStreamJSSource):
(WebCore::ReadableStreamJSSource::start):
(WebCore::ReadableJSStream::jsController):
* bindings/js/ReadableStreamJSSource.h:

LayoutTests:

Moved flaky test to streams/readable-stream-gc.html.
Updated flaky test to check that the controller methods work well even if readable stream reference is lost by script.

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

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

LayoutTests/ChangeLog
LayoutTests/streams/readable-stream-expected.txt
LayoutTests/streams/readable-stream-gc-expected.txt [new file with mode: 0644]
LayoutTests/streams/readable-stream-gc.html [new file with mode: 0644]
LayoutTests/streams/readable-stream.html
Source/WebCore/ChangeLog
Source/WebCore/Modules/streams/ReadableStreamController.h
Source/WebCore/bindings/js/JSReadableStreamControllerCustom.cpp
Source/WebCore/bindings/js/ReadableStreamJSSource.cpp
Source/WebCore/bindings/js/ReadableStreamJSSource.h

index ee21733..1c2afa6 100644 (file)
@@ -1,3 +1,18 @@
+2015-05-05  Xabier Rodriguez Calvar  <calvaris@igalia.com> and Youenn Fablet  <youenn.fablet@crf.canon.fr>
+
+        streams/readable-stream.html is very flaky
+        https://bugs.webkit.org/show_bug.cgi?id=144455
+
+        Reviewed by Darin Adler.
+
+        Moved flaky test to streams/readable-stream-gc.html.
+        Updated flaky test to check that the controller methods work well even if readable stream reference is lost by script.
+
+        * streams/readable-stream-expected.txt:
+        * streams/readable-stream-gc-expected.txt: Added.
+        * streams/readable-stream-gc.html: Added.
+        * streams/readable-stream.html:
+
 2015-05-05  Marcos Chavarría Teijeiro  <chavarria1991@gmail.com>
 
         [GTK] Gardening 4th May
index a791305..7a1619e 100644 (file)
@@ -2,5 +2,4 @@
 PASS ReadableStream can't be constructed with garbage 
 PASS ReadableStream start should be called with the proper parameters 
 PASS ReadableStream should be able to call start method within prototype chain of its source 
-PASS A readable stream controller methods should throw if its readablestream is collected 
 
diff --git a/LayoutTests/streams/readable-stream-gc-expected.txt b/LayoutTests/streams/readable-stream-gc-expected.txt
new file mode 100644 (file)
index 0000000..e6791aa
--- /dev/null
@@ -0,0 +1,3 @@
+
+PASS Readable stream controller methods should continue working properly when scripts are loosing reference to the readable stream 
+
diff --git a/LayoutTests/streams/readable-stream-gc.html b/LayoutTests/streams/readable-stream-gc.html
new file mode 100644 (file)
index 0000000..f957684
--- /dev/null
@@ -0,0 +1,36 @@
+<!DOCTYPE html>
+<script src='../resources/testharness.js'></script>
+<script src='../resources/testharnessreport.js'></script>
+<script src='../resources/gc.js'></script>
+<script>
+
+function garbageCollectAndDo(task)
+{
+    var timeout = 50;
+    if (window.GCController)
+        window.GCController.collect();
+    else if (window.gc)
+        window.gc();
+    else
+        timeout = 400;
+    setTimeout(task, timeout);
+}
+
+t1 = async_test('Readable stream controller methods should continue working properly when scripts are loosing reference to the readable stream');
+t1.step(function() {
+    var controller;
+    new ReadableStream({
+        start: function(c) {
+            controller = c;
+        }
+    });
+
+    garbageCollectAndDo(t1.step_func(function() {
+        controller.close();
+        assert_throws(new TypeError(), function() { controller.close(); });
+        assert_throws(new TypeError(), function() { controller.error(); });
+        t1.done();
+    }));
+});
+
+</script>
index e5aeff7..44e8651 100644 (file)
@@ -1,7 +1,6 @@
 <!DOCTYPE html>
 <script src='../resources/testharness.js'></script>
 <script src='../resources/testharnessreport.js'></script>
-<script src='../resources/gc.js'></script>
 <script>
 test(function() {
     assert_throws(new TypeError(), function() {
@@ -58,20 +57,4 @@ test(function()
     assert_true(isStartCalled);
 }, 'ReadableStream should be able to call start method within prototype chain of its source');
 
-t1 = async_test('A readable stream controller methods should throw if its readablestream is collected');
-t1.step(function() {
-    var controller;
-    new ReadableStream({
-        start: function(c) {
-            controller = c;
-        }
-    });
-    setTimeout(t1.step_func(function() {
-        window.gc();
-        assert_throws(new TypeError(), function() { controller.close(); });
-        assert_throws(new TypeError(), function() { controller.error(); });
-        assert_throws(new TypeError(), function() { controller.enqueue(); });
-        t1.done();
-    }), 10);
-});
 </script>
index 3678c3d..f7046f0 100644 (file)
@@ -1,3 +1,33 @@
+2015-05-05  Xabier Rodriguez Calvar  <calvaris@igalia.com> and Youenn Fablet  <youenn.fablet@crf.canon.fr>
+
+        streams/readable-stream.html is very flaky
+        https://bugs.webkit.org/show_bug.cgi?id=144455
+
+        Reviewed by Darin Adler.
+
+        Changed the link between readadable stream and controller.
+        Controller ref()/deref() now increments/decrements its stream ref counter.
+        This ensures that even if JS scripts do not keep track of the readable stream,
+        the readable stream will not be disposed as long as the JS script has access to its controller.
+
+        Test: streams/readable-stream-gc.html
+
+        * Modules/streams/ReadableStreamController.h:
+        (WebCore::ReadableStreamController::ReadableStreamController):
+        (WebCore::ReadableStreamController::ref):
+        (WebCore::ReadableStreamController::deref):
+        (WebCore::ReadableStreamController::create): Deleted.
+        (WebCore::ReadableStreamController::stream): Deleted.
+        * bindings/js/JSReadableStreamControllerCustom.cpp:
+        (WebCore::JSReadableStreamController::close):
+        (WebCore::JSReadableStreamController::enqueue):
+        (WebCore::JSReadableStreamController::error):
+        * bindings/js/ReadableStreamJSSource.cpp:
+        (WebCore::ReadableStreamJSSource::~ReadableStreamJSSource):
+        (WebCore::ReadableStreamJSSource::start):
+        (WebCore::ReadableJSStream::jsController):
+        * bindings/js/ReadableStreamJSSource.h:
+
 2015-05-05  Myles C. Maxfield  <mmaxfield@apple.com>
 
         Small cleanup in RenderText::computePreferredLogicalWidths()
index 6fd9d21..f3415c1 100644 (file)
 
 #if ENABLE(STREAMS_API)
 
-#include <wtf/Ref.h>
-#include <wtf/RefCounted.h>
+#include "ReadableStreamJSSource.h"
 
 namespace WebCore {
 
-class ReadableJSStream;
-
 // This class is only used for JS source readable streams to allow enqueuing, closing or erroring a readable stream.
 // Its definition is at https://streams.spec.whatwg.org/#rs-controller-class.
 // Note that its constructor is taking a ReadableJSStream as it should only be used for JS sources.
-class ReadableStreamController : public RefCounted<ReadableStreamController> {
+class ReadableStreamController {
 public:
-    static Ref<ReadableStreamController> create(ReadableJSStream& stream)
-    {
-        auto controller = adoptRef(*new ReadableStreamController(stream));
-        return controller;
-    }
-    ~ReadableStreamController() { }
+    explicit ReadableStreamController(ReadableJSStream& stream)
+        : m_stream(stream) { }
 
-    void resetStream() { m_stream = nullptr; }
-    ReadableJSStream* stream() { return m_stream; }
+    ReadableJSStream& stream() { return m_stream; }
 
-private:
-    ReadableStreamController(ReadableJSStream& stream) { m_stream = &stream; }
+    void ref() { m_stream.ref(); }
+    void deref() { m_stream.deref(); }
 
-    ReadableJSStream* m_stream;
+private:
+    ReadableJSStream& m_stream;
 };
 
 }
index 846ac70..fb5d6f8 100644 (file)
@@ -43,31 +43,24 @@ namespace WebCore {
 
 JSValue JSReadableStreamController::close(ExecState* exec)
 {
-    ReadableJSStream* stream = impl().stream();
-    if (!stream)
-        return exec->vm().throwException(exec, createTypeError(exec, ASCIILiteral("Controller has no readablestream")));
+    ReadableJSStream& stream = impl().stream();
     // FIXME: Handle the case of draining.
-    if (stream->internalState() != ReadableStream::State::Readable)
+    if (stream.internalState() != ReadableStream::State::Readable)
         return exec->vm().throwException(exec, createTypeError(exec, ASCIILiteral("Calling close on a stream which is not readable")));
-    stream->changeStateToClosed();
+    stream.changeStateToClosed();
     return jsUndefined();
 }
 
-JSValue JSReadableStreamController::enqueue(ExecState* exec)
+JSValue JSReadableStreamController::enqueue(ExecState*)
 {
-    ReadableJSStream* stream = impl().stream();
-    if (!stream)
-        return exec->vm().throwException(exec, createTypeError(exec, ASCIILiteral("Controller has no readablestream")));
     notImplemented();
     return jsBoolean(false);
 }
 
 JSValue JSReadableStreamController::error(ExecState* exec)
 {
-    ReadableJSStream* stream = impl().stream();
-    if (!stream)
-        return exec->vm().throwException(exec, createTypeError(exec, ASCIILiteral("Controller has no readablestream")));
-    if (stream->internalState() != ReadableStream::State::Readable)
+    ReadableJSStream& stream = impl().stream();
+    if (stream.internalState() != ReadableStream::State::Readable)
         return exec->vm().throwException(exec, createTypeError(exec, ASCIILiteral("Calling error on a stream which is not readable")));
     notImplemented();
     return jsUndefined();
index 2220ecd..94e910b 100644 (file)
@@ -35,6 +35,7 @@
 #include "DOMWrapperWorld.h"
 #include "JSDOMPromise.h"
 #include "JSReadableStream.h"
+#include "JSReadableStreamController.h"
 #include "NotImplemented.h"
 #include "ScriptExecutionContext.h"
 #include <runtime/Error.h>
@@ -90,8 +91,6 @@ ReadableStreamJSSource::ReadableStreamJSSource(JSC::ExecState* exec)
 
 ReadableStreamJSSource::~ReadableStreamJSSource()
 {
-    if (m_controller)
-        m_controller.get()->impl().resetStream();
 }
 
 static void startReadableStreamAsync(ReadableStream& readableStream)
@@ -111,9 +110,6 @@ void ReadableStreamJSSource::start(ExecState& exec, ReadableJSStream& readableSt
 {
     JSLockHolder lock(&exec);
 
-    Ref<ReadableStreamController> controller = ReadableStreamController::create(readableStream);
-    m_controller.set(exec.vm(), jsDynamicCast<JSReadableStreamController*>(toJS(&exec, globalObject(), controller)));
-
     JSValue startFunction = getPropertyFromObject(&exec, m_source.get(), "start");
     if (!startFunction.isFunction()) {
         if (!startFunction.isUndefined())
@@ -124,7 +120,7 @@ void ReadableStreamJSSource::start(ExecState& exec, ReadableJSStream& readableSt
     }
 
     MarkedArgumentBuffer arguments;
-    arguments.append(m_controller.get());
+    arguments.append(readableStream.jsController(exec, globalObject()));
 
     JSValue exception;
     callFunction(&exec, startFunction, m_source.get(), arguments, &exception);
@@ -161,6 +157,13 @@ ReadableStreamJSSource& ReadableJSStream::jsSource()
     return static_cast<ReadableStreamJSSource&>(source());
 }
 
+JSValue ReadableJSStream::jsController(ExecState& exec, JSDOMGlobalObject* globalObject)
+{
+    if (!m_controller)
+        m_controller = std::make_unique<ReadableStreamController>(*this);
+    return toJS(&exec, globalObject, m_controller.get());
+}
+
 Ref<ReadableJSStream::Reader> ReadableJSStream::Reader::create(ReadableJSStream& stream)
 {
     return adoptRef(*new Reader(stream));
index 131d411..30e3b1d 100644 (file)
@@ -32,7 +32,6 @@
 
 #if ENABLE(STREAMS_API)
 
-#include "JSReadableStreamController.h"
 #include "ReadableStream.h"
 #include "ReadableStreamReader.h"
 #include "ReadableStreamSource.h"
 
 namespace WebCore {
 
+class JSDOMGlobalObject;
+class ReadableJSStream;
+class ReadableStreamController;
+
 class ReadableStreamJSSource: public ReadableStreamSource {
 public:
     static Ref<ReadableStreamJSSource> create(JSC::ExecState*);
@@ -57,15 +60,15 @@ private:
 
     // Object passed to constructor.
     JSC::Strong<JSC::JSObject> m_source;
-
-    JSC::Strong<JSReadableStreamController> m_controller;
 };
 
 class ReadableJSStream: public ReadableStream {
 public:
     static Ref<ReadableJSStream> create(JSC::ExecState&, ScriptExecutionContext&);
     virtual Ref<ReadableStreamReader> createReader() override;
+
     ReadableStreamJSSource& jsSource();
+    JSC::JSValue jsController(JSC::ExecState&, JSDOMGlobalObject*);
 
 private:
     ReadableJSStream(ScriptExecutionContext&, Ref<ReadableStreamJSSource>&&);
@@ -76,6 +79,8 @@ private:
     private:
         explicit Reader(ReadableJSStream&);
     };
+
+    std::unique_ptr<ReadableStreamController> m_controller;
 };
 
 void setInternalSlotToObject(JSC::ExecState*, JSC::JSValue, JSC::PrivateName&, JSC::JSValue);