DOM: Fix Observable iterator return() method handling
#52155
Merged
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
In WICG/observable#208 it was discovered that the Chromium Observable implementation has a bug where when you unsubscribe from a sync iterator and `ScriptIterator::CloseSync()` tries to find the iterator's `return()` method, if the method doesn't exist, an error is thrown. This deviates from the spec, where TC39 (delegated to by the Observable specification) permits a null or undefined `return()` method without throwing. This CL fixes that bug in `ScriptIterator::CloseSync()` and adds a test. From reading the code, the same issue should happen for async iterators too, but I cannot reproduce it with the following code: ```js window.onunhandledrejection = e => console.log('UNHANDLED!!!'); window.onerror = e => console.log('My stuff'); const asyncIterator = { [Symbol.asyncIterator]() { return { i: 10, next () { return {value: this.i, done: this.i === 2 ? true : false}; }, return: 12, } } }; const source = Observable.from(asyncIterator); const value = await source.first(); console.log(value); ``` ...so I will consider investigating in a follow-up. Bug: 40282760 Change-Id: Ie5f98c97a0602bf702eb7d2bfb082014381e50cc Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/6480575 Commit-Queue: Dominic Farolino <dom@chromium.org> Reviewed-by: Mason Freed <masonf@chromium.org> Cr-Commit-Position: refs/heads/main@{#1451144}
chromium-wpt-export-bot
force-pushed
the
chromium-export-cl-6480575
branch
from
200fbd8 to
41e3bf7
Compare
wpt-pr-bot
approved these changes
Apr 24, 2025
Collaborator
wpt-pr-bot
left a comment
wpt-pr-bot
left a comment
There was a problem hiding this comment.
The review process for this patch is being conducted in the Chromium project.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
In WICG/observable#208 it was discovered that
the Chromium Observable implementation has a bug where when you
unsubscribe from a sync iterator and
ScriptIterator::CloseSync()triesto find the iterator's
return()method, if the method doesn't exist,an error is thrown.
This deviates from the spec, where TC39 (delegated to by the Observable
specification) permits a null or undefined
return()method withoutthrowing.
This CL fixes that bug in
ScriptIterator::CloseSync()and adds a test.From reading the code, the same issue should happen for async iterators
too, but I cannot reproduce it with the following code:
...so I will consider investigating in a follow-up.
Bug: 40282760
Change-Id: Ie5f98c97a0602bf702eb7d2bfb082014381e50cc
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/6480575
Commit-Queue: Dominic Farolino <dom@chromium.org>
Reviewed-by: Mason Freed <masonf@chromium.org>
Cr-Commit-Position: refs/heads/main@{#1451144}