Add access check for observed objects

test/cctest/test-object-observe.cc

Index: test/cctest/test-object-observe.cc
diff --git a/test/cctest/test-object-observe.cc b/test/cctest/test-object-observe.cc
index 44ddb6fa2061a482dacdf1c639080c922a0cd47a..677c3f891720bf2ee4acb9bdeeec854ed945fe29 100644
--- a/test/cctest/test-object-observe.cc
+++ b/test/cctest/test-object-observe.cc
@@ -313,11 +313,13 @@ static void ExpectRecords(Handle<Value> records,
         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"))));
+      }
     }
   }
 }
@@ -446,3 +448,150 @@ TEST(ObservationWeakMap) {
   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);
+}