Reviewers: rossberg,
Message:
PTAL. Let me know if you'd like to see a helper for computing is_observed
for
these cases which takes the object and key value.
Description:
Prevent changes to hidden properties from being observable via
Object.observe
This addresses the leak that mstarzinger points out
(https://codereview.chromium.org/26390003/) and includes the test.
Note that this adds a test that observing changes to the empty-string
property
remains possible.
BUG=
Please review this at https://codereview.chromium.org/26592012/
SVN Base: https://v8.googlecode.com/svn/branches/bleeding_edge
Affected files (+46, -7 lines):
M src/objects.cc
M test/cctest/test-object-observe.cc
M test/mjsunit/harmony/object-observe.js
Index: src/objects.cc
diff --git a/src/objects.cc b/src/objects.cc
index
4aef8088860f275126fd9eb2ea7c5365635016fe..2f84975d6fa77d9ecf1e3f0aa9aaa9378027f24f
100644
--- a/src/objects.cc
+++ b/src/objects.cc
@@ -2137,7 +2137,9 @@ Handle<Object> JSObject::AddProperty(Handle<JSObject>
object,
AddSlowProperty(object, name, value, attributes);
}
- if (FLAG_harmony_observation && object->map()->is_observed()) {
+ if (FLAG_harmony_observation &&
+ object->map()->is_observed() &&
+ *name != isolate->heap()->hidden_string()) {
Handle<Object> old_value = isolate->factory()->the_hole_value();
EnqueueChangeRecord(object, "new", name, old_value);
}
@@ -4015,8 +4017,10 @@ Handle<Object>
JSObject::SetPropertyForResult(Handle<JSObject> object,
}
Handle<Object> old_value = isolate->factory()->the_hole_value();
- if (FLAG_harmony_observation &&
- object->map()->is_observed() && lookup->IsDataProperty()) {
+ bool is_observed = FLAG_harmony_observation &&
+ object->map()->is_observed() &&
+ *name != isolate->heap()->hidden_string();
+ if (is_observed && lookup->IsDataProperty()) {
old_value = Object::GetProperty(object, name);
}
@@ -4055,7 +4059,7 @@ Handle<Object>
JSObject::SetPropertyForResult(Handle<JSObject> object,
RETURN_IF_EMPTY_HANDLE_VALUE(isolate, result, Handle<Object>());
- if (FLAG_harmony_observation && object->map()->is_observed()) {
+ if (is_observed) {
if (lookup->IsTransition()) {
EnqueueChangeRecord(object, "new", name, old_value);
} else {
@@ -4156,7 +4160,9 @@ Handle<Object>
JSObject::SetLocalPropertyIgnoreAttributes(
Handle<Object> old_value = isolate->factory()->the_hole_value();
PropertyAttributes old_attributes = ABSENT;
- bool is_observed = FLAG_harmony_observation &&
object->map()->is_observed();
+ bool is_observed = FLAG_harmony_observation &&
+ object->map()->is_observed() &&
+ *name != isolate->heap()->hidden_string();
if (is_observed && lookup.IsProperty()) {
if (lookup.IsDataProperty()) old_value =
Object::GetProperty(object, name);
@@ -5220,7 +5226,9 @@ Handle<Object>
JSObject::DeleteProperty(Handle<JSObject> object,
}
Handle<Object> old_value = isolate->factory()->the_hole_value();
- bool is_observed = FLAG_harmony_observation &&
object->map()->is_observed();
+ bool is_observed = FLAG_harmony_observation &&
+ object->map()->is_observed() &&
+ *name != isolate->heap()->hidden_string();
if (is_observed && lookup.IsDataProperty()) {
old_value = Object::GetProperty(object, name);
}
@@ -6292,7 +6300,9 @@ void JSObject::DefineAccessor(Handle<JSObject> object,
bool is_element = name->AsArrayIndex(&index);
Handle<Object> old_value = isolate->factory()->the_hole_value();
- bool is_observed = FLAG_harmony_observation &&
object->map()->is_observed();
+ bool is_observed = FLAG_harmony_observation &&
+ object->map()->is_observed() &&
+ *name != isolate->heap()->hidden_string();
bool preexists = false;
if (is_observed) {
if (is_element) {
Index: test/cctest/test-object-observe.cc
diff --git a/test/cctest/test-object-observe.cc
b/test/cctest/test-object-observe.cc
index
a9f840e7adc7f0d0b48ed3a600baed1698ce64b2..b4488a603a981a9e420abda0fc00f66e28c4b4de
100644
--- a/test/cctest/test-object-observe.cc
+++ b/test/cctest/test-object-observe.cc
@@ -720,3 +720,18 @@ TEST(AccessCheckDisallowApiModifications) {
}
CHECK(CompileRun("records")->IsNull());
}
+
+
+TEST(HiddenPropertiesLeakage) {
+ HarmonyIsolate isolate;
+ HandleScope scope(isolate.GetIsolate());
+ LocalContext context(isolate.GetIsolate());
+ CompileRun("var obj = {};"
+ "var records = null;"
+ "var observer = function(r) { records = r };"
+ "Object.observe(obj, observer);");
+ Handle<Value> obj = context->Global()->Get(String::New("obj"));
+ Handle<Object>::Cast(obj)->SetHiddenValue(String::New("foo"), Null());
+ CompileRun(""); // trigger delivery
+ CHECK(CompileRun("records")->IsNull());
+}
Index: test/mjsunit/harmony/object-observe.js
diff --git a/test/mjsunit/harmony/object-observe.js
b/test/mjsunit/harmony/object-observe.js
index
f982a66bc428227d2aaf3fb2ae24dc30fc0bcfaf..f94ab75e9a84d2cf0d819510782b23a660057bcc
100644
--- a/test/mjsunit/harmony/object-observe.js
+++ b/test/mjsunit/harmony/object-observe.js
@@ -286,6 +286,20 @@ observer.assertCallbackRecords([
{ object: obj, type: 'new', name: 'id' },
]);
+// The empty-string property is observable
+reset();
+var obj = {};
+Object.observe(obj, observer.callback);
+obj[''] = '';
+obj[''] = ' ';
+delete obj[''];
+Object.deliverChangeRecords(observer.callback);
+observer.assertCallbackRecords([
+ { object: obj, type: 'new', name: '' },
+ { object: obj, type: 'updated', name: '', oldValue: '' },
+ { object: obj, type: 'deleted', name: '', oldValue: ' ' },
+]);
+
// Observing a continuous stream of changes, while itermittantly
unobserving.
reset();
Object.observe(obj, observer.callback);
--
--
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.
