[proxies] Implement [[PreventExtensions]] and [[IsExtensible]].

src/objects.cc

 // Copyright 2013 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 888 matching lines @@
     THROW_NEW_ERROR(isolate,
                     NewTypeError(MessageTemplate::kProxyHandlerTrapMissing,
                                  handler, trap_name),
                     Object);
   }
   // 13. Return handlerProto.
   return handler_proto;
 }
 
 
+bool JSProxy::IsRevoked(Handle<JSProxy> proxy) {
+  // TODO(neis): Decide on how to represent revocation.  For now, revocation is

[Jakob Kummerow, 2015/11/17 12:12:35]

The spec for various internal methods keeps checking for handler/target being
null, which IIUC is the case after revocation. So how about the following
implementation:

DCHECK(proxy->handler()->IsNull() == proxy->target()->IsNull());
return proxy->handler()->IsNull();

+  // unsupported.
+  DCHECK(proxy->target()->IsJSReceiver());
+  DCHECK(proxy->handler()->IsJSReceiver());
+  return false;
+}
+
+
 MaybeHandle<Object> JSProxy::GetPropertyWithHandler(Handle<JSProxy> proxy,
                                                     Handle<Object> receiver,
                                                     Handle<Name> name) {
   Isolate* isolate = proxy->GetIsolate();
 
   // TODO(rossberg): adjust once there is a story for symbols vs proxies.
   if (name->IsSymbol()) return isolate->factory()->undefined_value();
 
   Handle<Object> args[] = { receiver, name };
   return CallTrap(
@@ skipping 3810 matching lines @@
   }
 
   int attributes = NONE;
   if (!enumerable->BooleanValue()) attributes |= DONT_ENUM;
   if (!configurable->BooleanValue()) attributes |= DONT_DELETE;
   if (!writable->BooleanValue()) attributes |= READ_ONLY;
   return Just(static_cast<PropertyAttributes>(attributes));
 }
 
 
+MaybeHandle<Object> JSProxy::GetTrap(Handle<JSProxy> proxy,
+                                     Handle<String> trap) {
+  DCHECK(!IsRevoked(proxy));
+  Isolate* isolate = proxy->GetIsolate();

[Camillo Bruni, 2015/11/17 08:57:17]

I would pass the isolate along, since we kept it in a local variable in all the
trap methods so far.

+  Handle<JSReceiver> handler(JSReceiver::cast(proxy->handler()), isolate);
+  return Object::GetMethod(handler, trap);
+}
+
+
 MaybeHandle<Object> JSProxy::CallTrap(Handle<JSProxy> proxy,
                                       const char* name,
                                       Handle<Object> derived,
                                       int argc,
                                       Handle<Object> argv[]) {
   Isolate* isolate = proxy->GetIsolate();
   Handle<Object> handler(proxy->handler(), isolate);
-
   Handle<String> trap_name = isolate->factory()->InternalizeUtf8String(name);
   Handle<Object> trap;
-  ASSIGN_RETURN_ON_EXCEPTION(
-      isolate, trap,
-      Object::GetPropertyOrElement(handler, trap_name),
-      Object);
+  ASSIGN_RETURN_ON_EXCEPTION(isolate, trap, GetTrap(proxy, trap_name), Object);
 
   if (trap->IsUndefined()) {
     if (derived.is_null()) {
       THROW_NEW_ERROR(isolate,
                       NewTypeError(MessageTemplate::kProxyHandlerTrapMissing,
                                    handler, trap_name),
                       Object);
     }
     trap = Handle<Object>(derived);
   }
@@ skipping 2266 matching lines @@
     }
   }
 
   // No references to object.
   return false;
 }
 
 
 Maybe<bool> JSReceiver::PreventExtensions(Handle<JSReceiver> object,
                                           ShouldThrow should_throw) {
-  if (!object->IsJSObject()) return Just(false);
-  // TODO(neis): Deal with proxies.
+  if (object->IsJSProxy()) {
+    return JSProxy::PreventExtensions(Handle<JSProxy>::cast(object),
+                                      should_throw);
+  }
+  DCHECK(object->IsJSObject());
   return JSObject::PreventExtensions(Handle<JSObject>::cast(object),
                                      should_throw);
 }
 
 

[Camillo Bruni, 2015/11/17 08:57:16]

Please add a tiny header:
// ES6 9.5.?
// static

[neis, 2015/11/17 23:19:03]

Added spec reference to the header file.

+Maybe<bool> JSProxy::PreventExtensions(Handle<JSProxy> proxy,
+                                       ShouldThrow should_throw) {

[Camillo Bruni, 2015/11/17 08:57:17]

Could you copy in the spec steps? (see the other proxy traps)
We did that to make sure that we can easily compare the steps if they change in
the future.

[neis, 2015/11/17 23:19:03]

I don't know what others think about this, but for me copying the spec into the
code makes it harder to read the code because because it's just so cluttered.  I
prefer to have the spec in a separate window.

[Jakob Kummerow, 2015/11/18 13:58:27]

I'm strongly in favor of adding the spec steps as comments, for the following
reasons:

(1) Doing so makes it easier to verify that the code is correct, 
  (a) while writing it [you may disagree],
  (b) when reviewing it,
  (c) when chasing bugs months later.

(2) It documents why the code does what it does, in particular distinguishes
implementation details that can be refactored at will vs. behavior that's
mandated by the spec. Side effects:
  (a) forces us to be strict about following the spec step by step; any
shortcuts or abstractions or convenience helpers are easy to find and their
validity is easy to reason about
  (b) helps prevent growth of a big mess of similarly named functions that
implement various parts or variants of a spec'ed algorithm (which we've seen
happening way too often)

(3) As the spec evolves, it documents clearly what semantics we've implemented.
  (a) This helps figure out what's going on when bugs are filed in the future
claiming spec violations.
  (b) This also makes it easier to proactively find code that needs updating
after spec changes (by comparing the comments with the new spec).

So yeah, please add those comments :-)

+  Isolate* isolate = proxy->GetIsolate();
+  Factory* factory = isolate->factory();
+  Handle<String> trap_name = factory->preventExtensions_string();
+
+  if (IsRevoked(proxy)) {

[Camillo Bruni, 2015/11/17 08:57:17]

nice ;) we were too lazy so far to introduce this helper.

+    isolate->Throw(
+        *factory->NewTypeError(MessageTemplate::kProxyRevoked, trap_name));
+    return Nothing<bool>();
+  }
+
+  Handle<JSReceiver> target(JSReceiver::cast(proxy->target()), isolate);
+  Handle<JSReceiver> handler(JSReceiver::cast(proxy->handler()), isolate);
+
+  Handle<Object> trap;
+  ASSIGN_RETURN_ON_EXCEPTION_VALUE(isolate, trap, GetTrap(proxy, trap_name),
+                                   Nothing<bool>());
+  if (trap->IsUndefined()) {
+    return JSReceiver::PreventExtensions(target, should_throw);
+  }
+
+  Handle<Object> trap_result;
+  Handle<Object> args[] = {target};
+  ASSIGN_RETURN_ON_EXCEPTION_VALUE(
+      isolate, trap_result,
+      Execution::Call(isolate, trap, handler, arraysize(args), args),
+      Nothing<bool>());
+  if (!trap_result->BooleanValue()) {
+    RETURN_FAILURE(isolate, should_throw,
+                   NewTypeError(MessageTemplate::kProxyHandlerReturned, handler,
+                                factory->false_string(),
+                                factory->preventExtensions_string()));

[Camillo Bruni, 2015/11/17 08:57:16]

you could use trap_name here.

[neis, 2015/11/17 23:19:02]

Done.

+  }
+
+  // Enforce the invariant.
+  Maybe<bool> target_result = JSReceiver::IsExtensible(target);
+  MAYBE_RETURN(target_result, Nothing<bool>());
+  if (target_result.FromJust()) {
+    isolate->Throw(*factory->NewTypeError(
+        MessageTemplate::kProxyPreventExtensionsViolatesInvariant));
+    return Nothing<bool>();
+  }
+  return Just(true);
+}
+
+
 Maybe<bool> JSObject::PreventExtensions(Handle<JSObject> object,
                                         ShouldThrow should_throw) {
   Isolate* isolate = object->GetIsolate();
 
   if (!object->HasSloppyArgumentsElements() && !object->map()->is_observed()) {
     return PreventExtensionsWithTransition<NONE>(object, should_throw);
   }
 
   if (object->IsAccessCheckNeeded() &&
       !isolate->MayAccess(handle(isolate->context()), object)) {
@@ skipping 37 matching lines @@
     RETURN_ON_EXCEPTION_VALUE(
         isolate,
         EnqueueChangeRecord(object, "preventExtensions", Handle<Name>(),
                             isolate->factory()->the_hole_value()),
         Nothing<bool>());
   }
   return Just(true);
 }
 
 
-// static
 Maybe<bool> JSReceiver::IsExtensible(Handle<JSReceiver> object) {
   if (object->IsJSProxy()) {
-    // TODO(neis,cbruni): Redirect to the trap on JSProxy.
-    return Just(true);
+    return JSProxy::IsExtensible(Handle<JSProxy>::cast(object));
   }
+  DCHECK(object->IsJSObject());

[rossberg, 2015/11/17 13:29:09]

Nit: this check is redundant, since it's implied by the cast below.

[neis, 2015/11/17 23:19:03]

Done.

   return Just(JSObject::IsExtensible(Handle<JSObject>::cast(object)));
 }
 
 

[Camillo Bruni, 2015/11/17 08:57:16]

ditto: missing spec chapter comment.

[neis, 2015/11/17 23:19:03]

Done.

+Maybe<bool> JSProxy::IsExtensible(Handle<JSProxy> proxy) {

[Camillo Bruni, 2015/11/17 08:57:17]

ditto: spec steps.

+  Isolate* isolate = proxy->GetIsolate();
+  Factory* factory = isolate->factory();
+  Handle<String> trap_name = factory->isExtensible_string();
+
+  if (IsRevoked(proxy)) {
+    isolate->Throw(
+        *factory->NewTypeError(MessageTemplate::kProxyRevoked, trap_name));
+    return Nothing<bool>();
+  }
+
+  Handle<JSReceiver> target(JSReceiver::cast(proxy->target()), isolate);
+  Handle<JSReceiver> handler(JSReceiver::cast(proxy->handler()), isolate);
+
+  Handle<Object> trap;
+  ASSIGN_RETURN_ON_EXCEPTION_VALUE(isolate, trap, GetTrap(proxy, trap_name),
+                                   Nothing<bool>());
+  if (trap->IsUndefined()) {
+    return JSReceiver::IsExtensible(target);
+  }
+
+  Handle<Object> trap_result;
+  Handle<Object> args[] = {target};
+  ASSIGN_RETURN_ON_EXCEPTION_VALUE(
+      isolate, trap_result,
+      Execution::Call(isolate, trap, handler, arraysize(args), args),
+      Nothing<bool>());
+
+  // Enforce the invariant.
+  Maybe<bool> target_result = JSReceiver::IsExtensible(target);
+  MAYBE_RETURN(target_result, Nothing<bool>());
+  if (target_result.FromJust() != trap_result->BooleanValue()) {
+    isolate->Throw(*factory->NewTypeError(
+        MessageTemplate::kProxyIsExtensibleViolatesInvariant));
+    return Nothing<bool>();
+  }
+  return target_result;
+}
+
+
 bool JSObject::IsExtensible(Handle<JSObject> object) {
   Isolate* isolate = object->GetIsolate();
   if (object->IsAccessCheckNeeded() &&
       !isolate->MayAccess(handle(isolate->context()), object)) {
     return true;
   }
   if (object->IsJSGlobalProxy()) {
     PrototypeIterator iter(isolate, *object);
     if (iter.IsAtEnd()) return false;
     DCHECK(iter.GetCurrent()->IsJSGlobalObject());
@@ skipping 10999 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