From e3fa6d67ca6f6447797123e52b28d82f2f77da16 Mon Sep 17 00:00:00 2001 From: "jochen@chromium.org" Date: Fri, 3 Jan 2014 11:19:13 +0000 Subject: [PATCH] Avoid duplication of a hidden & inherited prototype's properties. In Runtime_GetLocalPropertyNames(), the hidden prototypes of an object are also consulted when deriving the property name set. However, if given a function object and its template was inherited from the template of one of its hidden prototypes, that hidden prototype's property accessors will be present on the object already. Unwanted duplicates will therefore appear. Hence, go through the property names that the hidden prototypes contribute and remove any already occurring ones. Assumed to be a rare constellation, so the cost of this extra pass is considered acceptable. LOG=N R=dcarney@chromium.org, jochen@chromium.org, rossberg@chromium.org BUG=269562 Review URL: https://codereview.chromium.org/116533003 Patch from Sigbjorn Finne . git-svn-id: http://v8.googlecode.com/svn/branches/bleeding_edge@18448 ce2b1a6d-e550-0410-aec6-3dcde31c8c00 --- src/runtime.cc | 33 +++++++++++++++++++++++++----- test/cctest/test-api.cc | 54 +++++++++++++++++++++++++++++++++++++++++++++++++ 2 files changed, 82 insertions(+), 5 deletions(-) diff --git a/src/runtime.cc b/src/runtime.cc index a175836..713d450 100644 --- a/src/runtime.cc +++ b/src/runtime.cc @@ -5801,32 +5801,55 @@ RUNTIME_FUNCTION(MaybeObject*, Runtime_GetLocalPropertyNames) { // Get the property names. jsproto = obj; - int proto_with_hidden_properties = 0; int next_copy_index = 0; + int hidden_strings = 0; for (int i = 0; i < length; i++) { jsproto->GetLocalPropertyNames(*names, next_copy_index, filter); + if (i > 0) { + // Names from hidden prototypes may already have been added + // for inherited function template instances. Count the duplicates + // and stub them out; the final copy pass at the end ignores holes. + for (int j = next_copy_index; + j < next_copy_index + local_property_count[i]; + j++) { + Object* name_from_hidden_proto = names->get(j); + for (int k = 0; k < next_copy_index; k++) { + if (names->get(k) != isolate->heap()->hidden_string()) { + Object* name = names->get(k); + if (name_from_hidden_proto == name) { + names->set(j, isolate->heap()->hidden_string()); + hidden_strings++; + break; + } + } + } + } + } next_copy_index += local_property_count[i]; if (jsproto->HasHiddenProperties()) { - proto_with_hidden_properties++; + hidden_strings++; } if (i < length - 1) { jsproto = Handle(JSObject::cast(jsproto->GetPrototype())); } } - // Filter out name of hidden properties object. - if (proto_with_hidden_properties > 0) { + // Filter out name of hidden properties object and + // hidden prototype duplicates. + if (hidden_strings > 0) { Handle old_names = names; names = isolate->factory()->NewFixedArray( - names->length() - proto_with_hidden_properties); + names->length() - hidden_strings); int dest_pos = 0; for (int i = 0; i < total_property_count; i++) { Object* name = old_names->get(i); if (name == isolate->heap()->hidden_string()) { + hidden_strings--; continue; } names->set(dest_pos++, name); } + ASSERT_EQ(0, hidden_strings); } return *isolate->factory()->NewJSArrayWithElements(names); diff --git a/test/cctest/test-api.cc b/test/cctest/test-api.cc index 7e699af..916a1d7 100644 --- a/test/cctest/test-api.cc +++ b/test/cctest/test-api.cc @@ -10063,6 +10063,60 @@ THREADED_TEST(Regress91517) { } +// Getting property names of an object with a hidden and inherited +// prototype should not duplicate the accessor properties inherited. +THREADED_TEST(Regress269562) { + i::FLAG_allow_natives_syntax = true; + LocalContext context; + v8::HandleScope handle_scope(context->GetIsolate()); + + Local t1 = + v8::FunctionTemplate::New(context->GetIsolate()); + t1->SetHiddenPrototype(true); + + Local i1 = t1->InstanceTemplate(); + i1->SetAccessor(v8_str("foo"), + SimpleAccessorGetter, SimpleAccessorSetter); + i1->SetAccessor(v8_str("bar"), + SimpleAccessorGetter, SimpleAccessorSetter); + i1->SetAccessor(v8_str("baz"), + SimpleAccessorGetter, SimpleAccessorSetter); + i1->Set(v8_str("n1"), v8_num(1)); + i1->Set(v8_str("n2"), v8_num(2)); + + Local o1 = t1->GetFunction()->NewInstance(); + Local t2 = + v8::FunctionTemplate::New(context->GetIsolate()); + t2->SetHiddenPrototype(true); + + // Inherit from t1 and mark prototype as hidden. + t2->Inherit(t1); + t2->InstanceTemplate()->Set(v8_str("mine"), v8_num(4)); + + Local o2 = t2->GetFunction()->NewInstance(); + CHECK(o2->SetPrototype(o1)); + + v8::Local sym = v8::Symbol::New(context->GetIsolate(), "s1"); + o1->Set(sym, v8_num(3)); + o1->SetHiddenValue(v8_str("h1"), v8::Integer::New(2013)); + + // Call the runtime version of GetLocalPropertyNames() on + // the natively created object through JavaScript. + context->Global()->Set(v8_str("obj"), o2); + context->Global()->Set(v8_str("sym"), sym); + CompileRun("var names = %GetLocalPropertyNames(obj, true);"); + + ExpectInt32("names.length", 7); + ExpectTrue("names.indexOf(\"foo\") >= 0"); + ExpectTrue("names.indexOf(\"bar\") >= 0"); + ExpectTrue("names.indexOf(\"baz\") >= 0"); + ExpectTrue("names.indexOf(\"n1\") >= 0"); + ExpectTrue("names.indexOf(\"n2\") >= 0"); + ExpectTrue("names.indexOf(sym) >= 0"); + ExpectTrue("names.indexOf(\"mine\") >= 0"); +} + + THREADED_TEST(FunctionReadOnlyPrototype) { LocalContext context; v8::Isolate* isolate = context->GetIsolate(); -- 2.7.4