Properly fix enumerate / Object.keys wrt access checked objects

BUG=chromium:509936
LOG=y

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

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

src/objects.cc

@@ -6151,10 +6151,13 @@ MaybeHandle<FixedArray> JSReceiver::GetKeys(Handle<JSReceiver> object,
   Handle<JSFunction> arguments_function(
       JSFunction::cast(isolate->sloppy_arguments_map()->GetConstructor()));
 
+  PrototypeIterator::WhereToEnd end = type == OWN_ONLY
+                                          ? PrototypeIterator::END_AT_NON_HIDDEN
+                                          : PrototypeIterator::END_AT_NULL;
   // Only collect keys if access is permitted.
   for (PrototypeIterator iter(isolate, object,
                               PrototypeIterator::START_AT_RECEIVER);
-       !iter.IsAtEnd(); iter.Advance()) {
+       !iter.IsAtEnd(end); iter.Advance()) {
     if (PrototypeIterator::GetCurrent(iter)->IsJSProxy()) {
       Handle<JSProxy> proxy(JSProxy::cast(*PrototypeIterator::GetCurrent(iter)),
                             isolate);
@@ -6181,7 +6184,11 @@ MaybeHandle<FixedArray> JSReceiver::GetKeys(Handle<JSReceiver> object,
 
     // Check access rights if required.
     if (current->IsAccessCheckNeeded() && !isolate->MayAccess(current)) {
-      return content;
+      if (iter.IsAtEnd(PrototypeIterator::END_AT_NON_HIDDEN)) {
+        isolate->ReportFailedAccessCheck(current);
+        RETURN_EXCEPTION_IF_SCHEDULED_EXCEPTION(isolate, FixedArray);
+      }
+      break;
     }
 
     // Compute the element keys.
@@ -6241,10 +6248,6 @@ MaybeHandle<FixedArray> JSReceiver::GetKeys(Handle<JSReceiver> object,
       }
       DCHECK(ContainsOnlyValidKeys(content));
     }
-
-    // If we only want own properties we bail out after the first
-    // iteration.
-    if (type == OWN_ONLY) break;
   }
   return content;
 }

src/runtime/runtime-object.cc

@@ -984,20 +984,6 @@ RUNTIME_FUNCTION(Runtime_OwnKeys) {
   CONVERT_ARG_CHECKED(JSObject, raw_object, 0);
   Handle<JSObject> object(raw_object);
 
-  if (object->IsJSGlobalProxy()) {
-    // Do access checks before going to the global object.
-    if (object->IsAccessCheckNeeded() && !isolate->MayAccess(object)) {
-      isolate->ReportFailedAccessCheck(object);
-      RETURN_FAILURE_IF_SCHEDULED_EXCEPTION(isolate);
-      return *isolate->factory()->NewJSArray(0);
-    }
-
-    PrototypeIterator iter(isolate, object);
-    // If proxy is detached we simply return an empty array.
-    if (iter.IsAtEnd()) return *isolate->factory()->NewJSArray(0);
-    object = Handle<JSObject>::cast(PrototypeIterator::GetCurrent(iter));
-  }
-
   Handle<FixedArray> contents;
   ASSIGN_RETURN_FAILURE_ON_EXCEPTION(
       isolate, contents, JSReceiver::GetKeys(object, JSReceiver::OWN_ONLY));

test/cctest/test-api.cc

@@ -8140,7 +8140,7 @@ THREADED_TEST(CrossDomainIsPropertyEnumerable) {
 }
 
 
-THREADED_TEST(CrossDomainForIn) {
+THREADED_TEST(CrossDomainFor) {
   LocalContext env1;
   v8::HandleScope handle_scope(env1->GetIsolate());
   v8::Handle<Context> env2 = Context::New(env1->GetIsolate());
@@ -8164,9 +8164,8 @@ THREADED_TEST(CrossDomainForIn) {
     Context::Scope scope_env2(env2);
     Local<Value> result = CompileRun(
         "(function() {"
-        "  var obj = { '__proto__': env1 };"
         "  try {"
-        "    for (var p in obj) {"
+        "    for (var p in env1) {"
         "      if (p == 'prop') return false;"
         "    }"
         "    return true;"
@@ -8179,6 +8178,45 @@ THREADED_TEST(CrossDomainForIn) {
 }
 
 
+THREADED_TEST(CrossDomainForInOnPrototype) {
+  LocalContext env1;
+  v8::HandleScope handle_scope(env1->GetIsolate());
+  v8::Handle<Context> env2 = Context::New(env1->GetIsolate());
+
+  Local<Value> foo = v8_str("foo");
+  Local<Value> bar = v8_str("bar");
+
+  // Set to the same domain.
+  env1->SetSecurityToken(foo);
+  env2->SetSecurityToken(foo);
+
+  env1->Global()->Set(v8_str("prop"), v8_num(3));
+  env2->Global()->Set(v8_str("env1"), env1->Global());
+
+  // Change env2 to a different domain and set env1's global object
+  // as the __proto__ of an object in env2 and enumerate properties
+  // in for-in. It shouldn't enumerate properties on env1's global
+  // object.
+  env2->SetSecurityToken(bar);
+  {
+    Context::Scope scope_env2(env2);
+    Local<Value> result = CompileRun(
+        "(function() {"
+        "  var obj = { '__proto__': env1 };"
+        "  try {"
+        "    for (var p in obj) {"
+        "      if (p == 'prop') return false;"
+        "    }"
+        "    return false;"
+        "  } catch (e) {"
+        "    return true;"
+        "  }"
+        "})()");
+    CHECK(result->IsTrue());
+  }
+}
+
+
 TEST(ContextDetachGlobal) {
   LocalContext env1;
   v8::HandleScope handle_scope(env1->GetIsolate());
@@ -8627,9 +8665,9 @@ TEST(AccessControl) {
       "        return false;"
       "      }"
       "    }"
-      "    return true;"
-      "  } catch (e) {"
       "    return false;"
+      "  } catch (e) {"
+      "    return true;"
       "  }"
       "})()");
   CHECK(value->IsTrue());
@@ -8673,7 +8711,7 @@ TEST(AccessControlES5) {
   global1->Set(v8_str("other"), global0);
 
   // Regression test for issue 1154.
-  CHECK(CompileRun("Object.keys(other)").IsEmpty());
+  CHECK(CompileRun("Object.keys(other).length == 0")->BooleanValue());
   CHECK(CompileRun("other.blocked_prop").IsEmpty());
 
   // Regression test for issue 1027.