Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[WebCryptoAPI] Remove race condition #14291

Merged
merged 2 commits into from
Dec 10, 2018

Conversation

jugglinmike
Copy link
Contributor

Configure testharness.js to wait until the global done function is
invoked before reporting results. This ensures that all
asynchronously-declared tests are defined and subsequently executed
regardless of variations in timing.


The instability is apparent when comparing the test results between Chrome, Firefox, and Safari, e.g. https://wpt.fyi/results/WebCryptoAPI/sign_verify?sha=58523789a0

2018-11-28-crypto-race

Firefox and Safari are missing results from tests that depend on WebCryptoAPI/sign_verify/ecdsa.js due to the race condition which this patch corrects.

Configure testharness.js to wait until the global `done` function is
invoked before reporting results. This ensures that all
asynchronously-declared tests are defined and subsequently executed
regardless of variations in timing.
@jimsch
Copy link
Contributor

jimsch commented Nov 28, 2018

I would just like to verify that it does not matter when the call to setup is made. Specifically that it does not matter if it is not called from all of the run_test functions.

@jugglinmike
Copy link
Contributor Author

setup should be called before the load event on the document has fired. In worker contexts (where there is no document), explicit_done is automatically set to true. We could move the setting to the HTML file, but I feel that it's better to have it in-line with the code that requires it. There's more detail in the framework's documentation: https://web-platform-tests.org/writing-tests/testharness-api.html

I'm glad you mentioned the other run_test functions. I was focused on the instability we're experiencing today, which happens to be limited to the tests for ECDSA. However, rsa.js and hmac.js use the same pattern and could exhibit the same problem, so I've updated those as well.

Copy link
Contributor

@Ms2ger Ms2ger left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is good. However, the tests are written as

var promise = importVectorKeys(vector, ["verify"], ["sign"]).then(function(vectors) {
  promise_test(function(test) { /* test stuff */ });
}, function(err) {
  promise_test(function(test) { assert_unreached(/* ... */) });
});

… while I would expect

promise_test(function(test) {
  return importVectorKeys(vector, ["verify"], ["sign"]).then(function(vectors) {
    /* test stuff */
  });
});

Why is that?

@jimsch
Copy link
Contributor

jimsch commented Nov 29, 2018

The reason is that not all algorithms and not all sizes may be supported. This provides a uniform error in those cases.

@jugglinmike
Copy link
Contributor Author

@jimsch Thanks for the review. Would you mind merging on my behalf?

@jimsch jimsch merged commit 8eab58f into web-platform-tests:master Dec 10, 2018
@jimsch
Copy link
Contributor

jimsch commented Dec 10, 2018

I am used to this happening automatically, but two of the tests never finished

@jugglinmike
Copy link
Contributor Author

Those "checks" are intended to produce test results to assist in review. They're still under development, though, which is why they are not required for merging.

@lukebjerring Would you like me to let you know when/if I see issues with those checks?

@lukebjerring
Copy link
Contributor

Yep; this bad state shouldn't occur anymore, and I've also updated the body of the check summary to include a link for reporting issues, e.g. https://github.com/web-platform-tests/wpt/pull/14291/checks?check_run_id=39572969

Note: wpt.fyi checks are still in beta!
See something wrong? Please file an issue!

@jugglinmike
Copy link
Contributor Author

Very nice! Thanks, Luke

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants