Bug 199117 – REGRESSION: [ Mojave+ Debug WK1 ] Layout Test imported/blink/storage/indexeddb/blob-basics-metadata.html is a flaky timeout

• Component: Tools / Tests
• Status: RESOLVED FIXED

[Truitt Savell]

The following layout test is flaky on Mojave Debug WK1

imported/blink/storage/indexeddb/blob-basics-metadata.html

Probable cause:

Unknown, this issue first appeared on this build: https://build.webkit.org/builders/Apple%20Mojave%20Debug%20WK1%20(Tests)/builds/3751
Possibly 246039

Flakiness Dashboard:

https://webkit-test-results.webkit.org/dashboards/flakiness_dashboard.html#showAllRuns=true&tests=imported%2Fblink%2Fstorage%2Findexeddb%2Fblob-basics-metadata.html

These was a pass at 29 seconds, possibly marking this test as slow will let it pass.

[Truitt Savell]

I cannot reproduce this locally, I am getting instant passes.

[Alexey Proskuryakov]

storage/indexeddb/modern/objectstore-autoincrement-types.html got slow at the same time.

Looking at the bots on the queue, it looks like this started after testing was interrupted, which likely means that we installed a new OS, or maybe just rebooted. Ryan, do you remember what happened to these bots on June 3rd?

PASS
3745 r246034 bot183
3746 r246035 bot183

Test interrupted:
3747
3748

FAIL
3749 r246036 bot184
3750 r246037 bot183
3751 r246038 bot184

[Alexey Proskuryakov]

Actually, I suspect that this could be similar to rdar://problem/50948871. Let's discuss in e-mail if mitigations for that radar work here too.

[Radar WebKit Bug Importer]

<rdar://problem/52967035>

[Alexey Proskuryakov]

*** Bug 200002 has been marked as a duplicate of this bug. ***

[Jonathan Bedard]

According to Alexey: "We probably just need to add opt out in JSC, disabling SMT mitigations on tests (that would be in JSC and webkitpy code, not in machine configuration)."

I've added this to run-javascriptcore-tests as well as WebKitTestRunner and DumpRenderTree.

[Jonathan Bedard]

Created attachment 385052 [details]
Patch

[Alexey Proskuryakov]

Comment on attachment 385052 [details]
Patch

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

> Source/JavaScriptCore/runtime/JSCConfig.h:55
> +        setenv("JSC_useKernTCSM", "false", 0);

This doesn't compile on Windows.

But also, is it right to do this in configureForTesting? We certainly don't want to disable this for performance tests, for example. This function name is so generic that I can't tell.

[Jonathan Bedard]

(In reply to Alexey Proskuryakov from comment #8)
> Comment on attachment 385052 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=385052&action=review
> 
> > Source/JavaScriptCore/runtime/JSCConfig.h:55
> > +        setenv("JSC_useKernTCSM", "false", 0);
> 
> This doesn't compile on Windows.
> 
> But also, is it right to do this in configureForTesting? We certainly don't
> want to disable this for performance tests, for example. This function name
> is so generic that I can't tell.

I think this is the right place to do it, but we might want some JSC folks to verify.

As far as I can tell, this function is used in a few API tests along with DumpRenderTree and some of the JSC test binaries. (I should actually remove the environment variable in run-javascriptcore-tests) Since we tend to use production binaries to do performance testing, I don't believe it will be run during performance testing, but if we have any performance tests which use DumpRenderTree or WebKitTestRunner as their harness.

We could also move these to the harness script entirely, but (as previously mentioned) that wouldn't apply to manual test runs.

[Mark Lam]

Comment on attachment 385052 [details]
Patch

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

>>> Source/JavaScriptCore/runtime/JSCConfig.h:55
>>> +        setenv("JSC_useKernTCSM", "false", 0);
>> 
>> This doesn't compile on Windows.
>> 
>> But also, is it right to do this in configureForTesting? We certainly don't want to disable this for performance tests, for example. This function name is so generic that I can't tell.
> 
> I think this is the right place to do it, but we might want some JSC folks to verify.
> 
> As far as I can tell, this function is used in a few API tests along with DumpRenderTree and some of the JSC test binaries. (I should actually remove the environment variable in run-javascriptcore-tests) Since we tend to use production binaries to do performance testing, I don't believe it will be run during performance testing, but if we have any performance tests which use DumpRenderTree or WebKitTestRunner as their harness.
> 
> We could also move these to the harness script entirely, but (as previously mentioned) that wouldn't apply to manual test runs.

I think this is not necessary.  It is also hacky to set env vars inside JavaScriptCore.

> Tools/Scripts/run-javascriptcore-tests:492
> +if (isAppleMacWebKit() || isEmbeddedWebKit()) {
> +    $envVars .= " JSC_useKernTCSM=false";
> +}

I think this should be sufficient for JSC tests.

> Tools/WebKitTestRunner/ios/TestControllerIOS.mm:109
> +    setenv("JSC_useKernTCSM", "false", 0);
> +    setenv("__XPC_JSC_useKernTCSM", "false", 0);

This is also hacky.  Can you set this in the driver script instead?

> Tools/WebKitTestRunner/mac/TestControllerMac.mm:93
> +    setenv("JSC_useKernTCSM", "false", 0);
> +    setenv("__XPC_JSC_useKernTCSM", "false", 0);

Ditto.

[Jonathan Bedard]

Created attachment 385171 [details]
Patch

[Mark Lam]

Comment on attachment 385171 [details]
Patch

LGTM, but perhaps another reviewer who knows the test scripts better should take a look as well.

[Saam Barati]

Comment on attachment 385171 [details]
Patch

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

> Tools/Scripts/run-javascriptcore-tests:491
> +    $envVars .= " JSC_useKernTCSM=false";

Why? Don’t we want our testing to test this?

[Jonathan Bedard]

(In reply to Saam Barati from comment #13)
> Comment on attachment 385171 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=385171&action=review
> 
> > Tools/Scripts/run-javascriptcore-tests:491
> > +    $envVars .= " JSC_useKernTCSM=false";
> 
> Why? Don’t we want our testing to test this?

I think it debatable if we want this variable set.

We definitely need it for layout tests, since we have so many processes running at once, it's about a 10% performance improvement on test runtime, last I checked.

I know Alexey thought that JSC correctness tests could benefit from the gains, but I don't actually know if they're faster in practice.

[Alexey Proskuryakov]

> Why? Don’t we want our testing to test this?

When this was discussed a few months back, pretty sure the consensus was that we want to run the tests quickly, so we need to disable the feature.

As for setting it in the binary - this is desirable because the fewer differences there are between running tests in the harness and without the harness, the better.

[Jonathan Bedard]

(In reply to Alexey Proskuryakov from comment #15)
> > Why? Don’t we want our testing to test this?
> 
> When this was discussed a few months back, pretty sure the consensus was
> that we want to run the tests quickly, so we need to disable the feature.
> 
> As for setting it in the binary - this is desirable because the fewer
> differences there are between running tests in the harness and without the
> harness, the better.

Maybe we should land it in the scripts first and then have a separate discussion about if we want it in the binaries. There are arguments both ways, but I don't think anyone has said we shouldn't be doing this in automation.

[Truitt Savell]

marked storage/indexeddb/modern/objectstore-autoincrement-types.html as timing out in https://trac.webkit.org/changeset/254691/webkit

[Alexey Proskuryakov]

Comment on attachment 385171 [details]
Patch

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

> Tools/Scripts/webkitpy/port/base.py:904
> +            'JSC_useKernTCSM',

It's not immediately clear why this doesn't include an __XPC_DYLD variant.

> Tools/Scripts/webkitpy/port/driver.py:440
> +        environment['__XPC_JSC_useKernTCSM'] = environment['JSC_useKernTCSM']

This looks incorrect, should it be __XPC_DYLD_JSC_useKernTCSM

[Mark Lam]

Comment on attachment 385171 [details]
Patch

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

>> Tools/Scripts/webkitpy/port/driver.py:440
>> +        environment['__XPC_JSC_useKernTCSM'] = environment['JSC_useKernTCSM']
> 
> This looks incorrect, should it be __XPC_DYLD_JSC_useKernTCSM

__XPC_ is correct.  Should not be adding DYLD.

[Alexey Proskuryakov]

Yes, you are correct of course.

[Jonathan Bedard]

Created attachment 396455 [details]
Patch

[Jonathan Bedard]

(In reply to Jonathan Bedard from comment #21)
> Created attachment 396455 [details]
> Patch

Tested this again with layout tests, verified that it's about a 10% speed up (ran only fast).

I think we should land this.

[Alexey Proskuryakov]

Comment on attachment 396455 [details]
Patch

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

Seems like there is agreement about that, marking r+.

> Tools/Scripts/run-javascriptcore-tests:526
> +if (isAppleMacWebKit() || isEmbeddedWebKit()) {

Does this work for embedded, and is it necessary?

> Tools/Scripts/webkitpy/port/driver.py:444
> +
> +

Two newlines?

[Jonathan Bedard]

Comment on attachment 396455 [details]
Patch

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

>> Tools/Scripts/run-javascriptcore-tests:526
>> +if (isAppleMacWebKit() || isEmbeddedWebKit()) {
> 
> Does this work for embedded, and is it necessary?

Looking through the code, seems like it will not work for embedded builds (it only works on x86, to be more specific)

[Jonathan Bedard]

Created attachment 396532 [details]
Patch

[EWS]

Committed r260130: <https://trac.webkit.org/changeset/260130>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 396532 [details].

[Truitt Savell]

Remove expectations for this test in https://trac.webkit.org/changeset/260141/webkit