Closed Bug 1734244 Opened 2 years ago Closed 6 months ago

Implement async iteration of ReadableStream


(Core :: DOM: Streams, task, P3)




110 Branch
Tracking Status
firefox110 --- fixed


(Reporter: mgaudet, Assigned: evilpie)


(Blocks 3 open bugs)


(Keywords: dev-doc-complete)


(2 files)

No description provided.
Blocks: dom-streams
Blocks: 1730584
No longer blocks: dom-streams

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.

Severity: -- → S3
Priority: -- → P3
Blocks: 1751802
Depends on: 1577383
Depends on: 1781730
Depends on: 1782399, 1782400
Depends on: 1788969

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 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.

Flags: needinfo?(evilpies)

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.

Assignee: nobody → evilpies
Flags: needinfo?(evilpies)
Attachment #9287855 - Attachment description: WIP: Bug 1734244 → Bug 1734244 - Implement async iteration of ReadableStream. r?mgaudet!,peterv!

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.

Depends on: 1794127
Depends on: 1796804
Depends on: 1796832

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 when clicking Re-run.

Flags: needinfo?(jstutte)
Attached file gdb backtrace

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

Flags: needinfo?(jstutte) → needinfo?(echuang)
Pushed by
Implement async iteration of ReadableStream. r=mgaudet,peterv
Pushed by
Implement async iteration of ReadableStream. r=mgaudet,peterv
Flags: needinfo?(evilpies)
Flags: needinfo?(echuang)

Looks like we got a new Worker related failure now.

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

Flags: needinfo?(evilpies) → needinfo?(echuang)

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.

Flags: needinfo?(echuang)
Blocks: 1804241
Pushed by
Implement async iteration of ReadableStream. r=mgaudet,peterv
Flags: needinfo?(evilpies)

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.

Flags: needinfo?(peterv)
Depends on: 1806647

Leak fix is in bug 1806647.

Flags: needinfo?(peterv)

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

Pushed by
Implement async iteration of ReadableStream. r=mgaudet,peterv
Closed: 6 months ago
Resolution: --- → FIXED
Target Milestone: --- → 110 Branch
Keywords: dev-doc-needed

Tracking issue for MDN work

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;
async function logChunks(readableXX) {
  for await (const chunk of readableXX) {
    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', () => {
        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"?
  1. 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.

  2. 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?

  3. 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?

Flags: needinfo?(evilpies)

I am probably not the best person to answer some of these questions. I suggest you ask the spec authors directly:

  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.
Flags: needinfo?(evilpies)
Regressions: 1818630
You need to log in before you can comment on or make changes to this bug.