Reviewers: Toon Verwaest,
Message:
This should unblock the V8 roll. Stupid, stupid bug...
Description:
Fix Object.freeze for objects with mixed accessors and data properties
The bug in the existing code was that it modified the |attributes|
local variable on its way through the loop in CopyUpToAddAttributes.
But that affected any properties updated after an accessor property.
The code now sets up a mask each time and applies that instead of
mutating |attributes|.
Please review this at https://codereview.chromium.org/16051002/
SVN Base: https://v8.googlecode.com/svn/branches/bleeding_edge
Affected files:
M src/objects.cc
M test/mjsunit/object-freeze.js
Index: src/objects.cc
diff --git a/src/objects.cc b/src/objects.cc
index
99fd8e21f0a6dfd0ba2019289caf06c770f241a0..c5abb109cd9ff3df0e4d5fb64bf0447638c6421a
100644
--- a/src/objects.cc
+++ b/src/objects.cc
@@ -6744,11 +6744,14 @@ MaybeObject* DescriptorArray::CopyUpToAddAttributes(
for (int i = 0; i < size; ++i) {
Object* value = GetValue(i);
PropertyDetails details = GetDetails(i);
+ int mask = DONT_DELETE | DONT_ENUM;
// READ_ONLY is an invalid attribute for JS setters/getters.
- if (details.type() == CALLBACKS && value->IsAccessorPair()) {
- attributes = static_cast<PropertyAttributes>(attributes &
~READ_ONLY);
+ if (details.type() != CALLBACKS || !value->IsAccessorPair()) {
+ mask |= READ_ONLY;
}
- Descriptor desc(GetKey(i), value,
details.CopyAddAttributes(attributes));
+ details = details.CopyAddAttributes(
+ static_cast<PropertyAttributes>(attributes & mask));
+ Descriptor desc(GetKey(i), value, details);
descriptors->Set(i, &desc, witness);
}
} else {
Index: test/mjsunit/object-freeze.js
diff --git a/test/mjsunit/object-freeze.js b/test/mjsunit/object-freeze.js
index
7b457900aa01b470754d63e817e98db40b4f52a4..a0717a171ca5044a4a404ab2238369f01e13c2ce
100644
--- a/test/mjsunit/object-freeze.js
+++ b/test/mjsunit/object-freeze.js
@@ -296,3 +296,21 @@ assertEquals(42, obj.accessor);
accessorDidRun = false;
obj.accessor = 'ignored value';
assertTrue(accessorDidRun);
+
+// Test for regression in mixed accessor/data property objects.
+// The strict function is one such object.
+assertTrue(Object.isFrozen(Object.freeze(function(){"use strict";})));
+
+// Also test a simpler case
+obj = {};
+Object.defineProperty(obj, 'accessor', {
+ get: function() { return 42 },
+ set: function() { accessorDidRun = true },
+ configurable: true,
+ enumerable: true
+});
+obj.data = 'foo';
+assertTrue(%HasFastProperties(obj));
+Object.freeze(obj);
+assertTrue(%HasFastProperties(obj));
+assertTrue(Object.isFrozen(obj));
--
--
v8-dev mailing list
[email protected]
http://groups.google.com/group/v8-dev
---
You received this message because you are subscribed to the Google Groups "v8-dev" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to [email protected].
For more options, visit https://groups.google.com/groups/opt_out.
