RESOLVED FIXED 152340
[GTK] Problem running promises code in workers
https://bugs.webkit.org/show_bug.cgi?id=152340
Summary [GTK] Problem running promises code in workers
Xabier Rodríguez Calvar
Reported 2015-12-16 10:04:17 PST
Created attachment 267463 [details] This patch makes the test easier When running that test with JIT activated in a worker, test times out badly, without JIT, it works as expected. I narrowed the code as much as I could so that it can be easier to chase the root cause and for that you can apply the attached patch.
Attachments
This patch makes the test easier (8.62 KB, application/octet-stream)
2015-12-16 10:04 PST, Xabier Rodríguez Calvar
no flags
Complete test (2.13 KB, text/plain)
2015-12-18 09:14 PST, Xabier Rodríguez Calvar
no flags
Complete test (2.57 KB, patch)
2015-12-18 09:29 PST, Xabier Rodríguez Calvar
no flags
youenn fablet
Comment 1 2015-12-17 02:42:31 PST
On my machine, I can further simplify the test to: Given the /resources/... links, it needs to be run behind WPT server. /////// test html file/////// <script src="/resources/testharness.js"></script> <script src="/resources/testharnessreport.js"></script> <script> 'use strict'; fetch_tests_from_worker(new Worker('test.js')); </script> /////// test.js file/////// 'use strict'; if (self.importScripts) { self.importScripts('/resources/testharness.js'); } const reader = new ReadableStream().getReader(); // Changing from 2 to 1 makes the test pass. for (let i = 0; i < 1; i++) { promise_test(t => { for (let j = 0; j < 10000; j++) reader.read().then(); return new Promise(resolve => step_timeout(resolve, 500)); }, 'Streams and promises in Worker'); } done();
youenn fablet
Comment 2 2015-12-17 08:10:22 PST
> // Changing from 2 to 1 makes the test pass. > for (let i = 0; i < 1; i++) { Of course, it should be " Changing from 1 to 2 makes the test time out". It might be good to try making the test ReadableStream agnostic.
youenn fablet
Comment 3 2015-12-17 08:14:13 PST
This further simplified version is also timing out for me. //////// test.js //////// var a = []; // Changing from 2 to 1 makes the test pass. for (let i = 0; i < 2; i++) { promise_test(t => { for (let j = 0; j < 10000; j++) a.push(new Promise(function() {})); return new Promise(resolve => step_timeout(resolve, 500)); }, 'Streams and promises in Worker'); } done();
Xabier Rodríguez Calvar
Comment 4 2015-12-17 08:44:47 PST
(In reply to comment #3) > This further simplified version is also timing out for me. I can confirm that it creates the same effect.
Xabier Rodríguez Calvar
Comment 5 2015-12-18 08:37:21 PST
I did further checks and: * It seems test fails regardless of using JIT or not. * It does happen only in workers (it doesn't happen in regular code). * It happens in GTK but it does not happen in Mac.
Xabier Rodríguez Calvar
Comment 6 2015-12-18 09:14:08 PST
Created attachment 267633 [details] Complete test
Xabier Rodríguez Calvar
Comment 7 2015-12-18 09:29:25 PST
Created attachment 267636 [details] Complete test Added expectations
Zan Dobersek
Comment 8 2016-01-03 01:56:53 PST
Is this still reproducible?
youenn fablet
Comment 9 2016-01-04 00:45:08 PST
(In reply to comment #8) > Is this still reproducible? In my environment, yes. The test is timing out when looping twice over the internal promise test.
Carlos Garcia Campos
Comment 10 2016-01-25 07:01:02 PST
Patch attached to bug #153194 fixed this test for me.
Michael Catanzaro
Comment 11 2016-01-25 07:54:39 PST
Let's dup this then. *** This bug has been marked as a duplicate of bug 153194 ***
Carlos Garcia Campos
Comment 12 2016-01-25 08:01:31 PST
I left this one open, because I don't really know if it's the same bug, same patch fixing two bugs doesn't mean both bugs are the same. So, my plan was to use this bug to include the layout test attached here.
Carlos Garcia Campos
Comment 13 2016-01-27 02:23:08 PST
Xabier, could you update your patch to actually add the new tests?
Xabier Rodríguez Calvar
Comment 14 2016-01-27 06:39:00 PST
Comment on attachment 267636 [details] Complete test It seems the test works. Asking for r+ to be able to land and avoid regressions.
Carlos Garcia Campos
Comment 15 2016-01-27 06:43:59 PST
Comment on attachment 267636 [details] Complete test Please wait for EWS before landing to ensure it passes test in mac too, otherwise we would need to change the test expectations
Xabier Rodríguez Calvar
Comment 16 2016-01-27 06:58:00 PST
Comment on attachment 267636 [details] Complete test (In reply to comment #15) > Comment on attachment 267636 [details] > Complete test > > Please wait for EWS before landing to ensure it passes test in mac too, > otherwise we would need to change the test expectations I think it is safe to land in Mac as it was passing already when we wrote the test but yes, there's no problem in waiting.
Xabier Rodríguez Calvar
Comment 17 2016-01-27 07:19:20 PST
Comment on attachment 267636 [details] Complete test (In reply to comment #16) > I think it is safe to land in Mac as it was passing already when we wrote > the test but yes, there's no problem in waiting. It works, landing.
WebKit Commit Bot
Comment 18 2016-01-27 07:36:06 PST
Comment on attachment 267636 [details] Complete test Clearing flags on attachment: 267636 Committed r195672: <http://trac.webkit.org/changeset/195672>
WebKit Commit Bot
Comment 19 2016-01-27 07:36:10 PST
All reviewed patches have been landed. Closing bug.
youenn fablet
Comment 20 2016-02-05 00:34:17 PST
(In reply to comment #17) > Comment on attachment 267636 [details] > Complete test > > (In reply to comment #16) > > I think it is safe to land in Mac as it was passing already when we wrote > > the test but yes, there's no problem in waiting. > > It works, landing. Great to see that fixed!
Note You need to log in before you can comment on or make changes to this bug.