Refactored the mirror representation of properties. Removed the AssessorMirror...

Refactored the mirror representation of properties. Removed the AssessorMirror and InterceptorPropertyMirror and moved all reflection for properties to PropertyMirror. From a PropertyMirror it can be checked whether a property has getter/setter defined in JavaScript and information on the getter/setter functions are now available. If calling the getter resulted in an exception this is reflected as well.

Properties from interceptors are also reflected through PropertyMirror as the distinction did not make sense seen from a JavaScript debugging perspective. The isNative function on a PropertyMirror can be used to check whether a property is defined natively by the host (or V8).

Simplified the local property lookup in the debug runtime call to just call GetProperty as the property is known to be a local property.
Review URL: http://codereview.chromium.org/17377

git-svn-id: http://v8.googlecode.com/svn/branches/bleeding_edge@1068 ce2b1a6d-e550-0410-aec6-3dcde31c8c00

src/runtime.cc

@@ -4503,30 +4503,32 @@ static Object* Runtime_Break(Arguments args) {
 }
 
 
-static Object* DebugLookupResultValue(LookupResult* result) {
-  Object* value;
+static Object* DebugLookupResultValue(Object* obj, String* name,
+                                      LookupResult* result,
+                                      bool* caught_exception) {
   switch (result->type()) {
-    case NORMAL: {
-      Dictionary* dict =
-          JSObject::cast(result->holder())->property_dictionary();
-      value = dict->ValueAt(result->GetDictionaryEntry());
-      if (value->IsTheHole()) {
-        return Heap::undefined_value();
-      }
-      return value;
-    }
+    case NORMAL:
     case FIELD:
-      value =
-          JSObject::cast(
-              result->holder())->FastPropertyAt(result->GetFieldIndex());
-      if (value->IsTheHole()) {
-        return Heap::undefined_value();
+    case CONSTANT_FUNCTION:
+      return obj->GetProperty(name);
+    case CALLBACKS: {
+      // Get the property value. If there is an exception it must be thown from
+      // a JavaScript getter.
+      Object* value;
+      value = obj->GetProperty(name);
+      if (value->IsException()) {
+        if (caught_exception != NULL) {
+          *caught_exception = true;
+        }
+        value = Top::pending_exception();
+        Top::optional_reschedule_exception(true);
       }
+      ASSERT(!Top::has_pending_exception());
+      ASSERT(!Top::external_caught_exception());
       return value;
-    case CONSTANT_FUNCTION:
-      return result->GetConstantFunction();
-    case CALLBACKS:
+    }
     case INTERCEPTOR:
+      return obj->GetProperty(name);
     case MAP_TRANSITION:
     case CONSTANT_TRANSITION:
     case NULL_DESCRIPTOR:
@@ -4539,6 +4541,18 @@ static Object* DebugLookupResultValue(LookupResult* result) {
 }
 
 
+// Get debugger related details for an object property.
+// args[0]: object holding property
+// args[1]: name of the property
+//
+// The array returned contains the following information:
+// 0: Property value
+// 1: Property details
+// 2: Property value is exception
+// 3: Getter function if defined
+// 4: Setter function if defined
+// Items 2-4 are only filled if the property has either a getter or a setter
+// defined through __defineGetter__ and/or __defineSetter__.
 static Object* Runtime_DebugGetPropertyDetails(Arguments args) {
   HandleScope scope;
 
@@ -4559,12 +4573,26 @@ static Object* Runtime_DebugGetPropertyDetails(Arguments args) {
 
   // Perform standard local lookup on the object.
   LookupResult result;
-  obj->Lookup(*name, &result);
+  obj->LocalLookup(*name, &result);
   if (result.IsProperty()) {
-    Handle<Object> value(DebugLookupResultValue(&result));
-    Handle<FixedArray> details = Factory::NewFixedArray(2);
+    bool caught_exception = false;
+    Handle<Object> value(DebugLookupResultValue(*obj, *name, &result,
+                                                &caught_exception));
+    // If the callback object is a fixed array then it contains JavaScript
+    // getter and/or setter.
+    bool hasJavaScriptAccessors = result.type() == CALLBACKS &&
+                                  result.GetCallbackObject()->IsFixedArray();
+    Handle<FixedArray> details =
+        Factory::NewFixedArray(hasJavaScriptAccessors ? 5 : 2);
     details->set(0, *value);
     details->set(1, result.GetPropertyDetails().AsSmi());
+    if (hasJavaScriptAccessors) {
+      details->set(2,
+                   caught_exception ? Heap::true_value() : Heap::false_value());
+      details->set(3, FixedArray::cast(result.GetCallbackObject())->get(0));
+      details->set(4, FixedArray::cast(result.GetCallbackObject())->get(1));
+    }
+
     return *Factory::NewJSArrayWithElements(details);
   }
   return Heap::undefined_value();
@@ -4582,7 +4610,7 @@ static Object* Runtime_DebugGetProperty(Arguments args) {
   LookupResult result;
   obj->Lookup(*name, &result);
   if (result.IsProperty()) {
-    return DebugLookupResultValue(&result);
+    return DebugLookupResultValue(*obj, *name, &result, NULL);
   }
   return Heap::undefined_value();
 }
@@ -4676,10 +4704,11 @@ static Object* Runtime_DebugNamedInterceptorPropertyNames(Arguments args) {
   HandleScope scope;
   ASSERT(args.length() == 1);
   CONVERT_ARG_CHECKED(JSObject, obj, 0);
-  RUNTIME_ASSERT(obj->HasNamedInterceptor());
 
-  v8::Handle<v8::Array> result = GetKeysForNamedInterceptor(obj, obj);
-  if (!result.IsEmpty()) return *v8::Utils::OpenHandle(*result);
+  if (obj->HasNamedInterceptor()) {
+    v8::Handle<v8::Array> result = GetKeysForNamedInterceptor(obj, obj);
+    if (!result.IsEmpty()) return *v8::Utils::OpenHandle(*result);
+  }
   return Heap::undefined_value();
 }
 
@@ -4690,10 +4719,11 @@ static Object* Runtime_DebugIndexedInterceptorElementNames(Arguments args) {
   HandleScope scope;
   ASSERT(args.length() == 1);
   CONVERT_ARG_CHECKED(JSObject, obj, 0);
-  RUNTIME_ASSERT(obj->HasIndexedInterceptor());
 
-  v8::Handle<v8::Array> result = GetKeysForIndexedInterceptor(obj, obj);
-  if (!result.IsEmpty()) return *v8::Utils::OpenHandle(*result);
+  if (obj->HasIndexedInterceptor()) {
+    v8::Handle<v8::Array> result = GetKeysForIndexedInterceptor(obj, obj);
+    if (!result.IsEmpty()) return *v8::Utils::OpenHandle(*result);
+  }
   return Heap::undefined_value();
 }
 

test/cctest/test-debug.cc

@@ -2660,58 +2660,76 @@ TEST(InterceptorPropertyMirror) {
 
   // Get the property names from the interceptors
   CompileRun(
-      "named_names = named_mirror.interceptorPropertyNames();"
-      "indexed_names = indexed_mirror.interceptorPropertyNames();"
-      "both_names = both_mirror.interceptorPropertyNames()");
+      "named_names = named_mirror.propertyNames();"
+      "indexed_names = indexed_mirror.propertyNames();"
+      "both_names = both_mirror.propertyNames()");
   CHECK_EQ(3, CompileRun("named_names.length")->Int32Value());
   CHECK_EQ(2, CompileRun("indexed_names.length")->Int32Value());
   CHECK_EQ(5, CompileRun("both_names.length")->Int32Value());
 
   // Check the expected number of properties.
   const char* source;
-  source = "named_mirror.interceptorProperties().length";
+  source = "named_mirror.properties().length";
   CHECK_EQ(3, CompileRun(source)->Int32Value());
 
-  source = "indexed_mirror.interceptorProperties().length";
+  source = "indexed_mirror.properties().length";
   CHECK_EQ(2, CompileRun(source)->Int32Value());
 
-  source = "both_mirror.interceptorProperties().length";
+  source = "both_mirror.properties().length";
   CHECK_EQ(5, CompileRun(source)->Int32Value());
 
-  source = "both_mirror.interceptorProperties(1).length";
+  // 1 is PropertyKind.Named;
+  source = "both_mirror.properties(1).length";
   CHECK_EQ(3, CompileRun(source)->Int32Value());
 
-  source = "both_mirror.interceptorProperties(2).length";
+  // 2 is PropertyKind.Indexed;
+  source = "both_mirror.properties(2).length";
   CHECK_EQ(2, CompileRun(source)->Int32Value());
 
-  source = "both_mirror.interceptorProperties(3).length";
+  // 3 is PropertyKind.Named  | PropertyKind.Indexed;
+  source = "both_mirror.properties(3).length";
   CHECK_EQ(5, CompileRun(source)->Int32Value());
 
-  // Get the interceptor properties for the object with both types of
-  // interceptors.
-  CompileRun("both_values = both_mirror.interceptorProperties()");
+  // Get the interceptor properties for the object with only named interceptor.
+  CompileRun("named_values = named_mirror.properties()");
 
-  // Check the mirror hierachy
-  source = "both_values[0] instanceof debug.PropertyMirror";
-  CHECK(CompileRun(source)->BooleanValue());
+  // Check that the properties are interceptor properties.
+  for (int i = 0; i < 3; i++) {
+    EmbeddedVector<char, SMALL_STRING_BUFFER_SIZE> buffer;
+    OS::SNPrintF(buffer,
+                 "named_values[%d] instanceof debug.PropertyMirror", i);
+    CHECK(CompileRun(buffer.start())->BooleanValue());
 
-  source = "both_values[0] instanceof debug.InterceptorPropertyMirror";
-  CHECK(CompileRun(source)->BooleanValue());
+    // 4 is PropertyType.Interceptor
+    OS::SNPrintF(buffer, "named_values[%d].propertyType()", i);
+    CHECK_EQ(4, CompileRun(buffer.start())->Int32Value());
 
-  source = "both_values[1] instanceof debug.InterceptorPropertyMirror";
-  CHECK(CompileRun(source)->BooleanValue());
+    OS::SNPrintF(buffer, "named_values[%d].isNative()", i);
+    CHECK(CompileRun(buffer.start())->BooleanValue());
+  }
 
-  source = "both_values[2] instanceof debug.InterceptorPropertyMirror";
-  CHECK(CompileRun(source)->BooleanValue());
+  // Get the interceptor properties for the object with only indexed
+  // interceptor.
+  CompileRun("indexed_values = indexed_mirror.properties()");
 
-  source = "both_values[3] instanceof debug.PropertyMirror";
-  CHECK(CompileRun(source)->BooleanValue());
+  // Check that the properties are interceptor properties.
+  for (int i = 0; i < 2; i++) {
+    EmbeddedVector<char, SMALL_STRING_BUFFER_SIZE> buffer;
+    OS::SNPrintF(buffer,
+                 "indexed_values[%d] instanceof debug.PropertyMirror", i);
+    CHECK(CompileRun(buffer.start())->BooleanValue());
+  }
 
-  source = "both_values[3] instanceof debug.InterceptorPropertyMirror";
-  CHECK(CompileRun(source)->BooleanValue());
+  // Get the interceptor properties for the object with both types of
+  // interceptors.
+  CompileRun("both_values = both_mirror.properties()");
 
-  source = "both_values[4] instanceof debug.InterceptorPropertyMirror";
-  CHECK(CompileRun(source)->BooleanValue());
+  // Check that the properties are interceptor properties.
+  for (int i = 0; i < 5; i++) {
+    EmbeddedVector<char, SMALL_STRING_BUFFER_SIZE> buffer;
+    OS::SNPrintF(buffer, "both_values[%d] instanceof debug.PropertyMirror", i);
+    CHECK(CompileRun(buffer.start())->BooleanValue());
+  }
 
   // Check the property names.
   source = "both_values[0].name() == 'a'";

test/mjsunit/mirror-object.js

@@ -107,8 +107,7 @@ function testObjectMirror(o, cls_name, ctor_name, hasSpecialProperties) {
         } else {
           assertTrue(typeof(fromJSON.properties[i].attributes) === 'undefined');
         }
-        if (!properties[i].value() instanceof debug.AccessorMirror &&
-            properties[i].value().isPrimitive()) {
+        if (!properties[i].value().isPrimitive()) {
           // NaN is not equal to NaN.
           if (isNaN(properties[i].value().value())) {
             assertTrue(isNaN(fromJSON.properties[i].value.value));
@@ -157,27 +156,46 @@ assertFalse(math_mirror.property("E").canDelete());
 
 // Test objects with JavaScript accessors.
 o = {}
-o.__defineGetter__('a', function(){throw 'a';})
-o.__defineSetter__('b', function(){throw 'b';})
-o.__defineGetter__('c', function(){throw 'c';})
-o.__defineSetter__('c', function(){throw 'c';})
+o.__defineGetter__('a', function(){return 'a';});
+o.__defineSetter__('b', function(){});
+o.__defineGetter__('c', function(){throw 'c';});
+o.__defineSetter__('c', function(){throw 'c';});
 testObjectMirror(o, 'Object', 'Object');
 mirror = debug.MakeMirror(o);
 // a has getter but no setter.
-assertTrue(mirror.property('a').value() instanceof debug.AccessorMirror);
+assertTrue(mirror.property('a').hasGetter());
+assertFalse(mirror.property('a').hasSetter());
 assertEquals(debug.PropertyType.Callbacks, mirror.property('a').propertyType());
+assertEquals('function', mirror.property('a').getter().type());
+assertEquals('undefined', mirror.property('a').setter().type());
+assertEquals('function (){return \'a\';}', mirror.property('a').getter().source());
+assertEquals('a', mirror.property('a').value().value());
+assertFalse(mirror.property('a').isException());
 // b has setter but no getter.
-assertTrue(mirror.property('b').value() instanceof debug.AccessorMirror);
+assertFalse(mirror.property('b').hasGetter());
+assertTrue(mirror.property('b').hasSetter());
 assertEquals(debug.PropertyType.Callbacks, mirror.property('b').propertyType());
-// c has both getter and setter.
-assertTrue(mirror.property('c').value() instanceof debug.AccessorMirror);
+assertEquals('undefined', mirror.property('b').getter().type());
+assertEquals('function', mirror.property('b').setter().type());
+assertEquals('function (){}', mirror.property('b').setter().source());
+assertFalse(mirror.property('b').isException());
+// c has both getter and setter. The getter throws an exception.
+assertTrue(mirror.property('c').hasGetter());
+assertTrue(mirror.property('c').hasSetter());
 assertEquals(debug.PropertyType.Callbacks, mirror.property('c').propertyType());
+assertEquals('function', mirror.property('c').getter().type());
+assertEquals('function', mirror.property('c').setter().type());
+assertEquals('function (){throw \'c\';}', mirror.property('c').getter().source());
+assertEquals('function (){throw \'c\';}', mirror.property('c').setter().source());
+assertEquals('c', mirror.property('c').value().value());
+assertTrue(mirror.property('c').isException());
 
 // Test objects with native accessors.
 mirror = debug.MakeMirror(new String('abc'));
 assertTrue(mirror instanceof debug.ObjectMirror);
-assertTrue(mirror.property('length').value() instanceof debug.AccessorMirror);
-assertTrue(mirror.property('length').value().isNative());
+assertFalse(mirror.property('length').hasGetter());
+assertFalse(mirror.property('length').hasSetter());
+assertTrue(mirror.property('length').isNative());
 assertEquals('a', mirror.property(0).value().value());
 assertEquals('b', mirror.property(1).value().value());
 assertEquals('c', mirror.property(2).value().value());