From 2e27a18887ac163757967ff3a542e1aac5ae238c Mon Sep 17 00:00:00 2001 From: Yang Guo Date: Tue, 20 Nov 2018 08:59:38 +0100 Subject: [PATCH] deps: cherry-pick 88f8fe1 from upstream V8 Original commit message: Fix collection iterator preview with deleted entries We used to assume that we know the remaining entries returned by the iterator based on the current index. However, that is not accurate, since entries skipped by the current index could be deleted. In the new approach, we allocate conservatively and shrink the result. R=neis@chromium.org Bug: v8:8433 Change-Id: I38a3004dc3af292daabb454bb76f38d65ef437e8 Reviewed-on: https://chromium-review.googlesource.com/c/1325966 Commit-Queue: Yang Guo Reviewed-by: Georg Neis Cr-Commit-Position: refs/heads/master@{#57360} Refs: https://github.com/v8/v8/commit/88f8fe19a863c6392bd296faf86c06eff2a41bc1 --- common.gypi | 2 +- deps/v8/src/api.cc | 52 ++++---- deps/v8/test/cctest/test-api.cc | 214 ++++++++++++++++++++++++++++++++ 3 files changed, 242 insertions(+), 26 deletions(-) diff --git a/common.gypi b/common.gypi index a1ecdc6d6f8294..363eefb9a6cff8 100644 --- a/common.gypi +++ b/common.gypi @@ -31,7 +31,7 @@ # Reset this number to 0 on major V8 upgrades. # Increment by one for each non-official patch applied to deps/v8. - 'v8_embedder_string': '-node.11', + 'v8_embedder_string': '-node.12', # Enable disassembler for `--print-code` v8 options 'v8_enable_disassembler': 1, diff --git a/deps/v8/src/api.cc b/deps/v8/src/api.cc index c1afe8d93b9d0d..091b40cf2fdbe9 100644 --- a/deps/v8/src/api.cc +++ b/deps/v8/src/api.cc @@ -7103,30 +7103,30 @@ i::Handle MapAsArray(i::Isolate* isolate, i::Object* table_obj, i::Factory* factory = isolate->factory(); i::Handle table(i::OrderedHashMap::cast(table_obj), isolate); - if (offset >= table->NumberOfElements()) return factory->NewJSArray(0); - int length = (table->NumberOfElements() - offset) * - (kind == MapAsArrayKind::kEntries ? 2 : 1); - i::Handle result = factory->NewFixedArray(length); + const bool collect_keys = + kind == MapAsArrayKind::kEntries || kind == MapAsArrayKind::kKeys; + const bool collect_values = + kind == MapAsArrayKind::kEntries || kind == MapAsArrayKind::kValues; + int capacity = table->UsedCapacity(); + int max_length = + (capacity - offset) * ((collect_keys && collect_values) ? 2 : 1); + i::Handle result = factory->NewFixedArray(max_length); int result_index = 0; { i::DisallowHeapAllocation no_gc; - int capacity = table->UsedCapacity(); i::Oddball* the_hole = i::ReadOnlyRoots(isolate).the_hole_value(); - for (int i = 0; i < capacity; ++i) { + for (int i = offset; i < capacity; ++i) { i::Object* key = table->KeyAt(i); if (key == the_hole) continue; - if (offset-- > 0) continue; - if (kind == MapAsArrayKind::kEntries || kind == MapAsArrayKind::kKeys) { - result->set(result_index++, key); - } - if (kind == MapAsArrayKind::kEntries || kind == MapAsArrayKind::kValues) { - result->set(result_index++, table->ValueAt(i)); - } + if (collect_keys) result->set(result_index++, key); + if (collect_values) result->set(result_index++, table->ValueAt(i)); } } - DCHECK_EQ(result_index, result->length()); - DCHECK_EQ(result_index, length); - return factory->NewJSArrayWithElements(result, i::PACKED_ELEMENTS, length); + DCHECK_GE(max_length, result_index); + if (result_index == 0) return factory->NewJSArray(0); + result->Shrink(isolate, result_index); + return factory->NewJSArrayWithElements(result, i::PACKED_ELEMENTS, + result_index); } } // namespace @@ -7211,24 +7211,26 @@ i::Handle SetAsArray(i::Isolate* isolate, i::Object* table_obj, i::Factory* factory = isolate->factory(); i::Handle table(i::OrderedHashSet::cast(table_obj), isolate); - int length = table->NumberOfElements() - offset; - if (length <= 0) return factory->NewJSArray(0); - i::Handle result = factory->NewFixedArray(length); + // Elements skipped by |offset| may already be deleted. + int capacity = table->UsedCapacity(); + int max_length = capacity - offset; + if (max_length == 0) return factory->NewJSArray(0); + i::Handle result = factory->NewFixedArray(max_length); int result_index = 0; { i::DisallowHeapAllocation no_gc; - int capacity = table->UsedCapacity(); i::Oddball* the_hole = i::ReadOnlyRoots(isolate).the_hole_value(); - for (int i = 0; i < capacity; ++i) { + for (int i = offset; i < capacity; ++i) { i::Object* key = table->KeyAt(i); if (key == the_hole) continue; - if (offset-- > 0) continue; result->set(result_index++, key); } } - DCHECK_EQ(result_index, result->length()); - DCHECK_EQ(result_index, length); - return factory->NewJSArrayWithElements(result, i::PACKED_ELEMENTS, length); + DCHECK_GE(max_length, result_index); + if (result_index == 0) return factory->NewJSArray(0); + result->Shrink(isolate, result_index); + return factory->NewJSArrayWithElements(result, i::PACKED_ELEMENTS, + result_index); } } // namespace diff --git a/deps/v8/test/cctest/test-api.cc b/deps/v8/test/cctest/test-api.cc index f7365c8f31a8c8..1b74ecfd70c655 100644 --- a/deps/v8/test/cctest/test-api.cc +++ b/deps/v8/test/cctest/test-api.cc @@ -28777,3 +28777,217 @@ TEST(TestSetWasmThreadsEnabledCallback) { i::FLAG_experimental_wasm_threads = false; CHECK(i_isolate->AreWasmThreadsEnabled(i_context)); } + +TEST(PreviewSetIteratorEntriesWithDeleted) { + LocalContext env; + v8::HandleScope handle_scope(env->GetIsolate()); + v8::Local context = env.local(); + + { + // Create set, delete entry, create iterator, preview. + v8::Local iterator = + CompileRun("var set = new Set([1,2,3]); set.delete(1); set.keys()") + ->ToObject(context) + .ToLocalChecked(); + bool is_key; + v8::Local entries = + iterator->PreviewEntries(&is_key).ToLocalChecked(); + CHECK(!is_key); + CHECK_EQ(2, entries->Length()); + CHECK_EQ(2, entries->Get(context, 0) + .ToLocalChecked() + ->Int32Value(context) + .FromJust()); + CHECK_EQ(3, entries->Get(context, 1) + .ToLocalChecked() + ->Int32Value(context) + .FromJust()); + } + { + // Create set, create iterator, delete entry, preview. + v8::Local iterator = + CompileRun("var set = new Set([1,2,3]); set.keys()") + ->ToObject(context) + .ToLocalChecked(); + CompileRun("set.delete(1);"); + bool is_key; + v8::Local entries = + iterator->PreviewEntries(&is_key).ToLocalChecked(); + CHECK(!is_key); + CHECK_EQ(2, entries->Length()); + CHECK_EQ(2, entries->Get(context, 0) + .ToLocalChecked() + ->Int32Value(context) + .FromJust()); + CHECK_EQ(3, entries->Get(context, 1) + .ToLocalChecked() + ->Int32Value(context) + .FromJust()); + } + { + // Create set, create iterator, delete entry, iterate, preview. + v8::Local iterator = + CompileRun("var set = new Set([1,2,3]); var it = set.keys(); it") + ->ToObject(context) + .ToLocalChecked(); + CompileRun("set.delete(1); it.next();"); + bool is_key; + v8::Local entries = + iterator->PreviewEntries(&is_key).ToLocalChecked(); + CHECK(!is_key); + CHECK_EQ(1, entries->Length()); + CHECK_EQ(3, entries->Get(context, 0) + .ToLocalChecked() + ->Int32Value(context) + .FromJust()); + } + { + // Create set, create iterator, delete entry, iterate until empty, preview. + v8::Local iterator = + CompileRun("var set = new Set([1,2,3]); var it = set.keys(); it") + ->ToObject(context) + .ToLocalChecked(); + CompileRun("set.delete(1); it.next(); it.next();"); + bool is_key; + v8::Local entries = + iterator->PreviewEntries(&is_key).ToLocalChecked(); + CHECK(!is_key); + CHECK_EQ(0, entries->Length()); + } + { + // Create set, create iterator, delete entry, iterate, trigger rehash, + // preview. + v8::Local iterator = + CompileRun("var set = new Set([1,2,3]); var it = set.keys(); it") + ->ToObject(context) + .ToLocalChecked(); + CompileRun("set.delete(1); it.next();"); + CompileRun("for (var i = 4; i < 20; i++) set.add(i);"); + bool is_key; + v8::Local entries = + iterator->PreviewEntries(&is_key).ToLocalChecked(); + CHECK(!is_key); + CHECK_EQ(17, entries->Length()); + for (uint32_t i = 0; i < 17; i++) { + CHECK_EQ(i + 3, entries->Get(context, i) + .ToLocalChecked() + ->Int32Value(context) + .FromJust()); + } + } +} + +TEST(PreviewMapIteratorEntriesWithDeleted) { + LocalContext env; + v8::HandleScope handle_scope(env->GetIsolate()); + v8::Local context = env.local(); + + { + // Create map, delete entry, create iterator, preview. + v8::Local iterator = CompileRun( + "var map = new Map();" + "var key = {}; map.set(key, 1);" + "map.set({}, 2); map.set({}, 3);" + "map.delete(key);" + "map.values()") + ->ToObject(context) + .ToLocalChecked(); + bool is_key; + v8::Local entries = + iterator->PreviewEntries(&is_key).ToLocalChecked(); + CHECK(!is_key); + CHECK_EQ(2, entries->Length()); + CHECK_EQ(2, entries->Get(context, 0) + .ToLocalChecked() + ->Int32Value(context) + .FromJust()); + CHECK_EQ(3, entries->Get(context, 1) + .ToLocalChecked() + ->Int32Value(context) + .FromJust()); + } + { + // Create map, create iterator, delete entry, preview. + v8::Local iterator = CompileRun( + "var map = new Map();" + "var key = {}; map.set(key, 1);" + "map.set({}, 2); map.set({}, 3);" + "map.values()") + ->ToObject(context) + .ToLocalChecked(); + CompileRun("map.delete(key);"); + bool is_key; + v8::Local entries = + iterator->PreviewEntries(&is_key).ToLocalChecked(); + CHECK(!is_key); + CHECK_EQ(2, entries->Length()); + CHECK_EQ(2, entries->Get(context, 0) + .ToLocalChecked() + ->Int32Value(context) + .FromJust()); + CHECK_EQ(3, entries->Get(context, 1) + .ToLocalChecked() + ->Int32Value(context) + .FromJust()); + } + { + // Create map, create iterator, delete entry, iterate, preview. + v8::Local iterator = CompileRun( + "var map = new Map();" + "var key = {}; map.set(key, 1);" + "map.set({}, 2); map.set({}, 3);" + "var it = map.values(); it") + ->ToObject(context) + .ToLocalChecked(); + CompileRun("map.delete(key); it.next();"); + bool is_key; + v8::Local entries = + iterator->PreviewEntries(&is_key).ToLocalChecked(); + CHECK(!is_key); + CHECK_EQ(1, entries->Length()); + CHECK_EQ(3, entries->Get(context, 0) + .ToLocalChecked() + ->Int32Value(context) + .FromJust()); + } + { + // Create map, create iterator, delete entry, iterate until empty, preview. + v8::Local iterator = CompileRun( + "var map = new Map();" + "var key = {}; map.set(key, 1);" + "map.set({}, 2); map.set({}, 3);" + "var it = map.values(); it") + ->ToObject(context) + .ToLocalChecked(); + CompileRun("map.delete(key); it.next(); it.next();"); + bool is_key; + v8::Local entries = + iterator->PreviewEntries(&is_key).ToLocalChecked(); + CHECK(!is_key); + CHECK_EQ(0, entries->Length()); + } + { + // Create map, create iterator, delete entry, iterate, trigger rehash, + // preview. + v8::Local iterator = CompileRun( + "var map = new Map();" + "var key = {}; map.set(key, 1);" + "map.set({}, 2); map.set({}, 3);" + "var it = map.values(); it") + ->ToObject(context) + .ToLocalChecked(); + CompileRun("map.delete(key); it.next();"); + CompileRun("for (var i = 4; i < 20; i++) map.set({}, i);"); + bool is_key; + v8::Local entries = + iterator->PreviewEntries(&is_key).ToLocalChecked(); + CHECK(!is_key); + CHECK_EQ(17, entries->Length()); + for (uint32_t i = 0; i < 17; i++) { + CHECK_EQ(i + 3, entries->Get(context, i) + .ToLocalChecked() + ->Int32Value(context) + .FromJust()); + } + } +}