Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fetch, Web IDL: Test that Headers iteration happens on live value pairs. #36455

Merged

Conversation

rakuco
Copy link
Member

@rakuco rakuco commented Oct 14, 2022

Add some tests to verify the pair iterator behavior specified in Web IDL, namely that the value pairs to be iterated over are retrieved on every invocation of the iterator prototype object's next() function.

In practice, this means that every iteration step needs to be performed on a Headers' sorted-and-combined headers list that should not be cached. Gecko seems to be the only engine doing this right, as both WebKit and Blink appear to carry out the entire iteration on a cached copy of the header list.

The tests were based on the ones in
webidl/ecmascript-binding/iterator-invalidation-foreach.html.

Add some tests to verify the pair iterator behavior specified in Web IDL,
namely that the value pairs to be iterated over are retrieved on every
invocation of the iterator prototype object's `next()` function.

In practice, this means that every iteration step needs to be performed on a
Headers' sorted-and-combined headers list that should not be cached. Gecko
seems to be the only engine doing this right, as both WebKit and Blink
appear to carry out the entire iteration on a cached copy of the header
list.

The tests were based on the ones in
webidl/ecmascript-binding/iterator-invalidation-foreach.html.
@rakuco
Copy link
Member Author

rakuco commented Oct 14, 2022

@annevk I'm tentatively assigning to you because you're involved with both Fetch and Web IDL.

I wasn't sure if it made sense to add these new tests to fetch/ or to webidl/ecmascript-binding to be honest; I'm interested in this from a Web IDL point of view while reviewing some changes to the Blink iterator code, but adding it to fetch/ increases the chances of the implementations being fixed by the right people, I guess?

Copy link
Member

@annevk annevk left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I could have sworn there was test coverage for this, but perhaps it was an equivalent data structure...

This seems reasonable.

Final optional nit: maybe use short function syntax, i.e., () => { ... }.

fetch/api/headers/headers-basic.any.js Outdated Show resolved Hide resolved
fetch/api/headers/headers-basic.any.js Outdated Show resolved Hide resolved
fetch/api/headers/headers-basic.any.js Outdated Show resolved Hide resolved
* Use `() => {}` form for inner test functions.
* Stop breaking declaration of `headers` into multiple lines.
* Use `const` in the for..of loop.
* Improve description of the last test.
@annevk annevk merged commit a31d3ba into web-platform-tests:master Oct 14, 2022
@rakuco rakuco deleted the fetch-check-value-pairs-freshness branch October 14, 2022 14:23
Copy link

@stonigrl stonigrl left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍

rakuco added a commit to rakuco/wpt that referenced this pull request Oct 18, 2022
This is similar to web-platform-tests#36455 and web-platform-tests#20445: we want to make sure that adding and
deleting elements during iteration is reflected in FormData's value pairs to
iterate over (i.e. iteration does not happen on a cached version).
annevk pushed a commit that referenced this pull request Oct 19, 2022
This is similar to #36455 and #20445: we want to make sure that adding and deleting elements during iteration is reflected in FormData's value pairs to iterate over (i.e., iteration does not happen on a cached version).
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants