Skip to content

Commit b049a74

Browse files
hashseedaddaleax
hashseed
authored and
addaleax
committed
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 <yangguo@chromium.org> Reviewed-by: Georg Neis <neis@chromium.org> Cr-Commit-Position: refs/heads/master@{nodejs#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: v8/v8@88f8fe1 Fixes: nodejs#27882 PR-URL: nodejs#24514 Refs: nodejs#24053 Reviewed-By: Michaël Zasso <targos@protonmail.com> Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: Gus Caplan <me@gus.host> Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com>
1 parent cda0d16 commit b049a74

File tree

3 files changed

+246
-28
lines changed

3 files changed

+246
-28
lines changed

‎common.gypi‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -33,7 +33,7 @@
3333

3434
# Reset this number to 0 on major V8 upgrades.
3535
# Increment by one for each non-official patch applied to deps/v8.
36-
'v8_embedder_string': '-node.52',
36+
'v8_embedder_string': '-node.53',
3737

3838
# Enable disassembler for `--print-code` v8 options
3939
'v8_enable_disassembler': 1,

‎deps/v8/src/api.cc‎

Lines changed: 31 additions & 27 deletions
Original file line numberDiff line numberDiff line change
@@ -7143,31 +7143,32 @@ enum class MapAsArrayKind {
71437143
i::Handle<i::JSArray> MapAsArray(i::Isolate* isolate, i::Object* table_obj,
71447144
int offset, MapAsArrayKind kind) {
71457145
i::Factory* factory = isolate->factory();
7146-
i::Handle<i::OrderedHashMap> table(i::OrderedHashMap::cast(table_obj));
7147-
if (offset >= table->NumberOfElements()) return factory->NewJSArray(0);
7148-
int length = (table->NumberOfElements() - offset) *
7149-
(kind == MapAsArrayKind::kEntries ? 2 : 1);
7150-
i::Handle<i::FixedArray> result = factory->NewFixedArray(length);
7146+
i::Handle<i::OrderedHashMap> table(i::OrderedHashMap::cast(table_obj),
7147+
isolate);
7148+
const bool collect_keys =
7149+
kind == MapAsArrayKind::kEntries || kind == MapAsArrayKind::kKeys;
7150+
const bool collect_values =
7151+
kind == MapAsArrayKind::kEntries || kind == MapAsArrayKind::kValues;
7152+
int capacity = table->UsedCapacity();
7153+
int max_length =
7154+
(capacity - offset) * ((collect_keys && collect_values) ? 2 : 1);
7155+
i::Handle<i::FixedArray> result = factory->NewFixedArray(max_length);
71517156
int result_index = 0;
71527157
{
71537158
i::DisallowHeapAllocation no_gc;
7154-
int capacity = table->UsedCapacity();
71557159
i::Oddball* the_hole = isolate->heap()->the_hole_value();
7156-
for (int i = 0; i < capacity; ++i) {
7160+
for (int i = offset; i < capacity; ++i) {
71577161
i::Object* key = table->KeyAt(i);
71587162
if (key == the_hole) continue;
7159-
if (offset-- > 0) continue;
7160-
if (kind == MapAsArrayKind::kEntries || kind == MapAsArrayKind::kKeys) {
7161-
result->set(result_index++, key);
7162-
}
7163-
if (kind == MapAsArrayKind::kEntries || kind == MapAsArrayKind::kValues) {
7164-
result->set(result_index++, table->ValueAt(i));
7165-
}
7163+
if (collect_keys) result->set(result_index++, key);
7164+
if (collect_values) result->set(result_index++, table->ValueAt(i));
71667165
}
71677166
}
7168-
DCHECK_EQ(result_index, result->length());
7169-
DCHECK_EQ(result_index, length);
7170-
return factory->NewJSArrayWithElements(result, i::PACKED_ELEMENTS, length);
7167+
DCHECK_GE(max_length, result_index);
7168+
if (result_index == 0) return factory->NewJSArray(0);
7169+
result->Shrink(result_index);
7170+
return factory->NewJSArrayWithElements(result, i::PACKED_ELEMENTS,
7171+
result_index);
71717172
}
71727173

71737174
} // namespace
@@ -7250,25 +7251,28 @@ namespace {
72507251
i::Handle<i::JSArray> SetAsArray(i::Isolate* isolate, i::Object* table_obj,
72517252
int offset) {
72527253
i::Factory* factory = isolate->factory();
7253-
i::Handle<i::OrderedHashSet> table(i::OrderedHashSet::cast(table_obj));
7254-
int length = table->NumberOfElements() - offset;
7255-
if (length <= 0) return factory->NewJSArray(0);
7256-
i::Handle<i::FixedArray> result = factory->NewFixedArray(length);
7254+
i::Handle<i::OrderedHashSet> table(i::OrderedHashSet::cast(table_obj),
7255+
isolate);
7256+
// Elements skipped by |offset| may already be deleted.
7257+
int capacity = table->UsedCapacity();
7258+
int max_length = capacity - offset;
7259+
if (max_length == 0) return factory->NewJSArray(0);
7260+
i::Handle<i::FixedArray> result = factory->NewFixedArray(max_length);
72577261
int result_index = 0;
72587262
{
72597263
i::DisallowHeapAllocation no_gc;
7260-
int capacity = table->UsedCapacity();
72617264
i::Oddball* the_hole = isolate->heap()->the_hole_value();
7262-
for (int i = 0; i < capacity; ++i) {
7265+
for (int i = offset; i < capacity; ++i) {
72637266
i::Object* key = table->KeyAt(i);
72647267
if (key == the_hole) continue;
7265-
if (offset-- > 0) continue;
72667268
result->set(result_index++, key);
72677269
}
72687270
}
7269-
DCHECK_EQ(result_index, result->length());
7270-
DCHECK_EQ(result_index, length);
7271-
return factory->NewJSArrayWithElements(result, i::PACKED_ELEMENTS, length);
7271+
DCHECK_GE(max_length, result_index);
7272+
if (result_index == 0) return factory->NewJSArray(0);
7273+
result->Shrink(result_index);
7274+
return factory->NewJSArrayWithElements(result, i::PACKED_ELEMENTS,
7275+
result_index);
72727276
}
72737277
} // namespace
72747278

‎deps/v8/test/cctest/test-api.cc‎

Lines changed: 214 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -28066,3 +28066,217 @@ TEST(BigIntAPI) {
2806628066
CHECK_EQ(word_count, 2);
2806728067
}
2806828068
}
28069+
28070+
TEST(PreviewSetIteratorEntriesWithDeleted) {
28071+
LocalContext env;
28072+
v8::HandleScope handle_scope(env->GetIsolate());
28073+
v8::Local<v8::Context> context = env.local();
28074+
28075+
{
28076+
// Create set, delete entry, create iterator, preview.
28077+
v8::Local<v8::Object> iterator =
28078+
CompileRun("var set = new Set([1,2,3]); set.delete(1); set.keys()")
28079+
->ToObject(context)
28080+
.ToLocalChecked();
28081+
bool is_key;
28082+
v8::Local<v8::Array> entries =
28083+
iterator->PreviewEntries(&is_key).ToLocalChecked();
28084+
CHECK(!is_key);
28085+
CHECK_EQ(2, entries->Length());
28086+
CHECK_EQ(2, entries->Get(context, 0)
28087+
.ToLocalChecked()
28088+
->Int32Value(context)
28089+
.FromJust());
28090+
CHECK_EQ(3, entries->Get(context, 1)
28091+
.ToLocalChecked()
28092+
->Int32Value(context)
28093+
.FromJust());
28094+
}
28095+
{
28096+
// Create set, create iterator, delete entry, preview.
28097+
v8::Local<v8::Object> iterator =
28098+
CompileRun("var set = new Set([1,2,3]); set.keys()")
28099+
->ToObject(context)
28100+
.ToLocalChecked();
28101+
CompileRun("set.delete(1);");
28102+
bool is_key;
28103+
v8::Local<v8::Array> entries =
28104+
iterator->PreviewEntries(&is_key).ToLocalChecked();
28105+
CHECK(!is_key);
28106+
CHECK_EQ(2, entries->Length());
28107+
CHECK_EQ(2, entries->Get(context, 0)
28108+
.ToLocalChecked()
28109+
->Int32Value(context)
28110+
.FromJust());
28111+
CHECK_EQ(3, entries->Get(context, 1)
28112+
.ToLocalChecked()
28113+
->Int32Value(context)
28114+
.FromJust());
28115+
}
28116+
{
28117+
// Create set, create iterator, delete entry, iterate, preview.
28118+
v8::Local<v8::Object> iterator =
28119+
CompileRun("var set = new Set([1,2,3]); var it = set.keys(); it")
28120+
->ToObject(context)
28121+
.ToLocalChecked();
28122+
CompileRun("set.delete(1); it.next();");
28123+
bool is_key;
28124+
v8::Local<v8::Array> entries =
28125+
iterator->PreviewEntries(&is_key).ToLocalChecked();
28126+
CHECK(!is_key);
28127+
CHECK_EQ(1, entries->Length());
28128+
CHECK_EQ(3, entries->Get(context, 0)
28129+
.ToLocalChecked()
28130+
->Int32Value(context)
28131+
.FromJust());
28132+
}
28133+
{
28134+
// Create set, create iterator, delete entry, iterate until empty, preview.
28135+
v8::Local<v8::Object> iterator =
28136+
CompileRun("var set = new Set([1,2,3]); var it = set.keys(); it")
28137+
->ToObject(context)
28138+
.ToLocalChecked();
28139+
CompileRun("set.delete(1); it.next(); it.next();");
28140+
bool is_key;
28141+
v8::Local<v8::Array> entries =
28142+
iterator->PreviewEntries(&is_key).ToLocalChecked();
28143+
CHECK(!is_key);
28144+
CHECK_EQ(0, entries->Length());
28145+
}
28146+
{
28147+
// Create set, create iterator, delete entry, iterate, trigger rehash,
28148+
// preview.
28149+
v8::Local<v8::Object> iterator =
28150+
CompileRun("var set = new Set([1,2,3]); var it = set.keys(); it")
28151+
->ToObject(context)
28152+
.ToLocalChecked();
28153+
CompileRun("set.delete(1); it.next();");
28154+
CompileRun("for (var i = 4; i < 20; i++) set.add(i);");
28155+
bool is_key;
28156+
v8::Local<v8::Array> entries =
28157+
iterator->PreviewEntries(&is_key).ToLocalChecked();
28158+
CHECK(!is_key);
28159+
CHECK_EQ(17, entries->Length());
28160+
for (uint32_t i = 0; i < 17; i++) {
28161+
CHECK_EQ(i + 3, entries->Get(context, i)
28162+
.ToLocalChecked()
28163+
->Int32Value(context)
28164+
.FromJust());
28165+
}
28166+
}
28167+
}
28168+
28169+
TEST(PreviewMapIteratorEntriesWithDeleted) {
28170+
LocalContext env;
28171+
v8::HandleScope handle_scope(env->GetIsolate());
28172+
v8::Local<v8::Context> context = env.local();
28173+
28174+
{
28175+
// Create map, delete entry, create iterator, preview.
28176+
v8::Local<v8::Object> iterator = CompileRun(
28177+
"var map = new Map();"
28178+
"var key = {}; map.set(key, 1);"
28179+
"map.set({}, 2); map.set({}, 3);"
28180+
"map.delete(key);"
28181+
"map.values()")
28182+
->ToObject(context)
28183+
.ToLocalChecked();
28184+
bool is_key;
28185+
v8::Local<v8::Array> entries =
28186+
iterator->PreviewEntries(&is_key).ToLocalChecked();
28187+
CHECK(!is_key);
28188+
CHECK_EQ(2, entries->Length());
28189+
CHECK_EQ(2, entries->Get(context, 0)
28190+
.ToLocalChecked()
28191+
->Int32Value(context)
28192+
.FromJust());
28193+
CHECK_EQ(3, entries->Get(context, 1)
28194+
.ToLocalChecked()
28195+
->Int32Value(context)
28196+
.FromJust());
28197+
}
28198+
{
28199+
// Create map, create iterator, delete entry, preview.
28200+
v8::Local<v8::Object> iterator = CompileRun(
28201+
"var map = new Map();"
28202+
"var key = {}; map.set(key, 1);"
28203+
"map.set({}, 2); map.set({}, 3);"
28204+
"map.values()")
28205+
->ToObject(context)
28206+
.ToLocalChecked();
28207+
CompileRun("map.delete(key);");
28208+
bool is_key;
28209+
v8::Local<v8::Array> entries =
28210+
iterator->PreviewEntries(&is_key).ToLocalChecked();
28211+
CHECK(!is_key);
28212+
CHECK_EQ(2, entries->Length());
28213+
CHECK_EQ(2, entries->Get(context, 0)
28214+
.ToLocalChecked()
28215+
->Int32Value(context)
28216+
.FromJust());
28217+
CHECK_EQ(3, entries->Get(context, 1)
28218+
.ToLocalChecked()
28219+
->Int32Value(context)
28220+
.FromJust());
28221+
}
28222+
{
28223+
// Create map, create iterator, delete entry, iterate, preview.
28224+
v8::Local<v8::Object> iterator = CompileRun(
28225+
"var map = new Map();"
28226+
"var key = {}; map.set(key, 1);"
28227+
"map.set({}, 2); map.set({}, 3);"
28228+
"var it = map.values(); it")
28229+
->ToObject(context)
28230+
.ToLocalChecked();
28231+
CompileRun("map.delete(key); it.next();");
28232+
bool is_key;
28233+
v8::Local<v8::Array> entries =
28234+
iterator->PreviewEntries(&is_key).ToLocalChecked();
28235+
CHECK(!is_key);
28236+
CHECK_EQ(1, entries->Length());
28237+
CHECK_EQ(3, entries->Get(context, 0)
28238+
.ToLocalChecked()
28239+
->Int32Value(context)
28240+
.FromJust());
28241+
}
28242+
{
28243+
// Create map, create iterator, delete entry, iterate until empty, preview.
28244+
v8::Local<v8::Object> iterator = CompileRun(
28245+
"var map = new Map();"
28246+
"var key = {}; map.set(key, 1);"
28247+
"map.set({}, 2); map.set({}, 3);"
28248+
"var it = map.values(); it")
28249+
->ToObject(context)
28250+
.ToLocalChecked();
28251+
CompileRun("map.delete(key); it.next(); it.next();");
28252+
bool is_key;
28253+
v8::Local<v8::Array> entries =
28254+
iterator->PreviewEntries(&is_key).ToLocalChecked();
28255+
CHECK(!is_key);
28256+
CHECK_EQ(0, entries->Length());
28257+
}
28258+
{
28259+
// Create map, create iterator, delete entry, iterate, trigger rehash,
28260+
// preview.
28261+
v8::Local<v8::Object> iterator = CompileRun(
28262+
"var map = new Map();"
28263+
"var key = {}; map.set(key, 1);"
28264+
"map.set({}, 2); map.set({}, 3);"
28265+
"var it = map.values(); it")
28266+
->ToObject(context)
28267+
.ToLocalChecked();
28268+
CompileRun("map.delete(key); it.next();");
28269+
CompileRun("for (var i = 4; i < 20; i++) map.set({}, i);");
28270+
bool is_key;
28271+
v8::Local<v8::Array> entries =
28272+
iterator->PreviewEntries(&is_key).ToLocalChecked();
28273+
CHECK(!is_key);
28274+
CHECK_EQ(17, entries->Length());
28275+
for (uint32_t i = 0; i < 17; i++) {
28276+
CHECK_EQ(i + 3, entries->Get(context, i)
28277+
.ToLocalChecked()
28278+
->Int32Value(context)
28279+
.FromJust());
28280+
}
28281+
}
28282+
}

0 commit comments

Comments
 (0)