jsdom / webidl2js

Auto-generate JS class structures for Web IDL specifications
MIT License
79 stars 30 forks source link

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

Closed ninevra closed 3 years ago

ninevra commented 3 years ago

This PR intends to allow webidl2js to use the correct realm's AsyncIteratorPrototype. It uses a structure similar to https://github.com/jsdom/webidl2js/pull/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 https://github.com/jsdom/jsdom/pull/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:

domenic commented 3 years ago

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. (https://github.com/jsdom/webidl2js/issues/160 discusses this.)

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

ninevra commented 3 years ago

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 commented 3 years ago

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?

ninevra commented 3 years ago

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 commented 3 years ago

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 https://github.com/jsdom/jsdom/issues/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 commented 3 years ago

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:

What do people think?

domenic commented 3 years ago

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 commented 3 years ago

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.

ninevra commented 3 years ago

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