Bug 144455 – streams/readable-stream.html is very flaky

• Component: Tools / Tests
• Status: RESOLVED FIXED

[Alexey Proskuryakov]

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

[youenn fablet]

Created attachment 252072 [details]
Patch

[Xabier Rodríguez Calvar]

Comment on attachment 252072 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=252072&action=review

> LayoutTests/ChangeLog:1
> [phone]  Youenn Fablet  <[e-mail]>

Don't you feel lonely here? :)

[youenn fablet]

(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.

[Alexey Proskuryakov]

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?

[Xabier Rodríguez Calvar]

(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 ].

[youenn fablet]

(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
> > [phone]  Youenn Fablet  <[e-mail]>
> 
> 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.

[youenn fablet]

(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.

[Xabier Rodríguez Calvar]

(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.

[Alexey Proskuryakov]

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.

[Alexey Proskuryakov]

Comment on attachment 252072 [details]
Patch

Please fix the actual bug.

[youenn fablet]

Created attachment 252301 [details]
Updating controller/stream link

[youenn fablet]

(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?

[Xabier Rodríguez Calvar]

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?

[Darin Adler]

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.

[Alexey Proskuryakov]

> I guess leak tests can capture that?

No, that only captures leaks (i.e. objects that don't have pointers to).

[youenn fablet]

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).

[youenn fablet]

Created attachment 252377 [details]
Patch for landing

[WebKit Commit Bot]

Comment on attachment 252377 [details]
Patch for landing

Clearing flags on attachment: 252377

Committed r183803: <http://trac.webkit.org/changeset/183803>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.