From b049a74a63cb15921aef4dff9a1d2bb26cf5090c 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 MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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} [The backport to v10.x resolves merge conflicts due to a different way of accessing the “hole” value in V8, different signatures of the `Handle` constructor and the `Shrink()` method, and neighbouring-line conflicts in the test file.] Refs: https://github.com/v8/v8/commit/88f8fe19a863c6392bd296faf86c06eff2a41bc1 Fixes: https://github.com/nodejs/node/issues/27882 PR-URL: https://github.com/nodejs/node/pull/24514 Refs: https://github.com/nodejs/node/issues/24053 Reviewed-By: Michaël Zasso Reviewed-By: Anna Henningsen Reviewed-By: Gus Caplan Reviewed-By: Joyee Cheung --- common.gypi | 2 +- deps/v8/src/api.cc | 58 +++++---- deps/v8/test/cctest/test-api.cc | 214 ++++++++++++++++++++++++++++++++ 3 files changed, 246 insertions(+), 28 deletions(-) diff --git a/common.gypi b/common.gypi index 4b702eaff582d4..12f4205db256e5 100644 --- a/common.gypi +++ b/common.gypi @@ -33,7 +33,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.52', + 'v8_embedder_string': '-node.53', # 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 cacd3995fc67b9..16acd7ea2098a7 100644 --- a/deps/v8/src/api.cc +++ b/deps/v8/src/api.cc @@ -7143,31 +7143,32 @@ enum class MapAsArrayKind { i::Handle MapAsArray(i::Isolate* isolate, i::Object* table_obj, int offset, MapAsArrayKind kind) { i::Factory* factory = isolate->factory(); - i::Handle table(i::OrderedHashMap::cast(table_obj)); - if (offset >= table->NumberOfElements()) return factory->NewJSArray(0); - int length = (table->NumberOfElements() - offset) * - (kind == MapAsArrayKind::kEntries ? 2 : 1); - i::Handle result = factory->NewFixedArray(length); + i::Handle table(i::OrderedHashMap::cast(table_obj), + isolate); + 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 = isolate->heap()->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(result_index); + return factory->NewJSArrayWithElements(result, i::PACKED_ELEMENTS, + result_index); } } // namespace @@ -7250,25 +7251,28 @@ namespace { i::Handle SetAsArray(i::Isolate* isolate, i::Object* table_obj, int offset) { i::Factory* factory = isolate->factory(); - i::Handle table(i::OrderedHashSet::cast(table_obj)); - int length = table->NumberOfElements() - offset; - if (length <= 0) return factory->NewJSArray(0); - i::Handle result = factory->NewFixedArray(length); + i::Handle table(i::OrderedHashSet::cast(table_obj), + isolate); + // 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 = isolate->heap()->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(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 5bea5b14d06ee1..4cdb65062219e8 100644 --- a/deps/v8/test/cctest/test-api.cc +++ b/deps/v8/test/cctest/test-api.cc @@ -28066,3 +28066,217 @@ TEST(BigIntAPI) { CHECK_EQ(word_count, 2); } } + +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()); + } + } +}