Create async iterators and iterator result objects in the correct realm #247

ninevra · 2021-06-24T21:03:28Z

This PR intends to allow webidl2js to use the correct realm's AsyncIteratorPrototype. It uses a structure similar to #234, registering an AsyncIteratorPrototype in each global object's ctorRegistry and inheriting from that prototype in install().

#234 obtained the IteratorPrototype from globalObject.Array; unfortunately, afaik there are no ecmascript globals that link to the AsyncIteratorPrototype, so that approach is not possible here.

This PR exposes a utility, registerConstructor(globalObject, key, constructor) (bikeshedding appreciated), which sets a binding in the globalObject's ctorRegistry. Consumers can use this to register the correct AsyncIteratorPrototype prior to installing interfaces on their global object, using the key "%AsyncIteratorPrototype%".

Updated: This PR uses globalObject.eval("(async function* () {})") to find the correct realm's AsyncIteratorPrototype. It also creates the iterator result objects ({value, done}) in the correct realm, though it does not use the correct realm's Promises.

As a fallback, if eval() doesn't work, initCtorRegistry() initializes "%AsyncIteratorPrototype%" to the generated class's realm's AsyncIteratorPrototype.

Helps with jsdom/jsdom#3200.

I'm not sure how to test this. Does webidl2js currently have any form of behavior testing, or just generated code snapshots? Currently, I'm testing by linking this into a local fork of jsdom.

Remaining issues:

the {value, done} objects returned by the async iterator are still created in the wrong realm, causing readable-streams/async-iterator.any.js to still fail...

domenic · 2021-06-28T15:35:58Z

This looks good in general!

We have a very small set of behavior tests for the utils file: https://github.com/jsdom/webidl2js/blob/master/test/utils.test.js . It might be worth trying to add another, but it seems hard given that everything is tied to file I/O right now. (#160 discusses this.)

The { value, done } object realm can probably be fixed by storing the Object constructor similar to what is done for Array...

These _should_ be created in the current realm, but I don't see how to do that yet.

ninevra · 2021-06-28T18:13:21Z

I added Object to the list of things to capture in initCtorRegistry() and started using it to construct the iterator return results. I think this is technically incorrect, the captured Object is from the relevant realm and the iterator return values ought to be from the current realm, but I'm not entirely sure about that, nor do I know how to access the current realm at all.

Also added (very) basic tests for the new utilities; not sure if they're valuable, but they're there.

ninevra · 2021-06-28T18:19:07Z

The main remaining question is what to call the new API and how to document it. registerConstructor() and setInConstructorRegistry() are the two names that have been floated so far. "Constructor registry" is itself a misnomer; since #234 there are now prototypes in it as well as constructors.

Should all the existing "well-known keys" be documented or just "%AsyncIteratorPrototype%"? Should registerConstructor() be marked as unstable, so that e.g. #242 wouldn't have to be a major version bump?

TimothyGu

Just curious, can we possibly get the object through globalObject.eval("(async function* () {})")?

Either way, even if eval works, I think this is a good approach, just in case folks have CSP or something similar enabled that prevents eval from executing.

TimothyGu · 2021-06-28T22:45:45Z

lib/output/utils.js

  ctorRegistry["%IteratorPrototype%"] = Object.getPrototypeOf(
    Object.getPrototypeOf(new ctorRegistry["%Array%"]()[Symbol.iterator]())
  );
+  ctorRegistry["%AsyncIteratorPrototype%"] = AsyncIteratorPrototype;


Add a comment here saying that this is not actually correct, and integrators should use registerConstructor to do the right thing.

TimothyGu · 2021-06-28T22:46:06Z

lib/output/utils.js


  globalObject[ctorRegistrySymbol] = ctorRegistry;
  return ctorRegistry;
 }

+// Associates a constructor with the given global object. Intended as a public
+// utility for the purpose of registering "%AsyncIteratorPrototype%".
+function registerConstructor(globalObject, key, constructor) {


The name is a bit imprecise, since we allow registering any intrinsic, not just constructors.

Edit: I see you noticed this as well. We can do registerIntrinsic maybe?

ninevra · 2021-06-29T00:08:02Z

Just curious, can we possibly get the object through globalObject.eval("(async function* () {})")?

Either way, even if eval works, I think this is a good approach, just in case folks have CSP or something similar enabled that prevents eval from executing.

That's brilliant! eval() does appear to work. And it's even isomorphic with respect to runScripts, since jsdom fills in global.eval when not using vm.

domenic · 2021-06-29T21:13:36Z

Hmm so if eval() works I'm tempted to omit this public API for now... especially since I'd rather have AsyncIteratorPrototype use eval by default, so that it's from the same realm as IteratorPrototype by default.

Regarding relevant vs. current, yeah, we're not going to be able to get current unless we do the work in jsdom/jsdom#2727 (roughly, eval the entire webidl2js bundle inside the jsdom realm).

In general I don't go for the philosophy of introducing unstable APIs that don't benefit from semver. New major version bumps are relatively cheap.

ninevra · 2021-06-30T00:30:33Z

Added a commit switching to using eval() to get the right AsyncIteratorPrototype, per comments above. (I'm probably more comfortable with registerIntrinsic() than eval(), personally, but eval() is quite elegant here.) This does work, and removes the need for the registerIntrinsic() api.

Possible downsides:

It's eval(), it's scary and dangerous; and further it's the outer Node realms' eval in at least some common cases.
- The argument to eval is just "(async function* () {})", which looks safe to me and doesn't include any dynamic values or user input, but I wouldn't rely on my own understanding here.
It adds another requirement that globalObjects must meet: they must have Object, Array, and now eval properties, possibly others.
- Could include a fallback to the current realm if eval isn't defined / isn't callable?
The (new) newObjectInRealm util tests now need // eslint-disable-next-line no-eval because they mock globalObject
- Could remove these tests or use a better mock (e.g. a vm context with its intrinsics exposed like jsdom does).
Might raise alarms on downstream projects that notice the use of eval in the code or tests?

What do people think?

domenic · 2021-06-30T17:13:03Z

Given that this is mostly a Node-focused project, and we already use vm.runInContext() a lot which is equivalent to eval(), I think it's fine. @TimothyGu?

TimothyGu · 2021-07-06T21:50:39Z

My main concern here is that this is webidl2js, not jsdom. Webidl2js is used for some more "isomorphic" projects like whatwg-url, which we do expect to run in browsers. In the past, we've seen some issues with using webidl2js in browsers, such as with Safari and SharedArrayBuffer; but it was reasonably easy to work around that issue by polyfilling globalThis.SharedArrayBuffer. With eval and CSP compatibility though, it's harder to understand what's going on from error messages, and there isn't an escape hatch like we do with SharedArrayBuffer.

I think I'd be fine with throwing a try-catch around the eval, and set ctorRegistry["%AsyncIteratorPrototype%"] to the parent realm's AsyncIteratorPrototype if there was an exception.

TimothyGu · 2021-07-06T21:51:47Z

lib/output/utils.js

+function newObjectInRealm(globalObject, object) {
+  const ctorRegistry = initCtorRegistry(globalObject);
+  return Object.defineProperties(
+    Object.create(ctorRegistry["%Object%"].prototype),


nit: could we additionally capture %Object.prototype% in ctorRegistry, and use that here? One can imagine some weird client code overwriting Object.prototype.

Good catch!

lib/output/utils.js

ninevra · 2021-07-07T19:24:12Z

lib/output/utils.js

+const IteratorPrototype = Object.getPrototypeOf(Object.getPrototypeOf([][Symbol.iterator]()));
+const AsyncIteratorPrototype = Object.getPrototypeOf(Object.getPrototypeOf(async function* () {}).prototype);


These exports could be removed now; should they be?

I think removing them would technically be a major bump. Maybe add a comment here saying we should remove them when we bump?

Makes sense. It's worth noting though that there's already a bit of incompatibility between the main branch and the last release; interfaces generated after #234 expect initCtorRegistry to be the only way of initializing the ctorRegistry, while code generated before #234 naturally doesn't call initCtorRegistry, as it didn't exist. Installing pre-#234 interfaces before post-#234 interfaces causes the latter to throw, because the ctorRegistry was created but %IteratorPrototype% was not captured. There's a workaround (calling initCtorRegistry manually before installing interfaces), which I've been using in local testing, but it's not documented.

I sort of assumed that the next release would be major for this reason, though admittedly that would probably be more of a pain for jsdom than just using the workaround. I'm not terribly clear on what the semver guarantees mean for codegen projects, much less for domexception and other projects whose public interface is only part codegen.

ninevra · 2021-07-07T21:59:29Z

This PR is ready for review/merge as far as I'm aware. I've updated the title & header to reflect the current implementation.

ninevra added 4 commits June 24, 2021 12:50

Capture (wrong realm's) AsyncIterator prototype

1748942

Use the captured async iterator prototype

07a8b85

Add registerConstructor utility

2df034f

Update snapshots

6497682

ninevra added 6 commits June 28, 2021 08:38

Create iterator results in the relevant realm

57d54e6

These _should_ be created in the current realm, but I don't see how to do that yet.

Update snapshots

58a1509

Store Object in the ctorRegistry

4e716a4

Test registerConstructor()

3ee1af8

Test newObjectInRealm()

79bedc3

Try to document registerConstructor()

e553d7f

ninevra marked this pull request as ready for review June 28, 2021 18:19

TimothyGu reviewed Jun 28, 2021

View reviewed changes

ninevra added 2 commits June 29, 2021 08:34

Rename registerConstructor to registerIntrinsic

9552606

Add an explanatory comment

229b06d

Use eval() to create an async generator function

9aa0049

TimothyGu reviewed Jul 6, 2021

View reviewed changes

ninevra mentioned this pull request Jul 7, 2021

Implement streams by adopting the reference implementation jsdom/jsdom#3200

Draft

10 tasks

ninevra added 3 commits July 7, 2021 11:27

Capture Object.prototype directly

268fc9d

Use fallback AsyncIteratorPrototype if eval fails

682cc7d

Remove unused error variable

c56aeae

TimothyGu approved these changes Jul 7, 2021

View reviewed changes

Remove eval from tests

50c7379

Remove blank line

02dcb67

ninevra commented Jul 7, 2021

View reviewed changes

Comment unused exports

3091543

ninevra changed the title ~~Allow injecting correct realm's AsyncIteratorPrototype~~ Create async iterators and iterator result objects in the correct realm Jul 7, 2021

Remove exports since we're bumping the major version anyway

c60840f

domenic merged commit b8f389f into jsdom:master Sep 12, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Create async iterators and iterator result objects in the correct realm #247

Create async iterators and iterator result objects in the correct realm #247

ninevra commented Jun 24, 2021 •

edited

Loading

domenic commented Jun 28, 2021

ninevra commented Jun 28, 2021

ninevra commented Jun 28, 2021

TimothyGu left a comment

TimothyGu Jun 28, 2021

TimothyGu Jun 28, 2021 •

edited

Loading

ninevra commented Jun 29, 2021

domenic commented Jun 29, 2021

ninevra commented Jun 30, 2021 •

edited

Loading

domenic commented Jun 30, 2021

TimothyGu commented Jul 6, 2021

TimothyGu Jul 6, 2021

ninevra Jul 7, 2021

ninevra Jul 7, 2021

TimothyGu Jul 7, 2021

ninevra Jul 7, 2021

ninevra commented Jul 7, 2021

		const IteratorPrototype = Object.getPrototypeOf(Object.getPrototypeOf([][Symbol.iterator]()));
		const AsyncIteratorPrototype = Object.getPrototypeOf(Object.getPrototypeOf(async function* () {}).prototype);

Create async iterators and iterator result objects in the correct realm #247

Create async iterators and iterator result objects in the correct realm #247

Conversation

ninevra commented Jun 24, 2021 • edited Loading

domenic commented Jun 28, 2021

ninevra commented Jun 28, 2021

ninevra commented Jun 28, 2021

TimothyGu left a comment

Choose a reason for hiding this comment

TimothyGu Jun 28, 2021

Choose a reason for hiding this comment

TimothyGu Jun 28, 2021 • edited Loading

Choose a reason for hiding this comment

ninevra commented Jun 29, 2021

domenic commented Jun 29, 2021

ninevra commented Jun 30, 2021 • edited Loading

domenic commented Jun 30, 2021

TimothyGu commented Jul 6, 2021

TimothyGu Jul 6, 2021

Choose a reason for hiding this comment

ninevra Jul 7, 2021

Choose a reason for hiding this comment

ninevra Jul 7, 2021

Choose a reason for hiding this comment

TimothyGu Jul 7, 2021

Choose a reason for hiding this comment

ninevra Jul 7, 2021

Choose a reason for hiding this comment

ninevra commented Jul 7, 2021

ninevra commented Jun 24, 2021 •

edited

Loading

TimothyGu Jun 28, 2021 •

edited

Loading

ninevra commented Jun 30, 2021 •

edited

Loading