Could this be the same as bug 1525852?

Yes but this is for DOM Streams which is a re-implementation. I guess we can go close some JS stream issues as we want to focus on DOM streams.

I'm currently restructuring the async iterable code quite a bit (see bug 1788969, bug 1777145 and bug 1782400), and I'm hoping to add some documentation this week. After those changes land you'll need to make some changes to your implementation.

1. The data that you need to store per iterator should be in a struct or class named IteratorData, publicly declared nested inside the C++ class that implements the iterable (I think ReadableStream in this case)
2. InitAsyncIterator should have the signature void InitAsyncIteratorData(IteratorData& aData, Iterator::IteratorType aType, [any arguments coming from the asynchronously iterable declaration, ]ErrorResult& aError). This needs to implement the asynchronous iterator initialization steps, probably setting members in aData in the process.
3. DestroyAsyncIterator goes away
4. If the IteratorData holds things that should be cycle collected then it should have void Traverse(nsCycleCollectionTraversalCallback& cb) and void Unlink() methods to traverse and unlink that data.
5. The AsyncIterableIterator<…> has a Data() method for getting the data.
6. GetNextIterationResult should have the signature already_AddRefed<Promise> GetNextIterationResult(Iterator* aIterator, ErrorResult& aError). This just needs to implement the get the next iteration result algorithm (it wasn't clear before, we used to also expect this to implement a big part of the steps for next in https://webidl.spec.whatwg.org/#es-asynchronous-iterator-prototype-object but not anymore). The code should resolve the promise that this returns either simply with the value (no need to call iterator_utils::ResolvePromiseWithKeyOrValue, which is going away, but just Promise::MaybeResolve) or by calling iterator_utils::ResolvePromiseForFinished(promise).
7. If there is an asynchronous iterator return algorithm then the async iterable declaration needs to have a [GenerateReturnMethod] extended attribute in the WebIDL, and there needs to be a IteratorReturn method that has the signature already_AddRefed<Promise> IteratorReturn(JSContext* aCx, Iterator* aIterator, JS::Handle<JS::Value> aValue, ErrorResult& aError) and which implements that algorithm.

I hope I didn't miss anything. Do let me know if there's anything that doesn't work or that makes things difficult!

I do see that we might need to change the GetNextIterationResult signature to pass you a JSContext* again? I seem to have missed that when doing my changes.

Thanks Peter for the detailed description. I am going to wait for your code to land, because I get iterator related failure in FileSystem classes now.

Peter, like you said GetNextIterationResult doesn't have a JSContext argument anymore, but I think we already use AutoJSAPI for something similar in other places.

There is also one failure that I think is related to the bindings implementation

Async iterator instances should have the correct list of properties

It seems like the length property of the return method is 0, but should should be 1.

(In reply to Tom Schuster [:evilpie] from comment #7)

Peter, like you said GetNextIterationResult doesn't have a JSContext argument anymore, but I think we already use AutoJSAPI for something similar in other places.

I'll take a look, if it's not ok then I can write a patch to pass in a JSContext.

There is also one failure that I think is related to the bindings implementation

Async iterator instances should have the correct list of properties

It seems like the length property of the return method is 0, but should should be 1.

Yeah, it's not clear to me that that's a valid test. AFAICT the value argument for return methods is supposed to be optional, and length shouldn't count optional arguments. I'll probably file a WebIDL spec bug to clarify this.

There seems to be some Worker specific issues on try: MOZ_CRASH(Found dangling CheckedUnsafePtr).

Hi Jens! It looks like you recently added these CheckedUnsafePtr to WorkerPrivate. Could you take a look at this? Afaik my patch isn't Worker specific in any way and reliably crashes on http://wpt.live/streams/readable-streams/async-iterator.any.sharedworker.html when clicking Re-run.

Hi! Eden took over the work of debugging those, see bug 1789399.

Backed out for causing failures at idlharness-shadowrealm.window.html.

Backout link: https://hg.mozilla.org/integration/autoland/rev/744aec96da7a1dafeed207fb693a4ae7e4b05305

Push with failures: https://treeherder.mozilla.org/jobs?repo=autoland&resultStatus=testfailed%2Cbusted%2Cexception%2Cretry%2Cusercancel&revision=fd13db3ea2b3bc2c96ef541680dd44f61aa2ce30&selectedTaskRun=O3EdqYLsRsK1AiGbYltaxw.0

Failure log: https://treeherder.mozilla.org/logviewer?job_id=398281672&repo=autoland&lineNumber=1938

Backed out for causing wpt failures at RuntimeService.cpp

Backout link: https://hg.mozilla.org/integration/autoland/rev/2961614653c8dd6eb9d90e18270572e5677d8e05

Push with failures: https://treeherder.mozilla.org/jobs?repo=autoland&selectedTaskRun=KAxW_8OZTCC81YMcHd3Z5Q.0&resultStatus=testfailed%2Cbusted%2Cexception%2Cretry%2Cusercancel&revision=2ce86a76d99dc03cb8f88799680824bb4fcb0fa0

Failure log: https://treeherder.mozilla.org/logviewer?job_id=398307265&repo=autoland&lineNumber=17887

Looks like we got a new Worker related failure now.

Assertion failure: !globalScopeAlive, at /builds/worker/checkouts/gecko/dom/workers/RuntimeService.cpp:2094

This is an expected issue. Bug 1752856 provides a workaround for fixing the issue that WorkerScope lives longer than expected.
With the patch, we null the PerformanceWorker's WorkerPrivate pointer of WorkerGlobalScope, to reduce the noise from CheckedUnsafePtr checking.
However, the patch does not resolve other context's JS object that holds the WorkerGlobalScope issue.
So we still get a MOZ_ASSERT complaint that WorkerGlobalScope is still alive when WorkerPrivate is going to be released.
But it doesn't mean that it must cause UAF or other memory issues. Since the WorkerGlobalScope might not be used anymore even if it holds by some objects.

Here I suggest we can add an expected crash result for builds that enable MOZ_ASSERT, such as debug build. And land the patch for shipping. Then we can have a follow-up bug to figure the root cause out for ReadableStream implementation. My guess is the iterator of ReadableStream is not correctly notified it should be unrooted in the JS world when WorkerPrivate is released. Even if we use GC/CC to release these objects in Worker, it does not work. Therefore, it makes WorkerGlobalScope keep alive.

Besides, for worker implementation, right now, we are refactoring the architecture to fix the worker's complicated lifecycle issues.
And one of them is bug 1350337, which is used to break the direct connection between the objects and the WorkerPrivate and make the connection be easily handled in WorkerGlobalScope only.

Backed out for causing leaks.

Backout link: https://hg.mozilla.org/integration/autoland/rev/b454dde4dfc5daedb167679a17e9f09d6ce2e3e9

Push with failures: https://treeherder.mozilla.org/jobs?repo=autoland&selectedTaskRun=D4XuFSHARwW7LgRPATB2VQ.0&resultStatus=testfailed%2Cbusted%2Cexception%2Cretry%2Cusercancel&revision=68ca2f48ea95576a330906ffa0f860c2d783f429

Failure log: https://treeherder.mozilla.org/logviewer?job_id=398795608&repo=autoland&lineNumber=19466

Looks like there is an actual leak somewhere, fixing that might also resolve the Worker issue. I suspect this might be an implementation issue of the Iterator code. As far as I can tell we correctly trace the IteratorData and IteratorReadRequest.

Leak fix is in bug 1806647.

Very cool, thanks Peter. I am optimistic that this also fixed the Worker issues.

https://hg.mozilla.org/mozilla-central/rev/19547ae8dc19

Tracking issue for MDN work https://github.com/mdn/content/issues/23678

I'm looking at this for the MDN documentation.

Reading a stream looks pretty straightforward; you get a stream and read it in chunks using for await (const chunk of mystream) {}:

let bytes = 0;
logChunks(mystream);
async function logChunks(readableXX) {
  for await (const chunk of readableXX) {
    bytes+=chunk.length;
    logConsumer( `Chunk: ${chunk}. Read ${bytes} characters.`);
  }
}

1. What is the "right way" to cancel a stream read during async iteration from a button press?

   If you're writing your own underlying source then you can add a listener that closes the stream on button click using controller.close().

   return new ReadableStream({
       start(controller) {
       button.addEventListener('click', () => {
         controller.close();
         });
       readRepeatedly().catch((e) => controller.error(e));
       ...
   

   But if you're using fetch() you have no control over the underlying source - you get a ReadableStream back from the response.

   • you can't call cancel on the stream because it is locked to the default reader, for which you don't have a handle (since you're working direct with the readablestream).
   • You could set a boolean in the event handler, then in the for loop you could use it to return, cancelling the operation. But this would have to await at least one more chunk, which could in theory take some time to arrive.

• What is the "general recommendation"?

2. Similarly, how are you supposed to handle errors from the source - e.g. a TypeError or a network error or something? I tried putting try/catch around the logChunks() above and various other places but I don't seem to be able to catch them. Using the reader I could have looked at the promised returned by closed, but again, I don't have that.

3. What is the recommended way to handle the case of a browser that does not support this feature? Is there a polyfill that we should point users to?

4. Tracking bugs seem to indicate this is not yet in Safari or Chrome. Do you happen to know if Deno/NodeJS support this, and if so, whether it is compatible to the streams spec?

I am probably not the best person to answer some of these questions. I suggest you ask the spec authors directly: https://github.com/whatwg/streams/issues.

1. I don't know how the "proper" way to cancel a stream during iteration. As you said there would be a delay until the next chunk is read.
2. Putting a try..catch around the for..of loop should work.
3. I think it would be possible to write a polyfill for this, but I don't know of any.
4. I don't know.