Borrow panics in Map

[Razican]

It turns out that we have a couple of panics when borrowing maps that are already borrowed.

When running this ECMAScript test, we get the following panic:

thread 'main' panicked at 'Object already borrowed: BorrowMutError', boa/src/builtins/map/mod.rs:221:39

This happens because when we call map.delete(), the map is already borrowed mutably by the forEach function, and Rust doesn't allow two mutable borrows at the same time.

The first borrow happens here, and the second one happens when calling context.call(), that ends up calling delete, and finally borrowing the map again here. The backtrace is this one:

   0: rust_begin_unwind
             at /rustc/e1884a8e3c3e813aada8254edfa120e85bf5ffca/library/std/src/panicking.rs:495:5
   1: core::panicking::panic_fmt
             at /rustc/e1884a8e3c3e813aada8254edfa120e85bf5ffca/library/core/src/panicking.rs:92:14
   2: core::option::expect_none_failed
             at /rustc/e1884a8e3c3e813aada8254edfa120e85bf5ffca/library/core/src/option.rs:1268:5
   3: boa::builtins::map::Map::delete
   4: boa::object::gcobject::GcObject::call
   5: <boa::syntax::ast::node::call::Call as boa::exec::Executable>::run
   6: <boa::syntax::ast::node::block::Block as boa::exec::Executable>::run
   7: <boa::syntax::ast::node::Node as boa::exec::Executable>::run
   8: <boa::syntax::ast::node::statement_list::StatementList as boa::exec::Executable>::run
   9: boa::object::gcobject::GcObject::call
  10: boa::builtins::map::Map::for_each
  11: boa::object::gcobject::GcObject::call
  12: <boa::syntax::ast::node::call::Call as boa::exec::Executable>::run
  13: <boa::syntax::ast::node::statement_list::StatementList as boa::exec::Executable>::run
  14: boa::context::Context::eval
  15: boa_tester::exec::<impl boa_tester::Test>::run_once
  16: boa_tester::exec::<impl boa_tester::Test>::run
  17: boa_tester::main

This seems tricky to fix. The forEach call should be able to change anything in the map, but our guarantees make this a bit difficult to make it happen.

This also happens with this test and with this one, but in line 189, in the set() function.

Backtrace here:

   0: rust_begin_unwind
             at /rustc/e1884a8e3c3e813aada8254edfa120e85bf5ffca/library/std/src/panicking.rs:495:5
   1: core::panicking::panic_fmt
             at /rustc/e1884a8e3c3e813aada8254edfa120e85bf5ffca/library/core/src/panicking.rs:92:14
   2: core::option::expect_none_failed
             at /rustc/e1884a8e3c3e813aada8254edfa120e85bf5ffca/library/core/src/option.rs:1268:5
   3: boa::builtins::map::Map::set
   4: boa::object::gcobject::GcObject::call
   5: <boa::syntax::ast::node::call::Call as boa::exec::Executable>::run
   6: <boa::syntax::ast::node::block::Block as boa::exec::Executable>::run
   7: <boa::syntax::ast::node::Node as boa::exec::Executable>::run
   8: <boa::syntax::ast::node::statement_list::StatementList as boa::exec::Executable>::run
   9: boa::object::gcobject::GcObject::call
  10: boa::builtins::map::Map::for_each
  11: boa::object::gcobject::GcObject::call
  12: <boa::syntax::ast::node::call::Call as boa::exec::Executable>::run
  13: <boa::syntax::ast::node::statement_list::StatementList as boa::exec::Executable>::run
  14: boa::context::Context::eval
  15: boa_tester::exec::<impl boa_tester::Test>::run_once
  16: boa_tester::exec::<impl boa_tester::Test>::run
  17: boa_tester::main

You can run these tests by running these commands, for the first and second case, respectively:

RUST_BACKTRACE=1 cargo run --release --bin boa_tester -- run -v -s test/built-ins/Map/prototype/forEach/deleted-values-during-foreach.js

cargo run --release --bin boa_tester -- run -v -s test/built-ins/Map/prototype/forEach/iterates-values-added-after-foreach-begins.js

[joshwd36]

I've had a go at this, and I've managed to fix the panics by going to an index based iteration. However, this still fails on iterates-values-deleted-then-readded, as when the element is removed the indexes are shifted back so the second element never gets visited. One solution could be to use the empty marker elements used in the spec, though this would effectively create a memory leak when elements are removed. I'd be open to thoughts about an approach to this.

[joshwd36]

Should there be another issue for the iterates-values-deleted-then-readded problem?

[Razican]

Should there be another issue for the iterates-values-deleted-then-readded problem?

Yes, please, could you create the issue explaining what happens and why?