Skip to content

Commit

Permalink
Merged: [runtime] Fix object cloning with spreads
Browse files Browse the repository at this point in the history 
When cloning objects using spread and update properties (e.g.
obj = {...o, x: 0}), we wrongly used the setter for the update argument
if one was set.
This CL changes the behaviour such that all arguments following the
spread are treated as dynamic arguments.

(cherry picked from commit 732d09a)

Bug: chromium:1251366
No-Try: true
No-Presubmit: true
No-Tree-Checks: true
Change-Id: Ie71dbe3f420a68e5e7e7aa23bc3ef8caccf6180c
Reviewed-on: https://chromium-review.googlesource.com/c/v8/v8/+/3219081
Reviewed-by: Deepti Gandluri <gdeepti@chromium.org>
Commit-Queue: Adam Klein <adamk@chromium.org>
Cr-Commit-Position: refs/branch-heads/9.5@{v8#40}
Cr-Branched-From: 4a03d61-refs/heads/9.5.172@{#1}
Cr-Branched-From: 9a60704-refs/heads/main@{#76741}
  • Loading branch information
Patrick Thier authored and V8 LUCI CQ committed Oct 12, 2021
1 parent 9310dae commit 813333d
Show file tree
Hide file tree
Showing 2 changed files with 101 additions and 80 deletions.
168 changes: 88 additions & 80 deletions src/interpreter/bytecode-generator.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -3005,96 +3005,104 @@ void BytecodeGenerator::VisitObjectLiteral(ObjectLiteral* expr) {
BuildCreateObjectLiteral(literal, flags, entry);
}

// Store computed values into the literal.
AccessorTable<ObjectLiteral::Property> accessor_table(zone());
for (; property_index < expr->properties()->length(); property_index++) {
ObjectLiteral::Property* property = expr->properties()->at(property_index);
if (property->is_computed_name()) break;
if (!clone_object_spread && property->IsCompileTimeValue()) continue;

RegisterAllocationScope inner_register_scope(this);
Literal* key = property->key()->AsLiteral();
switch (property->kind()) {
case ObjectLiteral::Property::SPREAD:
UNREACHABLE();
case ObjectLiteral::Property::CONSTANT:
case ObjectLiteral::Property::MATERIALIZED_LITERAL:
DCHECK(clone_object_spread || !property->value()->IsCompileTimeValue());
V8_FALLTHROUGH;
case ObjectLiteral::Property::COMPUTED: {
// It is safe to use [[Put]] here because the boilerplate already
// contains computed properties with an uninitialized value.
if (key->IsStringLiteral()) {
DCHECK(key->IsPropertyName());
object_literal_context_scope.SetEnteredIf(
property->value()->IsConciseMethodDefinition());
if (property->emit_store()) {
builder()->SetExpressionPosition(property->value());
VisitForAccumulatorValue(property->value());
FeedbackSlot slot = feedback_spec()->AddStoreOwnICSlot();
builder()->StoreNamedOwnProperty(literal, key->AsRawPropertyName(),
feedback_index(slot));
// If we used CloneObject for the first element is spread case, we already
// copied accessors. Therefore skip the static initialization and treat all
// properties after the spread as dynamic.
// TOOD(v8:9888): Use new Define ICs instead of Set ICs in the clone object
// spread case.
if (!clone_object_spread) {
// Store computed values into the literal.
AccessorTable<ObjectLiteral::Property> accessor_table(zone());
for (; property_index < expr->properties()->length(); property_index++) {
ObjectLiteral::Property* property =
expr->properties()->at(property_index);
if (property->is_computed_name()) break;
if (property->IsCompileTimeValue()) continue;

RegisterAllocationScope inner_register_scope(this);
Literal* key = property->key()->AsLiteral();
switch (property->kind()) {
case ObjectLiteral::Property::SPREAD:
case ObjectLiteral::Property::CONSTANT:
UNREACHABLE();
case ObjectLiteral::Property::MATERIALIZED_LITERAL:
DCHECK(!property->value()->IsCompileTimeValue());
V8_FALLTHROUGH;
case ObjectLiteral::Property::COMPUTED: {
// It is safe to use [[Put]] here because the boilerplate already
// contains computed properties with an uninitialized value.
if (key->IsStringLiteral()) {
DCHECK(key->IsPropertyName());
object_literal_context_scope.SetEnteredIf(
property->value()->IsConciseMethodDefinition());
if (property->emit_store()) {
builder()->SetExpressionPosition(property->value());
VisitForAccumulatorValue(property->value());
FeedbackSlot slot = feedback_spec()->AddStoreOwnICSlot();
builder()->StoreNamedOwnProperty(
literal, key->AsRawPropertyName(), feedback_index(slot));
} else {
builder()->SetExpressionPosition(property->value());
VisitForEffect(property->value());
}
} else {
RegisterList args = register_allocator()->NewRegisterList(3);

builder()->MoveRegister(literal, args[0]);
builder()->SetExpressionPosition(property->key());
VisitForRegisterValue(property->key(), args[1]);

object_literal_context_scope.SetEnteredIf(
property->value()->IsConciseMethodDefinition());
builder()->SetExpressionPosition(property->value());
VisitForEffect(property->value());
VisitForRegisterValue(property->value(), args[2]);
if (property->emit_store()) {
builder()->CallRuntime(Runtime::kSetKeyedProperty, args);
}
}
} else {
RegisterList args = register_allocator()->NewRegisterList(3);

break;
}
case ObjectLiteral::Property::PROTOTYPE: {
// __proto__:null is handled by CreateObjectLiteral.
if (property->IsNullPrototype()) break;
DCHECK(property->emit_store());
DCHECK(!property->NeedsSetFunctionName());
RegisterList args = register_allocator()->NewRegisterList(2);
builder()->MoveRegister(literal, args[0]);
builder()->SetExpressionPosition(property->key());
VisitForRegisterValue(property->key(), args[1]);

object_literal_context_scope.SetEnteredIf(
property->value()->IsConciseMethodDefinition());
object_literal_context_scope.SetEnteredIf(false);
builder()->SetExpressionPosition(property->value());
VisitForRegisterValue(property->value(), args[2]);
VisitForRegisterValue(property->value(), args[1]);
builder()->CallRuntime(Runtime::kInternalSetPrototype, args);
break;
}
case ObjectLiteral::Property::GETTER:
if (property->emit_store()) {
builder()->CallRuntime(Runtime::kSetKeyedProperty, args);
accessor_table.LookupOrInsert(key)->getter = property;
}
}
break;
}
case ObjectLiteral::Property::PROTOTYPE: {
// __proto__:null is handled by CreateObjectLiteral.
if (property->IsNullPrototype()) break;
DCHECK(property->emit_store());
DCHECK(!property->NeedsSetFunctionName());
RegisterList args = register_allocator()->NewRegisterList(2);
builder()->MoveRegister(literal, args[0]);
object_literal_context_scope.SetEnteredIf(false);
builder()->SetExpressionPosition(property->value());
VisitForRegisterValue(property->value(), args[1]);
builder()->CallRuntime(Runtime::kInternalSetPrototype, args);
break;
break;
case ObjectLiteral::Property::SETTER:
if (property->emit_store()) {
accessor_table.LookupOrInsert(key)->setter = property;
}
break;
}
case ObjectLiteral::Property::GETTER:
if (property->emit_store()) {
accessor_table.LookupOrInsert(key)->getter = property;
}
break;
case ObjectLiteral::Property::SETTER:
if (property->emit_store()) {
accessor_table.LookupOrInsert(key)->setter = property;
}
break;
}
}

// Define accessors, using only a single call to the runtime for each pair of
// corresponding getters and setters.
object_literal_context_scope.SetEnteredIf(true);
for (auto accessors : accessor_table.ordered_accessors()) {
RegisterAllocationScope inner_register_scope(this);
RegisterList args = register_allocator()->NewRegisterList(5);
builder()->MoveRegister(literal, args[0]);
VisitForRegisterValue(accessors.first, args[1]);
VisitLiteralAccessor(accessors.second->getter, args[2]);
VisitLiteralAccessor(accessors.second->setter, args[3]);
builder()
->LoadLiteral(Smi::FromInt(NONE))
.StoreAccumulatorInRegister(args[4])
.CallRuntime(Runtime::kDefineAccessorPropertyUnchecked, args);
// Define accessors, using only a single call to the runtime for each pair
// of corresponding getters and setters.
object_literal_context_scope.SetEnteredIf(true);
for (auto accessors : accessor_table.ordered_accessors()) {
RegisterAllocationScope inner_register_scope(this);
RegisterList args = register_allocator()->NewRegisterList(5);
builder()->MoveRegister(literal, args[0]);
VisitForRegisterValue(accessors.first, args[1]);
VisitLiteralAccessor(accessors.second->getter, args[2]);
VisitLiteralAccessor(accessors.second->setter, args[3]);
builder()
->LoadLiteral(Smi::FromInt(NONE))
.StoreAccumulatorInRegister(args[4])
.CallRuntime(Runtime::kDefineAccessorPropertyUnchecked, args);
}
}

// Object literals have two parts. The "static" part on the left contains no
Expand Down
13 changes: 13 additions & 0 deletions test/mjsunit/regress/regress-crbug-1251366.js
View file
Original file line number Diff line number Diff line change
@@ -0,0 +1,13 @@
// Copyright 2021 the V8 project authors. All rights reserved.
// Use of this source code is governed by a BSD-style license that can be
// found in the LICENSE file.

Object.defineProperty(Object.prototype, "x", {
set: function (val) {}
});

let o = {...{}, x: 3};
assertEquals(o.x, 3);

let o2 = {...{x: 1}, x: 3};
assertEquals(o2.x, 3);

0 comments on commit 813333d

Please sign in to comment.