LookupIterator should find private symbols on JSProxies

src/objects.cc

Index: src/objects.cc
diff --git a/src/objects.cc b/src/objects.cc
index 7984a86307b58f7c80b17c38fd0947d3285637fc..b6ea5ff0b37790551ee2c4e000fd691cb5c2c3fe 100644
--- a/src/objects.cc
+++ b/src/objects.cc
@@ -6195,14 +6195,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(
+        Handle<JSObject>::cast(object)->global_dictionary());

[Toon Verwaest, 2016/01/15 14:22:42]

JSObject::cast(*object)->

     DCHECK_NE(GlobalDictionary::kNotFound, entry);
 
     auto cell = PropertyCell::InvalidateEntry(dictionary, entry);
@@ -6232,8 +6233,11 @@ Maybe<bool> JSReceiver::DeleteProperty(LookupIterator* it,
   }
 
   if (it->GetReceiver()->IsJSProxy()) {

[Toon Verwaest, 2016/01/15 14:22:42]

We don't need this branch anymore now right?

[jochen (gone - plz use gerrit), 2016/01/15 14:44:49]

below, we cast the receiver to JSObject which won't work (and the entire loop
assumes it's a proxy)

-    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());
@@ -7078,6 +7082,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()) {
+    AddPrivateProperty(isolate, proxy, Handle<Name>::cast(key), desc);
+    return Just(true);
+  }
   Handle<String> trap_name = isolate->factory()->defineProperty_string();
   // 1. Assert: IsPropertyKey(P) is true.
   DCHECK(key->IsName() || key->IsNumber());
@@ -7114,6 +7122,7 @@ Maybe<bool> JSProxy::DefineOwnProperty(Isolate* isolate, Handle<JSProxy> proxy,
           : Handle<Name>::cast(isolate->factory()->NumberToString(key));
   // Do not leak private property names.
   if (property_name->IsPrivate()) {

[Toon Verwaest, 2016/01/15 14:22:42]

DCHECK(!property_name->IsPrivate()) ?

+    UNREACHABLE();
     RETURN_FAILURE(isolate, should_throw,
                    NewTypeError(MessageTemplate::kProxyPrivate));

[Toon Verwaest, 2016/01/15 14:22:42]

Get rid of kProxyPrivate?

[jochen (gone - plz use gerrit), 2016/01/15 14:44:49]

I still need it (see next patchset). tl;dr builtins could still try to set
symbols in expected ways, and at least the unit tests expect this to thro

   }
@@ -7184,6 +7193,39 @@ Maybe<bool> JSProxy::DefineOwnProperty(Isolate* isolate, Handle<JSProxy> proxy,
 
 
 // static
+void JSProxy::AddPrivateProperty(Isolate* isolate, Handle<JSProxy> proxy,
+                                 Handle<Name> private_name,

[Toon Verwaest, 2016/01/15 14:22:42]

Handle<Symbol> ?

+                                 PropertyDescriptor* desc) {
+  // Despite the generic name, this can only add private data properties.
+  DCHECK(PropertyDescriptor::IsDataDescriptor(desc));
+  DCHECK_EQ(DONT_ENUM, desc->ToAttributes());
+  DCHECK(proxy->map()->is_dictionary_map());
+  Handle<Object> value(
+      desc->has_value()
+          ? desc->value()
+          : Handle<Object>::cast(isolate->factory()->undefined_value()));
+
+  bool success = false;
+  LookupIterator it = LookupIterator::PropertyOrElement(

[Toon Verwaest, 2016/01/15 14:22:42]

LookupIterator it(proxy, private_name); should suffice

+      isolate, proxy, private_name, &success, LookupIterator::HIDDEN);
+  DCHECK(success);
+
+  if (it.IsFound()) {
+    DCHECK_EQ(LookupIterator::DATA, it.state());
+    DCHECK_EQ(DONT_ENUM, it.property_details().attributes());
+    it.WriteDataValue(value);
+    return;
+  }
+
+  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 != *result) proxy->set_properties(*result);

[Toon Verwaest, 2016/01/15 14:22:42]

if (!dict.is_identical_to(result)) proxy->set_properties(*result);

+}
+
+
+// static
 Maybe<bool> JSReceiver::GetOwnPropertyDescriptor(Isolate* isolate,
                                                  Handle<JSReceiver> object,
                                                  Handle<Object> key,