WebKit Bugzilla
New
Browse
Log In
×
Sign in with GitHub
or
Remember my login
Create Account
·
Forgot Password
Forgotten password account recovery
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
Details
Complete test
(2.13 KB, text/plain)
2015-12-18 09:14 PST
,
Xabier Rodríguez Calvar
no flags
Details
Complete test
(2.57 KB, patch)
2015-12-18 09:29 PST
,
Xabier Rodríguez Calvar
no flags
Details
Formatted Diff
Diff
Show Obsolete
(2)
View All
Add attachment
proposed patch, testcase, etc.
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.
Top of Page
Format For Printing
XML
Clone This Bug