Bug 152340 – [GTK] Problem running promises code in workers

• Component: JavaScriptCore
• Status: RESOLVED FIXED

[Xabier Rodríguez Calvar]

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.

[youenn fablet]

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]

> // 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]

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]

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

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]

Created attachment 267633 [details]
Complete test

[Xabier Rodríguez Calvar]

Created attachment 267636 [details]
Complete test

Added expectations

[Zan Dobersek]

Is this still reproducible?

[youenn fablet]

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

Patch attached to bug #153194 fixed this test for me.

[Michael Catanzaro]

Let's dup this then.

*** This bug has been marked as a duplicate of bug 153194 ***

[Carlos Garcia Campos]

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]

Xabier, could you update your patch to actually add the new tests?

[Xabier Rodríguez Calvar]

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 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 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 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 on attachment 267636 [details]
Complete test

Clearing flags on attachment: 267636

Committed r195672: <http://trac.webkit.org/changeset/195672>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[youenn fablet]

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