Add access check for observed objects

test/cctest/test-object-observe.cc

 // Copyright 2012 the V8 project authors. All rights reserved.
 // Redistribution and use in source and binary forms, with or without
 // modification, are permitted provided that the following conditions are
 // met:
 //
 //     * Redistributions of source code must retain the above copyright
 //       notice, this list of conditions and the following disclaimer.
 //     * Redistributions in binary form must reproduce the above
 //       copyright notice, this list of conditions and the following
 //       disclaimer in the documentation and/or other materials provided
@@ skipping 295 matching lines @@
   Handle<Array> recordArray = records.As<Array>();
   CHECK_EQ(num, static_cast<int>(recordArray->Length()));
   for (int i = 0; i < num; ++i) {
     Handle<Value> record = recordArray->Get(i);
     CHECK(record->IsObject());
     Handle<Object> recordObj = record.As<Object>();
     CHECK(expectations[i].object->StrictEquals(
         recordObj->Get(String::New("object"))));
     CHECK(String::New(expectations[i].type)->Equals(
         recordObj->Get(String::New("type"))));
-    CHECK(String::New(expectations[i].name)->Equals(
-        recordObj->Get(String::New("name"))));
-    if (!expectations[i].old_value.IsEmpty()) {
-      CHECK(expectations[i].old_value->Equals(
-          recordObj->Get(String::New("oldValue"))));
+    if (strcmp("splice", expectations[i].type) != 0) {
+      CHECK(String::New(expectations[i].name)->Equals(
+          recordObj->Get(String::New("name"))));
+      if (!expectations[i].old_value.IsEmpty()) {
+        CHECK(expectations[i].old_value->Equals(
+            recordObj->Get(String::New("oldValue"))));
+      }
     }
   }
 }
 
 #define EXPECT_RECORDS(records, expectations) \
     ExpectRecords(records, expectations, ARRAY_SIZE(expectations))
 
 TEST(APITestBasicMutation) {
   HarmonyIsolate isolate;
   HandleScope scope(isolate.GetIsolate());
@@ skipping 108 matching lines @@
       i::Handle<i::JSWeakMap>::cast(
           i::GetProperty(observation_state, "notifierTargetMap"));
   CHECK_EQ(1, NumberOfElements(callbackInfoMap));
   CHECK_EQ(1, NumberOfElements(objectInfoMap));
   CHECK_EQ(1, NumberOfElements(notifierTargetMap));
   HEAP->CollectAllGarbage(i::Heap::kAbortIncrementalMarkingMask);
   CHECK_EQ(0, NumberOfElements(callbackInfoMap));
   CHECK_EQ(0, NumberOfElements(objectInfoMap));
   CHECK_EQ(0, NumberOfElements(notifierTargetMap));
 }
+
+
+static bool NamedAccessAlwaysAllowed(Local<Object>, Local<Value>, AccessType,
+                                     Local<Value>) {
+  return true;
+}
+
+
+static bool IndexedAccessAlwaysAllowed(Local<Object>, uint32_t, AccessType,
+                                       Local<Value>) {
+  return true;
+}
+
+
+static bool NamedAccessAllowUnlessBlocked(Local<Object> host, Local<Value> key,
+                                          AccessType type, Local<Value>) {
+  Handle<Context> context = Context::GetCurrent();
+  return !context->Global()->Get(String::New("blockAccess"))->BooleanValue();
+}
+
+
+static bool IndexedAccessAllowUnlessBlocked(Local<Object> host, uint32_t index,
+                                            AccessType type, Local<Value>) {
+  Handle<Context> context = Context::GetCurrent();
+  return !context->Global()->Get(String::New("blockAccess"))->BooleanValue();
+}
+
+
+TEST(ObserveNamedAccessCheck) {

[rossberg, 2013/08/16 13:51:54]

Would be good to refine the test such that only individual properties are
blocked, while others pass (for both named and indexed). E.g. by replacing your
blockAccess mechanics with a black list.

[adamk, 2013/08/20 21:38:16]

Done.

+  HarmonyIsolate isolate;
+  HandleScope scope;
+  LocalContext context;
+  Handle<ObjectTemplate> tmpl = ObjectTemplate::New();
+  tmpl->SetAccessCheckCallbacks(NamedAccessAllowUnlessBlocked,
+                                IndexedAccessAlwaysAllowed);
+  Handle<Object> instance = tmpl->NewInstance();
+  context->Global()->Set(String::New("obj"), instance);
+  CompileRun("records = null;"

[rossberg, 2013/08/16 13:51:54]

Nit: can we make these proper 'var' declarations?

[adamk, 2013/08/20 21:38:16]

Done.

+             "objNoCheck = {};"
+             "blockAccess = true;"
+             "observer = function(r) { records = r };"
+             "Object.observe(obj, observer);"
+             "Object.observe(objNoCheck, observer);");
+  Handle<Value> obj_no_check = CompileRun("objNoCheck");
+  {
+    LocalContext context2;
+    context2->Global()->Set(String::New("obj"), instance);
+    context2->Global()->Set(String::New("objNoCheck"), obj_no_check);
+    CompileRun("records2 = null;"

[rossberg, 2013/08/16 13:51:54]

Same here.

[adamk, 2013/08/20 21:38:16]

Done.

+               "observer2 = function(r) { records2 = r };"
+               "Object.observe(obj, observer2);"
+               "Object.observe(objNoCheck, observer2);"
+               "obj.foo = 'bar';"

[rossberg, 2013/08/16 13:51:54]

Please add other forms of mutation, in particular, via Object.defineProperty and
via the API.

[adamk, 2013/08/20 21:38:16]

Happy to expand this a little bit, but I'd be interested in what you think
counts as good coverage; see my question below about complex array methods.

+               "objNoCheck.baz = 'quux'");
+    const RecordExpectation expected_records2[] = {
+      { instance, "new", "foo", Handle<Value>() },
+      { obj_no_check, "new", "baz", Handle<Value>() },
+    };
+    EXPECT_RECORDS(CompileRun("records2"), expected_records2);
+  }
+  const RecordExpectation expected_records[] = {
+    { obj_no_check, "new", "baz", Handle<Value>() }
+  };
+  EXPECT_RECORDS(CompileRun("records"), expected_records);
+}
+
+
+TEST(ObserveIndexedAccessCheck) {
+  HarmonyIsolate isolate;
+  HandleScope scope;
+  LocalContext context;
+  Handle<ObjectTemplate> tmpl = ObjectTemplate::New();
+  tmpl->SetAccessCheckCallbacks(NamedAccessAlwaysAllowed,
+                                IndexedAccessAllowUnlessBlocked);
+  Handle<Object> instance = tmpl->NewInstance();
+  context->Global()->Set(String::New("obj"), instance);
+  CompileRun("records = null;"
+             "objNoCheck = {};"
+             "blockAccess = true;"
+             "observer = function(r) { records = r };"
+             "Object.observe(obj, observer);"
+             "Object.observe(objNoCheck, observer);");
+  Handle<Value> obj_no_check = CompileRun("objNoCheck");
+  {
+    LocalContext context2;
+    context2->Global()->Set(String::New("obj"), instance);
+    context2->Global()->Set(String::New("objNoCheck"), obj_no_check);
+    CompileRun("records2 = null;"
+               "observer2 = function(r) { records2 = r };"
+               "Object.observe(obj, observer2);"
+               "Object.observe(objNoCheck, observer2);"
+               "obj[7] = 'bar';"
+               "objNoCheck[42] = 'quux'");
+    const RecordExpectation expected_records2[] = {
+      { instance, "new", "7", Handle<Value>() },
+      { obj_no_check, "new", "42", Handle<Value>() }
+    };
+    EXPECT_RECORDS(CompileRun("records2"), expected_records2);
+  }
+  const RecordExpectation expected_records[] = {
+    { obj_no_check, "new", "42", Handle<Value>() }
+  };
+  EXPECT_RECORDS(CompileRun("records"), expected_records);
+}
+
+
+TEST(ObserveSpliceAccessCheck) {
+  HarmonyIsolate isolate;
+  HandleScope scope;
+  LocalContext context;
+  Handle<ObjectTemplate> tmpl = ObjectTemplate::New();
+  tmpl->SetAccessCheckCallbacks(NamedAccessAlwaysAllowed,
+                                IndexedAccessAllowUnlessBlocked);
+  Handle<Object> instance = tmpl->NewInstance();
+  context->Global()->Set(String::New("obj"), instance);
+  CompileRun("records = null;"
+             "obj[1] = 'foo';"
+             "obj.length = 2;"
+             "objNoCheck = {1: 'bar', length: 2};"
+             "blockAccess = true;"
+             "observer = function(r) { records = r };"
+             "Array.observe(obj, observer);"
+             "Array.observe(objNoCheck, observer);");
+  Handle<Value> obj_no_check = CompileRun("objNoCheck");
+  {
+    LocalContext context2;
+    context2->Global()->Set(String::New("obj"), instance);
+    context2->Global()->Set(String::New("objNoCheck"), obj_no_check);
+    CompileRun("records2 = null;"
+               "observer2 = function(r) { records2 = r };"
+               "Array.observe(obj, observer2);"
+               "Array.observe(objNoCheck, observer2);"
+               // No one should hear about this: no splice records are emitted
+               // for access-checked objects
+               "[].pop.call(obj);"

[rossberg, 2013/08/16 13:51:54]

Can we check some more complex array functions, too?

[adamk, 2013/08/20 21:38:16]

How complex do you want? It's not clear to me what stuff should be tested here,
vs just testing access checks in general and testing record enqueueing in
general.

We have tests in mjsunit for testing that we get the right records in
Object.observe; as long as there's a reasonably tight bottleneck in
src/object-observe.js for where we enqueue records, I wouldn't expect there to
be any increase in test coverage from testing, say, 2 array methods compared to
5 array methods.

I ask because writing tests in this style (as API tests) is super-verbose and
time consuming, compared to writing tests in JS, so if I can get more coverage
out of the JS tests I'd much rather do that.

[rossberg, 2013/08/21 10:45:52]

Doesn't have to be crazy. I'd be content with one more method that creates more
than just a single straightforward splice record.

[adamk, 2013/08/21 17:18:36]

The nature of splice records is that there aren't any array operations that
create more than one record, but I've updated the test to do more than one
operation on the object.

+               "[].pop.call(objNoCheck);");
+    // TODO(adamk): Support splice records

[rossberg, 2013/08/16 13:51:54]

What is this TODO about?

[adamk, 2013/08/20 21:38:16]

Made it longer.

+    const RecordExpectation expected_records2[] = {
+      { obj_no_check, "splice", "", Handle<Value>() }
+    };
+    EXPECT_RECORDS(CompileRun("records2"), expected_records2);
+  }
+  const RecordExpectation expected_records[] = {
+    { obj_no_check, "splice", "", Handle<Value>() }
+  };
+  EXPECT_RECORDS(CompileRun("records"), expected_records);
+}