Auto merge of #24866 - Manishearth:iterator-invalidation, r=jdm

Fix iterator invalidation in our forEach implementation. Currently we iterate over iterables in forEach based on the length they report when we start iterating, however the inner callback is able to change this. ```js let params = new URLSearchParams("foo=bar&baz=qux"); params.forEach((p) => { console.log(p); params.delete("baz"); }) ``` This causes us to panic [here](https://github.com/servo/servo/blob/f1aa5d8dbdc32f6f474b09234558652b9bead686/components/script/dom/bindings/codegen/CodegenRust.py#L7412) over an attempt to access out of bounds. Relevant spec: https://heycam.github.io/webidl/#es-forEach r? @jdm
servo · Nov 26, 2019 · 74bf0ce · 74bf0ce
2 parents f1aa5d8 + 38a6667
commit 74bf0ce
Show file tree

Hide file tree

Showing 3 changed files with 62 additions and 1 deletion.
diff --git a/components/script/dom/bindings/codegen/CodegenRust.py b/components/script/dom/bindings/codegen/CodegenRust.py
@@ -7408,7 +7408,16 @@ def __init__(self, descriptor, iterable, methodName):
                 rooted!(in(*cx) let mut call_arg2 = UndefinedValue());
                 let mut call_args = vec![UndefinedValue(), UndefinedValue(), ObjectValue(*_obj)];
                 rooted!(in(*cx) let mut ignoredReturnVal = UndefinedValue());
-                for i in 0..(*this).get_iterable_length() {
+
+                // This has to be a while loop since get_iterable_length() may change during
+                // the callback, and we need to avoid iterator invalidation.
+                //
+                // It is possible for this to loop infinitely, but that matches the spec
+                // and other browsers.
+                //
+                // https://heycam.github.io/webidl/#es-forEach
+                let mut i = 0;
+                while i < (*this).get_iterable_length() {
                   (*this).get_value_at_index(i).to_jsval(*cx, call_arg1.handle_mut());
                   (*this).get_key_at_index(i).to_jsval(*cx, call_arg2.handle_mut());
                   call_args[0] = call_arg1.handle().get();
@@ -7418,6 +7427,8 @@ def __init__(self, descriptor, iterable, methodName):
                            ignoredReturnVal.handle_mut()) {
                     return false;
                   }
+
+                  i += 1;
                 }
 
                 let result = ();

diff --git a/tests/wpt/metadata/MANIFEST.json b/tests/wpt/metadata/MANIFEST.json
@@ -308306,6 +308306,12 @@
      {}
     ]
    ],
+   "WebIDL/ecmascript-binding/iterator-invalidation-foreach.html": [
+    [
+     "WebIDL/ecmascript-binding/iterator-invalidation-foreach.html",
+     {}
+    ]
+   ],
    "WebIDL/ecmascript-binding/iterator-prototype-object.html": [
     [
      "WebIDL/ecmascript-binding/iterator-prototype-object.html",
@@ -466654,6 +466660,10 @@
    "03ada7aa0d4d43811652fc679a00a41b9653013d",
    "testharness"
   ],
+  "WebIDL/ecmascript-binding/iterator-invalidation-foreach.html": [
+   "d6498c3e388e0c637830fa080cca78b0ab0e5305",
+   "testharness"
+  ],
   "WebIDL/ecmascript-binding/iterator-prototype-object.html": [
    "5a935fa20135d88a7268b872b68ab7fe548ab5c7",
    "testharness"

diff --git a/tests/wpt/web-platform-tests/WebIDL/ecmascript-binding/iterator-invalidation-foreach.html b/tests/wpt/web-platform-tests/WebIDL/ecmascript-binding/iterator-invalidation-foreach.html
@@ -0,0 +1,40 @@
+<!doctype html>
+<meta charset="utf-8">
+<title>Behavior of iterators when modified within foreach</title>
+<script src="/resources/testharness.js"></script>
+<script src="/resources/testharnessreport.js"></script>
+<link rel="help" href="https://heycam.github.io/webidl/#es-forEach">
+<link rel="author" title="Manish Goregaokar" href="mailto:manishsmail@gmail.com">
+<script>
+test(function() {
+    let params = new URLSearchParams("a=1&b=2&c=3");
+    let arr = [];
+    params.forEach((p) => {
+        arr.push(p);
+        params.delete("b");
+    })
+    assert_array_equals(arr, ["1", "3"]);
+}, "forEach will not iterate over elements removed during iteration");
+test(function() {
+    let params = new URLSearchParams("a=1&b=2&c=3&d=4");
+    let arr = [];
+    params.forEach((p) => {
+        arr.push(p);
+        if (p == "2") {
+            params.delete("a");
+        }
+    })
+    assert_array_equals(arr, ["1", "2", "4"]);
+}, "Removing elements already iterated over during forEach will cause iterator to skip an element");
+test(function() {
+    let params = new URLSearchParams("a=1&b=2&c=3&d=4");
+    let arr = [];
+    params.forEach((p) => {
+        arr.push(p);
+        if (p == "2") {
+            params.append("e", "5");
+        }
+    })
+    assert_array_equals(arr, ["1", "2", "3", "4", "5"]);
+}, "Elements added during iteration with forEach will be reached");
+</script>