Observable#from() conversion doesn't match the spec [363015168]

Verified

Bug

Status Update

No update yet.

Description

do...@chromium.org

created issue #1

Aug 29, 2024 07:08PM

See discussion in https://github.com/WICG/observable/issues/127 and https://github.com/WICG/observable/pull/160.

Observable#from() is split into two sections:

Detection phase: Doing just enough work to detect what kind of object (WebIDL any value) was passed in. That is, detecting whether it is an Observable, an async iterable, an iterable, or a Promise.
Conversion phase: Actually transforming the opaque any value into something we extract values from to emit to the returned Observable, once the detection phase tells us what type it is. This is mostly lazy. We create the Observable up-front, but we don't actually exhaust the passed-in value eagerly (i.e., if it is an iterable). We leave that to the Observable's subscribe callback, so that the iterable isn't exhausted until the consumer of the converted value subscribes.

In terms of web-observable behavior, the current implementation of Observable#from() does the following:

Detection semantics:
1. Get %Symbol.iterator% method. Invokes getter if one exists, re-throws errors
  1. If %Symbol.iterator% method is null or undefined, silently fail detection, try and detect the next type
  2. If method property is not callable, throw TypeError
2. Call %Symbol.iterator%` method to get iterator object. Re-throw errors.
  1. If returned value is not an Object, throw TypeError
3. Get next method (don't invoke it). Invokes getter if one exists, re-throw errors.
Conversion semantics:
1. Perform the same exact detection steps as above
2. Actually repeatedly invoke the iterator's next() method to retrieve values

There two are problems with the above:

Since the detection phase delegates to ScriptIterator::FromIterator(), we run more script and do more web-observable work than is required to detect if an object implements the iterator protocol. We should just do step 1.1 (%Symbol.iterator% detection). No more steps underneath 1 are required. This is being fixed in https://crrev.com/c/5824955.
During the conversion steps which run upon subscription to the returned Observable, we re-run the detection steps. This is a problem because the detection steps are very lenient. First, we re-pull the %Symbol.iterator% because we did not hold onto it from earlier[^1]. Second, these steps silently return/permit null or undefined methods, which is wrong because at this point we've confirmed the underlying any value is an iterable (via detection steps). If a %Symbol.iterator% method getter has decided to change values at this point, we are in the right to throw an exception. Importantly, that's what ECMAScript's GetIterator() abstract option does, which we'll use in the spec. That abstract operation expects iterable conformance and throws otherwise, which is what our implementation should do as well.

[^1]: See https://github.com/WICG/observable/issues/127. Whether we hold onto the method implementation in between detection and conversion or not doesn't really matter that much. It is an extremely corner case, although it is indeed web-observable.

Comments

ap...@google.com <ap...@google.com> #2Aug 30, 2024 02:22AM

Project: chromium/src
Branch: main

commit 911786c0908132abf07c42dff93d952f2c188d2a
Author: Dominic Farolino <dom@chromium.org>
Date: Fri Aug 30 02:20:24 2024

DOM: Fix `Observable#from()` [Symbol.iterator] semantics (1/2)

See

https://github.com/WICG/observable/pull/160, which specs the
`Observable#from()` semantics, matching these tests. See also

https://crbug.com/363015168 which describes the ways in which our
current implementation of `Observable#from()`'s detection semantics
are overbroad.

This CL makes the implementation of the "detection semantics" match the
desired behavior outlined in that issue, and adds a bunch of tests.

R=masonf@chromium.org

Bug: 363015168, 40282760
Change-Id: Id6cfdd45c44286b298e107635e4283b018f50aaf
Reviewed-on:

https://chromium-review.googlesource.com/c/chromium/src/+/5824955
Commit-Queue: Dominic Farolino <dom@chromium.org>
Reviewed-by: Mason Freed <masonf@chromium.org>
Cr-Commit-Position: refs/heads/main@{#1349019}

M third_party/blink/renderer/core/dom/observable.cc
M third_party/blink/web_tests/external/wpt/dom/observable/tentative/observable-from.any.js

https://chromium-review.googlesource.com/5824955

ap...@google.com <ap...@google.com> #3Sep 10, 2024 05:07AM

Project: chromium/src
Branch: main

commit 663974d6ce78c47e24d497069e97d3f6f53e405a
Author: Dominic Farolino <dom@chromium.org>
Date: Tue Sep 10 05:04:29 2024

Bindings: Implement basic async iterable support

This CL adapts blink::ScriptIterator to support async iterables. The
major API surface changes are:
- Exposing ScriptIterator::Kind enum {kSync, kAsync}
- Making the ScriptIterator() default constructor public, for ease of
use in the kAsync case.

Currently, blink::ScriptIterator is very simple and only operates on
synchronous iterables. This is trivial because inside the sequence, each
value is synchronously available. Async iterables however, are not
*directly* iterable as an entire sequence, so the changes made to
blink::ScriptIterator to support async iterables are fairly minimal.
These changes mostly include changes to the
blink::ScriptIterator::FromIterable() to make it more closely resemble
ECMAScript's GetIterator(obj, kind) [1] abstract algorithm, whose
`kind` argument directs the algorithm to treat the input object as
either an async or sync iterable.

One difference between ECMAScript's GetIterator() and this CL is that
ECMAScript calls for async iterables to have a fall-back mode, whereby
if @@asyncIterator is not present but @@iterator is, an async iterator
is fashioned from the @@iterator implementation, and the values are
emitted asynchronously. See [2]. This CL does not yet include this part
of the implementation yet; it is left for a follow-up.

The changes to blink::ScriptIterator::Next() are fairly minimal. Most
of the existing algorithm applies, but we split the bottom half into
two:
- The pre-existing sync half expects the value returned from the
iterator's `next()` method to be an Iterator Result, and to have a
`value` and `done` member.
- The new sync half mostly ignores this work, since the expected return
value of an *async* iterator's `next()` method is a Promise that
*resolves* to an Iterator Result.

For async iterators, blink::ScriptIterator::Value() returns the last
obtained object (probably Promise) from the iterator, and the consumer
is expected to call `.Then()` on it and handle the Iterator Result
accordingly.

========

Note that this related to, but generally distinct from the work required
for

https://crbug.com/356891478. The work for that bug includes Web IDL
bindings work, and will likely involve the creation of classes that
use `blink::ScriptIterator::FromIterable(..., Kind::Async)` under the
hood, like the example provided in the new blink::ScriptIterator docs,
and manage the returned Promise according to the semantics being
ironed out in

https://github.com/whatwg/webidl/pull/1397.

[1]:

https://tc39.es/ecma262/#sec-getiterator
[2]:

https://tc39.es/ecma262/#sec-createasyncfromsynciterator

Bug: 363015168, 40282760
Change-Id: I5b3ea724914584aacdedb5a73491ad4f4ed281df
Reviewed-on:

https://chromium-review.googlesource.com/c/chromium/src/+/5840672
Reviewed-by: Nate Chapin <japhet@chromium.org>
Commit-Queue: Dominic Farolino <dom@chromium.org>
Cr-Commit-Position: refs/heads/main@{#1353138}

M third_party/blink/renderer/bindings/core/v8/native_value_traits_impl.h
M third_party/blink/renderer/bindings/core/v8/script_iterator.cc
M third_party/blink/renderer/bindings/core/v8/script_iterator.h
M third_party/blink/renderer/bindings/scripts/bind_gen/union.py
M third_party/blink/renderer/core/animation/effect_input.cc
M third_party/blink/renderer/core/dom/observable.cc

https://chromium-review.googlesource.com/5840672

ap...@google.com <ap...@google.com> #4Sep 17, 2024 12:48AM

Project: chromium/src
Branch: main

commit 77a8e718d17f92c2aa95f4c13783f68424012805
Author: Dominic Farolino <dom@chromium.org>
Date: Tue Sep 17 00:46:45 2024

DOM: Implement async iterable conversion support for Observables

This CL implements async iterator support for Observables, and adds tons
of WPTs exercising subtle functionality of iterable and async iterable
Observable conversion semantics. It implements the spec text in

https://github.com/WICG/observable/pull/160, and is a follow-up to

https://crrev.com/c/5840672, which brings async iterable support to core
bindings code.

This CL amounts to a partial implementation of async iterable support;
what's missing and what will be included as a follow-up is:

1. Support for calling the Async Iterator's `return()` method [1] when
an Observable — when consuming an async iterable — aborts its
subscription before iterable exhaustion.
3. A possible refactor to move some of the logic that handles the
`ScriptIterator` into `ScriptIterator` itself, per discussion in
[2].

[1]:

https://tc39.es/ecma262/#sec-asynciterator-interface
[2]:

https://chromium-review.googlesource.com/c/chromium/src/+/5840672/comment/72df95a9_dd32d801/

R=masonf@chromium.org

Bug: 40282760, 363015168
Change-Id: I5f31f4028613245025de71b8975fc92e9d1def0a
Reviewed-on:

https://chromium-review.googlesource.com/c/chromium/src/+/5850509
Reviewed-by: Mason Freed <masonf@chromium.org>
Commit-Queue: Dominic Farolino <dom@chromium.org>
Cr-Commit-Position: refs/heads/main@{#1356228}

M third_party/blink/renderer/core/dom/observable.cc
D third_party/blink/web_tests/external/wpt/dom/observable/tentative/observable-catch.any-expected.txt
D third_party/blink/web_tests/external/wpt/dom/observable/tentative/observable-catch.any.worker-expected.txt
A third_party/blink/web_tests/external/wpt/dom/observable/tentative/observable-from.any-expected.txt
M third_party/blink/web_tests/external/wpt/dom/observable/tentative/observable-from.any.js
A third_party/blink/web_tests/external/wpt/dom/observable/tentative/observable-from.any.worker-expected.txt
A third_party/blink/web_tests/wpt_internal/observable/async-iterator-multiple-subscriptions-gc.any.js

https://chromium-review.googlesource.com/5850509

ap...@google.com <ap...@google.com> #5Jan 25, 2025 05:32AM

Project: chromium/src
Branch: main
Author: Dominic Farolino <dom@chromium.org>
Link: https://chromium-review.googlesource.com/6199726

DOM: Bring Observable iterable conversion inline with spec

Expand for full commit details

DOM: Bring Observable iterable conversion inline with spec 
 
This CL ensures that after `Observable.from()` is called with an 
iterable, if the resulting Observable fails to iterate when subscribed 
to, all errors are plumbed to the subscriber. 
 
This matches the spec PR in https://github.com/WICG/observable/pull/160. 
 
R=masonf 
 
Bug: 363015168 
Change-Id: I31a38d7f57ec7ab5b29388d908a6a58e0b3c1905 
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/6199726 
Reviewed-by: Mason Freed <masonf@chromium.org> 
Commit-Queue: Dominic Farolino <dom@chromium.org> 
Cr-Commit-Position: refs/heads/main@{#1411273}

Files:

M third_party/blink/renderer/core/dom/observable.cc
M third_party/blink/web_tests/external/wpt/dom/observable/tentative/observable-from.any.js

Hash: 4b00956a8d971cf5ec8ec45a105b5dd8802d4a42
Date: Fri Jan 24 21:30:26 2025

ap...@google.com <ap...@google.com> #6Jan 28, 2025 12:27AM

Project: chromium/src
Branch: main
Author: Dominic Farolino <dom@chromium.org>
Link: https://chromium-review.googlesource.com/6199630

DOM: Simplify Observable async iterable subscription

Expand for full commit details

DOM: Simplify Observable async iterable subscription 
 
This CL slightly simplifies the Observable async iterable subscription 
flow by moving the abort signal check *after* the null `ScriptIterator` 
check. This has two effects: 
  1. Allows us to remove one call to `ClearAbortAlgorithm()`, since we 
     are assigning the abort algorithm handle *after* the null 
     `ScriptIterator` check, and that check is the last opportunity to 
     cancel the subscription 
  2. Brings the async iterable subscription path inline with the *sync* 
     subscription path, which is ordered similarly [1] 
 
This has a very subtle behavior change, so this CL also adds a test for 
this case. The spec will be updated accordingly. 
 
[1]: 
https://source.chromium.org/chromium/chromium/src/+/main:third_party/blink/renderer/core/dom/observable.cc;l=1990-2019;drc=4b00956a8d971cf5ec8ec45a105b5dd8802d4a42 
 
R=masonf 
 
Bug: 363015168 
Change-Id: Idf43bed901b1339d3581ce97c1cd02a0ac6b8b48 
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/6199630 
Reviewed-by: Mason Freed <masonf@chromium.org> 
Commit-Queue: Dominic Farolino <dom@chromium.org> 
Cr-Commit-Position: refs/heads/main@{#1411976}

Files:

M third_party/blink/renderer/core/dom/observable.cc
M third_party/blink/web_tests/external/wpt/dom/observable/tentative/observable-from.any.js

Hash: b6729c19afe07f9e514b530a854ebedaf22e33a7
Date: Mon Jan 27 16:23:40 2025

do...@chromium.org <do...@chromium.org> #7Jan 28, 2025 02:59AM

Verified by do...@chromium.org.

With https://chromium-review.googlesource.com/c/chromium/src/+/6199726 and to a lesser extent https://chromium-review.googlesource.com/c/chromium/src/+/6199630, this issue has been fixed and can now be closed.

ap...@google.com <ap...@google.com> #8Jan 28, 2025 04:17PM

Project: chromium/src
Branch: main
Author: Dominic Farolino <dom@chromium.org>
Link: https://chromium-review.googlesource.com/6202182

DOM: Add more Observable iterable tests

Expand for full commit details

DOM: Add more Observable iterable tests 
 
This CL adds and supplements a few tests: 
 
  1. First we modify the existing "subscribe with aborted signal" tests. 
     Specifically, we expand their assertions to not only assert that 
     `next()` isn't ever called, but make more assertions about the 
     iterator protocol getter and function invocations in general. 
  2. Second, we modify the test that asserts `next()` is not called 
     when you subscribe with an unaborted signal, but that signal gets 
     aborted while the iterator protocol methods are called during 
     subscription of the Observable. We expand the assertions in the 
     same way as (1), and combine the two separate tests into one that 
     covers both sync and async iterators, also to match (1). 
  3. Finally, this CL adds a sync iterable version of the test added in 
     https://crrev.com/c/6199630. The test scenario is: you subscribe to 
     a sync iterable with an unaborted signal that gets aborted while 
     obtaining the iterator (just like (2)), BUT while getting the 
     iterator, an error is thrown. The tests asserts that the error is 
     reported to the global before we consult the aborted signal and 
     stop the subscription process. This ensures that the exception is 
     not swallowed, but is appropriately surfaced, even though the 
     subscription is aborted. 
 
This corresponds with the spec PR: 
https://github.com/WICG/observable/pull/192. 
 
R=masonf 
 
Bug: 363015168 
Change-Id: Ida605c49a2d73cd407a9dc3c392d6b2f338855be 
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/6202182 
Commit-Queue: Dominic Farolino <dom@chromium.org> 
Reviewed-by: Mason Freed <masonf@chromium.org> 
Cr-Commit-Position: refs/heads/main@{#1412315}

Files:

M third_party/blink/web_tests/external/wpt/dom/observable/tentative/observable-from.any.js

Hash: 0cd67687c7bd0664e937e0ab897a3e55f0aaf880
Date: Tue Jan 28 08:15:24 2025

ap...@google.com <ap...@google.com> #9Feb 5, 2025 03:34PM

Project: chromium/src
Branch: main
Author: Dominic Farolino <dom@chromium.org>
Link: https://chromium-review.googlesource.com/6225862

DOM: Remove impl-specific Observable error assertions

Expand for full commit details

DOM: Remove impl-specific Observable error assertions 
 
R=jarhar 
 
Bug: 363015168 
Change-Id: I3d587abc26144af3ff992d13d5578adf7f07b604 
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/6225862 
Reviewed-by: Joey Arhar <jarhar@chromium.org> 
Commit-Queue: Dominic Farolino <dom@chromium.org> 
Cr-Commit-Position: refs/heads/main@{#1416144}

Files:

M third_party/blink/web_tests/external/wpt/dom/observable/tentative/observable-constructor.any.js
M third_party/blink/web_tests/external/wpt/dom/observable/tentative/observable-from.any.js
M third_party/blink/web_tests/external/wpt/dom/observable/tentative/observable-takeUntil.any.js

Hash: 3fcb998ed9037e574623064f33e29f8250be8200
Date: Wed Feb 05 07:33:02 2025

ap...@google.com <ap...@google.com> #10Feb 14, 2025 07:54PM

Project: chromium/src
Branch: main
Author: Dominic Farolino <dom@chromium.org>
Link: https://chromium-review.googlesource.com/6221901

DOM: Implement ref-counted Observable producer

Expand for full commit details

DOM: Implement ref-counted Observable producer 
 
This CL implements ref-counted producers, which came out of the W3C 
TPAC discussions in 2024. After this CL, multiple 
`ObservableInternalObserver` objects can be associated/registered with 
a single active `Subscriber`. The main meat of this CL involves the 
logic managing consumer unsubscription, to ensure that only once *all* 
associated consumers/observers unsubscribe do we actually close down 
a `Subscriber`. 
 
See https://github.com/WICG/observable/pull/197. 
 
R=masonf 
 
Bug: 363015168 
Change-Id: I67b63a3f4e38bf5be0236fd1b8f025648a3089bf 
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/6221901 
Reviewed-by: Mason Freed <masonf@chromium.org> 
Commit-Queue: Dominic Farolino <dom@chromium.org> 
Cr-Commit-Position: refs/heads/main@{#1420647}

Files:

M third_party/blink/renderer/core/dom/observable.cc
M third_party/blink/renderer/core/dom/observable.h
M third_party/blink/renderer/core/dom/subscriber.cc
M third_party/blink/renderer/core/dom/subscriber.h
M third_party/blink/web_tests/external/wpt/dom/observable/tentative/observable-constructor.any.js
M third_party/blink/web_tests/external/wpt/dom/observable/tentative/observable-from.any.js
D third_party/blink/web_tests/wpt_internal/observable/async-iterator-multiple-subscriptions-gc.any.js

Hash: e24c9459e6542d5e593c39a66609c02bbaf79234
Date: Fri Feb 14 11:52:35 2025

Issue 363015168

Description

Issue summary

Comments

ap...@google.com <ap...@google.com> #2Aug 30, 2024 02:22AM

ap...@google.com <ap...@google.com> #3Sep 10, 2024 05:07AM

ap...@google.com <ap...@google.com> #4Sep 17, 2024 12:48AM

ap...@google.com <ap...@google.com> #5Jan 25, 2025 05:32AM

ap...@google.com <ap...@google.com> #6Jan 28, 2025 12:27AM

do...@chromium.org <do...@chromium.org> #7Jan 28, 2025 02:59AM

ap...@google.com <ap...@google.com> #8Jan 28, 2025 04:17PM

ap...@google.com <ap...@google.com> #9Feb 5, 2025 03:34PM

ap...@google.com <ap...@google.com> #10Feb 14, 2025 07:54PM

Add comment

Issue metadata