fix(fetch): re-calculate iterator after each iteration (nodejs#1706)

Author: KhafraDev

lib/fetch/formdata.js

@@ -206,8 +206,9 @@ class FormData {
     }
 
     return makeIterator(
-      makeIterable(this[kState], 'entries'),
-      'FormData'
+      () => this[kState].map(pair => [pair.name, pair.value]),
+      'FormData',
+      'key+value'
     )
   }
 
@@ -217,8 +218,9 @@ class FormData {
     }
 
     return makeIterator(
-      makeIterable(this[kState], 'keys'),
-      'FormData'
+      () => this[kState].map(pair => [pair.name, pair.value]),
+      'FormData',
+      'key'
     )
   }
 
@@ -228,8 +230,9 @@ class FormData {
     }
 
     return makeIterator(
-      makeIterable(this[kState], 'values'),
-      'FormData'
+      () => this[kState].map(pair => [pair.name, pair.value]),
+      'FormData',
+      'value'
     )
   }
 
@@ -304,18 +307,4 @@ function makeEntry (name, value, filename) {
   return { name, value }
 }
 
-function * makeIterable (entries, type) {
-  // The value pairs to iterate over are this’s entry list’s entries
-  // with the key being the name and the value being the value.
-  for (const { name, value } of entries) {
-    if (type === 'entries') {
-      yield [name, value]
-    } else if (type === 'values') {
-      yield value
-    } else {
-      yield name
-    }
-  }
-}
-
 module.exports = { FormData }

lib/fetch/headers.js

@@ -147,20 +147,10 @@ class HeadersList {
     return this[kHeadersMap].has(name)
   }
 
-  keys () {
-    return this[kHeadersMap].keys()
-  }
-
-  values () {
-    return this[kHeadersMap].values()
-  }
-
-  entries () {
-    return this[kHeadersMap].entries()
-  }
-
-  [Symbol.iterator] () {
-    return this[kHeadersMap][Symbol.iterator]()
+  * [Symbol.iterator] () {
+    for (const pair of this[kHeadersMap]) {
+      yield pair
+    }
   }
 }
 
@@ -413,23 +403,35 @@ class Headers {
       throw new TypeError('Illegal invocation')
     }
 
-    return makeIterator(this[kHeadersSortedMap].keys(), 'Headers')
+    return makeIterator(
+      () => [...this[kHeadersSortedMap].entries()],
+      'Headers',
+      'key'
+    )
   }
 
   values () {
     if (!(this instanceof Headers)) {
       throw new TypeError('Illegal invocation')
     }
 
-    return makeIterator(this[kHeadersSortedMap].values(), 'Headers')
+    return makeIterator(
+      () => [...this[kHeadersSortedMap].entries()],
+      'Headers',
+      'value'
+    )
   }
 
   entries () {
     if (!(this instanceof Headers)) {
       throw new TypeError('Illegal invocation')
     }
 
-    return makeIterator(this[kHeadersSortedMap].entries(), 'Headers')
+    return makeIterator(
+      () => [...this[kHeadersSortedMap].entries()],
+      'Headers',
+      'key+value'
+    )
   }
 
   /**

lib/fetch/util.js

@@ -684,17 +684,61 @@ function serializeJavascriptValueToJSONString (value) {
 // https://tc39.es/ecma262/#sec-%25iteratorprototype%25-object
 const esIteratorPrototype = Object.getPrototypeOf(Object.getPrototypeOf([][Symbol.iterator]()))
 
-// https://webidl.spec.whatwg.org/#dfn-iterator-prototype-object
-function makeIterator (iterator, name) {
+/**
+ * @see https://webidl.spec.whatwg.org/#dfn-iterator-prototype-object
+ * @param {() => unknown[]} iterator
+ * @param {string} name name of the instance
+ * @param {'key'|'value'|'key+value'} kind
+ */
+function makeIterator (iterator, name, kind) {
+  const object = {
+    index: 0,
+    kind,
+    target: iterator
+  }
+
   const i = {
     next () {
+      // 1. Let interface be the interface for which the iterator prototype object exists.
+
+      // 2. Let thisValue be the this value.
+
+      // 3. Let object be ? ToObject(thisValue).
+
+      // 4. If object is a platform object, then perform a security
+      //    check, passing:
+
+      // 5. If object is not a default iterator object for interface,
+      //    then throw a TypeError.
       if (Object.getPrototypeOf(this) !== i) {
         throw new TypeError(
           `'next' called on an object that does not implement interface ${name} Iterator.`
         )
       }
 
-      return iterator.next()
+      // 6. Let index be object’s index.
+      // 7. Let kind be object’s kind.
+      // 8. Let values be object’s target's value pairs to iterate over.
+      const { index, kind, target } = object
+      const values = target()
+
+      // 9. Let len be the length of values.
+      const len = values.length
+
+      // 10. If index is greater than or equal to len, then return
+      //     CreateIterResultObject(undefined, true).
+      if (index >= len) {
+        return { value: undefined, done: true }
+      }
+
+      // 11. Let pair be the entry in values at index index.
+      const pair = values[index]
+
+      // 12. Set object’s index to index + 1.
+      object.index = index + 1
+
+      // 13. Return the iterator result for pair and kind.
+      return iteratorResult(pair, kind)
     },
     // The class string of an iterator prototype object for a given interface is the
     // result of concatenating the identifier of the interface and the string " Iterator".
@@ -708,6 +752,48 @@ function makeIterator (iterator, name) {
   return Object.setPrototypeOf({}, i)
 }
 
+// https://webidl.spec.whatwg.org/#iterator-result
+function iteratorResult (pair, kind) {
+  let result
+
+  // 1. Let result be a value determined by the value of kind:
+  switch (kind) {
+    case 'key': {
+      // 1. Let idlKey be pair’s key.
+      // 2. Let key be the result of converting idlKey to an
+      //    ECMAScript value.
+      // 3. result is key.
+      result = pair[0]
+      break
+    }
+    case 'value': {
+      // 1. Let idlValue be pair’s value.
+      // 2. Let value be the result of converting idlValue to
+      //    an ECMAScript value.
+      // 3. result is value.
+      result = pair[1]
+      break
+    }
+    case 'key+value': {
+      // 1. Let idlKey be pair’s key.
+      // 2. Let idlValue be pair’s value.
+      // 3. Let key be the result of converting idlKey to an
+      //    ECMAScript value.
+      // 4. Let value be the result of converting idlValue to
+      //    an ECMAScript value.
+      // 5. Let array be ! ArrayCreate(2).
+      // 6. Call ! CreateDataProperty(array, "0", key).
+      // 7. Call ! CreateDataProperty(array, "1", value).
+      // 8. result is array.
+      result = pair
+      break
+    }
+  }
+
+  // 2. Return CreateIterResultObject(result, false).
+  return { value: result, done: false }
+}
+
 /**
  * @see https://fetch.spec.whatwg.org/#body-fully-read
  */

test/fetch/headers.js

@@ -329,7 +329,7 @@ tap.test('Headers forEach', t => {
 })
 
 tap.test('Headers as Iterable', t => {
-  t.plan(8)
+  t.plan(7)
 
   t.test('should freeze values while iterating', t => {
     t.plan(1)
@@ -338,8 +338,8 @@ tap.test('Headers as Iterable', t => {
       ['bar', '456']
     ]
     const expected = [
-      ['x-bar', '456'],
-      ['x-foo', '123']
+      ['foo', '123'],
+      ['x-x-bar', '456']
     ]
     const headers = new Headers(init)
     for (const [key, val] of headers) {
@@ -349,28 +349,6 @@ tap.test('Headers as Iterable', t => {
     t.strictSame([...headers], expected)
   })
 
-  t.test('prevent infinite, continuous iteration', t => {
-    t.plan(2)
-
-    const headers = new Headers({
-      z: 1,
-      y: 2,
-      x: 3
-    })
-
-    const order = []
-    for (const [key] of headers) {
-      order.push(key)
-      headers.append(key + key, 1)
-    }
-
-    t.strictSame(order, ['x', 'y', 'z'])
-    t.strictSame(
-      [...headers.keys()],
-      ['x', 'xx', 'y', 'yy', 'z', 'zz']
-    )
-  })
-
   t.test('returns combined and sorted entries using .forEach()', t => {
     t.plan(8)
     const init = [

test/wpt/tests/fetch/api/headers/headers-basic.any.js

@@ -218,3 +218,58 @@ test(function() {
   });
   assert_true(reference.next().done);
 }, "Check forEach method");
+
+test(() => {
+  const headers = new Headers({"foo": "2", "baz": "1", "BAR": "0"});
+  const actualKeys = [];
+  const actualValues = [];
+  for (const [header, value] of headers) {
+    actualKeys.push(header);
+    actualValues.push(value);
+    headers.delete("foo");
+  }
+  assert_array_equals(actualKeys, ["bar", "baz"]);
+  assert_array_equals(actualValues, ["0", "1"]);
+}, "Iteration skips elements removed while iterating");
+
+test(() => {
+  const headers = new Headers({"foo": "2", "baz": "1", "BAR": "0", "quux": "3"});
+  const actualKeys = [];
+  const actualValues = [];
+  for (const [header, value] of headers) {
+    actualKeys.push(header);
+    actualValues.push(value);
+    if (header === "baz")
+      headers.delete("bar");
+  }
+  assert_array_equals(actualKeys, ["bar", "baz", "quux"]);
+  assert_array_equals(actualValues, ["0", "1", "3"]);
+}, "Removing elements already iterated over causes an element to be skipped during iteration");
+
+test(() => {
+  const headers = new Headers({"foo": "2", "baz": "1", "BAR": "0", "quux": "3"});
+  const actualKeys = [];
+  const actualValues = [];
+  for (const [header, value] of headers) {
+    actualKeys.push(header);
+    actualValues.push(value);
+    if (header === "baz")
+      headers.append("X-yZ", "4");
+  }
+  assert_array_equals(actualKeys, ["bar", "baz", "foo", "quux", "x-yz"]);
+  assert_array_equals(actualValues, ["0", "1", "2", "3", "4"]);
+}, "Appending a value pair during iteration causes it to be reached during iteration");
+
+test(() => {
+  const headers = new Headers({"foo": "2", "baz": "1", "BAR": "0", "quux": "3"});
+  const actualKeys = [];
+  const actualValues = [];
+  for (const [header, value] of headers) {
+    actualKeys.push(header);
+    actualValues.push(value);
+    if (header === "baz")
+      headers.append("abc", "-1");
+  }
+  assert_array_equals(actualKeys, ["bar", "baz", "baz", "foo", "quux"]);
+  assert_array_equals(actualValues, ["0", "1", "1", "2", "3"]);
+}, "Prepending a value pair before the current element position causes it to be skipped during iteration and adds the current element a second time");