LookupIterator should find private symbols on JSProxies

src/objects.cc

 // Copyright 2015 the V8 project authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #include "src/objects.h"
 
 #include <cmath>
 #include <iomanip>
 #include <sstream>
 
@@ skipping 6177 matching lines @@
   if (result.IsEmpty()) return Nothing<bool>();
 
   DCHECK(result->IsBoolean());
   Handle<Object> result_internal = v8::Utils::OpenHandle(*result);
   result_internal->VerifyApiCallResultType();
   // Rebox CustomArguments::kReturnValueOffset before returning.
   return Just(result_internal->BooleanValue());
 }
 
 
-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);
     cell->set_value(isolate->heap()->the_hole_value());
     // TODO(ishell): InvalidateForDelete
     cell->set_property_details(
         cell->property_details().set_cell_type(PropertyCellType::kInvalidated));
   } else {
     Handle<NameDictionary> dictionary(object->property_dictionary());
     DCHECK_NE(NameDictionary::kNotFound, entry);
 
     NameDictionary::DeleteProperty(dictionary, entry);
     Handle<NameDictionary> new_properties =
         NameDictionary::Shrink(dictionary, name);
     object->set_properties(*new_properties);
   }
 }
 
 
 Maybe<bool> JSReceiver::DeleteProperty(LookupIterator* it,
                                        LanguageMode language_mode) {
   Isolate* isolate = it->isolate();
 
   if (it->state() == LookupIterator::JSPROXY) {
     return JSProxy::DeletePropertyOrElement(it->GetHolder<JSProxy>(),
                                             it->GetName(), language_mode);
   }
 
   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());
 
   bool is_observed =
       receiver->map()->is_observed() &&
       (it->IsElement() || !isolate->IsInternallyUsedPropertyName(it->name()));
 
   Handle<Object> old_value = it->factory()->the_hole_value();
 
@@ skipping 824 matching lines @@
 }
 
 
 // ES6 9.5.6
 // static
 Maybe<bool> JSProxy::DefineOwnProperty(Isolate* isolate, Handle<JSProxy> proxy,
                                        Handle<Object> key,
                                        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());
   // 2. Let handler be the value of the [[ProxyHandler]] internal slot of O.
   Handle<Object> handler(proxy->handler(), isolate);
   // 3. If handler is null, throw a TypeError exception.
   // 4. Assert: Type(handler) is Object.
   if (proxy->IsRevoked()) {
     isolate->Throw(*isolate->factory()->NewTypeError(
         MessageTemplate::kProxyRevoked, trap_name));
@@ skipping 15 matching lines @@
   }
   // 8. Let descObj be FromPropertyDescriptor(Desc).
   Handle<Object> desc_obj = desc->ToObject(isolate);
   // 9. Let booleanTrapResult be
   //    ToBoolean(? Call(trap, handler, «target, P, descObj»)).
   Handle<Name> property_name =
       key->IsName()
           ? Handle<Name>::cast(key)
           : 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

   }
   Handle<Object> trap_result_obj;
   Handle<Object> args[] = {target, property_name, desc_obj};
   ASSIGN_RETURN_ON_EXCEPTION_VALUE(
       isolate, trap_result_obj,
       Execution::Call(isolate, trap, handler, arraysize(args), args),
       Nothing<bool>());
   // 10. If booleanTrapResult is false, return false.
   if (!trap_result_obj->BooleanValue()) {
     RETURN_FAILURE(isolate, should_throw,
@@ skipping 48 matching lines @@
           MessageTemplate::kProxyDefinePropertyNonConfigurable, property_name));
       return Nothing<bool>();
     }
   }
   // 17. Return true.
   return Just(true);
 }
 
 
 // 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,
                                                  PropertyDescriptor* desc) {
   bool success = false;
   DCHECK(key->IsName() || key->IsNumber());  // |key| is a PropertyKey...
   LookupIterator it = LookupIterator::PropertyOrElement(
       isolate, object, key, &success, LookupIterator::HIDDEN);
   DCHECK(success);  // ...so creating a LookupIterator can't fail.
   return GetOwnPropertyDescriptor(&it, desc);
@@ skipping 12493 matching lines @@
   if (cell->value() != *new_value) {
     cell->set_value(*new_value);
     Isolate* isolate = cell->GetIsolate();
     cell->dependent_code()->DeoptimizeDependentCodeGroup(
         isolate, DependentCode::kPropertyCellChangedGroup);
   }
 }
 
 }  // namespace internal
 }  // namespace v8