LookupIterator should find private symbols on JSProxies

BUG=chromium:571365
R=verwaest@chromium.org,neis@chromium.org
LOG=n

Review URL: https://codereview.chromium.org/1587633002

Cr-Commit-Position: refs/heads/master@{#33358}

src/lookup.cc

@@ -270,23 +270,27 @@ void LookupIterator::ApplyTransitionToDataProperty() {
 
 
 void LookupIterator::Delete() {
-  Handle<JSObject> holder = Handle<JSObject>::cast(holder_);
+  Handle<JSReceiver> holder = Handle<JSReceiver>::cast(holder_);
   if (IsElement()) {
-    ElementsAccessor* accessor = holder->GetElementsAccessor();
-    accessor->Delete(holder, number_);
+    Handle<JSObject> object = Handle<JSObject>::cast(holder);
+    ElementsAccessor* accessor = object->GetElementsAccessor();
+    accessor->Delete(object, number_);
   } else {
     PropertyNormalizationMode mode = holder->map()->is_prototype_map()
                                          ? KEEP_INOBJECT_PROPERTIES
                                          : CLEAR_INOBJECT_PROPERTIES;
 
     if (holder->HasFastProperties()) {
-      JSObject::NormalizeProperties(holder, mode, 0, "DeletingProperty");
+      JSObject::NormalizeProperties(Handle<JSObject>::cast(holder), mode, 0,
+                                    "DeletingProperty");
       holder_map_ = handle(holder->map(), isolate_);
       ReloadPropertyInformation();
     }
     // TODO(verwaest): Get rid of the name_ argument.
-    JSObject::DeleteNormalizedProperty(holder, name_, number_);
-    JSObject::ReoptimizeIfPrototype(holder);
+    JSReceiver::DeleteNormalizedProperty(holder, name_, number_);
+    if (holder->IsJSObject()) {
+      JSObject::ReoptimizeIfPrototype(Handle<JSObject>::cast(holder));
+    }
   }
 }
 
@@ -413,8 +417,8 @@ bool LookupIterator::InternalHolderIsReceiverOrHiddenPrototype() const {
 
 Handle<Object> LookupIterator::FetchValue() const {
   Object* result = NULL;
-  Handle<JSObject> holder = GetHolder<JSObject>();
   if (IsElement()) {
+    Handle<JSObject> holder = GetHolder<JSObject>();
     // TODO(verwaest): Optimize.
     if (holder->IsStringObjectWithCharacterAt(index_)) {
       Handle<JSValue> js_value = Handle<JSValue>::cast(holder);
@@ -426,12 +430,14 @@ Handle<Object> LookupIterator::FetchValue() const {
     ElementsAccessor* accessor = holder->GetElementsAccessor();
     return accessor->Get(handle(holder->elements()), number_);
   } else if (holder_map_->IsJSGlobalObjectMap()) {
+    Handle<JSObject> holder = GetHolder<JSObject>();
     result = holder->global_dictionary()->ValueAt(number_);
     DCHECK(result->IsPropertyCell());
     result = PropertyCell::cast(result)->value();
   } else if (holder_map_->is_dictionary_map()) {
-    result = holder->property_dictionary()->ValueAt(number_);
+    result = holder_->property_dictionary()->ValueAt(number_);
   } else if (property_details_.type() == v8::internal::DATA) {
+    Handle<JSObject> holder = GetHolder<JSObject>();
     FieldIndex field_index = FieldIndex::ForDescriptor(*holder_map_, number_);
     return JSObject::FastPropertyAt(holder, property_details_.representation(),
                                     field_index);
@@ -506,20 +512,21 @@ Handle<Object> LookupIterator::GetDataValue() const {
 
 void LookupIterator::WriteDataValue(Handle<Object> value) {
   DCHECK_EQ(DATA, state_);
-  Handle<JSObject> holder = GetHolder<JSObject>();
+  Handle<JSReceiver> holder = GetHolder<JSReceiver>();
   if (IsElement()) {
-    ElementsAccessor* accessor = holder->GetElementsAccessor();
-    accessor->Set(holder->elements(), number_, *value);
+    Handle<JSObject> object = Handle<JSObject>::cast(holder);
+    ElementsAccessor* accessor = object->GetElementsAccessor();
+    accessor->Set(object->elements(), number_, *value);
   } else if (holder->IsJSGlobalObject()) {
     Handle<GlobalDictionary> property_dictionary =
-        handle(holder->global_dictionary());
+        handle(JSObject::cast(*holder)->global_dictionary());
     PropertyCell::UpdateCell(property_dictionary, dictionary_entry(), value,
                              property_details_);
   } else if (holder_map_->is_dictionary_map()) {
     NameDictionary* property_dictionary = holder->property_dictionary();
     property_dictionary->ValueAtPut(dictionary_entry(), *value);
   } else if (property_details_.type() == v8::internal::DATA) {
-    holder->WriteToField(descriptor_number(), *value);
+    JSObject::cast(*holder)->WriteToField(descriptor_number(), *value);
   } else {
     DCHECK_EQ(v8::internal::DATA_CONSTANT, property_details_.type());
   }
@@ -611,8 +618,7 @@ LookupIterator::State LookupIterator::LookupInHolder(Map* const map,
     case NOT_FOUND:
       if (map->IsJSProxyMap()) {
         // Do not leak private property names.
-        if (!name_.is_null() && name_->IsPrivate()) return NOT_FOUND;
-        return JSPROXY;
+        if (IsElement() || !name_->IsPrivate()) return JSPROXY;
       }
       if (map->is_access_check_needed() &&
           (IsElement() || !isolate_->IsInternallyUsedPropertyName(name_))) {
@@ -668,7 +674,7 @@ LookupIterator::State LookupIterator::LookupInHolder(Map* const map,
         if (cell->value()->IsTheHole()) return NOT_FOUND;
         property_details_ = cell->property_details();
       } else {
-        NameDictionary* dict = JSObject::cast(holder)->property_dictionary();
+        NameDictionary* dict = holder->property_dictionary();
         int number = dict->FindEntry(name_);
         if (number == NameDictionary::kNotFound) return NOT_FOUND;
         number_ = static_cast<uint32_t>(number);

src/objects.cc

@@ -4158,16 +4158,16 @@ Maybe<bool> Object::SetPropertyInternal(LookupIterator* it,
 Maybe<bool> Object::SetProperty(LookupIterator* it, Handle<Object> value,
                                 LanguageMode language_mode,
                                 StoreFromKeyed store_mode) {
-  bool found = false;
-  Maybe<bool> result =
-      SetPropertyInternal(it, value, language_mode, store_mode, &found);
-  if (found) return result;
   ShouldThrow should_throw =
       is_sloppy(language_mode) ? DONT_THROW : THROW_ON_ERROR;
   if (it->GetReceiver()->IsJSProxy() && it->GetName()->IsPrivate()) {
     RETURN_FAILURE(it->isolate(), should_throw,
                    NewTypeError(MessageTemplate::kProxyPrivate));
   }
+  bool found = false;
+  Maybe<bool> result =
+      SetPropertyInternal(it, value, language_mode, store_mode, &found);
+  if (found) return result;
   return AddDataProperty(it, value, NONE, should_throw, store_mode);
 }
 
@@ -4178,15 +4178,15 @@ Maybe<bool> Object::SetSuperProperty(LookupIterator* it, Handle<Object> value,
   ShouldThrow should_throw =
       is_sloppy(language_mode) ? DONT_THROW : THROW_ON_ERROR;
   Isolate* isolate = it->isolate();
+  if (it->GetReceiver()->IsJSProxy() && it->GetName()->IsPrivate()) {
+    RETURN_FAILURE(isolate, should_throw,
+                   NewTypeError(MessageTemplate::kProxyPrivate));
+  }
 
   bool found = false;
   Maybe<bool> result =
       SetPropertyInternal(it, value, language_mode, store_mode, &found);
   if (found) return result;
-  if (it->GetReceiver()->IsJSProxy() && it->GetName()->IsPrivate()) {
-    RETURN_FAILURE(isolate, should_throw,
-                   NewTypeError(MessageTemplate::kProxyPrivate));
-  }
 
   // The property either doesn't exist on the holder or exists there as a data
   // property.
@@ -6196,14 +6196,15 @@ Maybe<bool> JSObject::DeletePropertyWithInterceptor(LookupIterator* it) {
 }
 
 
-void JSObject::DeleteNormalizedProperty(Handle<JSObject> object,
-                                        Handle<Name> name, int entry) {
+void JSReceiver::DeleteNormalizedProperty(Handle<JSReceiver> object,
+                                          Handle<Name> name, int entry) {
   DCHECK(!object->HasFastProperties());
   Isolate* isolate = object->GetIsolate();
 
   if (object->IsJSGlobalObject()) {
     // If we have a global object, invalidate the cell and swap in a new one.
-    Handle<GlobalDictionary> dictionary(object->global_dictionary());
+    Handle<GlobalDictionary> dictionary(
+        JSObject::cast(*object)->global_dictionary());
     DCHECK_NE(GlobalDictionary::kNotFound, entry);
 
     auto cell = PropertyCell::InvalidateEntry(dictionary, entry);
@@ -6233,8 +6234,11 @@ Maybe<bool> JSReceiver::DeleteProperty(LookupIterator* it,
   }
 
   if (it->GetReceiver()->IsJSProxy()) {
-    DCHECK(it->state() == LookupIterator::NOT_FOUND);
-    DCHECK(it->GetName()->IsPrivate());
+    if (it->state() != LookupIterator::NOT_FOUND) {
+      DCHECK_EQ(LookupIterator::DATA, it->state());
+      DCHECK(it->GetName()->IsPrivate());
+      it->Delete();
+    }
     return Just(true);
   }
   Handle<JSObject> receiver = Handle<JSObject>::cast(it->GetReceiver());
@@ -7079,6 +7083,10 @@ Maybe<bool> JSProxy::DefineOwnProperty(Isolate* isolate, Handle<JSProxy> proxy,
                                        PropertyDescriptor* desc,
                                        ShouldThrow should_throw) {
   STACK_CHECK(Nothing<bool>());
+  if (key->IsSymbol() && Handle<Symbol>::cast(key)->IsPrivate()) {
+    return AddPrivateProperty(isolate, proxy, Handle<Symbol>::cast(key), desc,
+                              should_throw);
+  }
   Handle<String> trap_name = isolate->factory()->defineProperty_string();
   // 1. Assert: IsPropertyKey(P) is true.
   DCHECK(key->IsName() || key->IsNumber());
@@ -7114,10 +7122,7 @@ Maybe<bool> JSProxy::DefineOwnProperty(Isolate* isolate, Handle<JSProxy> proxy,
           ? Handle<Name>::cast(key)
           : Handle<Name>::cast(isolate->factory()->NumberToString(key));
   // Do not leak private property names.
-  if (property_name->IsPrivate()) {
-    RETURN_FAILURE(isolate, should_throw,
-                   NewTypeError(MessageTemplate::kProxyPrivate));
-  }
+  DCHECK(!property_name->IsPrivate());
   Handle<Object> trap_result_obj;
   Handle<Object> args[] = {target, property_name, desc_obj};
   ASSIGN_RETURN_ON_EXCEPTION_VALUE(
@@ -7184,6 +7189,41 @@ Maybe<bool> JSProxy::DefineOwnProperty(Isolate* isolate, Handle<JSProxy> proxy,
 }
 
 
+// static
+Maybe<bool> JSProxy::AddPrivateProperty(Isolate* isolate, Handle<JSProxy> proxy,
+                                        Handle<Symbol> private_name,
+                                        PropertyDescriptor* desc,
+                                        ShouldThrow should_throw) {
+  // Despite the generic name, this can only add private data properties.
+  if (!PropertyDescriptor::IsDataDescriptor(desc) ||
+      desc->ToAttributes() != DONT_ENUM) {
+    RETURN_FAILURE(isolate, should_throw,
+                   NewTypeError(MessageTemplate::kProxyPrivate));
+  }
+  DCHECK(proxy->map()->is_dictionary_map());
+  Handle<Object> value =
+      desc->has_value()
+          ? desc->value()
+          : Handle<Object>::cast(isolate->factory()->undefined_value());
+
+  LookupIterator it(proxy, private_name);
+
+  if (it.IsFound()) {
+    DCHECK_EQ(LookupIterator::DATA, it.state());
+    DCHECK_EQ(DONT_ENUM, it.property_details().attributes());
+    it.WriteDataValue(value);
+    return Just(true);
+  }
+
+  Handle<NameDictionary> dict(proxy->property_dictionary());
+  PropertyDetails details(DONT_ENUM, DATA, 0, PropertyCellType::kNoCell);
+  Handle<NameDictionary> result =
+      NameDictionary::Add(dict, private_name, value, details);
+  if (!dict.is_identical_to(result)) proxy->set_properties(*result);
+  return Just(true);
+}
+
+
 // static
 Maybe<bool> JSReceiver::GetOwnPropertyDescriptor(Isolate* isolate,
                                                  Handle<JSReceiver> object,

src/objects.h

@@ -1811,6 +1811,10 @@ class JSReceiver: public HeapObject {
   // Gets slow properties for non-global objects.
   inline NameDictionary* property_dictionary();
 
+  // Deletes an existing named property in a normalized object.
+  static void DeleteNormalizedProperty(Handle<JSReceiver> object,
+                                       Handle<Name> name, int entry);
+
   DECLARE_CAST(JSReceiver)
 
   // ES6 section 7.1.1 ToPrimitive
@@ -2494,10 +2498,6 @@ class JSObject: public JSReceiver {
   // Gets the number of currently used elements.
   int GetFastElementsUsage();
 
-  // Deletes an existing named property in a normalized object.
-  static void DeleteNormalizedProperty(Handle<JSObject> object,
-                                       Handle<Name> name, int entry);
-
   static bool AllCanRead(LookupIterator* it);
   static bool AllCanWrite(LookupIterator* it);
 
@@ -9641,6 +9641,11 @@ class JSProxy: public JSReceiver {
   static Handle<Smi> GetOrCreateIdentityHash(Handle<JSProxy> proxy);
 
  private:
+  static Maybe<bool> AddPrivateProperty(Isolate* isolate, Handle<JSProxy> proxy,
+                                        Handle<Symbol> private_name,
+                                        PropertyDescriptor* desc,
+                                        ShouldThrow should_throw);
+
   DISALLOW_IMPLICIT_CONSTRUCTORS(JSProxy);
 };
 

test/cctest/test-api.cc

@@ -2738,6 +2738,106 @@ THREADED_TEST(SymbolTemplateProperties) {
 }
 
 
+THREADED_TEST(PrivatePropertiesOnProxies) {
+  i::FLAG_harmony_proxies = true;
+  LocalContext env;
+  v8::Isolate* isolate = env->GetIsolate();
+  v8::HandleScope scope(isolate);
+
+  v8::Local<v8::Object> target = CompileRun("({})").As<v8::Object>();
+  v8::Local<v8::Object> handler = CompileRun("({})").As<v8::Object>();
+
+  v8::Local<v8::Proxy> proxy =
+      v8::Proxy::New(env.local(), target, handler).ToLocalChecked();
+
+  v8::Local<v8::Private> priv1 = v8::Private::New(isolate);
+  v8::Local<v8::Private> priv2 =
+      v8::Private::New(isolate, v8_str("my-private"));
+
+  CcTest::heap()->CollectAllGarbage();
+
+  CHECK(priv2->Name()
+            ->Equals(env.local(),
+                     v8::String::NewFromUtf8(isolate, "my-private",
+                                             v8::NewStringType::kNormal)
+                         .ToLocalChecked())
+            .FromJust());
+
+  // Make sure delete of a non-existent private symbol property works.
+  proxy->DeletePrivate(env.local(), priv1).FromJust();
+  CHECK(!proxy->HasPrivate(env.local(), priv1).FromJust());
+
+  CHECK(proxy->SetPrivate(env.local(), priv1, v8::Integer::New(isolate, 1503))
+            .FromJust());
+  CHECK(proxy->HasPrivate(env.local(), priv1).FromJust());
+  CHECK_EQ(1503, proxy->GetPrivate(env.local(), priv1)
+                     .ToLocalChecked()
+                     ->Int32Value(env.local())
+                     .FromJust());
+  CHECK(proxy->SetPrivate(env.local(), priv1, v8::Integer::New(isolate, 2002))
+            .FromJust());
+  CHECK(proxy->HasPrivate(env.local(), priv1).FromJust());
+  CHECK_EQ(2002, proxy->GetPrivate(env.local(), priv1)
+                     .ToLocalChecked()
+                     ->Int32Value(env.local())
+                     .FromJust());
+
+  CHECK_EQ(0u,
+           proxy->GetOwnPropertyNames(env.local()).ToLocalChecked()->Length());
+  unsigned num_props =
+      proxy->GetPropertyNames(env.local()).ToLocalChecked()->Length();
+  CHECK(proxy->Set(env.local(), v8::String::NewFromUtf8(
+                                    isolate, "bla", v8::NewStringType::kNormal)
+                                    .ToLocalChecked(),
+                   v8::Integer::New(isolate, 20))
+            .FromJust());
+  CHECK_EQ(1u,
+           proxy->GetOwnPropertyNames(env.local()).ToLocalChecked()->Length());
+  CHECK_EQ(num_props + 1,
+           proxy->GetPropertyNames(env.local()).ToLocalChecked()->Length());
+
+  CcTest::heap()->CollectAllGarbage();
+
+  // Add another property and delete it afterwards to force the object in
+  // slow case.
+  CHECK(proxy->SetPrivate(env.local(), priv2, v8::Integer::New(isolate, 2008))
+            .FromJust());
+  CHECK_EQ(2002, proxy->GetPrivate(env.local(), priv1)
+                     .ToLocalChecked()
+                     ->Int32Value(env.local())
+                     .FromJust());
+  CHECK_EQ(2008, proxy->GetPrivate(env.local(), priv2)
+                     .ToLocalChecked()
+                     ->Int32Value(env.local())
+                     .FromJust());
+  CHECK_EQ(2002, proxy->GetPrivate(env.local(), priv1)
+                     .ToLocalChecked()
+                     ->Int32Value(env.local())
+                     .FromJust());
+  CHECK_EQ(1u,
+           proxy->GetOwnPropertyNames(env.local()).ToLocalChecked()->Length());
+
+  CHECK(proxy->HasPrivate(env.local(), priv1).FromJust());
+  CHECK(proxy->HasPrivate(env.local(), priv2).FromJust());
+  CHECK(proxy->DeletePrivate(env.local(), priv2).FromJust());
+  CHECK(proxy->HasPrivate(env.local(), priv1).FromJust());
+  CHECK(!proxy->HasPrivate(env.local(), priv2).FromJust());
+  CHECK_EQ(2002, proxy->GetPrivate(env.local(), priv1)
+                     .ToLocalChecked()
+                     ->Int32Value(env.local())
+                     .FromJust());
+  CHECK_EQ(1u,
+           proxy->GetOwnPropertyNames(env.local()).ToLocalChecked()->Length());
+
+  // Private properties are not inherited (for the time being).
+  v8::Local<v8::Object> child = v8::Object::New(isolate);
+  CHECK(child->SetPrototype(env.local(), proxy).FromJust());
+  CHECK(!child->HasPrivate(env.local(), priv1).FromJust());
+  CHECK_EQ(0u,
+           child->GetOwnPropertyNames(env.local()).ToLocalChecked()->Length());
+}
+
+
 THREADED_TEST(PrivateProperties) {
   LocalContext env;
   v8::Isolate* isolate = env->GetIsolate();