whatwg / streams

Streams Standard
https://streams.spec.whatwg.org/
Other
1.35k stars 160 forks source link

Specify realm for object creation #1213

Open domenic opened 2 years ago

domenic commented 2 years ago

As pointed out by @mgaudet in Matrix, the spec is not clear on what realms things should be created in, and the result is not interoperable across browsers.

To audit:

Per https://github.com/whatwg/webidl/issues/135 the intention is to use the relevant realm of this. Alternately we could fix that issue at its source per https://github.com/whatwg/webidl/issues/135#issuecomment-772716243 ... that's quite tempting.

ricea commented 2 years ago

Currently Blink's implementation is quite confused here, so it would be nice to have it clarified.

minfrin commented 2 years ago

Spent a while hitting my head on this, and not found a way around it yet:

https://bugzilla.mozilla.org/show_bug.cgi?id=1757066

The two errors encountered are:

TypeError: controller.enqueue is not a function Error: Permission denied to access property "then"

Streams can't work with web extensions on Firefox, which is a real pity - as streams are needed to handle the arbitrary size limits in the web extensions API.

ricea commented 2 years ago

TypeError: controller.enqueue is not a function Error: Permission denied to access property "then"

Streams can't work with web extensions on Firefox, which is a real pity - as streams are needed to handle the arbitrary size limits in the web extensions API.

This seems to be an issue with web extensions on Firefox rather than anything we can fix in the streams standard.

saschanaz commented 1 year ago

The new test in https://github.com/web-platform-tests/wpt/pull/38875 implicitly requires TransformStream to be created with the caller realm, which doesn't match with Gecko's this behavior. Perhaps we should finally get some normative spec here.

That said, what should happen when creating a promise with a detached realm?

ricea commented 1 year ago

The intent of the test is that it be created in the realm of the constructor. Probably we should add some comments.

Edit: Incidentally, the point of the test is that Chromium fails it, so we haven't actually tested the success case. The Chromium-only test https://source.chromium.org/chromium/chromium/src/+/main:third_party/blink/web_tests/wpt_internal/streams/transform-streams/invalid-realm.window.js documents our actual behaviour.

saschanaz commented 1 year ago

But wpt.fyi says it passes on Chrome? (Adding a failing test sounds weird, such test should at least be marked as tentative.)

ricea commented 1 year ago

But wpt.fyi says it passes on Chrome? (Adding a failing test sounds weird, such test should at least be marked as tentative.)

The test previously worked in Chrome, but updating our implementation to use read requests broke it (we switched from using promises to enqueuing microtasks directly, and it turns out that has different semantics in V8). Currently the Chrome dev channel is still on version 113, which is why it shows as passing on wpt.fyi. It will show as failing once the dev channel moves on to version 114.

As the test relies on behaviour that is not yet specced, I think you are right that it should be tentative. However, in general I think when we find a test gap it's good to write a test for it, even if for some reason we can't pass that test.

saschanaz commented 1 year ago

As the test relies on behaviour that is not yet specced, I think you are right that it should be tentative. However, in general I think when we find a test gap it's good to write a test for it, even if for some reason we can't pass that test.

+1. For now I'd like to mark it as tentative to prevent confusion from our side.