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

Queue tasks to resolve or reject promises #386

Open
wants to merge 9 commits into
base: main
Choose a base branch
from
Open

Queue tasks to resolve or reject promises #386

wants to merge 9 commits into from

Conversation

twiss
Copy link
Member

@twiss twiss commented Dec 13, 2024

Addresses #368 by creating a "crypto task source" and queueing tasks on that task source in order to resolve or reject all promises created in response to calls to methods of SubtleCrypto while running in parallel, to prevent race conditions.

In addition, create or convert to ECMAScript objects in those tasks, inside the top-level methods, rather than inside the internal operations; and let all internal operations consistently return IDL objects instead of ECMAScript objects (deriveBits already did this, but the others did not).

Finally, clarify some of the affected spec language and the types involved.

@dontcallmedom and/or @tidoust, could you please additionally review the last commit, in particular?

Instead of creating ArrayBuffers in internal operations, create them in
the top-level functions.
Copy link
Member

@dontcallmedom dontcallmedom left a comment

Choose a reason for hiding this comment

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

I reviewed only the last commit which looks correct to me, but I would trust @tidoust 's review more than mine

spec/Overview.html Outdated Show resolved Hide resolved
Co-authored-by: Dominique Hazael-Massieux <[email protected]>
Copy link
Member

@annevk annevk left a comment

Choose a reason for hiding this comment

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

This generally looks good to me, but it would be nice to at some point modernize more of the writing style.

spec/Overview.html Show resolved Hide resolved
Comment on lines 1415 to 1416
If the following steps or referenced procedures say to
[= exception/throw =] an error,
Copy link
Member

Choose a reason for hiding this comment

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

This is a rather confusing way of writing algorithms.

Copy link
Member Author

@twiss twiss Dec 13, 2024

Choose a reason for hiding this comment

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

Yeah, I agree. However, I also think it's a somewhat useful shorthand here because to get rid of this entirely, we would need to pass the promise and the realm to the internal operations, which would then each need to queue a task whenever it encounters an error or reaches a result.

Conceptually speaking, it's somewhat similar to saying

return Promise.try(async function() {
  // remaining steps of this algorithm
});

But perhaps there's a better way of saying that?

Copy link
Member

Choose a reason for hiding this comment

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

There is https://webidl.spec.whatwg.org/#an-exception-was-thrown but it's kinda weird to throw exceptions while in parallel as they are ECMAScript objects.

<dl class="switch">
<dt>
If |format| is equal to the strings {{KeyFormat/"raw"}},
{{KeyFormat/"pkcs8"}}, or {{KeyFormat/"spki"}}:
Copy link
Member

Choose a reason for hiding this comment

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

Infra keeps the quotation marks outside of the code markup. "{{KeyFormat/raw}}" works for that.

Copy link
Member Author

Choose a reason for hiding this comment

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

OK, yeah. I'll make a separate PR for that as this is used all over the place.

Copy link
Member

Choose a reason for hiding this comment

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

Seems the spec is in contradiction to the last paragraph of https://w3c.github.io/webcrypto/#conformance

Copy link
Member Author

Choose a reason for hiding this comment

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

I think that text was more relevant before #309, in which I put many quotes outside <code>.

... I was gonna say ".. but apparently not all of them", but it seems the markup works fine as is, as well: https://w3c.github.io/webcrypto/#SubtleCrypto-method-importKey (which has similar markup as this one).

spec/Overview.html Outdated Show resolved Hide resolved
Co-authored-by: Anne van Kesteren <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants