From 0a0ef6ed4297d3b996d3ff3c6397ab922c50315e Mon Sep 17 00:00:00 2001 From: Ruben Bridgewater Date: Mon, 4 Feb 2019 21:13:20 +0100 Subject: [PATCH 1/2] deps: V8: backport 74571c8 Original commit message: Fix preview of set entries Set entries return an array with the value as first and second entry. As such these are considered key value pairs to align with maps entries iterator. So far the return value was identical to the values iterator and that is misleading. This also adds tests to verify the results and improves the coverage a tiny bit by testing different iterators. Refs: https://github.com/nodejs/node/issues/24629 R=yangguo@chromium.org Change-Id: I669a724bb4afaf5a713e468b1f51691d22c25253 Reviewed-on: https://chromium-review.googlesource.com/c/1350790 Commit-Queue: Yang Guo Reviewed-by: Benedikt Meurer Reviewed-by: Jakob Gruber Reviewed-by: Yang Guo Cr-Commit-Position: refs/heads/master@{#59311} Refs: https://github.com/v8/v8/commit/74571c80a945f2bdf4094a090410ae02b9a69af6 --- common.gypi | 2 +- deps/v8/AUTHORS | 1 + deps/v8/src/api.cc | 31 ++- deps/v8/test/cctest/test-api.cc | 230 +++++++++++++++++- ...t-preview-internal-properties-expected.txt | 40 +-- .../object-preview-internal-properties.js | 2 +- .../internal-properties-entries-expected.txt | 3 +- 7 files changed, 279 insertions(+), 30 deletions(-) diff --git a/common.gypi b/common.gypi index 079ebb1f0a91..9c351583b695 100644 --- a/common.gypi +++ b/common.gypi @@ -37,7 +37,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.13', + 'v8_embedder_string': '-node.14', ##### V8 defaults for Node.js ##### diff --git a/deps/v8/AUTHORS b/deps/v8/AUTHORS index b935565945da..241c69f04655 100644 --- a/deps/v8/AUTHORS +++ b/deps/v8/AUTHORS @@ -143,6 +143,7 @@ Rick Waldron Rob Wu Robert Mustacchi Robert Nagy +Ruben Bridgewater Ryan Dahl Sakthipriyan Vairamani (thefourtheye) Sander Mathijs van Veen diff --git a/deps/v8/src/api.cc b/deps/v8/src/api.cc index 64676f06c1a3..09db471982ec 100644 --- a/deps/v8/src/api.cc +++ b/deps/v8/src/api.cc @@ -7028,6 +7028,11 @@ enum class MapAsArrayKind { kValues = i::JS_MAP_VALUE_ITERATOR_TYPE }; +enum class SetAsArrayKind { + kEntries = i::JS_SET_KEY_VALUE_ITERATOR_TYPE, + kValues = i::JS_SET_VALUE_ITERATOR_TYPE +}; + i::Handle MapAsArray(i::Isolate* isolate, i::Object* table_obj, int offset, MapAsArrayKind kind) { i::Factory* factory = isolate->factory(); @@ -7137,13 +7142,14 @@ Maybe Set::Delete(Local context, Local key) { namespace { i::Handle SetAsArray(i::Isolate* isolate, i::Object* table_obj, - int offset) { + int offset, SetAsArrayKind kind) { i::Factory* factory = isolate->factory(); 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; + const bool collect_key_values = kind == SetAsArrayKind::kEntries; + int max_length = (capacity - offset) * (collect_key_values ? 2 : 1); if (max_length == 0) return factory->NewJSArray(0); i::Handle result = factory->NewFixedArray(max_length); int result_index = 0; @@ -7154,6 +7160,7 @@ i::Handle SetAsArray(i::Isolate* isolate, i::Object* table_obj, i::Object* key = table->KeyAt(i); if (key == the_hole) continue; result->set(result_index++, key); + if (collect_key_values) result->set(result_index++, key); } } DCHECK_GE(max_length, result_index); @@ -7169,7 +7176,8 @@ Local Set::AsArray() const { i::Isolate* isolate = obj->GetIsolate(); LOG_API(isolate, Set, AsArray); ENTER_V8_NO_SCRIPT_NO_EXCEPTION(isolate); - return Utils::ToLocal(SetAsArray(isolate, obj->table(), 0)); + return Utils::ToLocal( + SetAsArray(isolate, obj->table(), 0, SetAsArrayKind::kValues)); } @@ -9524,21 +9532,22 @@ v8::MaybeLocal v8::Object::PreviewEntries(bool* is_key_value) { i::Handle::cast(object), 0)); } if (object->IsJSMapIterator()) { - i::Handle iterator = - i::Handle::cast(object); + i::Handle it = i::Handle::cast(object); MapAsArrayKind const kind = - static_cast(iterator->map()->instance_type()); + static_cast(it->map()->instance_type()); *is_key_value = kind == MapAsArrayKind::kEntries; - if (!iterator->HasMore()) return v8::Array::New(v8_isolate); - return Utils::ToLocal(MapAsArray(isolate, iterator->table(), - i::Smi::ToInt(iterator->index()), kind)); + if (!it->HasMore()) return v8::Array::New(v8_isolate); + return Utils::ToLocal( + MapAsArray(isolate, it->table(), i::Smi::ToInt(it->index()), kind)); } if (object->IsJSSetIterator()) { i::Handle it = i::Handle::cast(object); - *is_key_value = false; + SetAsArrayKind const kind = + static_cast(it->map()->instance_type()); + *is_key_value = kind == SetAsArrayKind::kEntries; if (!it->HasMore()) return v8::Array::New(v8_isolate); return Utils::ToLocal( - SetAsArray(isolate, it->table(), i::Smi::ToInt(it->index()))); + SetAsArray(isolate, it->table(), i::Smi::ToInt(it->index()), kind)); } return v8::MaybeLocal(); } diff --git a/deps/v8/test/cctest/test-api.cc b/deps/v8/test/cctest/test-api.cc index 9bf7870f7545..3b564d9bf3ea 100644 --- a/deps/v8/test/cctest/test-api.cc +++ b/deps/v8/test/cctest/test-api.cc @@ -28853,7 +28853,7 @@ TEST(TestGetEmbeddedCodeRange) { } } -TEST(PreviewSetIteratorEntriesWithDeleted) { +TEST(PreviewSetKeysIteratorEntriesWithDeleted) { LocalContext env; v8::HandleScope handle_scope(env->GetIsolate()); v8::Local context = env.local(); @@ -28952,7 +28952,142 @@ TEST(PreviewSetIteratorEntriesWithDeleted) { } } -TEST(PreviewMapIteratorEntriesWithDeleted) { +TEST(PreviewSetValuesIteratorEntriesWithDeleted) { + 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.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 set, create iterator, delete entry, preview. + v8::Local iterator = + CompileRun("var set = new Set([1,2,3]); set.values()") + ->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.values(); 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.values(); 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.values(); 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(PreviewMapEntriesIteratorEntries) { + LocalContext env; + v8::HandleScope handle_scope(env->GetIsolate()); + v8::Local context = env.local(); + { + // Create set, delete entry, create entries iterator, preview. + v8::Local iterator = + CompileRun("var set = new Set([1,2,3]); set.delete(2); set.entries()") + ->ToObject(context) + .ToLocalChecked(); + bool is_key; + v8::Local entries = + iterator->PreviewEntries(&is_key).ToLocalChecked(); + CHECK(is_key); + CHECK_EQ(4, entries->Length()); + uint32_t first = entries->Get(context, 0) + .ToLocalChecked() + ->Int32Value(context) + .FromJust(); + uint32_t second = entries->Get(context, 2) + .ToLocalChecked() + ->Int32Value(context) + .FromJust(); + CHECK_EQ(1, first); + CHECK_EQ(3, second); + CHECK_EQ(first, entries->Get(context, 1) + .ToLocalChecked() + ->Int32Value(context) + .FromJust()); + CHECK_EQ(second, entries->Get(context, 3) + .ToLocalChecked() + ->Int32Value(context) + .FromJust()); + } +} + +TEST(PreviewMapValuesIteratorEntriesWithDeleted) { LocalContext env; v8::HandleScope handle_scope(env->GetIsolate()); v8::Local context = env.local(); @@ -29066,3 +29201,94 @@ TEST(PreviewMapIteratorEntriesWithDeleted) { } } } + +TEST(PreviewMapKeysIteratorEntriesWithDeleted) { + 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 = 1; map.set(key, {});" + "map.set(2, {}); map.set(3, {});" + "map.delete(key);" + "map.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 map, create iterator, delete entry, preview. + v8::Local iterator = CompileRun( + "var map = new Map();" + "var key = 1; map.set(key, {});" + "map.set(2, {}); map.set(3, {});" + "map.keys()") + ->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 = 1; map.set(key, {});" + "map.set(2, {}); map.set(3, {});" + "var it = map.keys(); 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 = 1; map.set(key, {});" + "map.set(2, {}); map.set(3, {});" + "var it = map.keys(); 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()); + } +} diff --git a/deps/v8/test/inspector/debugger/object-preview-internal-properties-expected.txt b/deps/v8/test/inspector/debugger/object-preview-internal-properties-expected.txt index a24ba5c3700a..5ef83274995b 100644 --- a/deps/v8/test/inspector/debugger/object-preview-internal-properties-expected.txt +++ b/deps/v8/test/inspector/debugger/object-preview-internal-properties-expected.txt @@ -177,27 +177,39 @@ expression: (new Map([[1,2]])).entries() } ] -expression: (new Set([[1,2]])).entries() +expression: (new Set([1,2])).entries() [[Entries]]: [ [0] : { + key : { + description : 1 + overflow : false + properties : [ + ] + type : number + } value : { - description : Array(2) + description : 1 overflow : false properties : [ - [0] : { - name : 0 - type : number - value : 1 - } - [1] : { - name : 1 - type : number - value : 2 - } ] - subtype : array - type : object + type : number + } + } + [1] : { + key : { + description : 2 + overflow : false + properties : [ + ] + type : number + } + value : { + description : 2 + overflow : false + properties : [ + ] + type : number } } ] diff --git a/deps/v8/test/inspector/debugger/object-preview-internal-properties.js b/deps/v8/test/inspector/debugger/object-preview-internal-properties.js index fc7dabac1a93..5b1cc3b8a26a 100644 --- a/deps/v8/test/inspector/debugger/object-preview-internal-properties.js +++ b/deps/v8/test/inspector/debugger/object-preview-internal-properties.js @@ -46,7 +46,7 @@ InspectorTest.runTestSuite([ function iteratorObject(next) { checkExpression("(new Map([[1,2]])).entries()") - .then(() => checkExpression("(new Set([[1,2]])).entries()")) + .then(() => checkExpression("(new Set([1,2])).entries()")) .then(next); }, diff --git a/deps/v8/test/inspector/runtime/internal-properties-entries-expected.txt b/deps/v8/test/inspector/runtime/internal-properties-entries-expected.txt index 1d09e8dc1ebe..9082e9b26689 100644 --- a/deps/v8/test/inspector/runtime/internal-properties-entries-expected.txt +++ b/deps/v8/test/inspector/runtime/internal-properties-entries-expected.txt @@ -572,6 +572,7 @@ expression: it = new Set([1,2]).keys(); it.next(); it expression: it = new Set([1,2]).entries(); it.next(); it [ [0] : { + key : 2 value : 2 } ] @@ -592,7 +593,7 @@ expression: it = new Set([1,2]).entries(); it.next(); it name : 0 value : { className : Object - description : 2 + description : {2 => 2} objectId : subtype : internal#entry type : object From 305201bcbcf015967fc6b0a13cef955381d90656 Mon Sep 17 00:00:00 2001 From: Ruben Bridgewater Date: Tue, 5 Feb 2019 04:17:52 +0100 Subject: [PATCH 2/2] util: update set iterator entries inspection The inspection output for Set#entries() was wrong so far as it did not return an array as it should have. That was a bug in V8 that is now fixed and the code in Node.js has to be updated accordingly. --- lib/internal/util/inspect.js | 15 +++++---------- src/node_util.cc | 2 +- test/parallel/test-util-inspect.js | 5 +++-- 3 files changed, 9 insertions(+), 13 deletions(-) diff --git a/lib/internal/util/inspect.js b/lib/internal/util/inspect.js index 94d6ef8706f7..5246ed89b790 100644 --- a/lib/internal/util/inspect.js +++ b/lib/internal/util/inspect.js @@ -595,11 +595,11 @@ function formatRaw(ctx, value, recurseTimes, typedArray) { } else if (isMapIterator(value)) { keys = getKeys(value, ctx.showHidden); braces = [`[${tag}] {`, '}']; - formatter = formatMapIterator; + formatter = formatIterator; } else if (isSetIterator(value)) { keys = getKeys(value, ctx.showHidden); braces = [`[${tag}] {`, '}']; - formatter = formatSetIterator; + formatter = formatIterator; } else { noIterator = true; } @@ -731,10 +731,10 @@ function formatRaw(ctx, value, recurseTimes, typedArray) { } if (isMapIterator(value)) { braces = [`[${tag || 'Map Iterator'}] {`, '}']; - formatter = formatMapIterator; + formatter = formatIterator; } else if (isSetIterator(value)) { braces = [`[${tag || 'Set Iterator'}] {`, '}']; - formatter = formatSetIterator; + formatter = formatIterator; // Handle other regular objects again. } else if (keys.length === 0) { if (isExternal(value)) @@ -1091,12 +1091,7 @@ function formatWeakMap(ctx, value, recurseTimes) { return formatMapIterInner(ctx, recurseTimes, entries, kWeak); } -function formatSetIterator(ctx, value, recurseTimes) { - const entries = previewEntries(value); - return formatSetIterInner(ctx, recurseTimes, entries, kIterator); -} - -function formatMapIterator(ctx, value, recurseTimes) { +function formatIterator(ctx, value, recurseTimes) { const [entries, isKeyValue] = previewEntries(value, true); if (isKeyValue) { return formatMapIterInner(ctx, recurseTimes, entries, kMapEntries); diff --git a/src/node_util.cc b/src/node_util.cc index 4cca0cbb72ae..f2c008c797d6 100644 --- a/src/node_util.cc +++ b/src/node_util.cc @@ -100,7 +100,7 @@ static void PreviewEntries(const FunctionCallbackInfo& args) { Local entries; if (!args[0].As()->PreviewEntries(&is_key_value).ToLocal(&entries)) return; - // Fast path for WeakMap, WeakSet and Set iterators. + // Fast path for WeakMap and WeakSet. if (args.Length() == 1) return args.GetReturnValue().Set(entries); diff --git a/test/parallel/test-util-inspect.js b/test/parallel/test-util-inspect.js index 76f8caa31a88..a9afdbf8af04 100644 --- a/test/parallel/test-util-inspect.js +++ b/test/parallel/test-util-inspect.js @@ -995,7 +995,8 @@ if (typeof Symbol !== 'undefined') { const aSet = new Set([1, 3]); assert.strictEqual(util.inspect(aSet.keys()), '[Set Iterator] { 1, 3 }'); assert.strictEqual(util.inspect(aSet.values()), '[Set Iterator] { 1, 3 }'); - assert.strictEqual(util.inspect(aSet.entries()), '[Set Iterator] { 1, 3 }'); + assert.strictEqual(util.inspect(aSet.entries()), + '[Set Iterator] { [ 1, 1 ], [ 3, 3 ] }'); // Make sure the iterator doesn't get consumed. const keys = aSet.keys(); assert.strictEqual(util.inspect(keys), '[Set Iterator] { 1, 3 }'); @@ -1609,7 +1610,7 @@ assert.strictEqual(util.inspect('"\'${a}'), "'\"\\'${a}'"); [{ a: 5 }, '{ a: 5 }'], [new Set([1, 2]), 'Set { 1, 2 }'], [new Map([[1, 2]]), 'Map { 1 => 2 }'], - [new Set([1, 2]).entries(), '[Set Iterator] { 1, 2 }'], + [new Set([1, 2]).entries(), '[Set Iterator] { [ 1, 1 ], [ 2, 2 ] }'], [new Map([[1, 2]]).keys(), '[Map Iterator] { 1 }'], [new Date(2000), '1970-01-01T00:00:02.000Z'], [new Uint8Array(2), 'Uint8Array [ 0, 0 ]'],