streams/readable-stream.html fails quite a bit, especially on slower bots. https://webkit-test-results.appspot.com/dashboards/flakiness_dashboard.html#showAllRuns=true&tests=streams%2Freadable-stream.html @@ -2,5 +2,5 @@ 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 +FAIL A readable stream controller methods should throw if its readablestream is collected assert_throws: function "function () { controller.close(); }" did not throw This is very easily reproducible locally with a debug build: run-webkit-tests streams/readable-stream.html --repeat 100 -f ... 7 tests ran as expected, 93 didn't
Created attachment 252072 [details] Patch
Comment on attachment 252072 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=252072&action=review > LayoutTests/ChangeLog:1 > +2015-04-30 Youenn Fablet <youenn.fablet@crf.canon.fr> Don't you feel lonely here? :)
(In reply to comment #1) > Created attachment 252072 [details] > Patch Improved the gc technique, and waiting before evaluating assertions after gc. Hopefully, that will solve the issue, and could be refactored in LayoutTests/resources/gc.js Split flaky test in its own file so that we can update expectations if it is still flaky. It is planned to add more gc tests to readable-stream-gc.html in the future.
Comment on attachment 252072 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=252072&action=review > LayoutTests/streams/readable-stream-gc.html:15 > + timeout = 400; I don't think that changing the timeout from 10ms to 400ms is the right approach. This difference doesn't make sense in the context of layout tests - the process can easily be suspended for several seconds. Why are you trying to wait at all here?
(In reply to comment #3) > Split flaky test in its own file so that we can update expectations if it is > still flaky. > It is planned to add more gc tests to readable-stream-gc.html in the future. If flakyness doesn't disappear completely (or almost) we might want to mark this new test as [ PASS TIMEOUT ].
(In reply to comment #2) > Comment on attachment 252072 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=252072&action=review > > > LayoutTests/ChangeLog:1 > > +2015-04-30 Youenn Fablet <youenn.fablet@crf.canon.fr> > > Don't you feel lonely here? :) Sure, I'll fix that at cq time ;) (In reply to comment #4) > Comment on attachment 252072 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=252072&action=review > > > LayoutTests/streams/readable-stream-gc.html:15 > > + timeout = 400; > > I don't think that changing the timeout from 10ms to 400ms is the right > approach. This difference doesn't make sense in the context of layout tests > - the process can easily be suspended for several seconds. > > Why are you trying to wait at all here? In the previous test, we were doing: wait, gc then check assertions. We are now doing: gc, wait, then check assertions. GC may also use window.GCController if available. We are only waiting 400ms if neither window.gc nor wingow.GCController are available. This is the same code as LayoutTests/fast/canvas/canvas-bg-multiple-removal.html. It would be good if a single technique be recommended for those kind of tests.
(In reply to comment #5) > (In reply to comment #3) > > Split flaky test in its own file so that we can update expectations if it is > > still flaky. > > It is planned to add more gc tests to readable-stream-gc.html in the future. > > If flakyness doesn't disappear completely (or almost) we might want to mark > this new test as [ PASS TIMEOUT ]. Yep, that is another bonus of splitting the test. [ Pass Failure ] in this case.
(In reply to comment #7) > > If flakyness doesn't disappear completely (or almost) we might want to mark > > this new test as [ PASS TIMEOUT ]. > > Yep, that is another bonus of splitting the test. > [ Pass Failure ] in this case. Yes, my bad. Anyway, this could be a first step towards recovering all those commented out tests, which breaks my heart because it is going to be hell to maintain.
I tested, and this test still relies on the 50 ms timeout - it fails if I change that to 0 ms. Why, what's going on here? I thought that GCController.collect was synchronous. Also, the test appears to verify that we have a bug. Garbage collection should never be observable from JS, but this test verifies the opposite.
Comment on attachment 252072 [details] Patch Please fix the actual bug.
Created attachment 252301 [details] Updating controller/stream link
(In reply to comment #9) > > Also, the test appears to verify that we have a bug. Garbage collection > should never be observable from JS, but this test verifies the opposite. The latest patch tries to fix that. We can no longer test whether garbage collection happens correctly though. I guess leak tests can capture that?
Comment on attachment 252301 [details] Updating controller/stream link View in context: https://bugs.webkit.org/attachment.cgi?id=252301&action=review > LayoutTests/streams/readable-stream-gc.html:31 > + controller.close(); > + assert_throws(new TypeError(), function() { controller.close(); }); > + assert_throws(new TypeError(), function() { controller.error(); }); Is this right?
Comment on attachment 252301 [details] Updating controller/stream link View in context: https://bugs.webkit.org/attachment.cgi?id=252301&action=review > Source/WebCore/Modules/streams/ReadableStreamController.h:45 > ~ReadableStreamController() { } Should delete this, the automatically generated one will do fine. > Source/WebCore/Modules/streams/ReadableStreamController.h:47 > ReadableJSStream* stream() { return m_stream; } Should return a reference, not a pointer. > Source/WebCore/Modules/streams/ReadableStreamController.h:53 > ReadableJSStream* m_stream; Should be a reference, not a pointer.
> I guess leak tests can capture that? No, that only captures leaks (i.e. objects that don't have pointers to).
Comment on attachment 252301 [details] Updating controller/stream link View in context: https://bugs.webkit.org/attachment.cgi?id=252301&action=review >> Source/WebCore/Modules/streams/ReadableStreamController.h:45 >> ~ReadableStreamController() { } > > Should delete this, the automatically generated one will do fine. OK >> Source/WebCore/Modules/streams/ReadableStreamController.h:47 >> ReadableJSStream* stream() { return m_stream; } > > Should return a reference, not a pointer. OK >> Source/WebCore/Modules/streams/ReadableStreamController.h:53 >> ReadableJSStream* m_stream; > > Should be a reference, not a pointer. OK >> LayoutTests/streams/readable-stream-gc.html:31 >> + assert_throws(new TypeError(), function() { controller.error(); }); > > Is this right? What we exercise here is that even if the stream reference is lost from the JS script, the controller still works properly. In that case, we validate that the stream gets closed on the first close() call, and subsequent close() and error() calls throw exceptions (as the stream is closed).
Created attachment 252377 [details] Patch for landing
Comment on attachment 252377 [details] Patch for landing Clearing flags on attachment: 252377 Committed r183803: <http://trac.webkit.org/changeset/183803>
All reviewed patches have been landed. Closing bug.