Bug 144455 - streams/readable-stream.html is very flaky
Summary: streams/readable-stream.html is very flaky
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Tools / Tests (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: youenn fablet
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2015-04-30 10:20 PDT by Alexey Proskuryakov
Modified: 2015-05-05 06:25 PDT (History)
5 users (show)

See Also:


Attachments
Patch (4.52 KB, patch)
2015-04-30 11:11 PDT, youenn fablet
no flags Details | Formatted Diff | Diff
Updating controller/stream link (14.45 KB, patch)
2015-05-04 03:31 PDT, youenn fablet
no flags Details | Formatted Diff | Diff
Patch for landing (14.04 KB, patch)
2015-05-05 05:36 PDT, youenn fablet
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Alexey Proskuryakov 2015-04-30 10:20:14 PDT
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
Comment 1 youenn fablet 2015-04-30 11:11:28 PDT
Created attachment 252072 [details]
Patch
Comment 2 Xabier Rodríguez Calvar 2015-04-30 11:15:18 PDT
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? :)
Comment 3 youenn fablet 2015-04-30 11:16:30 PDT
(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 4 Alexey Proskuryakov 2015-04-30 11:20:36 PDT
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?
Comment 5 Xabier Rodríguez Calvar 2015-04-30 11:28:53 PDT
(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 ].
Comment 6 youenn fablet 2015-04-30 11:34:38 PDT
(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.
Comment 7 youenn fablet 2015-04-30 11:35:48 PDT
(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.
Comment 8 Xabier Rodríguez Calvar 2015-04-30 11:51:59 PDT
(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.
Comment 9 Alexey Proskuryakov 2015-04-30 17:14:16 PDT
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 10 Alexey Proskuryakov 2015-04-30 17:15:01 PDT
Comment on attachment 252072 [details]
Patch

Please fix the actual bug.
Comment 11 youenn fablet 2015-05-04 03:31:14 PDT
Created attachment 252301 [details]
Updating controller/stream link
Comment 12 youenn fablet 2015-05-04 05:47:51 PDT
(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 13 Xabier Rodríguez Calvar 2015-05-04 06:52:41 PDT
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 14 Darin Adler 2015-05-04 16:02:05 PDT
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.
Comment 15 Alexey Proskuryakov 2015-05-04 19:50:38 PDT
> I guess leak tests can capture that?

No, that only captures leaks (i.e. objects that don't have pointers to).
Comment 16 youenn fablet 2015-05-05 05:33:20 PDT
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).
Comment 17 youenn fablet 2015-05-05 05:36:42 PDT
Created attachment 252377 [details]
Patch for landing
Comment 18 WebKit Commit Bot 2015-05-05 06:25:13 PDT
Comment on attachment 252377 [details]
Patch for landing

Clearing flags on attachment: 252377

Committed r183803: <http://trac.webkit.org/changeset/183803>
Comment 19 WebKit Commit Bot 2015-05-05 06:25:16 PDT
All reviewed patches have been landed.  Closing bug.